Avoid clicks when muting/unmuting a voe::Channel.

webrtc/modules/utility/source/audio_frame_operations.cc

Index: webrtc/modules/utility/source/audio_frame_operations.cc
diff --git a/webrtc/modules/utility/source/audio_frame_operations.cc b/webrtc/modules/utility/source/audio_frame_operations.cc
index fe09d7972f7fa2dd77fd25708b3e05fefa6b5625..beb5ba6277ca3b19fb28cf54bd8728e10a7677c4 100644
--- a/webrtc/modules/utility/source/audio_frame_operations.cc
+++ b/webrtc/modules/utility/source/audio_frame_operations.cc
@@ -10,8 +10,15 @@
 
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/modules/utility/include/audio_frame_operations.h"
+#include "webrtc/base/checks.h"
 
 namespace webrtc {
+namespace  {

[tlegrand-webrtc, 2016/03/23 13:42:41]

Is the extra namespace needed?

[the sun, 2016/03/23 14:25:59]

Yes, anything internal to the implementation should go in an anonymous
namespace.

+
+// 2.7ms @ 48kHz, 4ms @ 32kHz, 8ms @ 16kHz.
+const size_t kMuteFadeFrames = 128;
+const float kMuteFadeInc = 1.0f / kMuteFadeFrames;

[tlegrand-webrtc, 2016/03/23 13:42:41]

Not sure what the style guide says, but I'd find it easier to read with a new
line here.

[the sun, 2016/03/23 14:25:59]

Done.

+}  // namespace {
 
 void AudioFrameOperations::MonoToStereo(const int16_t* src_audio,
                                         size_t samples_per_channel,
@@ -69,9 +76,48 @@ void AudioFrameOperations::SwapStereoChannels(AudioFrame* frame) {
   }
 }
 
-void AudioFrameOperations::Mute(AudioFrame& frame) {
-  memset(frame.data_, 0, sizeof(int16_t) *
-      frame.samples_per_channel_ * frame.num_channels_);
+void AudioFrameOperations::Mute(AudioFrame* frame, bool start_muted,

[peah-webrtc, 2016/03/23 15:57:04]

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. 

[peah-webrtc, 2016/03/23 15:57:04]

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?

[peah-webrtc, 2016/03/23 15:57:04]

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.

[the sun, 2016/03/23 21:30:52]

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.

[the sun, 2016/03/23 21:30:52]

Hmm... "!(!previous_frame_was_muted && !current_frame_should_be_muted))"?? My
head exploded right there. I think my approach is easier to read.

[the sun, 2016/03/23 21:30:53]

Good suggestion. Makes the code much easier to read.

[peah-webrtc, 2016/03/24 07:04:54]

I agree with the explosion. But your revised variant is definitely better now.

[peah-webrtc, 2016/03/24 07:04:54]

I think that with the parameter name changes that is much more fine.
Acknowledged.

[peah-webrtc, 2016/03/24 07:04:54]

Acknowledged.

+                                bool end_muted) {

[tlegrand-webrtc, 2016/03/23 13:42:41]

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?

[the sun, 2016/03/23 14:25:59]

Added in the .h.

+  RTC_DCHECK(frame);
+  if (!start_muted && !end_muted) {
+    // Not muted.
+  } else if (start_muted && end_muted) {
+    // Frame fully muted.
+    memset(frame->data_, 0, sizeof(int16_t) *

[peah-webrtc, 2016/03/23 15:57:04]

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_.

[peah-webrtc, 2016/03/23 15:57:04]

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).

[the sun, 2016/03/23 21:30:52]

Yes, I blame copy+paste from the old version. :)

[the sun, 2016/03/23 21:30:52]

Good idea! Done.

[peah-webrtc, 2016/03/24 07:04:54]

Acknowledged.

[peah-webrtc, 2016/03/24 07:04:54]

Acknowledged.

+        frame->samples_per_channel_ * frame->num_channels_);
+  } else {
+    // Limit number of samples to fade, if frame isn't long enough.
+    size_t count = kMuteFadeFrames;
+    float inc = kMuteFadeInc;
+    if (frame->samples_per_channel_ < kMuteFadeFrames) {

[peah-webrtc, 2016/03/23 15:57:04]

If frame->samples_per_channel_ == 0 is treated as a special case, this if
statement can be done as a max operation.

[the sun, 2016/03/23 21:30:52]

No, we still need to do the division in case we have fewer than kMuteFadeFrames.

[peah-webrtc, 2016/03/24 07:04:54]

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).

[the sun, 2016/03/24 09:33:15]

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.

+      count = frame->samples_per_channel_;
+      if (count > 0) {

[peah-webrtc, 2016/03/23 15:57:04]

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.

[the sun, 2016/03/23 21:30:52]

There's nothing in AudioFrame that prevents it from being zero.
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.

[peah-webrtc, 2016/03/24 07:04:54]

True, but you anyway need to do that check for the division so there is no way
around that.

[the sun, 2016/03/24 09:33:15]

Yes, but that is not the common case.

+        inc = 1.0f / count;

[peah-webrtc, 2016/03/23 15:57:04]

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.

[the sun, 2016/03/23 21:30:53]

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.

[peah-webrtc, 2016/03/24 07:04:54]

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?

[the sun, 2016/03/24 09:33:15]

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.

+      }
+    }
+
+    // Ramp up the first N samples of frame.

[tlegrand-webrtc, 2016/03/23 13:42:41]

N-> "count", or?

The comment should probably mention ramp down as well.

[the sun, 2016/03/23 14:25:59]

Done.
It does so a few lines down, when we set up the parameters for fade out.

+    size_t start = 0;
+    float g = 0.0f;
+    if (!start_muted && end_muted) {
+      // Ramp down the last N samples of frame.
+      start = frame->samples_per_channel_ - count;
+      g = 1.0f;
+      inc = -inc;
+    } else {
+      RTC_DCHECK(start_muted && !end_muted);

[peah-webrtc, 2016/03/23 15:57:04]

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.

[the sun, 2016/03/23 21:30:52]

Good call, I've simplified the logic a bit. This assert is to verify that the
logic within the function isn't messed up.

+    }
+

[peah-webrtc, 2016/03/23 15:57:04]

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);

