Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(103)

Issue 2053773002: Keep track of the user-facing number of channels in a ChannelBuffer (Closed)

Created:
4 years, 6 months ago by aluebs-webrtc
Modified:
4 years, 5 months ago
CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, minyue-webrtc, the sun, bjornv1
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Keep track of the user-facing number of channels in a ChannelBuffer Before this change the ChannelBuffer had a fixed number of channels. This meant for example that when the Beamformer would reduce the number of channels to one, the merging filter bank was still merging all the channels, which was unnecessary since they were not processed and just discarded later. This change doesn't change the signal at all. It just reflects the number of channels in the ChannelBuffer, reducing the complexity. R=henrik.lundin@webrtc.org, peah@webrtc.org, tina.legrand@webrtc.org Committed: https://crrev.com/a181c9ad17dd749eaa384f48d5c953fcc66d435d Cr-Commit-Position: refs/heads/master@{#13352}

Patch Set 1 #

Total comments: 8

Patch Set 2 : Add unittests #

Total comments: 6

Patch Set 3 : Fix IFChannelBuffer::num_channels() #

Total comments: 9

Patch Set 4 : Add death tests #

Patch Set 5 : Rebasing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+158 lines, -14 lines) Patch
M webrtc/common_audio/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/common_audio/channel_buffer.h View 1 2 6 chunks +24 lines, -9 lines 0 comments Download
M webrtc/common_audio/channel_buffer.cc View 1 2 2 chunks +3 lines, -1 line 0 comments Download
A webrtc/common_audio/channel_buffer_unittest.cc View 1 2 3 1 chunk +65 lines, -0 lines 0 comments Download
M webrtc/common_audio/common_audio.gyp View 1 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/audio_processing/audio_buffer.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/audio_processing/audio_buffer.cc View 2 chunks +8 lines, -0 lines 0 comments Download
A webrtc/modules/audio_processing/audio_buffer_unittest.cc View 1 2 3 1 chunk +48 lines, -0 lines 0 comments Download
M webrtc/modules/audio_processing/splitting_filter.cc View 2 chunks +4 lines, -4 lines 0 comments Download
M webrtc/modules/modules.gyp View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 34 (11 generated)
aluebs-webrtc
4 years, 6 months ago (2016-06-09 19:15:34 UTC) #2
peah-webrtc
On 2016/06/09 19:15:34, aluebs-webrtc wrote: Thanks for making this a separate CL. The CL is, ...
4 years, 6 months ago (2016-06-10 11:16:49 UTC) #3
peah-webrtc
https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_buffer.cc File webrtc/common_audio/channel_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_buffer.cc#newcode57 webrtc/common_audio/channel_buffer.cc:57: fbuf_.set_num_channels(ibuf_.num_channels()); This seems wrong, similarly to what is described ...
4 years, 6 months ago (2016-06-10 11:39:35 UTC) #4
peah-webrtc
On 2016/06/10 11:39:35, peah-webrtc wrote: > https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_buffer.cc > File webrtc/common_audio/channel_buffer.cc (right): > > https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_buffer.cc#newcode57 > ...
4 years, 6 months ago (2016-06-10 11:40:25 UTC) #5
peah-webrtc
4 years, 6 months ago (2016-06-10 11:40:37 UTC) #6
aluebs-webrtc
Modified title, expanded description and added unittests. https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_buffer.cc File webrtc/common_audio/channel_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_buffer.cc#newcode57 webrtc/common_audio/channel_buffer.cc:57: fbuf_.set_num_channels(ibuf_.num_channels()); On ...
4 years, 6 months ago (2016-06-14 01:40:28 UTC) #8
aluebs-webrtc
peah, could you please have a look at this?
4 years, 6 months ago (2016-06-21 17:27:44 UTC) #9
peah-webrtc
https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/channel_buffer.cc File webrtc/common_audio/channel_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/channel_buffer.cc#newcode57 webrtc/common_audio/channel_buffer.cc:57: fbuf_.set_num_channels(ibuf_.num_channels()); Would it be possible to move this before ...
4 years, 6 months ago (2016-06-23 13:32:21 UTC) #10
aluebs-webrtc
https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/channel_buffer.cc File webrtc/common_audio/channel_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/channel_buffer.cc#newcode57 webrtc/common_audio/channel_buffer.cc:57: fbuf_.set_num_channels(ibuf_.num_channels()); On 2016/06/23 13:32:21, peah-webrtc wrote: > Would it ...
4 years, 6 months ago (2016-06-24 02:49:16 UTC) #11
peah-webrtc
On 2016/06/24 02:49:16, aluebs-webrtc wrote: > https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/channel_buffer.cc > File webrtc/common_audio/channel_buffer.cc (right): > > https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/channel_buffer.cc#newcode57 > ...
4 years, 6 months ago (2016-06-24 21:30:34 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2053773002/40001
4 years, 5 months ago (2016-06-27 16:06:47 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14516)
4 years, 5 months ago (2016-06-27 16:14:18 UTC) #16
aluebs-webrtc
tina.legrand, could you please have a look at common_audio/ and modules/. Thank you very much!
4 years, 5 months ago (2016-06-27 16:42:46 UTC) #18
tlegrand-webrtc
I'm missing a few tests, to check that we take care of misuse of the ...
4 years, 5 months ago (2016-06-28 13:48:05 UTC) #19
tlegrand-webrtc
On 2016/06/28 13:48:05, tlegrand-webrtc wrote: > I'm missing a few tests, to check that we ...
4 years, 5 months ago (2016-06-28 13:48:53 UTC) #20
aluebs-webrtc
https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/channel_buffer.h File webrtc/common_audio/channel_buffer.h (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/channel_buffer.h#newcode125 webrtc/common_audio/channel_buffer.h:125: RTC_DCHECK_LE(num_channels, num_allocated_channels_); On 2016/06/28 13:48:04, tlegrand-webrtc wrote: > This ...
4 years, 5 months ago (2016-06-29 00:48:40 UTC) #21
hlundin-webrtc
https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_processing/audio_buffer_unittest.cc File webrtc/modules/audio_processing/audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_processing/audio_buffer_unittest.cc#newcode26 webrtc/modules/audio_processing/audio_buffer_unittest.cc:26: } // namespace On 2016/06/29 00:48:40, aluebs-webrtc wrote: > ...
4 years, 5 months ago (2016-06-29 08:21:16 UTC) #23
aluebs-webrtc
https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/channel_buffer_unittest.cc File webrtc/common_audio/channel_buffer_unittest.cc (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/channel_buffer_unittest.cc#newcode25 webrtc/common_audio/channel_buffer_unittest.cc:25: On 2016/06/29 00:48:40, aluebs-webrtc wrote: > On 2016/06/28 13:48:04, ...
4 years, 5 months ago (2016-06-29 23:02:58 UTC) #24
tlegrand-webrtc
On 2016/06/29 23:02:58, aluebs-webrtc wrote: > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/channel_buffer_unittest.cc > File webrtc/common_audio/channel_buffer_unittest.cc (right): > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/channel_buffer_unittest.cc#newcode25 > ...
4 years, 5 months ago (2016-06-30 06:54:27 UTC) #25
hlundin-webrtc
LGTM for the death tests specifically.
4 years, 5 months ago (2016-06-30 09:04:05 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2053773002/60001
4 years, 5 months ago (2016-06-30 14:58:02 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14689)
4 years, 5 months ago (2016-06-30 15:10:55 UTC) #31
aluebs-webrtc
4 years, 5 months ago (2016-06-30 22:33:58 UTC) #33
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as
a181c9ad17dd749eaa384f48d5c953fcc66d435d (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698