Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1810413002:
Avoid clicks when muting/unmuting a voe::Channel. (Closed)
|
Created:
4 years, 9 months ago by the sun Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, henrika_webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, minyue-webrtc, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid clicks when muting/unmuting a voe::Channel.
Muting/unmuting is triggered in the PeerConnection API by calling setEnable() on an audio track.
BUG=webrtc:5671
Committed: https://crrev.com/1c2af8e3191e69aad6e233a6b2402579dcdbb740
Cr-Commit-Position: refs/heads/master@{#12121}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 66
Patch Set 3 : address comments #Patch Set 4 : Addressed comments #Patch Set 5 : removed commented out code #
Total comments: 2
Patch Set 6 : addressed comments #Patch Set 7 : undo loop unrolling #
Messages
Total messages: 24 (8 generated)
solenberg@webrtc.org changed reviewers: + peah@webrtc.org, tina.legrand@webrtc.org
On 2016/03/23 09:54:03, the sun wrote: Really nice change! Most of my remarks are regarding commenting, but as long as you fix those, LGTM.
https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/in... File webrtc/modules/utility/include/audio_frame_operations.h (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/in... webrtc/modules/utility/include/audio_frame_operations.h:48: // Zeros out the audio and sets |frame.energy| to zero. Is the function called both for mute and unmute? Change name? The comment should be updated, saying something like "faded to/from zero"? https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... File webrtc/modules/utility/source/audio_frame_operations.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:16: namespace { Is the extra namespace needed? https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:20: const float kMuteFadeInc = 1.0f / kMuteFadeFrames; Not sure what the style guide says, but I'd find it easier to read with a new line here. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:80: bool end_muted) { It's not super clear what start_muted and end_muted means. Could you add a comment before the function that explains how the input is used, and what the function does? https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:99: // Ramp up the first N samples of frame. N-> "count", or? The comment should probably mention ramp down as well. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... File webrtc/modules/utility/source/audio_frame_operations_unittest.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations_unittest.cc:249: // and stereo. Ending mute should yields a ramp down to zero. ramp down to zero -> ramp up from zero https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/trans... File webrtc/voice_engine/transmit_mixer.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/trans... webrtc/voice_engine/transmit_mixer.cc:362: AudioFrameOperations::Mute(&_audioFrame, _mute, _mute); There won't be any fading here, right?
https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/in... File webrtc/modules/utility/include/audio_frame_operations.h (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/in... webrtc/modules/utility/include/audio_frame_operations.h:48: // Zeros out the audio and sets |frame.energy| to zero. On 2016/03/23 13:42:41, tlegrand-webrtc wrote: > Is the function called both for mute and unmute? Change name? > > The comment should be updated, saying something like "faded to/from zero"? Done. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... File webrtc/modules/utility/source/audio_frame_operations.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:16: namespace { On 2016/03/23 13:42:41, tlegrand-webrtc wrote: > Is the extra namespace needed? Yes, anything internal to the implementation should go in an anonymous namespace. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:20: const float kMuteFadeInc = 1.0f / kMuteFadeFrames; On 2016/03/23 13:42:41, tlegrand-webrtc wrote: > Not sure what the style guide says, but I'd find it easier to read with a new > line here. Done. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:80: bool end_muted) { On 2016/03/23 13:42:41, tlegrand-webrtc wrote: > It's not super clear what start_muted and end_muted means. Could you add a > comment before the function that explains how the input is used, and what the > function does? Added in the .h. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:99: // Ramp up the first N samples of frame. On 2016/03/23 13:42:41, tlegrand-webrtc wrote: > N-> "count", or? Done. > The comment should probably mention ramp down as well. It does so a few lines down, when we set up the parameters for fade out. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... File webrtc/modules/utility/source/audio_frame_operations_unittest.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations_unittest.cc:249: // and stereo. Ending mute should yields a ramp down to zero. On 2016/03/23 13:42:41, tlegrand-webrtc wrote: > ramp down to zero -> ramp up from zero Done. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/trans... File webrtc/voice_engine/transmit_mixer.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/trans... webrtc/voice_engine/transmit_mixer.cc:362: AudioFrameOperations::Mute(&_audioFrame, _mute, _mute); On 2016/03/23 13:42:41, tlegrand-webrtc wrote: > There won't be any fading here, right? Correct.
https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/in... File webrtc/modules/utility/include/audio_frame_operations.h (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/in... webrtc/modules/utility/include/audio_frame_operations.h:49: static void Mute(AudioFrame* frame, bool start_muted, bool end_muted); Please add documentation of the start_muted and end_muted parameters. To me current names are quite general and a bit ambiguous and I think either clear documentation, or other more describing variable names are needed. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... File webrtc/modules/utility/source/audio_frame_operations.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:79: void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted, Why not split this function in two versions, one called Mute that takes only one AudioFrame as a parameter and does no fading, and one called FadedMute that does fading. That would make the behavior of the call where no fading is done much clearer. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:79: void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted, I think the naming of start_muted and end_muted makes the usage of the Mute function quite complicated. In my mind, what you want to do, based on seeing how the method is used, becomes much clearer if you rename them to previous_frame_was_muted and current_frame_should_be_muted. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:79: void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted, Having looked closer at the function, I think it would be clearer and simpler if you partition it into separate tasks: if ((frame->samples_per_channel_ > 0) && (!(!previous_frame_was_muted && !current_frame_should_be_muted))) { if (previous_frame_was_muted && current_frame_should_be_muted) { // memset to zero. } else if ((!previous_frame_was_muted && current_frame_should_be_muted) || (previous_frame_was_muted && !current_frame_should_be_muted)) { // Do fade in/fade out } } } // else no muting is needed. Wdyt? https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:86: memset(frame->data_, 0, sizeof(int16_t) * I would prefer the sizeof to be done on frame->data[0] rather than on int16_t as that would ensure proper memset behavior even if the type of frame->data changes (https://google.github.io/styleguide/cppguide.html#sizeof). https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:86: memset(frame->data_, 0, sizeof(int16_t) * Since you know the size of data in this case (it is public in AudioFrame) it would be possible to assert on the size before them memset to ensure that the memset does not set memory outside of data_. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:92: if (frame->samples_per_channel_ < kMuteFadeFrames) { If frame->samples_per_channel_ == 0 is treated as a special case, this if statement can be done as a max operation. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:94: if (count > 0) { This should always be true, right? Afaics count == min(frame->samples_per_channel_, kMuteFadeFrames). Or can frame->samples_per_channel_ be zero? If this code is needed for handling the case of frame->samples_per_channel_ == 0, I think it is better to treat that as a special case in a more explicit manner. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:95: inc = 1.0f / count; How common is it that frame->samples_per_channel_ < kMuteFadeFrames ? If it is common, I think it is better to always compute the division and remove the kMuteFadeInc constant as that would simplify the code. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:108: RTC_DCHECK(start_muted && !end_muted); Why do you assert on this? Should it not be allowed to transition from the previous frame being muted, to this frame not being muted? I can't see anything in the code using the function that prevents this. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:110: If you change the code so that you in this block know that muting transition is needed an that the frame length is positive, the above code could be simplified to something like: int count = std::min(frame->samples_per_channel_, kMuteFadeFrames); bool muted_to_unmuted = (previous_frame_was_muted && !current_frame_should_be_muted) int start_frame = ( muted_to_unmuted ? 0 : frame->samples_per_channel_ - count); float inc = (1.f / count) * (muted_to_unmuted ? 1.f : 1.f); https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:112: const size_t end = (start + count); This should always be start = frame->samples_per_channel_, right? Or is there any other reason for recomputing the end of the frame? https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:115: g += inc; You should ideally bound the gain to 0<=g<=1 using max/min operations. It is a bit overkill here though. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:118: } Since the number of channels typically is much smaller than the number of frames I think it should be faster to nest the loops the other way, e.g.: float g_start = (muted_to_unmuted ? 0.f : 1.f); for (size_t j = 0; j < channels; ++j) { float g = g_start; for (size_t i = 0; j < count; i+=channels) { g += inc; frame->data_[start + i + j] *= g; } } Wdyt? https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2974: if (is_muted && cached_input_mute_) { If you would rename cached_input_mute_ to previous_frame_was_muted, and is_muted to current_frame_is_muted this if-statement would be much clearer I think. The cached_input_mute_ is a bit less vague than needed as to what is the purpose of the caching. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:272: int SetInputMute(bool enable); What is motivates the name change to InputMute? Can there be an OutputMute? I think that it should be fine to continue naming it Mute. To me this name-change makes the purpose of the function less intuitive. A channel should only be muted or not muted, I would not be able to tell how an InputMuted channel behaves.
https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/in... File webrtc/modules/utility/include/audio_frame_operations.h (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/in... webrtc/modules/utility/include/audio_frame_operations.h:49: static void Mute(AudioFrame* frame, bool start_muted, bool end_muted); On 2016/03/23 15:57:04, peah-webrtc wrote: > Please add documentation of the start_muted and end_muted parameters. To me > current names are quite general and a bit ambiguous and I think either clear > documentation, or other more describing variable names are needed. I've renamed them and added documentation. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... File webrtc/modules/utility/source/audio_frame_operations.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:79: void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted, On 2016/03/23 15:57:04, peah-webrtc wrote: > I think the naming of start_muted and end_muted makes the usage of the Mute > function quite complicated. > In my mind, what you want to do, based on seeing how the method is used, becomes > much clearer if you rename them to previous_frame_was_muted and > current_frame_should_be_muted. Good suggestion. Makes the code much easier to read. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:79: void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted, On 2016/03/23 15:57:04, peah-webrtc wrote: > Having looked closer at the function, I think it would be clearer and simpler if > you partition it into separate tasks: > if ((frame->samples_per_channel_ > 0) && > (!(!previous_frame_was_muted && !current_frame_should_be_muted))) { > if (previous_frame_was_muted && current_frame_should_be_muted) { > // memset to zero. > } > else if ((!previous_frame_was_muted && current_frame_should_be_muted) || > (previous_frame_was_muted && !current_frame_should_be_muted)) { > // Do fade in/fade out > } > } > } > // else no muting is needed. > > Wdyt? Hmm... "!(!previous_frame_was_muted && !current_frame_should_be_muted))"?? My head exploded right there. I think my approach is easier to read. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:79: void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted, On 2016/03/23 15:57:04, peah-webrtc wrote: > Why not split this function in two versions, one called Mute that takes only one > AudioFrame as a parameter and does no fading, and one called FadedMute that > does fading. That would make the behavior of the call where no fading is done > much clearer. I think it makes sense to keep the logic within one function since we need the same functionality in two places. I propose to instead store the previous frame mute state in TransmitMixer so we avoid clicks there too. But I'll do so in a separate CL. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:86: memset(frame->data_, 0, sizeof(int16_t) * On 2016/03/23 15:57:04, peah-webrtc wrote: > Since you know the size of data in this case (it is public in AudioFrame) it > would be possible to assert on the size before them memset to ensure that the > memset does not set memory outside of data_. Good idea! Done. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:86: memset(frame->data_, 0, sizeof(int16_t) * On 2016/03/23 15:57:04, peah-webrtc wrote: > I would prefer the sizeof to be done on frame->data[0] rather than on int16_t as > that would ensure proper memset behavior even if the type of frame->data changes > (https://google.github.io/styleguide/cppguide.html#sizeof). Yes, I blame copy+paste from the old version. :) https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:92: if (frame->samples_per_channel_ < kMuteFadeFrames) { On 2016/03/23 15:57:04, peah-webrtc wrote: > If frame->samples_per_channel_ == 0 is treated as a special case, this if > statement can be done as a max operation. No, we still need to do the division in case we have fewer than kMuteFadeFrames. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:94: if (count > 0) { On 2016/03/23 15:57:04, peah-webrtc wrote: > This should always be true, right? Afaics count == > min(frame->samples_per_channel_, kMuteFadeFrames). Or can > frame->samples_per_channel_ be zero? There's nothing in AudioFrame that prevents it from being zero. > If this code is needed for handling the case of frame->samples_per_channel_ == > 0, I think it is better to treat that as a special case in a more explicit > manner. The downside with that is that you'll *always* be paying the penalty of checking for an empty frame, even though we can be quite certain that it will never happen. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:95: inc = 1.0f / count; On 2016/03/23 15:57:04, peah-webrtc wrote: > How common is it that frame->samples_per_channel_ < kMuteFadeFrames ? > > If it is common, I think it is better to always compute the division and remove > the kMuteFadeInc constant as that would simplify the code. Given that we usually store 10ms of data (though we can't be certain we *always* do that, because of how AudioFrame is designed), 32kHz -> 320 samples/frame, 16kHz -> 160 samples/frame, 8kHz -> 80 samples/frame. No, I wouldn't say < 128 frames is very common. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:108: RTC_DCHECK(start_muted && !end_muted); On 2016/03/23 15:57:04, peah-webrtc wrote: > Why do you assert on this? Should it not be allowed to transition from the > previous frame being muted, to this frame not being muted? I can't see anything > in the code using the function that prevents this. Good call, I've simplified the logic a bit. This assert is to verify that the logic within the function isn't messed up. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:110: On 2016/03/23 15:57:04, peah-webrtc wrote: > If you change the code so that you in this block know that muting transition is > needed an that the frame length is positive, the above code could be simplified > to something like: > > int count = std::min(frame->samples_per_channel_, kMuteFadeFrames); > bool muted_to_unmuted = (previous_frame_was_muted && > !current_frame_should_be_muted) > int start_frame = ( muted_to_unmuted ? 0 : frame->samples_per_channel_ - count); > float inc = (1.f / count) * (muted_to_unmuted ? 1.f : 1.f); You leave out the check for frame->samples_per_channel_ == 0 here, and you forget that we also need the initial gain value ('g'). If I add those to your simplification, it will always require 5 conditionals, one division and one multiplication. My implementation usually requires 2 conditionals and neither mul nor div outside the inner loop. But I agree mine is longer to read, though I don't think overuse of the ternary operator contributes to readability either. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:112: const size_t end = (start + count); On 2016/03/23 15:57:04, peah-webrtc wrote: > This should always be start = frame->samples_per_channel_, right? Or is there > any other reason for recomputing the end of the frame? end will be 'count' or 'frame->samples_per_channel_'. We fade up at start of frame and fade down at end of frame. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:115: g += inc; On 2016/03/23 15:57:04, peah-webrtc wrote: > You should ideally bound the gain to 0<=g<=1 using max/min operations. It is a > bit overkill here though. No, it is designed to never shoot outside, and the regression tests (should) verify that. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:118: } On 2016/03/23 15:57:04, peah-webrtc wrote: > Since the number of channels typically is much smaller than the number of frames > I think it should be faster to nest the loops the other way, e.g.: > > float g_start = (muted_to_unmuted ? 0.f : 1.f); > for (size_t j = 0; j < channels; ++j) { > float g = g_start; > for (size_t i = 0; j < count; i+=channels) { > g += inc; > frame->data_[start + i + j] *= g; > } > } > > Wdyt? I think that is evil to caches. For a 320 sample stereo frame we'll hit 20 cache lines. Twice with your implementation, once with mine. Again, I agree it may read prettier though. But thanks anyway for pointing it out - I realized an AudioFrame may actually contain non-interleaved data, so I've added a DCHECK on the interleaved_ flag (it appears as we never store non-interleaved data in it). https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2974: if (is_muted && cached_input_mute_) { On 2016/03/23 15:57:04, peah-webrtc wrote: > If you would rename > cached_input_mute_ to previous_frame_was_muted, > and > is_muted to current_frame_is_muted > this if-statement would be much clearer I think. The cached_input_mute_ is a bit > less vague than needed as to what is the purpose of the caching. Good idea! https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:272: int SetInputMute(bool enable); On 2016/03/23 15:57:04, peah-webrtc wrote: > What is motivates the name change to InputMute? Can there be an OutputMute? I > think that it should be fine to continue naming it Mute. To me this name-change > makes the purpose of the function less intuitive. A channel should only be muted > or not muted, I would not be able to tell how an InputMuted channel behaves. The voe::Channel is duplex. SetMute() could mean to mute either the input or the output. The functions on the VoEVolumeControl interface (to which this class is tightly tied) calling these functions are called SetInputMute() and GetInputMute().
https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... File webrtc/modules/utility/source/audio_frame_operations.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:79: void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted, On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > Why not split this function in two versions, one called Mute that takes only > one > > AudioFrame as a parameter and does no fading, and one called FadedMute that > > does fading. That would make the behavior of the call where no fading is done > > much clearer. > > I think it makes sense to keep the logic within one function since we need the > same functionality in two places. I propose to instead store the previous frame > mute state in TransmitMixer so we avoid clicks there too. But I'll do so in a > separate CL. Acknowledged. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:79: void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted, On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > Having looked closer at the function, I think it would be clearer and simpler > if > > you partition it into separate tasks: > > if ((frame->samples_per_channel_ > 0) && > > (!(!previous_frame_was_muted && !current_frame_should_be_muted))) { > > if (previous_frame_was_muted && current_frame_should_be_muted) { > > // memset to zero. > > } > > else if ((!previous_frame_was_muted && current_frame_should_be_muted) || > > (previous_frame_was_muted && !current_frame_should_be_muted)) { > > // Do fade in/fade out > > } > > } > > } > > // else no muting is needed. > > > > Wdyt? > > Hmm... "!(!previous_frame_was_muted && !current_frame_should_be_muted))"?? My > head exploded right there. I think my approach is easier to read. I agree with the explosion. But your revised variant is definitely better now. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:79: void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted, On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > Why not split this function in two versions, one called Mute that takes only > one > > AudioFrame as a parameter and does no fading, and one called FadedMute that > > does fading. That would make the behavior of the call where no fading is done > > much clearer. > > I think it makes sense to keep the logic within one function since we need the > same functionality in two places. I propose to instead store the previous frame > mute state in TransmitMixer so we avoid clicks there too. But I'll do so in a > separate CL. I think that with the parameter name changes that is much more fine. Acknowledged. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:86: memset(frame->data_, 0, sizeof(int16_t) * On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > I would prefer the sizeof to be done on frame->data[0] rather than on int16_t > as > > that would ensure proper memset behavior even if the type of frame->data > changes > > (https://google.github.io/styleguide/cppguide.html#sizeof). > > Yes, I blame copy+paste from the old version. :) Acknowledged. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:86: memset(frame->data_, 0, sizeof(int16_t) * On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > Since you know the size of data in this case (it is public in AudioFrame) it > > would be possible to assert on the size before them memset to ensure that the > > memset does not set memory outside of data_. > > Good idea! Done. Acknowledged. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:92: if (frame->samples_per_channel_ < kMuteFadeFrames) { On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > If frame->samples_per_channel_ == 0 is treated as a special case, this if > > statement can be done as a max operation. > > No, we still need to do the division in case we have fewer than kMuteFadeFrames. Yes, I agree. I looked at it a bit broader though: If you write it as RTC_DCHECK_GE(0u, frame->samples_per_channel_); size_t count = std::min(rame->samples_per_channel_,kMuteFadeFrames); float inc = 1.0f / count; You reduce the 8 lines of code required to 3 (or 2 depending how you count the DCHECK) (That code requires an early return for 0 length frames though). https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:94: if (count > 0) { On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > This should always be true, right? Afaics count == > > min(frame->samples_per_channel_, kMuteFadeFrames). Or can > > frame->samples_per_channel_ be zero? > > There's nothing in AudioFrame that prevents it from being zero. > > > If this code is needed for handling the case of frame->samples_per_channel_ == > > 0, I think it is better to treat that as a special case in a more explicit > > manner. > > The downside with that is that you'll *always* be paying the penalty of checking > for an empty frame, even though we can be quite certain that it will never > happen. True, but you anyway need to do that check for the division so there is no way around that. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:95: inc = 1.0f / count; On 2016/03/23 21:30:53, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > How common is it that frame->samples_per_channel_ < kMuteFadeFrames ? > > > > If it is common, I think it is better to always compute the division and > remove > > the kMuteFadeInc constant as that would simplify the code. > > Given that we usually store 10ms of data (though we can't be certain we *always* > do that, because of how AudioFrame is designed), 32kHz -> 320 samples/frame, > 16kHz -> 160 samples/frame, 8kHz -> 80 samples/frame. No, I wouldn't say < 128 > frames is very common. Would it be possible to reduce the fading time to 80 samples? Then perhaps it can be possible to DCHECK on the 10 ms content and be able to assume at least 10 ms of content? https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:110: On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > If you change the code so that you in this block know that muting transition > is > > needed an that the frame length is positive, the above code could be > simplified > > to something like: > > > > int count = std::min(frame->samples_per_channel_, kMuteFadeFrames); > > bool muted_to_unmuted = (previous_frame_was_muted && > > !current_frame_should_be_muted) > > int start_frame = ( muted_to_unmuted ? 0 : frame->samples_per_channel_ - > count); > > float inc = (1.f / count) * (muted_to_unmuted ? 1.f : 1.f); > > You leave out the check for frame->samples_per_channel_ == 0 here, and you > forget that we also need the initial gain value ('g'). > > If I add those to your simplification, it will always require 5 conditionals, > one division and one multiplication. My implementation usually requires 2 > conditionals and neither mul nor div outside the inner loop. But I agree mine is > longer to read, though I don't think overuse of the ternary operator contributes > to readability either. Acknowledged. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:110: On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > If you change the code so that you in this block know that muting transition > is > > needed an that the frame length is positive, the above code could be > simplified > > to something like: > > > > int count = std::min(frame->samples_per_channel_, kMuteFadeFrames); > > bool muted_to_unmuted = (previous_frame_was_muted && > > !current_frame_should_be_muted) > > int start_frame = ( muted_to_unmuted ? 0 : frame->samples_per_channel_ - > count); > > float inc = (1.f / count) * (muted_to_unmuted ? 1.f : 1.f); > > You leave out the check for frame->samples_per_channel_ == 0 here, and you > forget that we also need the initial gain value ('g'). > > If I add those to your simplification, it will always require 5 conditionals, > one division and one multiplication. My implementation usually requires 2 > conditionals and neither mul nor div outside the inner loop. But I agree mine is > longer to read, though I don't think overuse of the ternary operator contributes > to readability either. Acknowledged. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:112: const size_t end = (start + count); On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > This should always be start = frame->samples_per_channel_, right? Or is there > > any other reason for recomputing the end of the frame? > > end will be 'count' or 'frame->samples_per_channel_'. We fade up at start of > frame and fade down at end of frame. True, it feels a bit backward though to recompute it, but it is probably the most efficient way. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:115: g += inc; On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > You should ideally bound the gain to 0<=g<=1 using max/min operations. It is a > > bit overkill here though. > > No, it is designed to never shoot outside, and the regression tests (should) > verify that. Acknowledged. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:118: } On 2016/03/23 21:30:52, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > Since the number of channels typically is much smaller than the number of > frames > > I think it should be faster to nest the loops the other way, e.g.: > > > > float g_start = (muted_to_unmuted ? 0.f : 1.f); > > for (size_t j = 0; j < channels; ++j) { > > float g = g_start; > > for (size_t i = 0; j < count; i+=channels) { > > g += inc; > > frame->data_[start + i + j] *= g; > > } > > } > > > > Wdyt? > > I think that is evil to caches. For a 320 sample stereo frame we'll hit 20 cache > lines. Twice with your implementation, once with mine. Again, I agree it may > read prettier though. > > But thanks anyway for pointing it out - I realized an AudioFrame may actually > contain non-interleaved data, so I've added a DCHECK on the interleaved_ flag > (it appears as we never store non-interleaved data in it). Great find! We should probably deprecated that API call ASAP. Regarding the cache issue, you are definitely right. My concern was that the nested loop was difficult for the compiler to optimize, but probably it can do that well. The loop can be rewritten to be more cache friendly for the variant of loop nesting I proposed but if you think this will be fine for the compiler, lets keep it like this. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:272: int SetInputMute(bool enable); On 2016/03/23 21:30:53, the sun wrote: > On 2016/03/23 15:57:04, peah-webrtc wrote: > > What is motivates the name change to InputMute? Can there be an OutputMute? I > > think that it should be fine to continue naming it Mute. To me this > name-change > > makes the purpose of the function less intuitive. A channel should only be > muted > > or not muted, I would not be able to tell how an InputMuted channel behaves. > > The voe::Channel is duplex. SetMute() could mean to mute either the input or the > output. The functions on the VoEVolumeControl interface (to which this class is > tightly tied) calling these functions are called SetInputMute() and > GetInputMute(). Yes, I saw the other InputMute function but this change, or naming does not make sense to me, in particular since there is no OutputMute. Most likely it is because I don't yet know the voe code but could you please explain what the difference between an input and output mute would be. And also, as the voe::Channel is duplex, how you determine which is the input and which is the output. https://codereview.webrtc.org/1810413002/diff/80001/webrtc/voice_engine/trans... File webrtc/voice_engine/transmit_mixer.cc (left): https://codereview.webrtc.org/1810413002/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transmit_mixer.cc:365: } Sorry for the late comment on this, but why is there no fading added in transmit mixer? Or was that what was to be added in another CL?
https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... File webrtc/modules/utility/source/audio_frame_operations.cc (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:92: if (frame->samples_per_channel_ < kMuteFadeFrames) { On 2016/03/24 07:04:54, peah-webrtc wrote: > On 2016/03/23 21:30:52, the sun wrote: > > On 2016/03/23 15:57:04, peah-webrtc wrote: > > > If frame->samples_per_channel_ == 0 is treated as a special case, this if > > > statement can be done as a max operation. > > > > No, we still need to do the division in case we have fewer than > kMuteFadeFrames. > Yes, I agree. > > I looked at it a bit broader though: > If you write it as > RTC_DCHECK_GE(0u, frame->samples_per_channel_); > size_t count = std::min(rame->samples_per_channel_,kMuteFadeFrames); > float inc = 1.0f / count; > > You reduce the 8 lines of code required to 3 (or 2 depending how you count the > DCHECK) (That code requires an early return for 0 length frames though). > What I'd REALLY like to do instead is to send around same-size frames (e.g. 64 or 128), so we don't have to do this kind of nonsense. If you want the code to be really efficient, the early return in case of 0 length shouldn't even be in this function but somewhere much higher up the chain. Once we do get around to passing same-size frames, the version using constants is the most efficient and the conditional in question can simply be removed. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:94: if (count > 0) { On 2016/03/24 07:04:54, peah-webrtc wrote: > On 2016/03/23 21:30:52, the sun wrote: > > On 2016/03/23 15:57:04, peah-webrtc wrote: > > > This should always be true, right? Afaics count == > > > min(frame->samples_per_channel_, kMuteFadeFrames). Or can > > > frame->samples_per_channel_ be zero? > > > > There's nothing in AudioFrame that prevents it from being zero. > > > > > If this code is needed for handling the case of frame->samples_per_channel_ > == > > > 0, I think it is better to treat that as a special case in a more explicit > > > manner. > > > > The downside with that is that you'll *always* be paying the penalty of > checking > > for an empty frame, even though we can be quite certain that it will never > > happen. > > True, but you anyway need to do that check for the division so there is no way > around that. Yes, but that is not the common case. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:95: inc = 1.0f / count; On 2016/03/24 07:04:54, peah-webrtc wrote: > On 2016/03/23 21:30:53, the sun wrote: > > On 2016/03/23 15:57:04, peah-webrtc wrote: > > > How common is it that frame->samples_per_channel_ < kMuteFadeFrames ? > > > > > > If it is common, I think it is better to always compute the division and > > remove > > > the kMuteFadeInc constant as that would simplify the code. > > > > Given that we usually store 10ms of data (though we can't be certain we > *always* > > do that, because of how AudioFrame is designed), 32kHz -> 320 samples/frame, > > 16kHz -> 160 samples/frame, 8kHz -> 80 samples/frame. No, I wouldn't say < 128 > > frames is very common. > > Would it be possible to reduce the fading time to 80 samples? Then perhaps it > can be possible to DCHECK on the 10 ms content and be able to assume at least 10 > ms of content? That would be nice, but AudioFrame doesn't give us any such guarantees. If we change AudioFrame to do so, then I'd be more than happy to get rid of all the special casing. However I think that's a job much larger than this particular change. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:112: const size_t end = (start + count); On 2016/03/24 07:04:54, peah-webrtc wrote: > On 2016/03/23 21:30:52, the sun wrote: > > On 2016/03/23 15:57:04, peah-webrtc wrote: > > > This should always be start = frame->samples_per_channel_, right? Or is > there > > > any other reason for recomputing the end of the frame? > > > > end will be 'count' or 'frame->samples_per_channel_'. We fade up at start of > > frame and fade down at end of frame. > > True, it feels a bit backward though to recompute it, but it is probably the > most efficient way. You're right, the code is nicer if I just set up end like start, in the conditional above. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/modules/utility/so... webrtc/modules/utility/source/audio_frame_operations.cc:118: } On 2016/03/24 07:04:54, peah-webrtc wrote: > On 2016/03/23 21:30:52, the sun wrote: > > On 2016/03/23 15:57:04, peah-webrtc wrote: > > > Since the number of channels typically is much smaller than the number of > > frames > > > I think it should be faster to nest the loops the other way, e.g.: > > > > > > float g_start = (muted_to_unmuted ? 0.f : 1.f); > > > for (size_t j = 0; j < channels; ++j) { > > > float g = g_start; > > > for (size_t i = 0; j < count; i+=channels) { > > > g += inc; > > > frame->data_[start + i + j] *= g; > > > } > > > } > > > > > > Wdyt? > > > > I think that is evil to caches. For a 320 sample stereo frame we'll hit 20 > cache > > lines. Twice with your implementation, once with mine. Again, I agree it may > > read prettier though. > > > > But thanks anyway for pointing it out - I realized an AudioFrame may actually > > contain non-interleaved data, so I've added a DCHECK on the interleaved_ flag > > (it appears as we never store non-interleaved data in it). > > Great find! We should probably deprecated that API call ASAP. https://codereview.webrtc.org/1830713003/ > Regarding the cache issue, you are definitely right. My concern was that the > nested loop was difficult for the compiler to optimize, but probably it can do > that well. The loop can be rewritten to be more cache friendly for the variant > of loop nesting > I proposed but if you think this will be fine for the compiler, lets keep it > like this. Expanded the loops. https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/1810413002/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:272: int SetInputMute(bool enable); On 2016/03/24 07:04:54, peah-webrtc wrote: > On 2016/03/23 21:30:53, the sun wrote: > > On 2016/03/23 15:57:04, peah-webrtc wrote: > > > What is motivates the name change to InputMute? Can there be an OutputMute? > I > > > think that it should be fine to continue naming it Mute. To me this > > name-change > > > makes the purpose of the function less intuitive. A channel should only be > > muted > > > or not muted, I would not be able to tell how an InputMuted channel behaves. > > > > The voe::Channel is duplex. SetMute() could mean to mute either the input or > the > > output. The functions on the VoEVolumeControl interface (to which this class > is > > tightly tied) calling these functions are called SetInputMute() and > > GetInputMute(). > > Yes, I saw the other InputMute function but this change, or naming does not make > sense to me, in particular since there is no OutputMute. Most likely it is > because I don't yet know the voe code but could you please explain what the > difference between an input and output mute would be. Input mute = mute the microphone Output mute = mute the speaker The reason I changed it was that I needed to look through the code to make sure "mute" was only used for muting the input. Why would there be the distinction between "input mute" in VoEVolume, and just "mute" in voe::Channel? I figure it makes sense to be consistent. > And also, as the > voe::Channel is duplex, how you determine which is the input and which is the > output. The voe::Channel can be an input, and output or both at the same time. There's no determining. Yes, we want to get rid of this behavior. https://codereview.webrtc.org/1810413002/diff/80001/webrtc/voice_engine/trans... File webrtc/voice_engine/transmit_mixer.cc (left): https://codereview.webrtc.org/1810413002/diff/80001/webrtc/voice_engine/trans... webrtc/voice_engine/transmit_mixer.cc:365: } On 2016/03/24 07:04:54, peah-webrtc wrote: > Sorry for the late comment on this, but why is there no fading added in transmit > mixer? Or was that what was to be added in another CL? Exactly.
Nice CL! This will definitely achieve a quality increase with a measurable MOS gain. lgtm One small nit though, thanks for the loop unrolling, but it becomes quite a lot of code. I think that is fully fine though if you perceive that the compiler could have a problem with it. Do you think that the cache problem will be that big if the channel loop is outside the samples loop? The code is still very local, and the part of the frames being processed are fairly small so I would expect that the whole data segment, including both channels, that is processed in the loop to likely stay in the cache. But you know this better than me so please do as you want with that.
The CQ bit was checked by solenberg@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1810413002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1810413002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by solenberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tina.legrand@webrtc.org, peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/1810413002/#ps120001 (title: "undo loop unrolling")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1810413002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1810413002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by solenberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1810413002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1810413002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Avoid clicks when muting/unmuting a voe::Channel. Muting/unmuting is triggered in the PeerConnection API by calling setEnable() on an audio track. BUG=webrtc:5671 ========== to ========== Avoid clicks when muting/unmuting a voe::Channel. Muting/unmuting is triggered in the PeerConnection API by calling setEnable() on an audio track. BUG=webrtc:5671 Committed: https://crrev.com/1c2af8e3191e69aad6e233a6b2402579dcdbb740 Cr-Commit-Position: refs/heads/master@{#12121} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1c2af8e3191e69aad6e233a6b2402579dcdbb740 Cr-Commit-Position: refs/heads/master@{#12121} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