[the sun, 2016/03/23 21:30:52]

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.

[peah-webrtc, 2016/03/24 07:04:54]

Acknowledged.

[peah-webrtc, 2016/03/24 07:04:54]

Acknowledged.

+    // Perform fade.
+    const size_t end = (start + count);

[peah-webrtc, 2016/03/23 15:57:04]

This should always be start = frame->samples_per_channel_, right? Or is there
any other reason for recomputing the end of the frame?

[the sun, 2016/03/23 21:30:52]

end will be 'count' or 'frame->samples_per_channel_'. We fade up at start of
frame and fade down at end of frame.

[peah-webrtc, 2016/03/24 07:04:54]

True, it feels a bit backward though to recompute it, but it is probably the
most efficient way.

[the sun, 2016/03/24 09:33:15]

You're right, the code is nicer if I just set up end like start, in the
conditional above.

+    const size_t channels = frame->num_channels_;
+    for (size_t i = start * channels; i < end * channels; i += channels) {
+      g += inc;

[peah-webrtc, 2016/03/23 15:57:04]

You should ideally bound the gain to 0<=g<=1 using max/min operations. It is a
bit overkill here though.

[the sun, 2016/03/23 21:30:52]

No, it is designed to never shoot outside, and the regression tests (should)
verify that.

[peah-webrtc, 2016/03/24 07:04:54]

Acknowledged.

+      for (size_t j = 0; j < channels; ++j) {
+        frame->data_[i + j] = static_cast<int16_t>(g * frame->data_[i + j]);
+      }

[peah-webrtc, 2016/03/23 15:57:04]

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?

[the sun, 2016/03/23 21:30:52]

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).

[peah-webrtc, 2016/03/24 07:04:54]

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.

[the sun, 2016/03/24 09:33:15]

https://codereview.webrtc.org/1830713003/
Expanded the loops.

+    }
+  }
 }
 
 int AudioFrameOperations::Scale(float left, float right, AudioFrame& frame) {