New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 918677 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Today
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

AudioProcessing refers to incorrect AGC instance

Project Member Reported by kehuangli@google.com, Jan 2

Issue description

WebRTC AudioProcessing::gain_control() will return a pointer to experimental AGC if user doesn't explicitly set ExperimentalAgc to false in WebRTC config.
 
Cc: ossu@chromium.org aleloi@chromium.org
Components: -Internals>Media>Audio
Inside webrtc::AudioProcessing, ExperimentalAgc is enabled by default. I think this is intended.
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_processing/include/audio_processing.h?sq=package:chromium&g=0&l=136

MediaStreamAudioProcessor has a config flag AudioProcessingProperties::goog_experimental_auto_gain_control.
It is enabled by default. When explicitly set to false, MSAP does not do anything to the webrtc::Config:
https://cs.chromium.org/chromium/src/content/renderer/media/stream/media_stream_audio_processor.cc?type=cs&sq=package:chromium&g=0&l=587
The settings implicitly fall back to the webrtc::AudioProcessing default, enabled. This looks like a bug.

We may have the same issue in media::AudioProcessor:
https://cs.chromium.org/chromium/src/media/webrtc/audio_processor.cc?type=cs&g=0&l=230
Components: Internals>Media>Audio
Project Member

Comment 3 by bugdroid1@chromium.org, Jan 10

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

commit c066ec3dae113e6379b9009acab172cb5283eba4
Author: Oskar Sundbom <ossu@chromium.org>
Date: Thu Jan 10 10:53:55 2019

APM in AS: Explicitly set ExperimentalAgc to false when not enabled

It turns out enabling the experimental AGC is the default, so not
explicitly disabling it if not wanted will leave it on.

This mirrors the changes in [1] but for the APM integration used when
running the APM in the audio process.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1393340

Bug:  918677 
Change-Id: Iaad99218b6d5cbaa7c9bf7a7342521222440ab40
Reviewed-on: https://chromium-review.googlesource.com/c/1402793
Reviewed-by: Max Morin <maxmorin@chromium.org>
Commit-Queue: Oskar Sundbom <ossu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621538}
[modify] https://crrev.com/c066ec3dae113e6379b9009acab172cb5283eba4/media/webrtc/audio_processor.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Today (11 hours ago)

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

commit e8a581f40109cb90781a95dc317fc8150628de5c
Author: Kehuang Li <kehuangli@chromium.org>
Date: Tue Jan 22 19:27:51 2019

Explicitly set ExperimentalAgc to false when not enabled on Chromecast

ExperimentalAgc is by default created as enabled, and thus
gain_control() will return a pointer to ExperimentalAgc anyway no
matter if the google_experimental_auto_gain_control is set or not.
Explicitly disable ExperimentalAgc in WebRTC config when the flag is
false. We only fix this bug on Chromecast build, since other
applications may already, by default, be using the adaptive gain
control of ExperimentalAgc, which has better performance.

Bug:  918677 
Test: Add print log and see the correct instance is referenced.
Change-Id: I73f512f03bb8296053d93f1f9d1294dc087f231e
Reviewed-on: https://chromium-review.googlesource.com/c/1393340
Commit-Queue: Kehuang Li <kehuangli@chromium.org>
Reviewed-by: Oskar Sundbom <ossu@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624872}
[modify] https://crrev.com/e8a581f40109cb90781a95dc317fc8150628de5c/content/renderer/BUILD.gn
[modify] https://crrev.com/e8a581f40109cb90781a95dc317fc8150628de5c/content/renderer/media/stream/media_stream_audio_processor.cc

Comment 5 by kehuangli@google.com, Today (11 hours ago)

Status: Fixed (was: Started)

Sign in to add a comment