|
|
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. |
DescriptionKeep 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 #
Messages
Total messages: 34 (11 generated)
aluebs@webrtc.org changed reviewers: + peah@webrtc.org
On 2016/06/09 19:15:34, aluebs-webrtc wrote: Thanks for making this a separate CL. The CL is, however, missing context, purpose and description which makes it very hard for anyone not fully up to date with the code to judge what it does. Please: 1) Change the title of the CL to describe what it does. It should also be a proper sentence, with some further context in order to allow for brief browsing of the changelog. 2) Add a motivation to the changes in the CL in the description. 3) Add a description to what the effect of the CL on the signal will be (in this case, lower complexity, but bitexact behavior).
https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... File webrtc/common_audio/channel_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... webrtc/common_audio/channel_buffer.cc:57: fbuf_.set_num_channels(ibuf_.num_channels()); This seems wrong, similarly to what is described below. https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... webrtc/common_audio/channel_buffer.cc:72: ibuf_.set_num_channels(fbuf_.num_channels()); This seems wrong: Afaics: 1) First ibuf_.num_channels() channels are converted from float to int. 2) Then the number of channels are set to fbuf_.num_channels() in ibuf_.set_num_channels(fbuf_.num_channels()); I guess the above works, because by the usage of ibuf_ it is guaranteed that the preallocated number of channels is sufficiently large for both ibuf and fbuf. But this needs to be corrected regardless of that. Furthermore, if fbuf_ has 1 channel (but 2 allocated internally), and ibuf_ has 2 channels, this is twice as complex as required, right? https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... File webrtc/common_audio/channel_buffer.h (right): https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... webrtc/common_audio/channel_buffer.h:141: const size_t num_channels_; As it is now, both set_num_channels() and num_channels() return num_official_channels_ which is a bit confusing as the method names gives the feeling that they return num_channels_ Why not rename num_channels_ to something else (num_allocated_channels_?) and rename num_official_channels_ to num_channels_? Or, alternatively, but in my mind not as good, rename num_channels_ to something else (num_allocated_channels_?) and keep the name num_official_channels_. WDYT? https://codereview.webrtc.org/2053773002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_buffer.cc:351: num_channels_ = num_channels; Why is only num_channels_ updated here, and not num_proc_channels_? Both are set to num_process_channels during creation, but only num_channels_ is changed at the call to set_num_channels. Is that really as it should?
On 2016/06/10 11:39:35, peah-webrtc wrote: > https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... > File webrtc/common_audio/channel_buffer.cc (right): > > https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... > webrtc/common_audio/channel_buffer.cc:57: > fbuf_.set_num_channels(ibuf_.num_channels()); > This seems wrong, similarly to what is described below. > > https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... > webrtc/common_audio/channel_buffer.cc:72: > ibuf_.set_num_channels(fbuf_.num_channels()); > This seems wrong: > Afaics: > 1) First ibuf_.num_channels() channels are converted from float to int. > 2) Then the number of channels are set to fbuf_.num_channels() in > ibuf_.set_num_channels(fbuf_.num_channels()); > > I guess the above works, because by the usage of ibuf_ it is guaranteed that the > preallocated number of channels is sufficiently large for both ibuf and fbuf. > But this needs to be corrected regardless of that. > > Furthermore, if fbuf_ has 1 channel (but 2 allocated internally), and ibuf_ has > 2 channels, this is twice as complex as required, right? > > https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... > File webrtc/common_audio/channel_buffer.h (right): > > https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... > webrtc/common_audio/channel_buffer.h:141: const size_t num_channels_; > As it is now, both set_num_channels() and num_channels() return > num_official_channels_ which is a bit confusing as the method names gives the > feeling that they return num_channels_ > > Why not rename num_channels_ to something else (num_allocated_channels_?) and > rename num_official_channels_ to num_channels_? > > Or, alternatively, but in my mind not as good, rename num_channels_ to something > else (num_allocated_channels_?) and keep the name num_official_channels_. > > WDYT? > > https://codereview.webrtc.org/2053773002/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/audio_buffer.cc (right): > > https://codereview.webrtc.org/2053773002/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/audio_buffer.cc:351: num_channels_ = > num_channels; > Why is only num_channels_ updated here, and not num_proc_channels_? > > Both are set to num_process_channels during creation, but only num_channels_ is > changed at the call to set_num_channels. > Is that really as it should? Another general comment, is this code covered by unit tests?
Description was changed from ========== Also set_num_channels in ChannelBuffer when the AudioBuffer gets modified ========== to ========== 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. ==========
Modified title, expanded description and added unittests. https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... File webrtc/common_audio/channel_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... webrtc/common_audio/channel_buffer.cc:57: fbuf_.set_num_channels(ibuf_.num_channels()); On 2016/06/10 11:39:34, peah-webrtc wrote: > This seems wrong, similarly to what is described below. This is not similar to below. https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... webrtc/common_audio/channel_buffer.cc:72: ibuf_.set_num_channels(fbuf_.num_channels()); On 2016/06/10 11:39:34, peah-webrtc wrote: > This seems wrong: > Afaics: > 1) First ibuf_.num_channels() channels are converted from float to int. > 2) Then the number of channels are set to fbuf_.num_channels() in > ibuf_.set_num_channels(fbuf_.num_channels()); > > I guess the above works, because by the usage of ibuf_ it is guaranteed that the > preallocated number of channels is sufficiently large for both ibuf and fbuf. > But this needs to be corrected regardless of that. > > Furthermore, if fbuf_ has 1 channel (but 2 allocated internally), and ibuf_ has > 2 channels, this is twice as complex as required, right? > Good catch. This was not an issue before since they were fixed and the same, but now it is. I changed the for-loop to only go over fbuf_.num_channels(). https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... File webrtc/common_audio/channel_buffer.h (right): https://codereview.webrtc.org/2053773002/diff/1/webrtc/common_audio/channel_b... webrtc/common_audio/channel_buffer.h:141: const size_t num_channels_; On 2016/06/10 11:39:34, peah-webrtc wrote: > As it is now, both set_num_channels() and num_channels() return > num_official_channels_ which is a bit confusing as the method names gives the > feeling that they return num_channels_ > > Why not rename num_channels_ to something else (num_allocated_channels_?) and > rename num_official_channels_ to num_channels_? > > Or, alternatively, but in my mind not as good, rename num_channels_ to something > else (num_allocated_channels_?) and keep the name num_official_channels_. > > WDYT? Done. https://codereview.webrtc.org/2053773002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_buffer.cc:351: num_channels_ = num_channels; On 2016/06/10 11:39:34, peah-webrtc wrote: > Why is only num_channels_ updated here, and not num_proc_channels_? > > Both are set to num_process_channels during creation, but only num_channels_ is > changed at the call to set_num_channels. > Is that really as it should? Yes, num_proc_channels_ is a constant and its value represents the number of channels which the first processing components sees and is needed to tell the AudioBuffer if downmixing is necessary in CopyFrom. This depends on the input and output number of channels. On the other hand, num_channels_ is a variable and tracks the current number of channels. It happens to coincide with num_proc_channels_ for the first processing component, but can be dynamically modified (beamforming for example).
peah, could you please have a look at this?
https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... File webrtc/common_audio/channel_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... webrtc/common_audio/channel_buffer.cc:57: fbuf_.set_num_channels(ibuf_.num_channels()); Would it be possible to move this before the copy? As it is now, the DCHECK for the valid number of channels that occurs inside set_num_channels will happen after the sample copy, which means that the DCHECK does not properly protect agains incorrect buffer usage. https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... webrtc/common_audio/channel_buffer.cc:72: ibuf_.set_num_channels(fbuf_.num_channels()); Would it be possible to move this before the copy? As it is now, the DCHECK for the valid number of channels that occurs inside set_num_channels will happen after the sample copy, which means that the DCHECK does not properly protect agains incorrect buffer usage. https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... File webrtc/common_audio/channel_buffer.h (right): https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... webrtc/common_audio/channel_buffer.h:165: return std::min(ibuf_.num_channels(), fbuf_.num_channels()); Why should not these number of channels be equal? If they were not equal, how can you tell that the smallest number is correct? If they should be equal, would it not be better to add a DCHECK for that, and return only one of the num_channels?
https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... File webrtc/common_audio/channel_buffer.cc (right): https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... 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 be possible to move this before the copy? As it is now, the DCHECK for > the valid number of channels that occurs inside set_num_channels will happen > after the sample copy, which means that the DCHECK does not properly protect > agains incorrect buffer usage. Good point, although it should never happen. Done. https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... webrtc/common_audio/channel_buffer.cc:72: ibuf_.set_num_channels(fbuf_.num_channels()); On 2016/06/23 13:32:21, peah-webrtc wrote: > Would it be possible to move this before the copy? As it is now, the DCHECK for > the valid number of channels that occurs inside set_num_channels will happen > after the sample copy, which means that the DCHECK does not properly protect > agains incorrect buffer usage. Good point, although it should never happen. Done. https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... File webrtc/common_audio/channel_buffer.h (right): https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... webrtc/common_audio/channel_buffer.h:165: return std::min(ibuf_.num_channels(), fbuf_.num_channels()); On 2016/06/23 13:32:21, peah-webrtc wrote: > Why should not these number of channels be equal? If they were not equal, how > can you tell that the smallest number is correct? > If they should be equal, would it not be better to add a DCHECK for that, and > return only one of the num_channels? If one of the ChannelBuffers is accessed and the number of channels is changed they can differ until the other one is accessed and it is set correctly in Refresh*(). But you have a great point that not always the lowest is the correct one, that is only the case now with beamforming where we only go from N channels to 1 and not in the other direction. Fixed it.
On 2016/06/24 02:49:16, aluebs-webrtc wrote: > https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... > File webrtc/common_audio/channel_buffer.cc (right): > > https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... > 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 be possible to move this before the copy? As it is now, the DCHECK > for > > the valid number of channels that occurs inside set_num_channels will happen > > after the sample copy, which means that the DCHECK does not properly protect > > agains incorrect buffer usage. > > Good point, although it should never happen. Done. > > https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... > webrtc/common_audio/channel_buffer.cc:72: > ibuf_.set_num_channels(fbuf_.num_channels()); > On 2016/06/23 13:32:21, peah-webrtc wrote: > > Would it be possible to move this before the copy? As it is now, the DCHECK > for > > the valid number of channels that occurs inside set_num_channels will happen > > after the sample copy, which means that the DCHECK does not properly protect > > agains incorrect buffer usage. > > Good point, although it should never happen. Done. > > https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... > File webrtc/common_audio/channel_buffer.h (right): > > https://codereview.webrtc.org/2053773002/diff/20001/webrtc/common_audio/chann... > webrtc/common_audio/channel_buffer.h:165: return std::min(ibuf_.num_channels(), > fbuf_.num_channels()); > On 2016/06/23 13:32:21, peah-webrtc wrote: > > Why should not these number of channels be equal? If they were not equal, how > > can you tell that the smallest number is correct? > > If they should be equal, would it not be better to add a DCHECK for that, and > > return only one of the num_channels? > > If one of the ChannelBuffers is accessed and the number of channels is changed > they can differ until the other one is accessed and it is set correctly in > Refresh*(). But you have a great point that not always the lowest is the correct > one, that is only the case now with beamforming where we only go from N channels > to 1 and not in the other direction. Fixed it. lgtm
The CQ bit was checked by aluebs@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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)
aluebs@webrtc.org changed reviewers: + tina.legrand@webrtc.org
tina.legrand, could you please have a look at common_audio/ and modules/. Thank you very much!
I'm missing a few tests, to check that we take care of misuse of the new setter. https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... File webrtc/common_audio/channel_buffer.h (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... webrtc/common_audio/channel_buffer.h:125: RTC_DCHECK_LE(num_channels, num_allocated_channels_); This can never happen, unless someone do a programming error? https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... File webrtc/common_audio/channel_buffer_unittest.cc (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... webrtc/common_audio/channel_buffer_unittest.cc:25: You haven't added any tests where you try to break things, like setting a larger number of channels than what the Channel was configured for. https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_buffer_unittest.cc:26: } // namespace Fault tests?
On 2016/06/28 13:48:05, tlegrand-webrtc wrote: > I'm missing a few tests, to check that we take care of misuse of the new setter. > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... > File webrtc/common_audio/channel_buffer.h (right): > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... > webrtc/common_audio/channel_buffer.h:125: RTC_DCHECK_LE(num_channels, > num_allocated_channels_); > This can never happen, unless someone do a programming error? > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... > File webrtc/common_audio/channel_buffer_unittest.cc (right): > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... > webrtc/common_audio/channel_buffer_unittest.cc:25: > You haven't added any tests where you try to break things, like setting a larger > number of channels than what the Channel was configured for. > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_buffer_unittest.cc (right): > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_buffer_unittest.cc:26: } // namespace > Fault tests? Maybe it isn't possible to add such tests?
https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... File webrtc/common_audio/channel_buffer.h (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... 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 can never happen, unless someone do a programming error? As of today it can't and it never should if there is no bug introduced. https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... File webrtc/common_audio/channel_buffer_unittest.cc (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... webrtc/common_audio/channel_buffer_unittest.cc:25: On 2016/06/28 13:48:04, tlegrand-webrtc wrote: > You haven't added any tests where you try to break things, like setting a larger > number of channels than what the Channel was configured for. I don't think there is a way to catch DCHECKs, right? https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_buffer_unittest.cc:26: } // namespace On 2016/06/28 13:48:05, tlegrand-webrtc wrote: > Fault tests? I don't think there is a way to catch DCHECKs, right?
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_buffer_unittest.cc:26: } // namespace On 2016/06/29 00:48:40, aluebs-webrtc wrote: > On 2016/06/28 13:48:05, tlegrand-webrtc wrote: > > Fault tests? > > I don't think there is a way to catch DCHECKs, right? Yes, there is. You can use death tests. See https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid..., and grep our code base for ASSERT_DEATH or EXPECT_DEATH.
https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... File webrtc/common_audio/channel_buffer_unittest.cc (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... 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, tlegrand-webrtc wrote: > > You haven't added any tests where you try to break things, like setting a > larger > > number of channels than what the Channel was configured for. > > I don't think there is a way to catch DCHECKs, right? Now I added death tests. https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_buffer_unittest.cc:26: } // namespace On 2016/06/29 08:21:16, hlundin-webrtc wrote: > On 2016/06/29 00:48:40, aluebs-webrtc wrote: > > On 2016/06/28 13:48:05, tlegrand-webrtc wrote: > > > Fault tests? > > > > I don't think there is a way to catch DCHECKs, right? > > Yes, there is. You can use death tests. See > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid..., > and grep our code base for ASSERT_DEATH or EXPECT_DEATH. Thank you for pointing that out. Now I added death tests.
On 2016/06/29 23:02:58, aluebs-webrtc wrote: > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... > File webrtc/common_audio/channel_buffer_unittest.cc (right): > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/common_audio/chann... > 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, tlegrand-webrtc wrote: > > > You haven't added any tests where you try to break things, like setting a > > larger > > > number of channels than what the Channel was configured for. > > > > I don't think there is a way to catch DCHECKs, right? > > Now I added death tests. > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_buffer_unittest.cc (right): > > https://codereview.webrtc.org/2053773002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_buffer_unittest.cc:26: } // namespace > On 2016/06/29 08:21:16, hlundin-webrtc wrote: > > On 2016/06/29 00:48:40, aluebs-webrtc wrote: > > > On 2016/06/28 13:48:05, tlegrand-webrtc wrote: > > > > Fault tests? > > > > > > I don't think there is a way to catch DCHECKs, right? > > > > Yes, there is. You can use death tests. See > > > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid..., > > and grep our code base for ASSERT_DEATH or EXPECT_DEATH. > > Thank you for pointing that out. Now I added death tests. Great! LGTM
LGTM for the death tests specifically.
The CQ bit was checked by aluebs@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/2053773002/#ps60001 (title: "Add death tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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)
Description was changed from ========== 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. ========== to ========== 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://chromium.googlesource.com/external/webrtc/+/a181c9ad17dd749eaa384f48d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as a181c9ad17dd749eaa384f48d5c953fcc66d435d (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== 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://chromium.googlesource.com/external/webrtc/+/a181c9ad17dd749eaa384f48d... ========== to ========== 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} ========== |