Chrome-wide echo cancellation: Allow all sample rates, add parameter for disabling [chromium/src : main]
Sam Zackrisson (Gerrit)
Attention is currently required from: Olga Sharonova.
Sam Zackrisson would like Olga Sharonova to review this change.
Chrome-wide echo cancellation: Allow all sample rates, add parameter for disabling
This CL allows sample rates not divisible by 100 when audio processing runs in the audio service. An experiment parameter is added to disable this behavior.
Bug: chromium:1332484
Change-Id: Ia91832625e63df081e6ce3206f0c8718fdb35b6a
---
M chrome/browser/media/webrtc/webrtc_text_log_handler.cc
M content/browser/media/media_internals.cc
M media/base/media_switches.cc
M media/base/media_switches.h
M third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio.cc
M third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio_test.cc
6 files changed, 69 insertions(+), 19 deletions(-)
To view, visit change 3743770. To unsubscribe, or for help writing mail filters, visit settings.
Sam Zackrisson (Gerrit)
Attention is currently required from: Olga Sharonova.
1 comment:
Patchset:
Hi Olga, here is the kill-switch we discussed. I haven't tested it manually / run trybots yet, but the unit tests are updated to verify the parameter when on/off.
To view, visit change 3743770. To unsubscribe, or for help writing mail filters, visit settings.
Olga Sharonova (Gerrit)
Attention is currently required from: Sam Zackrisson.
2 comments:
File media/base/media_switches.cc:
Patch Set #1, Line 390:
// audio processing. https://crbug.com/1332484We need to add something like:
If disabled, only sample rates divisible by 100 are allowed; a request for a media stream with enabled audio processing will fail otherwise.
File third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio.cc:
Patch Set #1, Line 1158:
!media::kChromeWideEchoCancellationAllowAllSampleRates.Get() &&This won't compile if BUILDFLAG(CHROME_WIDE_ECHO_CANCELLATION) is false.
Make a helper media::ProcessingForAllSampleRatesAllowed() to cover both conditions?
To view, visit change 3743770. To unsubscribe, or for help writing mail filters, visit settings.
Sam Zackrisson (Gerrit)
Attention is currently required from: Olga Sharonova.
3 comments:
Patchset:
Hi! I addressed the comments.
I am struggling a bit with manual testing results though: I can't get my 22050 Hz headphones to _fail_ media constraint resolution neither with this CL and "allow_all_sample_rates/false" nor on an official Canary build.
File media/base/media_switches.cc:
Patch Set #1, Line 390:
// audio processing. https://crbug.com/1332484
We need to add something like: […]
Done
File third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio.cc:
Patch Set #1, Line 1158:
!media::kChromeWideEchoCancellationAllowAllSampleRates.Get() &&
This won't compile if BUILDFLAG(CHROME_WIDE_ECHO_CANCELLATION) is false. […]
Done!
To view, visit change 3743770. To unsubscribe, or for help writing mail filters, visit settings.
Olga Sharonova (Gerrit)
Attention is currently required from: Sam Zackrisson.
Patch set 4:Code-Review +1
1 comment:
Patchset:
LGTM
To view, visit change 3743770. To unsubscribe, or for help writing mail filters, visit settings.
Sam Zackrisson (Gerrit)
Attention is currently required from: Thomas Guilbert.
Sam Zackrisson would like Thomas Guilbert to review this change.
Chrome-wide echo cancellation: Allow all sample rates, add parameter for disabling
This CL allows sample rates not divisible by 100 when audio processing runs in the audio service. An experiment parameter is added to disable this behavior.
Bug: chromium:1332484
Change-Id: Ia91832625e63df081e6ce3206f0c8718fdb35b6a
---
M chrome/browser/media/webrtc/webrtc_text_log_handler.cc
M content/browser/media/media_internals.cc
M media/base/media_switches.cc
M media/base/media_switches.h
M third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio.cc
M third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio_test.cc
6 files changed, 88 insertions(+), 23 deletions(-)
Sam Zackrisson (Gerrit)
Attention is currently required from: Thomas Guilbert.
1 comment:
Patchset:
Hi Thomas! PTAL as owner of media/base/ and chrome/browser/media/webrtc/
To view, visit change 3743770. To unsubscribe, or for help writing mail filters, visit settings.
Sam Zackrisson (Gerrit)
Attention is currently required from: Thomas Guilbert.
Patch set 4:Auto-Submit +1
Thomas Guilbert (Gerrit)
Attention is currently required from: Sam Zackrisson.
Patch set 4:Code-Review +1
Sam Zackrisson (Gerrit)
Attention is currently required from: Sam Zackrisson.
Sam Zackrisson uploaded patch set #5 to this change.
Chrome-wide echo cancellation: Allow all sample rates, add parameter for disabling
This CL allows sample rates not divisible by 100 when audio processing runs in the audio service. An experiment parameter is added to disable this behavior.
Tested: Locally on Windows with --enable-features toggling param on/off, + updated unit tests.
Bug: chromium:1332484
Change-Id: Ia91832625e63df081e6ce3206f0c8718fdb35b6a
---
M chrome/browser/media/webrtc/webrtc_text_log_handler.cc
M content/browser/media/media_internals.cc
M media/base/media_switches.cc
M media/base/media_switches.h
M third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio.cc
M third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio_test.cc
6 files changed, 89 insertions(+), 23 deletions(-)
To view, visit change 3743770. To unsubscribe, or for help writing mail filters, visit settings.
Sam Zackrisson (Gerrit)
Attention is currently required from: Sam Zackrisson.
Patch set 5:Commit-Queue +2
Sam Zackrisson (Gerrit)
Sam Zackrisson (Gerrit)
Patch set 5:Auto-Submit +1
1 comment:
Patchset:
Hi! I addressed the comments. […]
..I figured it out. It's all good. Thanks for the offline discussion!
To view, visit change 3743770. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
4 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Chrome-wide echo cancellation: Allow all sample rates, add parameter for disabling
This CL allows sample rates not divisible by 100 when audio processing runs in the audio service. An experiment parameter is added to disable this behavior.
Tested: Locally on Windows with --enable-features toggling param on/off, + updated unit tests.
Bug: chromium:1332484
Change-Id: Ia91832625e63df081e6ce3206f0c8718fdb35b6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3743770
Reviewed-by: Thomas Guilbert <tgui...@chromium.org>
Commit-Queue: Sam Zackrisson <sa...@chromium.org>
Auto-Submit: Sam Zackrisson <sa...@chromium.org>
Reviewed-by: Olga Sharonova <ol...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1021781}
---
M chrome/browser/media/webrtc/webrtc_text_log_handler.cc
M content/browser/media/media_internals.cc
M media/base/media_switches.cc
M media/base/media_switches.h
M third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio.cc
M third_party/blink/renderer/modules/mediastream/media_stream_constraints_util_audio_test.cc
6 files changed, 95 insertions(+), 23 deletions(-)