Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 9 months ago by aleloi Modified:
3 years, 8 months ago Reviewers:
ivoc, hlundin-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, aleloi Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSend data from mixer to APM limiter more often.
Before this change, the APM limiter used in FrameCombiner (a
sub-component of AudioMixer) only gets to process the data when the
number of non-muted streams is >1. If this number varies between <=1
and >1, the limiter's view of the data will have gaps during the
periods with <= 1 active stream.
This leads to discontinuities in the applied gain. These
discontinuities cause clicks in the output audio. This change
activates APM limiter processing based on the number of audio streams,
independently of their mutedness status.
BUG=chromium:695993
Review-Url: https://codereview.webrtc.org/2776113002
Cr-Commit-Position: refs/heads/master@{#17442}
Committed: https://chromium.googlesource.com/external/webrtc/+/2c9306ed50190a60be4dfa037d7c8781b0004a60
Patch Set 1 #Patch Set 2 : wip #
Total comments: 6
Patch Set 3 : Add tests and address comments. #Patch Set 4 : Test 1 not needed. #
Total comments: 12
Patch Set 5 : Addressed comments, divided into files. #Patch Set 6 : Fxed GN dependency issue; passes GN checks. #Patch Set 7 : Conversions due to Win warnings. #
Total comments: 20
Patch Set 8 : Minor changes in response to hlundin@'s comments. #
Created: 3 years, 8 months ago
Messages
Total messages: 48 (35 generated)
aleloi@webrtc.org changed reviewers: + ivoc@webrtc.org
PTAL!
LGTM, see nits below. https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner.cc:39: // Here it's no need to apply half gain, since frame is zero anyway. Here there's no need... ... since the frame is zero anyway. https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner.h (right): https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner.h:33: int number_of_streams, Please add a comment to explain this new parameter. Also, you could make this a size_t, like number_of_channels.
On 2017/03/27 15:44:55, ivoc wrote: > LGTM, see nits below. > > https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... > File webrtc/modules/audio_mixer/frame_combiner.cc (right): > > https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... > webrtc/modules/audio_mixer/frame_combiner.cc:39: // Here it's no need to apply > half gain, since frame is zero anyway. > Here there's no need... > ... since the frame is zero anyway. > > https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... > File webrtc/modules/audio_mixer/frame_combiner.h (right): > > https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... > webrtc/modules/audio_mixer/frame_combiner.h:33: int number_of_streams, > Please add a comment to explain this new parameter. Also, you could make this a > size_t, like number_of_channels. Please, append chromium: to the bug number.
On 2017/03/28 12:05:47, hlundin-webrtc wrote: > On 2017/03/27 15:44:55, ivoc wrote: > > LGTM, see nits below. > > > > > https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... > > File webrtc/modules/audio_mixer/frame_combiner.cc (right): > > > > > https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... > > webrtc/modules/audio_mixer/frame_combiner.cc:39: // Here it's no need to apply > > half gain, since frame is zero anyway. > > Here there's no need... > > ... since the frame is zero anyway. > > > > > https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... > > File webrtc/modules/audio_mixer/frame_combiner.h (right): > > > > > https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... > > webrtc/modules/audio_mixer/frame_combiner.h:33: int number_of_streams, > > Please add a comment to explain this new parameter. Also, you could make this > a > > size_t, like number_of_channels. > > Please, append chromium: to the bug number. s/append/prepend/
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Drive-by LGTM with same nits as ivoc@. And fix the bug number. https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner.h (right): https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner.h:33: int number_of_streams, On 2017/03/27 15:44:55, ivoc wrote: > Please add a comment to explain this new parameter. Also, you could make this a > size_t, like number_of_channels. +1 for size_t.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Send data from mixer to APM limiter more often. Before this change, the APM limiter used in FrameCombiner (a sub-component of AudioMixer) only gets to process the data when the number of non-muted streams is >1. If this number varies between <=1 and >1, the limiter's view of the data will have gaps during the periods with <= 1 active stream. This leads to discontinuities in the applied gain. These discontinuities cause clicks in the output audio. This change activates APM limiter processing based on the number of audio streams, independently of their mutedness status. BUG=695993 ========== to ========== Send data from mixer to APM limiter more often. Before this change, the APM limiter used in FrameCombiner (a sub-component of AudioMixer) only gets to process the data when the number of non-muted streams is >1. If this number varies between <=1 and >1, the limiter's view of the data will have gaps during the periods with <= 1 active stream. This leads to discontinuities in the applied gain. These discontinuities cause clicks in the output audio. This change activates APM limiter processing based on the number of audio streams, independently of their mutedness status. BUG=chromium:695993 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23105)
Hi! I addressed the comments and added a test. Sorry for 200 LOC more after lg. https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner.cc:39: // Here it's no need to apply half gain, since frame is zero anyway. On 2017/03/27 15:44:55, ivoc wrote: > Here there's no need... > ... since the frame is zero anyway. Done. https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner.h (right): https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner.h:33: int number_of_streams, On 2017/03/27 15:44:55, ivoc wrote: > Please add a comment to explain this new parameter. Also, you could make this a > size_t, like number_of_channels. Done.
Some more comments. https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2776113002/diff/20001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner.cc:39: // Here it's no need to apply half gain, since frame is zero anyway. On 2017/03/28 14:49:05, aleloi wrote: > On 2017/03/27 15:44:55, ivoc wrote: > > Here there's no need... > > ... since the frame is zero anyway. > > Done. Is it? https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner_test_tools.cc (right): https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.cc:26: void SineWaveGenerator::GenerateNextFrame(AudioFrame* frame) { Please add DCHECK(frame) https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.cc:30: rtc::saturated_cast<int16_t>(amplitude_ * sin(phase_)); sinf, sin is for doubles. https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.cc:54: if (std::abs(in[i]) >= kReliabilityThreshold) { Won't this cause some discontinuities in the calculated gain? Or is it not a problem in practice? https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.cc:65: res += std::abs(f - last_value_); fabs, abs is for ints. https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner_test_tools.h (right): https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.h:20: class SineWaveGenerator { Please move to sine_wave_generator.{cc,h} https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.h:37: class GainChangeCalculator { And this to gain_change_calculator.{cc,h} https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner_unittest.cc (right): https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:162: {50, 100, 200, 400, 800, 1600, 3200}) { I don't think it's really necessary to test every single parameter combination in this test (unless it's really fast?). I would be fine with testing only the most common values.
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #5 (id:120001) has been deleted
Thanks, uploaded new version with comments addressed. https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner_test_tools.cc (right): https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.cc:30: rtc::saturated_cast<int16_t>(amplitude_ * sin(phase_)); On 2017/03/28 16:11:06, ivoc wrote: > sinf, sin is for doubles. Done. https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.cc:54: if (std::abs(in[i]) >= kReliabilityThreshold) { On 2017/03/28 16:11:06, ivoc wrote: > Won't this cause some discontinuities in the calculated gain? Or is it not a > problem in practice? If the threshold increases, the resulted sum of differences can only decrease. With higher threshold the test will be easier to pass, not harder. That may be a problem with the test, though. https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.cc:65: res += std::abs(f - last_value_); On 2017/03/28 16:11:06, ivoc wrote: > fabs, abs is for ints. I checked that this returns floats. fabs seems more safe, though. http://en.cppreference.com/w/cpp/numeric/math/fabs https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/frame_combiner_test_tools.h (right): https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.h:20: class SineWaveGenerator { On 2017/03/28 16:11:06, ivoc wrote: > Please move to sine_wave_generator.{cc,h} Done. https://codereview.webrtc.org/2776113002/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/frame_combiner_test_tools.h:37: class GainChangeCalculator { On 2017/03/28 16:11:06, ivoc wrote: > And this to gain_change_calculator.{cc,h} Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/18811) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24300) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23116) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/23404) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24306)
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/20117) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/24781)
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Thanks for making a test! A few minor comments. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner_unittest.cc (right): https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:45: ss << "limiter active: " << limiter_active << " ,"; Does the bool translate to something human-readable? https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:149: // Send a sine curve through the FrameCombiner, and check that the curve -> wave imho https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:161: const int number_of_channels = 2; constexpr (I guess you are preparing to iterate over number of channels too in the future, but let's cross that bridge when we come to it. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:176: for (int i = 0; i < iterations; ++i) { This is a bit of a mixture. int i and int16_t iterations. Suggest you make both size_t. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:189: const size_t number_of_streams = 2; constexpr https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/gain_change_calculator.cc (right): https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/gain_change_calculator.cc:13: #include <cmath> math.h We tend to use the C-version of a header that is available both as file.c and cfile. I don't really know why, though. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/gain_change_calculator.cc:29: CalculateGain(in, out, gain_view); std::vector should implicitly convert to rtc::ArrayView. You should be able to drop the local ArrayView variable and just pass gain into CalculateGain. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/gain_change_calculator.cc:30: return CalculateDifferences(gain_view); ... and here. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/sine_wave_generator.cc (right): https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/sine_wave_generator.cc:11: #include <cmath> math.h https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/sine_wave_generator.cc:14: #include "webrtc/modules/audio_mixer/sine_wave_generator.h" This one goes first of all.
The CQ bit was checked by aleloi@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner_unittest.cc (right): https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:45: ss << "limiter active: " << limiter_active << " ,"; On 2017/03/29 07:22:55, hlundin-webrtc wrote: > Does the bool translate to something human-readable? Now it does! (thx) https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:149: // Send a sine curve through the FrameCombiner, and check that the On 2017/03/29 07:22:55, hlundin-webrtc wrote: > curve -> wave > imho Done. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:161: const int number_of_channels = 2; On 2017/03/29 07:22:55, hlundin-webrtc wrote: > constexpr > (I guess you are preparing to iterate over number of channels too in the future, > but let's cross that bridge when we come to it. Done. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:176: for (int i = 0; i < iterations; ++i) { On 2017/03/29 07:22:55, hlundin-webrtc wrote: > This is a bit of a mixture. int i and int16_t iterations. Suggest you make both > size_t. Oh, there is no reason for iterations to be int16_t. Thanks! https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:189: const size_t number_of_streams = 2; On 2017/03/29 07:22:55, hlundin-webrtc wrote: > constexpr Done. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/gain_change_calculator.cc (right): https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/gain_change_calculator.cc:13: #include <cmath> On 2017/03/29 07:22:56, hlundin-webrtc wrote: > math.h > We tend to use the C-version of a header that is available both as file.c and > cfile. I don't really know why, though. Done. Consequence: cmath seems to put a 'fabs' in namespace std, the C-version doesn't have namespaces. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/gain_change_calculator.cc:29: CalculateGain(in, out, gain_view); On 2017/03/29 07:22:56, hlundin-webrtc wrote: > std::vector should implicitly convert to rtc::ArrayView. You should be able to > drop the local ArrayView variable and just pass gain into CalculateGain. Done. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/gain_change_calculator.cc:30: return CalculateDifferences(gain_view); On 2017/03/29 07:22:55, hlundin-webrtc wrote: > ... and here. Done. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/sine_wave_generator.cc (right): https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/sine_wave_generator.cc:11: #include <cmath> On 2017/03/29 07:22:56, hlundin-webrtc wrote: > math.h Done. https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/sine_wave_generator.cc:14: #include "webrtc/modules/audio_mixer/sine_wave_generator.h" On 2017/03/29 07:22:56, hlundin-webrtc wrote: > This one goes first of all. Done.
On 2017/03/29 10:56:11, aleloi wrote: > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > File webrtc/modules/audio_mixer/frame_combiner_unittest.cc (right): > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/frame_combiner_unittest.cc:45: ss << "limiter active: > " << limiter_active << " ,"; > On 2017/03/29 07:22:55, hlundin-webrtc wrote: > > Does the bool translate to something human-readable? > > Now it does! (thx) > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/frame_combiner_unittest.cc:149: // Send a sine curve > through the FrameCombiner, and check that the > On 2017/03/29 07:22:55, hlundin-webrtc wrote: > > curve -> wave > > imho > > Done. > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/frame_combiner_unittest.cc:161: const int > number_of_channels = 2; > On 2017/03/29 07:22:55, hlundin-webrtc wrote: > > constexpr > > (I guess you are preparing to iterate over number of channels too in the > future, > > but let's cross that bridge when we come to it. > > Done. > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/frame_combiner_unittest.cc:176: for (int i = 0; i < > iterations; ++i) { > On 2017/03/29 07:22:55, hlundin-webrtc wrote: > > This is a bit of a mixture. int i and int16_t iterations. Suggest you make > both > > size_t. > > Oh, there is no reason for iterations to be int16_t. Thanks! > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/frame_combiner_unittest.cc:189: const size_t > number_of_streams = 2; > On 2017/03/29 07:22:55, hlundin-webrtc wrote: > > constexpr > > Done. > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > File webrtc/modules/audio_mixer/gain_change_calculator.cc (right): > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/gain_change_calculator.cc:13: #include <cmath> > On 2017/03/29 07:22:56, hlundin-webrtc wrote: > > math.h > > We tend to use the C-version of a header that is available both as file.c and > > cfile. I don't really know why, though. > > Done. Consequence: cmath seems to put a 'fabs' in namespace std, the C-version > doesn't have namespaces. > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/gain_change_calculator.cc:29: CalculateGain(in, out, > gain_view); > On 2017/03/29 07:22:56, hlundin-webrtc wrote: > > std::vector should implicitly convert to rtc::ArrayView. You should be able to > > drop the local ArrayView variable and just pass gain into CalculateGain. > > Done. > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/gain_change_calculator.cc:30: return > CalculateDifferences(gain_view); > On 2017/03/29 07:22:55, hlundin-webrtc wrote: > > ... and here. > > Done. > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > File webrtc/modules/audio_mixer/sine_wave_generator.cc (right): > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/sine_wave_generator.cc:11: #include <cmath> > On 2017/03/29 07:22:56, hlundin-webrtc wrote: > > math.h > > Done. > > https://codereview.webrtc.org/2776113002/diff/200001/webrtc/modules/audio_mix... > webrtc/modules/audio_mixer/sine_wave_generator.cc:14: #include > "webrtc/modules/audio_mixer/sine_wave_generator.h" > On 2017/03/29 07:22:56, hlundin-webrtc wrote: > > This one goes first of all. > > Done. LGTM!
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2776113002/#ps220001 (title: "Minor changes in response to hlundin@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490786579082420,
"parent_rev": "3c5ec8d9185a2986c3dfc0bfe7202a1ed5bc5829", "commit_rev":
"2c9306ed50190a60be4dfa037d7c8781b0004a60"}
Message was sent while issue was closed.
Description was changed from ========== Send data from mixer to APM limiter more often. Before this change, the APM limiter used in FrameCombiner (a sub-component of AudioMixer) only gets to process the data when the number of non-muted streams is >1. If this number varies between <=1 and >1, the limiter's view of the data will have gaps during the periods with <= 1 active stream. This leads to discontinuities in the applied gain. These discontinuities cause clicks in the output audio. This change activates APM limiter processing based on the number of audio streams, independently of their mutedness status. BUG=chromium:695993 ========== to ========== Send data from mixer to APM limiter more often. Before this change, the APM limiter used in FrameCombiner (a sub-component of AudioMixer) only gets to process the data when the number of non-muted streams is >1. If this number varies between <=1 and >1, the limiter's view of the data will have gaps during the periods with <= 1 active stream. This leads to discontinuities in the applied gain. These discontinuities cause clicks in the output audio. This change activates APM limiter processing based on the number of audio streams, independently of their mutedness status. BUG=chromium:695993 Review-Url: https://codereview.webrtc.org/2776113002 Cr-Commit-Position: refs/heads/master@{#17442} Committed: https://chromium.googlesource.com/external/webrtc/+/2c9306ed50190a60be4dfa037... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001) as https://chromium.googlesource.com/external/webrtc/+/2c9306ed50190a60be4dfa037... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
