Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2692333002:
Optionally disable APM limiter in AudioMixer. (Closed)
DescriptionOptionally disable APM limiter in AudioMixer.
The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping).
To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner.
The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter
Inside of FrameCombiner, the meta-data handling and the audio sample addition are kept divided from each other.
This also fixes a few minor GN issues so that warnings do not have to be suppressed.
BUG=webrtc:7167
Review-Url: https://codereview.webrtc.org/2692333002
Cr-Commit-Position: refs/heads/master@{#16742}
Committed: https://chromium.googlesource.com/external/webrtc/+/24899e58ecb10157fea418d80022b938c0f806a9
Patch Set 1 : Deprecated old factory method. #
Total comments: 35
Patch Set 2 : Mini-change: participant -> source and for each loop. #
Total comments: 28
Patch Set 3 : Fix int <-> size_t conversion warning. #
Total comments: 6
Patch Set 4 : UpdateFrame to initialize data in AudioFrame. #
Total comments: 3
Patch Set 5 : Fix int16_t <-> size_t compilation warnings. #
Messages
Total messages: 63 (49 generated)
Description was changed from ========== TODO(aleloi): Structure this in better way. New limiting/mixing structure: initial upload. New limiting/mixing structure: initial upload. New limiter: initial upload. Initial commit (doesn't even compile) of mixer infrastructure changes (for float/int32 mixing and limiter experiments). BUG= ========== to ========== TODO(aleloi): Write better summary. New limiting/mixing structure: initial upload. BUG= ==========
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13474)
Description was changed from ========== TODO(aleloi): Write better summary. New limiting/mixing structure: initial upload. BUG= ========== to ========== TODO(aleloi): Write better summary. New limiting/mixing structure: initial upload. BUG=None # Write one later. ==========
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/...
Description was changed from ========== TODO(aleloi): Write better summary. New limiting/mixing structure: initial upload. BUG=None # Write one later. ========== to ========== TODO(aleloi): Write better summary. New limiting/mixing structure: initial upload. Also add some tests. BUG=None # Write one later. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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/21313)
Patchset #1 (id:1) 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/...
Description was changed from ========== TODO(aleloi): Write better summary. New limiting/mixing structure: initial upload. Also add some tests. BUG=None # Write one later. ========== to ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition is kept divided from each other. TODO(aleloi): tests? BUG=None # Write one later. This is large enough to merit a bug. There will be more CLs later. ==========
aleloi@webrtc.org changed reviewers: + ivoc@webrtc.org
Patchset #1 (id:20001) has been deleted
PTAL!
Description was changed from ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition is kept divided from each other. TODO(aleloi): tests? BUG=None # Write one later. This is large enough to merit a bug. There will be more CLs later. ========== to ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition is kept divided from each other. TODO(aleloi): tests? BUG=webrtc:7167 ==========
Description was changed from ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition is kept divided from each other. TODO(aleloi): tests? BUG=webrtc:7167 ========== to ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition are kept divided from each other. TODO(aleloi): tests? BUG=webrtc:7167 ==========
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Nice change! See some comments below. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:21: #include "webrtc/modules/audio_mixer/frame_combiner.h" Since only a pointer to FrameCombiner is used in this file, I suggest you add a forward declaration and move the include to the cc file. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:54: // the last one. See bugs.webrtc.org/7167. What do you mean by "the last one"? It's a bit unclear to me. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:473: TEST(AudioMixer, ZeroSourceRateShouldBeDecidedByRateCalculator) { Can you add a unit test that doesn't use a limiter? You can either test that it actually doesn't limit, or alternatively just test that it works and doesn't crash. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:18: #include "webrtc/base/logging.h" Order of includes https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:96: for (const auto& frame : mix_list) { Can be merged with the previous loop. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:100: // TODO(aleloi): Issue 3390. A url to the issue would be helpful here. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:101: // Audio frame timestamp . The 'timestamp_' field is set to dummy The space between "timestamp" and the "." bothers me :-) https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:102: // value '0', because it is only supported in one channel case and the one channel case https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:134: std::array<int32_t, 2 * 48 * 10> add_buffer; I suggest declaring these constants as constexprs in the anonymous namespace. Also, is it possible to avoid the intermediate variable and write to the audio_frame_for_mixing directly? https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:136: add_buffer.fill(0); It's more efficient to copy the first frame into the add_buffer instead of initializing it to zero. It eliminates one pass through the array. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:144: // Half all samples to avoid saturation before limiting. Halve https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:145: std::transform(add_buffer.begin(), add_buffer.begin() + frame_length, For maximum efficiency it would be more efficient to combine this with adding the last frame from input_frames (add it and saturate_cast the result). It would eliminate another pass through the buffer. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:170: std::transform(add_buffer.begin(), add_buffer.begin() + frame_length, See remark about combining with adding last frame. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.h (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.h:20: namespace webrtc { Please insert a blank line below. (between namespace and the class definition) https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.h:29: void Combine(const std::vector<AudioFrame*>& mix_list, Can the function be const?
Thanks for the comments, I've uploaded a new patch! https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:21: #include "webrtc/modules/audio_mixer/frame_combiner.h" On 2017/02/15 16:58:46, ivoc wrote: > Since only a pointer to FrameCombiner is used in this file, I suggest you add a > forward declaration and move the include to the cc file. I thought FrameCombiner would be an interface and used polymorphically first. Since it isn't, it shouldn't be a pointer. I have left the header and changed the frame_combiner_ member. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:54: // the last one. See bugs.webrtc.org/7167. On 2017/02/15 16:58:46, ivoc wrote: > What do you mean by "the last one"? It's a bit unclear to me. I changed the wording a bit. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:473: TEST(AudioMixer, ZeroSourceRateShouldBeDecidedByRateCalculator) { On 2017/02/15 16:58:46, ivoc wrote: > Can you add a unit test that doesn't use a limiter? You can either test that it > actually doesn't limit, or alternatively just test that it works and doesn't > crash. Testing that it doesn't limit is pretty hard, I think. The test would have to detect clipping in the mixed signal. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:18: #include "webrtc/base/logging.h" On 2017/02/15 16:58:47, ivoc wrote: > Order of includes Done. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:96: for (const auto& frame : mix_list) { On 2017/02/15 16:58:47, ivoc wrote: > Can be merged with the previous loop. Yes, but I'd like to keep the modification from the checks. I've changed the 'constness' so the first loop iterates over const frames, the 2nd one does not. With NDEBUG on, the compiler will eliminate the above loop anyway. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:100: // TODO(aleloi): Issue 3390. On 2017/02/15 16:58:47, ivoc wrote: > A url to the issue would be helpful here. Done. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:101: // Audio frame timestamp . The 'timestamp_' field is set to dummy On 2017/02/15 16:58:46, ivoc wrote: > The space between "timestamp" and the "." bothers me :-) Done. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:102: // value '0', because it is only supported in one channel case and On 2017/02/15 16:58:46, ivoc wrote: > the one channel case Done. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:134: std::array<int32_t, 2 * 48 * 10> add_buffer; On 2017/02/15 16:58:47, ivoc wrote: > I suggest declaring these constants as constexprs in the anonymous namespace. > Also, is it possible to avoid the intermediate variable and write to the > audio_frame_for_mixing directly? Done. Reasons for having an intermediate 32-bit array: * profiling shows it's a little faster for >= 3 streams (because of less calls to rtc::saturated_cast) and the difference is rather small for 2 streams * We lose less precision by halving at the end compared to halving before adding * This prepares for the hypothetical future, in which a new limiter could operate on the 32-bit result (although then everything should probably be re-done in floats...) https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:136: add_buffer.fill(0); On 2017/02/15 16:58:47, ivoc wrote: > It's more efficient to copy the first frame into the add_buffer instead of > initializing it to zero. It eliminates one pass through the array. I profiled this and some other changes. On x86-64 with a GN release build (Clang++ 4.0.0), there is almost no change in execution speed. The numbers I got are 82.49 81.43 80.67 81.74 where the first (82.49) is the current version and the other 3 are * 136 -> fill(add_buffer.begin(), add_buffer.begin() + frame_length) * 136 -> copy 1st frame to buffer + loops start from 2nd frame * the above + last step combined with last frame. Since the changes would lead to messier code, and there is no gain, I'd like to leave everything as is. I've added comments about the profiling in the code. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:144: // Half all samples to avoid saturation before limiting. On 2017/02/15 16:58:47, ivoc wrote: > Halve Done. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:145: std::transform(add_buffer.begin(), add_buffer.begin() + frame_length, On 2017/02/15 16:58:47, ivoc wrote: > For maximum efficiency it would be more efficient to combine this with adding > the last frame from input_frames (add it and saturate_cast the result). It would > eliminate another pass through the buffer. Actually no performance improvement. See comment above. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:170: std::transform(add_buffer.begin(), add_buffer.begin() + frame_length, On 2017/02/15 16:58:46, ivoc wrote: > See remark about combining with adding last frame. Here as well. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.h (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.h:20: namespace webrtc { On 2017/02/15 16:58:47, ivoc wrote: > Please insert a blank line below. (between namespace and the class definition) Done. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.h:29: void Combine(const std::vector<AudioFrame*>& mix_list, On 2017/02/15 16:58:47, ivoc wrote: > Can the function be const? Yes, thank you! CombineMultipleFrames too. They can't be static though, because of use_apm_limiter_.
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/...
Description was changed from ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition are kept divided from each other. TODO(aleloi): tests? BUG=webrtc:7167 ========== to ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition are kept divided from each other. BUG=webrtc:7167 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Nice. See some comments inline. Also, I'm missing frame_combiner_unittests.cc... https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:501: const auto mixer = Please, use a SCOPED_TRACE to annotate which lap in the for loop is being tested. This helps provide context in the error printout when something fails. See e.g., webrtc/modules/audio_processing/aec3/block_framer_unittest.cc for examples of how to use SCOPED_TRACE. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:48: if (!limiter.get()) { Under what conditions can this happen? Wouldn't this warrant a (D)CHECK instead? This gives a better context on why there was no limiter object, instead of finding out only when it was about to be used. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:52: if (limiter->gain_control()->set_mode(GainControl::kFixedDigital) != Same here. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:60: if (limiter->gain_control()->set_target_level_dbfs(7) != limiter->kNoError) { Same here. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:64: if (limiter->gain_control()->set_compression_gain_db(0) != Same here. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:69: if (limiter->gain_control()->enable_limiter(true) != limiter->kNoError) { Same here. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:73: if (limiter->gain_control()->Enable(true) != limiter->kNoError) { Same here. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:89: AudioFrame* audio_frame_for_mixing) const { RTC_DCHECK(audio_frame_for_mixing); https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:90: const size_t kSamplesPerChannel = static_cast<size_t>( I don't think it is correct to use kCamelCase naming for consts that depend on runtime variables. See https://google.github.io/styleguide/cppguide.html#Constant_Names. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:94: RTC_DCHECK_EQ(kSamplesPerChannel, frame->samples_per_channel_); A for loop only for DCHECKing? Why don't you perform those DCHECKs in the next loop (line 99)? https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:128: RTC_DCHECK(!input_frames.empty()); RTC_DCHECK(audio_frame_for_mixing); https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:131: for (const auto& frame : input_frames) { Again, incorporate this DCHECK into another loop. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:158: RTC_DCHECK(limiter_); With your current implementation of CreateLimiter(), it is not "impossible" to arrive here with a nullptr, making it somewhat incorrect to DCHECK. But with my suggested changes, this DCHECK can remain.
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 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/...
Description was changed from ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition are kept divided from each other. BUG=webrtc:7167 ========== to ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition are kept divided from each other. This also fixes a few minor GN issues so that warnings do not have to be suppressed. BUG=webrtc:7167 ==========
Thanks for the comments! I've uploaded a new patch addressing the comments. It also includes a new unit test file. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc (right): https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl_unittest.cc:501: const auto mixer = On 2017/02/17 11:04:12, hlundin-webrtc wrote: > Please, use a SCOPED_TRACE to annotate which lap in the for loop is being > tested. This helps provide context in the error printout when something fails. > > See e.g., webrtc/modules/audio_processing/aec3/block_framer_unittest.cc for > examples of how to use SCOPED_TRACE. Done. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:48: if (!limiter.get()) { On 2017/02/17 11:04:13, hlundin-webrtc wrote: > Under what conditions can this happen? Wouldn't this warrant a (D)CHECK instead? > This gives a better context on why there was no limiter object, instead of > finding out only when it was about to be used. Done. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:52: if (limiter->gain_control()->set_mode(GainControl::kFixedDigital) != On 2017/02/17 11:04:13, hlundin-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:60: if (limiter->gain_control()->set_target_level_dbfs(7) != limiter->kNoError) { On 2017/02/17 11:04:13, hlundin-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:64: if (limiter->gain_control()->set_compression_gain_db(0) != On 2017/02/17 11:04:13, hlundin-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:69: if (limiter->gain_control()->enable_limiter(true) != limiter->kNoError) { On 2017/02/17 11:04:13, hlundin-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:73: if (limiter->gain_control()->Enable(true) != limiter->kNoError) { On 2017/02/17 11:04:12, hlundin-webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:89: AudioFrame* audio_frame_for_mixing) const { On 2017/02/17 11:04:13, hlundin-webrtc wrote: > RTC_DCHECK(audio_frame_for_mixing); Done. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:90: const size_t kSamplesPerChannel = static_cast<size_t>( On 2017/02/17 11:04:12, hlundin-webrtc wrote: > I don't think it is correct to use kCamelCase naming for consts that depend on > runtime variables. See > https://google.github.io/styleguide/cppguide.html#Constant_Names. Good find! Fixed. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:94: RTC_DCHECK_EQ(kSamplesPerChannel, frame->samples_per_channel_); On 2017/02/17 11:04:13, hlundin-webrtc wrote: > A for loop only for DCHECKing? Why don't you perform those DCHECKs in the next > loop (line 99)? It's two against one now! I wanted to separate checking from modifying. Note that the DCHECK loop will be optimized out of existence in non-debug builds. WDYT, can I leave it? https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:128: RTC_DCHECK(!input_frames.empty()); On 2017/02/17 11:04:13, hlundin-webrtc wrote: > RTC_DCHECK(audio_frame_for_mixing); Done. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:131: for (const auto& frame : input_frames) { On 2017/02/17 11:04:13, hlundin-webrtc wrote: > Again, incorporate this DCHECK into another loop. I think it's more readable as is. Do you still want it in the other loop? https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:158: RTC_DCHECK(limiter_); On 2017/02/17 11:04:13, hlundin-webrtc wrote: > With your current implementation of CreateLimiter(), it is not "impossible" to > arrive here with a nullptr, making it somewhat incorrect to DCHECK. But with my > suggested changes, this DCHECK can remain. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/11133)
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 aleloi@webrtc.org
Patchset #3 (id:120001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #3 (id:160001) has been deleted
LGTM with a few comments/questions. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:94: RTC_DCHECK_EQ(kSamplesPerChannel, frame->samples_per_channel_); On 2017/02/20 10:41:33, aleloi wrote: > On 2017/02/17 11:04:13, hlundin-webrtc wrote: > > A for loop only for DCHECKing? Why don't you perform those DCHECKs in the next > > loop (line 99)? > > It's two against one now! > > I wanted to separate checking from modifying. Note that the DCHECK loop will be > optimized out of existence in non-debug builds. > > WDYT, can I leave it? Alright. I'll allow it. https://codereview.webrtc.org/2692333002/diff/140001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:131: for (const auto& frame : input_frames) { On 2017/02/20 10:41:33, aleloi wrote: > On 2017/02/17 11:04:13, hlundin-webrtc wrote: > > Again, incorporate this DCHECK into another loop. > > I think it's more readable as is. Do you still want it in the other loop? Leave it as it is. https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:50: const auto check_no_error = [](int x) { Oh. And there is no way the lambda gets inlined, with the result that the DCHECK's argument is not evaluated in release build? https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner_unittest.cc (right): https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:37: void SetUpFrames(int sample_rate_hz, int number_of_channels) { I cannot see that the actual data_ in the AudioFrames is ever set (to zero or anything else). Won't this mean that your tests are reading and acting upon undefined data? You could have this method call AudioFrame::UpdateFrame to resolve that. https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:112: std::iota(frame1.data_, frame1.data_ + number_of_channels * rate / 100, Didn't know about std::iota. Nice.
https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:50: const auto check_no_error = [](int x) { On 2017/02/20 14:16:31, hlundin-webrtc wrote: > Oh. And there is no way the lambda gets inlined, with the result that the > DCHECK's argument is not evaluated in release build? That would be a big change in the program semantics between optimized and unoptimized builds. Suppose DCHECK_IS_OFF. Then the compiler sees 'check_no_error' as a lambda fkn with empty body. With no optimization, the compiler does as you tell it to: it generates a function for 'check_no_error', evaluates its arguments (e.g. gain_control->set_mode(...)) and passes the result to the function (which does nothing). The point of optimization is that the compiler should generate code that does the same thing, but faster. In this case, it will remove the lambda call altogether, but it can't remove the other calls, (e.g. gain_control->set_mode) since they can have side effects. Doing so would change program semantics. Here is a relevant standard quote (1.9.15 from the draft at http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf) When calling a function (whether or not the function is inline), every value computation and side effect associated with any argument expression, or with the postfix expression designating the called function, is sequenced before execution of every expression or statement in the body of the called function If other reviewers want to play with godbolt: https://godbolt.org/g/pgpEXc https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner_unittest.cc (right): https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:37: void SetUpFrames(int sample_rate_hz, int number_of_channels) { On 2017/02/20 14:16:31, hlundin-webrtc wrote: > I cannot see that the actual data_ in the AudioFrames is ever set (to zero or > anything else). Won't this mean that your tests are reading and acting upon > undefined data? You could have this method call AudioFrame::UpdateFrame to > resolve that. Good point! Done.
Still LGTM. https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2692333002/diff/200001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:50: const auto check_no_error = [](int x) { On 2017/02/20 15:21:37, aleloi wrote: > On 2017/02/20 14:16:31, hlundin-webrtc wrote: > > Oh. And there is no way the lambda gets inlined, with the result that the > > DCHECK's argument is not evaluated in release build? > > That would be a big change in the program semantics between optimized and > unoptimized builds. > > Suppose DCHECK_IS_OFF. Then the compiler sees 'check_no_error' as a lambda fkn > with empty body. With no optimization, the compiler does as you tell it to: it > generates a function for 'check_no_error', evaluates its arguments (e.g. > gain_control->set_mode(...)) and passes the result to the function (which does > nothing). > > The point of optimization is that the compiler should generate code that does > the same thing, but faster. In this case, it will remove the lambda call > altogether, but it can't remove the other calls, (e.g. gain_control->set_mode) > since they can have side effects. Doing so would change program semantics. > > Here is a relevant standard quote (1.9.15 from the draft at > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf) > > When calling a function (whether or not the function is inline), every value > computation and side effect > associated with any argument expression, or with the postfix expression > designating the called function, is > sequenced before execution of every expression or statement in the body of > the called function > > If other reviewers want to play with godbolt: https://godbolt.org/g/pgpEXc This is exactly why I ask stupid questions when reviewing code -- I get to learn a lot! :) Thanks!
lgtm with one remaining question. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_mixer_impl.h:21: #include "webrtc/modules/audio_mixer/frame_combiner.h" On 2017/02/16 14:01:35, aleloi wrote: > On 2017/02/15 16:58:46, ivoc wrote: > > Since only a pointer to FrameCombiner is used in this file, I suggest you add > a > > forward declaration and move the include to the cc file. > > I thought FrameCombiner would be an interface and used polymorphically first. > Since it isn't, it shouldn't be a pointer. I have left the header and changed > the frame_combiner_ member. Acknowledged. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner.cc (right): https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:96: for (const auto& frame : mix_list) { On 2017/02/16 14:01:36, aleloi wrote: > On 2017/02/15 16:58:47, ivoc wrote: > > Can be merged with the previous loop. > > Yes, but I'd like to keep the modification from the checks. I've changed the > 'constness' so the first loop iterates over const frames, the 2nd one does not. > With NDEBUG on, the compiler will eliminate the above loop anyway. Acknowledged. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:134: std::array<int32_t, 2 * 48 * 10> add_buffer; On 2017/02/16 14:01:36, aleloi wrote: > On 2017/02/15 16:58:47, ivoc wrote: > > I suggest declaring these constants as constexprs in the anonymous namespace. > > Also, is it possible to avoid the intermediate variable and write to the > > audio_frame_for_mixing directly? > > Done. Reasons for having an intermediate 32-bit array: > > * profiling shows it's a little faster for >= 3 streams (because of less calls > to rtc::saturated_cast) and the difference is rather small for 2 streams > * We lose less precision by halving at the end compared to halving before adding > * This prepares for the hypothetical future, in which a new limiter could > operate on the 32-bit result (although then everything should probably be > re-done in floats...) Thanks for the profiling, that makes sense. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:136: add_buffer.fill(0); On 2017/02/16 14:01:35, aleloi wrote: > On 2017/02/15 16:58:47, ivoc wrote: > > It's more efficient to copy the first frame into the add_buffer instead of > > initializing it to zero. It eliminates one pass through the array. > > I profiled this and some other changes. On x86-64 with a GN release build > (Clang++ 4.0.0), there is almost no change in execution speed. The numbers I got > are > > 82.49 81.43 80.67 81.74 > > where the first (82.49) is the current version and the other 3 are > * 136 -> fill(add_buffer.begin(), add_buffer.begin() + frame_length) > * 136 -> copy 1st frame to buffer + loops start from 2nd frame > * the above + last step combined with last frame. > > Since the changes would lead to messier code, and there is no gain, I'd like to > leave everything as is. I've added comments about the profiling in the code. Great! Thanks for looking into this. I agree that if the effect is this small we should leave it as-is. https://codereview.webrtc.org/2692333002/diff/100001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner.cc:145: std::transform(add_buffer.begin(), add_buffer.begin() + frame_length, On 2017/02/16 14:01:36, aleloi wrote: > On 2017/02/15 16:58:47, ivoc wrote: > > For maximum efficiency it would be more efficient to combine this with adding > > the last frame from input_frames (add it and saturate_cast the result). It > would > > eliminate another pass through the buffer. > > Actually no performance improvement. See comment above. Acknowledged. https://codereview.webrtc.org/2692333002/diff/220001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner_unittest.cc (right): https://codereview.webrtc.org/2692333002/diff/220001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:55: ProduceDebugText(rate, number_of_channels, number_of_frames); Why is this debug string created and not used in any way? Or am I missing something? Same for other calls below. https://codereview.webrtc.org/2692333002/diff/220001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:113: std::iota(frame1.data_, frame1.data_ + number_of_channels * rate / 100, Looks like an interesting function, I wasn't aware of this one.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2692333002/#ps240001 (title: "Forgot SCOPED_TRACE")
https://codereview.webrtc.org/2692333002/diff/220001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/frame_combiner_unittest.cc (right): https://codereview.webrtc.org/2692333002/diff/220001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/frame_combiner_unittest.cc:113: std::iota(frame1.data_, frame1.data_ + number_of_channels * rate / 100, On 2017/02/21 09:59:16, ivoc wrote: > Looks like an interesting function, I wasn't aware of this one. Well spotted, thanks! The calls should be surrounded with 'SCOPED_TRACE' to have any effect.
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: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/22915)
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2692333002/#ps260001 (title: "Fix int16_t <-> size_t compilation warnings.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #3 (id:180001) has been deleted
Patchset #5 (id:240001) has been deleted
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1487676804604580,
"parent_rev": "76094ee6979f5299ffddd1909cef7eaf67855fcb", "commit_rev":
"24899e58ecb10157fea418d80022b938c0f806a9"}
Message was sent while issue was closed.
Description was changed from ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition are kept divided from each other. This also fixes a few minor GN issues so that warnings do not have to be suppressed. BUG=webrtc:7167 ========== to ========== Optionally disable APM limiter in AudioMixer. The APM limiter is a component for keeping the audio from clipping by smoothly reducing the amplitude of the audio samples. It can be rather expensive because of band-splitting & merging. Also, experiments indicate that it is of questionable benefit (adding several sources of human speech almost never cause clipping). To optionally disable the limiter, this CL does some refactoring on the (quite large) AudioMixerImpl. Functionality related to actual addition of frames and handling AudioFrame meta-data (sample_rate, num_channels, samples_per_channel, time_stamp, elapsed_time_ms) is broken out in a new sub-component called FrameCombiner. The FrameCombiner is initialized with a 'use_limiter' flag. To create a mixer without using the APM limiter Inside of FrameCombiner, the meta-data handling and the audio sample addition are kept divided from each other. This also fixes a few minor GN issues so that warnings do not have to be suppressed. BUG=webrtc:7167 Review-Url: https://codereview.webrtc.org/2692333002 Cr-Commit-Position: refs/heads/master@{#16742} Committed: https://chromium.googlesource.com/external/webrtc/+/24899e58ecb10157fea418d80... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:260001) as https://chromium.googlesource.com/external/webrtc/+/24899e58ecb10157fea418d80... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
