New issue
Advanced search Search tips

Issue 882360 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug


Show other hotlists

Hotlists containing this issue:
AudioService-FixIt


Sign in to add a comment

Filter out samples outside of [-1, 1] from the APM when running in the audio service.

Project Member Reported by maxmorin@chromium.org, Sep 10

Issue description

The APM will DCHECK pretty quickly if the renderer sends some NaN over as reference signal. We said we would filter them, but then we forgot to :).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a072b983b33cd2e8c022a20e6b2a2702e5940ff6

commit a072b983b33cd2e8c022a20e6b2a2702e5940ff6
Author: Max Morin <maxmorin@chromium.org>
Date: Mon Sep 10 14:27:14 2018

Sanitize audio data passed to APM in audio service.

The APM expects data in the range [-1, 1] only. This CL also avoids
sending bitstream stuff to snoopers, since they would misunderstand it
anyways. The sanitation is only carried out when using the APM, to
ensure that ordinary playback isn't impacted.

Bug: 851959, 882360
Change-Id: I8b51af68f3a3e53383eb6630f73b88205faac887
Reviewed-on: https://chromium-review.googlesource.com/1215865
Reviewed-by: Oskar Sundbom <ossu@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589905}
[modify] https://crrev.com/a072b983b33cd2e8c022a20e6b2a2702e5940ff6/services/audio/output_controller.cc

As noted on the CL, I think this should be done at the APM interface unless we plan to do this for everything coming to the browser process (and then remove the AudioBus interleave checks).
Ah, now I see https://cs.chromium.org/chromium/src/media/base/audio_sample_types.h?type=cs&sq=package:chromium&g=0&l=63, which implements what I implemented again :). If we also sanitize before handing the data over to the OS, it would make sense to sanitize it as early as possible to avoid doing it multiple times. This reasoning is also applicable to the APM, since a single output stream may be reference to several input streams. Doing the sanitation in AudioOutputController ensures we only have to do it once for all the APMs we feed the stream to. WDYT about me moving the clipping/cleaning to AudioSyncReader and removing it from the ToInterleaved? Maybe it's still needed when calling ToInterleaved in the renderer (haven't checked the call sites), so it takes a bool or there are several methods like ToInterleaved/ToInterleavedWithClipping?
Cc: dalecur...@chromium.org
CC Dale in case you didn't get a mail for that comment.
Yeah that sgtm, though care must be taken on how to do it in the implementation to not introduce slowdown in ToInterleaved. There's an AudioBus perftest to measure that. 
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/13ecd0d3640000

All of the runs failed. The most common error (1/20 runs) was:
IOError: [Errno 2] No such file or directory: '/b/s/w/it7IAewP/tmpwbU7OOtelemetry/histograms.json'
Labels: M-71
Labels: -M-71 M-72
Labels: -Pri-1 -M-72 Pri-2

Sign in to add a comment