WebRTC Code Reviews
Help | Chromium Project | Sign in
(8510)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by ajm
Modified:
4 years, 11 months ago
Reviewers:
bjornv, aluebs, kwiberg, Bjorn
CC:
webrtc-reviews_webrtc.org, rillian-moz, tterriberry, kwiberg, aluebs
Base URL:
https://webrtc.googlecode.com/svn/trunk
Visibility:
Public.

Description

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. #

Messages

Total messages: 10 (0 generated)
ajm
As you can guess, we should really get this into M39. So please have a ...
4 years, 11 months ago (2014-09-24 01:52:06 UTC) #1
ajm
...and to give you some background, I just happened to notice the offending TODO line ...
4 years, 11 months ago (2014-09-24 01:53:16 UTC) #2
kwiberg
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 ...
4 years, 11 months ago (2014-09-24 07:24:50 UTC) #3
aluebs
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; ...
4 years, 11 months ago (2014-09-24 07:45:21 UTC) #4
Bjorn
lgtm, but can you add the failures of ApmTest.Process in the description? It is otherwise ...
4 years, 11 months ago (2014-09-24 09:20:43 UTC) #5
Bjorn
On 2014/09/24 09:20:43, Bjorn wrote: > lgtm, but can you add the failures of ApmTest.Process ...
4 years, 11 months ago (2014-09-24 11:59:47 UTC) #6
ajm
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) { ...
4 years, 11 months ago (2014-09-24 19:20:23 UTC) #7
ajm
On 2014/09/24 09:20:43, Bjorn wrote: > lgtm, but can you add the failures of ApmTest.Process ...
4 years, 11 months ago (2014-09-24 19:30:49 UTC) #8
ajm
On 2014/09/24 11:59:47, Bjorn wrote: > BTW, can this have an impact on CommonFormats unittests? ...
4 years, 11 months ago (2014-09-24 19:32:00 UTC) #9
ajm
4 years, 11 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