WebRTC Code Reviews
Help | Chromium Project | Sign in

Issue 31459004: Enable render downmixing to mono in AudioProcessing. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
5 years, 4 months ago by ajm
5 years, 3 months ago
bjornv, aluebs, kwiberg, Bjorn
webrtc-reviews_webrtc.org, rillian-moz, tterriberry, kwiberg, aluebs
Base URL:


Enable render downmixing to mono in AudioProcessing. In practice, we have been doing this since time immemorial, but have relied on the user to do the downmixing (first voice engine then Chromium). It's more logical for this burden to fall on AudioProcessing, however, who can be expected to know that this is a reasonable approach for AEC. Permitting two render channels results in running two AECs serially. Critically, in my recent change to have Chromium adopt the float interface: https://codereview.chromium.org/420603004 I removed the downmixing by Chromium, forgetting that we hadn't yet enabled this feature in AudioProcessing. This corrects that oversight. The change in paths hit by production users is very minor. As commented it required adding downmixing to the int16_t path to satisfy bit-exactness tests. For reference, find the ApmTest.Process errors here: https://paste.googleplex.com/6372007910309888 BUG=webrtc:3853 TESTED=listened to the files output from the Process test, and verified that they sound as expected: higher echo while the AEC is adapting, but afterwards very close. R=aluebs@webrtc.org, bjornv@webrtc.org, kwiberg@webrtc.org Committed: https://code.google.com/p/webrtc/source/detail?r=7292

Patch Set 1 #

Patch Set 2 : Update output references. #

Total comments: 15

Patch Set 3 : Review comments. #

Patch Set 4 : Rebase and update float output. #


Total messages: 10 (0 generated)
As you can guess, we should really get this into M39. So please have a ...
5 years, 4 months ago (2014-09-24 01:52:06 UTC) #1
...and to give you some background, I just happened to notice the offending TODO line ...
5 years, 4 months ago (2014-09-24 01:53:16 UTC) #2
lgtm https://webrtc-codereview.appspot.com/31459004/diff/20001/webrtc/modules/audio_processing/audio_buffer.cc File webrtc/modules/audio_processing/audio_buffer.cc (right): https://webrtc-codereview.appspot.com/31459004/diff/20001/webrtc/modules/audio_processing/audio_buffer.cc#newcode420 webrtc/modules/audio_processing/audio_buffer.cc:420: // to satisfy tests checking for bit-exactness with ...
5 years, 4 months ago (2014-09-24 07:24:50 UTC) #3
lgtm % some comments https://webrtc-codereview.appspot.com/31459004/diff/20001/webrtc/modules/audio_processing/audio_buffer.cc File webrtc/modules/audio_processing/audio_buffer.cc (right): https://webrtc-codereview.appspot.com/31459004/diff/20001/webrtc/modules/audio_processing/audio_buffer.cc#newcode417 webrtc/modules/audio_processing/audio_buffer.cc:417: for (int i = 0; ...
5 years, 4 months ago (2014-09-24 07:45:21 UTC) #4
lgtm, but can you add the failures of ApmTest.Process in the description? It is otherwise ...
5 years, 4 months ago (2014-09-24 09:20:43 UTC) #5
On 2014/09/24 09:20:43, Bjorn wrote: > lgtm, but can you add the failures of ApmTest.Process ...
5 years, 4 months ago (2014-09-24 11:59:47 UTC) #6
https://review.webrtc.org/31459004/diff/20001/webrtc/modules/audio_processing/audio_buffer.cc File webrtc/modules/audio_processing/audio_buffer.cc (right): https://review.webrtc.org/31459004/diff/20001/webrtc/modules/audio_processing/audio_buffer.cc#newcode414 webrtc/modules/audio_processing/audio_buffer.cc:414: if (num_input_channels_ == 2 && num_proc_channels_ == 1) { ...
5 years, 3 months ago (2014-09-24 19:20:23 UTC) #7
On 2014/09/24 09:20:43, Bjorn wrote: > lgtm, but can you add the failures of ApmTest.Process ...
5 years, 3 months ago (2014-09-24 19:30:49 UTC) #8
On 2014/09/24 11:59:47, Bjorn wrote: > BTW, can this have an impact on CommonFormats unittests? ...
5 years, 3 months ago (2014-09-24 19:32:00 UTC) #9
5 years, 3 months ago (2014-09-24 20:06:30 UTC) #10
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 7292 (presubmit successful).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted