Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2398083005:
Changed ramping functionality of the AudioMixer. (Closed)
DescriptionChanged ramping functionality of the AudioMixer.
BUG=webrtc:6346
Committed: https://crrev.com/4b8bfb8ed3845ce214f5c586e521a442eb771e56
Cr-Commit-Position: refs/heads/master@{#14607}
Patch Set 1 : . #
Total comments: 12
Patch Set 2 : class -> struct and name changes + avoid division in ramp loop. #
Total comments: 10
Patch Set 3 : Added tests for Ramp and changed arg order in Ramp. #
Total comments: 24
Patch Set 4 : Moved audio_frame_manipulator_unittest, made style changes to the tests. #Patch Set 5 : Gmock -> gtest, nth -> ith #
Total comments: 4
Patch Set 6 : Changed argument names and comment in Ramp. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
aleloi@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, solenberg@webrtc.org
Here is the second part of the ramping and AudioMixerSource CL. This one contains the ramping.
https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right): https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:37: for (size_t i = 0; i < samples; i++) { I'm not an expert on performance, but I think it would be better to implement this as a nested for loop, rather than have a division in the critical path. In essence: size_t samples = audio_frame->samples_per_channel_; float increment = (target - current) / samples; float gain = current; for (size_t i = 0; i < samples; ++i) { for (size_t ch = 0; ch < audio_frame->num_channels_; ++ch) { audio_frame->data_[audio_frame->num_channels_ * i + ch] *= gain; } gain += increment; } WDYT? https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:25: class SourceFrame { I'm still inclined to say that this should be a struct. https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:27: SourceFrame(AudioMixerImpl::SourceStatus* source_status, This passing of non-const pointer makes me cringe a bit. But I think it is for another CL. Let's discuss offline. https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:77: float target_gain_ = source_frame.source_status_->is_mixed_ ? 1 : 0; target_gain without underscore. https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:36: Source* audio_source_ = nullptr; Structs don't have trailing underscore on the member names. https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:41: typedef std::vector<SourceStatus> MixerAudioSourceList; How about SourceStatusList?
https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right): https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:37: for (size_t i = 0; i < samples; i++) { On 2016/10/07 15:03:10, hlundin-webrtc wrote: > I'm not an expert on performance, but I think it would be better to implement > this as a nested for loop, rather than have a division in the critical path. > > In essence: > > size_t samples = audio_frame->samples_per_channel_; > float increment = (target - current) / samples; > float gain = current; > for (size_t i = 0; i < samples; ++i) { > for (size_t ch = 0; ch < audio_frame->num_channels_; ++ch) { > audio_frame->data_[audio_frame->num_channels_ * i + ch] *= gain; > } > gain += increment; > } > > WDYT? Sure! Your suggestion accesses the data in the same order and avoids division inside the loop. I don't know if it's faster, but it shouldn't be slower! https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:25: class SourceFrame { On 2016/10/07 15:03:10, hlundin-webrtc wrote: > I'm still inclined to say that this should be a struct. I'm quite sure that also was the conclusion of our offline discussion. Move the comparison function out and make SourceFrame a struct. I've changed. https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:27: SourceFrame(AudioMixerImpl::SourceStatus* source_status, On 2016/10/07 15:03:10, hlundin-webrtc wrote: > This passing of non-const pointer makes me cringe a bit. But I think it is for > another CL. Let's discuss offline. Acknowledged. https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:77: float target_gain_ = source_frame.source_status_->is_mixed_ ? 1 : 0; On 2016/10/07 15:03:10, hlundin-webrtc wrote: > target_gain without underscore. Oops! https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:36: Source* audio_source_ = nullptr; On 2016/10/07 15:03:10, hlundin-webrtc wrote: > Structs don't have trailing underscore on the member names. > https://google.github.io/styleguide/cppguide.html#Variable_Names Thanks! I think I should re-read the style guide again. https://codereview.webrtc.org/2398083005/diff/60001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:41: typedef std::vector<SourceStatus> MixerAudioSourceList; On 2016/10/07 15:03:10, hlundin-webrtc wrote: > How about SourceStatusList? Done.
LG but I'd like to see it rebased on your other interface cleanup CLs before I add the TM. :) https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right): https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:29: void Ramp(AudioFrame* audio_frame, float current, float target) { RTC_DCHECK(audio_frame); https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:29: void Ramp(AudioFrame* audio_frame, float current, float target) { Also, you should add (at least) two test cases for this function - one with increasing, one with decreasing gain. https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:33: size_t samples = audio_frame->samples_per_channel_; RTC_DCHECK_LT(0, samples); https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:74: float target_gain = source_frame.source_status->is_mixed ? 1 : 0; nit: 1.0f, 0.0f https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:38: float gain = 0; nit: 0.0f
Patchset #3 (id:100001) has been deleted
https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right): https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:29: void Ramp(AudioFrame* audio_frame, float current, float target) { On 2016/10/10 20:24:33, the sun wrote: > RTC_DCHECK(audio_frame); Done. https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:29: void Ramp(AudioFrame* audio_frame, float current, float target) { On 2016/10/10 20:24:33, the sun wrote: > Also, you should add (at least) two test cases for this function - one with > increasing, one with decreasing gain. Done. https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:33: size_t samples = audio_frame->samples_per_channel_; On 2016/10/10 20:24:32, the sun wrote: > RTC_DCHECK_LT(0, samples); Done. https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.cc (right): https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.cc:74: float target_gain = source_frame.source_status->is_mixed ? 1 : 0; On 2016/10/10 20:24:33, the sun wrote: > nit: 1.0f, 0.0f Done. https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... File webrtc/modules/audio_mixer/audio_mixer_impl.h (right): https://codereview.webrtc.org/2398083005/diff/80001/webrtc/modules/audio_mixe... webrtc/modules/audio_mixer/audio_mixer_impl.h:38: float gain = 0; On 2016/10/10 20:24:33, the sun wrote: > nit: 0.0f Done. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_frame_manipulator.h (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.h:24: void Ramp(float current, float target, AudioFrame* audio_frame); Moved input-only arguments to the beginning of the argument list (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...)
https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:35: RTC_DCHECK_LT(static_cast<size_t>(0), samples); you could just write "0u" to make it an unsigned. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:1: /* This file should live next to the .h and .cc - no need to move it in under test/ (same goes for audio_mixer_unittest.cc, which should be moved next to audio_mixer_impl.cc and be renamed). https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:20: void FillFrameWithConstants(int samples_per_channel, size_t https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:21: int number_of_channels, size_t https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:22: int value, int16_t https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:31: TEST(AudioFrameManipulator, CompareForwardRampWithExpectedResult) { Add "Stereo" to the name here https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:32: constexpr int samples_per_channel = 5; Although there's been debate around the naming of constants, to my knowledge the most common way in the codebase is to use the kSamplesPerChannel style. You may want to look this up in the style guide and prove me wrong though. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:39: Ramp(0, 1, &frame); 0.0f, 1.0f
Next verison. I've moveed test/audio_mixer_unittests.cc to audio_mixer_impl_unittests.cc in the last dependent CL ( https://codereview.webrtc.org/2408683002 ) to avoid potential rebasing trouble. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:35: RTC_DCHECK_LT(static_cast<size_t>(0), samples); On 2016/10/11 10:57:05, the sun wrote: > you could just write "0u" to make it an unsigned. Done. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:1: /* On 2016/10/11 10:57:05, the sun wrote: > This file should live next to the .h and .cc - no need to move it in under test/ > > (same goes for audio_mixer_unittest.cc, which should be moved next to > audio_mixer_impl.cc and be renamed). Done. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:20: void FillFrameWithConstants(int samples_per_channel, On 2016/10/11 10:57:05, the sun wrote: > size_t Done. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:21: int number_of_channels, On 2016/10/11 10:57:05, the sun wrote: > size_t Done. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:22: int value, On 2016/10/11 10:57:06, the sun wrote: > int16_t Of course! Thanks for noticing :) https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:31: TEST(AudioFrameManipulator, CompareForwardRampWithExpectedResult) { On 2016/10/11 10:57:06, the sun wrote: > Add "Stereo" to the name here Done. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:32: constexpr int samples_per_channel = 5; On 2016/10/11 10:57:06, the sun wrote: > Although there's been debate around the naming of constants, to my knowledge the > most common way in the codebase is to use the kSamplesPerChannel style. You may > want to look this up in the style guide and prove me wrong though. You are correct, I missed that. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:39: Ramp(0, 1, &frame); On 2016/10/11 10:57:05, the sun wrote: > 0.0f, 1.0f Done.
LG with minor comments. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:40: // apply the same gain change to the nth sample of every channel. Total nit: You are referring to the nth sample, but the index variable is called i. :) https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_frame_manipulator.h (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.h:24: void Ramp(float current, float target, AudioFrame* audio_frame); On 2016/10/11 10:24:08, aleloi wrote: > Moved input-only arguments to the beginning of the argument list > (https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...) Acknowledged. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:1: /* On 2016/10/11 10:57:05, the sun wrote: > This file should live next to the .h and .cc - no need to move it in under test/ > > (same goes for audio_mixer_unittest.cc, which should be moved next to > audio_mixer_impl.cc and be renamed). Acknowledged. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:15: #include "webrtc/test/gmock.h" gmock -> gtest You are not using mocks. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:32: constexpr int samples_per_channel = 5; On 2016/10/11 10:57:06, the sun wrote: > Although there's been debate around the naming of constants, to my knowledge the > most common way in the codebase is to use the kSamplesPerChannel style. You may > want to look this up in the style guide and prove me wrong though. Debate it has been. I prefer kConstantName, but I think that strictly speaking it is only mandatory for constants with static storage duration. See https://google.github.io/styleguide/cppguide.html#Constant_Names.
Patchset #5 (id:160001) has been deleted
Next patch set with minimal changes. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_frame_manipulator.cc (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.cc:40: // apply the same gain change to the nth sample of every channel. On 2016/10/11 11:29:58, hlundin-webrtc wrote: > Total nit: You are referring to the nth sample, but the index variable is called > i. :) Done. https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc (right): https://codereview.webrtc.org/2398083005/diff/120001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/test/audio_frame_manipulator_unittest.cc:15: #include "webrtc/test/gmock.h" On 2016/10/11 11:29:58, hlundin-webrtc wrote: > gmock -> gtest > You are not using mocks. Done.
lgtm
lgtm % nits https://codereview.webrtc.org/2398083005/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_frame_manipulator.h (right): https://codereview.webrtc.org/2398083005/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.h:22: // Ramps up or down the provided audio frame. Ramp(frame, 0, 1) will nit: arguments to Ramp() in comment are in wrong order https://codereview.webrtc.org/2398083005/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.h:24: void Ramp(float current, float target, AudioFrame* audio_frame); use start_gain and target_gain instead.
https://codereview.webrtc.org/2398083005/diff/180001/webrtc/modules/audio_mix... File webrtc/modules/audio_mixer/audio_frame_manipulator.h (right): https://codereview.webrtc.org/2398083005/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.h:22: // Ramps up or down the provided audio frame. Ramp(frame, 0, 1) will On 2016/10/11 18:04:13, the sun wrote: > nit: arguments to Ramp() in comment are in wrong order Done. https://codereview.webrtc.org/2398083005/diff/180001/webrtc/modules/audio_mix... webrtc/modules/audio_mixer/audio_frame_manipulator.h:24: void Ramp(float current, float target, AudioFrame* audio_frame); On 2016/10/11 18:04:13, the sun wrote: > use start_gain and target_gain instead. Done.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2398083005/#ps200001 (title: "Changed argument names and comment in Ramp.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #6 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Changed ramping functionality of the AudioMixer. BUG=webrtc:6346 ========== to ========== Changed ramping functionality of the AudioMixer. BUG=webrtc:6346 Committed: https://crrev.com/4b8bfb8ed3845ce214f5c586e521a442eb771e56 Cr-Commit-Position: refs/heads/master@{#14607} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4b8bfb8ed3845ce214f5c586e521a442eb771e56 Cr-Commit-Position: refs/heads/master@{#14607} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
