NetEq: Implement muted output

NetEq: Implement muted output

This CL implements the muted output functionality in NetEq. Tests are
added. The feature is currently off by default, and AcmReceiver makes
sure that the muted state is not engaged.

BUG=webrtc:5608
Committed: https://crrev.com/7a926812d8b6cba1e12f3d679c92052158a9e82f
Cr-Commit-Position: refs/heads/master@{#12711}

[hlundin-webrtc, 2016-05-10 12:27:32]

Patchset #1 (id:1) has been deleted

[hlundin-webrtc, 2016-05-10 12:27:40]

Patchset #1 (id:20001) has been deleted

[hlundin-webrtc, 2016-05-10 12:28:14]

henrik.lundin@webrtc.org changed reviewers:
+ minyue@webrtc.org

[hlundin-webrtc, 2016-05-10 12:28:15]

Minyue,

Please, review this second CL for NetEq muted state.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/acm_receiver.cc (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:145: RTC_DCHECK(!muted_output);
This DCHECK will be removed when the functionality is implemented in ACM.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:828: audio_frame->timestamp_ =
The setting of timestamp is identical to line 1028 at the end of
GetAudioInternal.

[minyue-webrtc, 2016-05-11 11:29:54]

nice work! See my a few comments.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/include/neteq.h (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/include/neteq.h:166: virtual int
GetAudio(AudioFrame* audio_frame, bool* muted_output) = 0;
suggest name: output_muted or even simply muted.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:208: 
how about allowing the bool* to be nullptr. calling does not need to define for
it if it is not cared.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:823: if (enable_muted_state_ &&
expand_->Muted() && packet_buffer_->Empty()) {
what if packet_buffer_ are not empty but all old packets?

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:824: RTC_DCHECK_EQ(last_mode_,
kModeExpand);
why should this be a CHECK but not a if condition?

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:876:
ASSERT_FALSE(muted_output);
does this need to be assert? + all similar places

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1573: // Verify that output
audio is not written during muted mode. Other parameters
is this important?

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1576: for (auto& d :
new_frame.data_) {
and how about memset and memcmp

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1594: PopulateRtpInfo(0, 16
* 10 * counter, &rtp_info);
what if the timestamp is not  16 * 10 * counter

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1659: sizeof(a.data_[0])) !=
0) {
weird wrapping

[hlundin-webrtc, 2016-05-12 07:20:23]

Addressing comments in the test

[hlundin-webrtc, 2016-05-12 07:40:28]

Addressing review comments

[hlundin-webrtc, 2016-05-12 07:41:19]

Name change muted_output -> muted

[hlundin-webrtc, 2016-05-12 07:41:36]

Patchset #2 (id:60001) has been deleted

[hlundin-webrtc, 2016-05-12 07:44:40]

PTAL again. Patch set 2 contains some minor updates; patch set 3 contains only
the name change of muted_output to muted.

Thanks!

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/include/neteq.h (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/include/neteq.h:166: virtual int
GetAudio(AudioFrame* audio_frame, bool* muted_output) = 0;
On 2016/05/11 11:29:53, minyue-webrtc wrote:
> suggest name: output_muted or even simply muted.

Done.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:208: 
On 2016/05/11 11:29:54, minyue-webrtc wrote:
> how about allowing the bool* to be nullptr. calling does not need to define
for
> it if it is not cared.

That is going to be a bit tricky, I think. What if you have enabled muted state
(through config.enable_muted_state), but you don't provide a valid pointer?
Should NetEq still be allowed to enter muted state, and leave the output samples
possibly undefined?

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:823: if (enable_muted_state_ &&
expand_->Muted() && packet_buffer_->Empty()) {
On 2016/05/11 11:29:53, minyue-webrtc wrote:
> what if packet_buffer_ are not empty but all old packets?

I think we want to exit muted state for this. It is safer, I think. It should be
rare, so exiting muted state for this reason is an acceptable cost IMO.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:824: RTC_DCHECK_EQ(last_mode_,
kModeExpand);
On 2016/05/11 11:29:53, minyue-webrtc wrote:
> why should this be a CHECK but not a if condition?

The logic in NetEq should lead to this being always true. Otherwise, we are
doing something wrong in the programming. Hence, DCHECK.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:876:
ASSERT_FALSE(muted_output);
On 2016/05/11 11:29:54, minyue-webrtc wrote:
> does this need to be assert? + all similar places

I choose to use ASSERT in all places where the test may read the samples from
the output frame after the GetAudio call. Since the samples are not set by NetEq
when muted_output is true, I'd rather abort the test than continue with
undefined data.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1573: // Verify that output
audio is not written during muted mode. Other parameters
On 2016/05/11 11:29:54, minyue-webrtc wrote:
> is this important?

Yes. One of the points with muted state is that we do not even spend cycles on
writing zero-samples. However, the other parameters in AudioFrame must still be
populated.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1576: for (auto& d :
new_frame.data_) {
On 2016/05/11 11:29:54, minyue-webrtc wrote:
> and how about memset and memcmp

I cannot set a non-zero value to elements wider than a byte using memset. And
for memcmp, I'd need a reference array, which I don't have. I thought this was
the most compact way to write it.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1594: PopulateRtpInfo(0, 16
* 10 * counter, &rtp_info);
On 2016/05/11 11:29:54, minyue-webrtc wrote:
> what if the timestamp is not  16 * 10 * counter

I changed to kSamples to be consistent within the test.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1659: sizeof(a.data_[0])) !=
0) {
On 2016/05/11 11:29:54, minyue-webrtc wrote:
> weird wrapping

Better?

[minyue-webrtc, 2016-05-12 10:56:07]

Thanks!

It LG if you consider some small comments + offline discuss.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:208: 
On 2016/05/12 07:44:40, hlundin-webrtc wrote:
> On 2016/05/11 11:29:54, minyue-webrtc wrote:
> > how about allowing the bool* to be nullptr. calling does not need to define
> for
> > it if it is not cared.
> 
> That is going to be a bit tricky, I think. What if you have enabled muted
state
> (through config.enable_muted_state), but you don't provide a valid pointer?
> Should NetEq still be allowed to enter muted state, and leave the output
samples
> possibly undefined?

good point.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:876:
ASSERT_FALSE(muted_output);
On 2016/05/12 07:44:40, hlundin-webrtc wrote:
> On 2016/05/11 11:29:54, minyue-webrtc wrote:
> > does this need to be assert? + all similar places
> 
> I choose to use ASSERT in all places where the test may read the samples from
> the output frame after the GetAudio call. Since the samples are not set by
NetEq
> when muted_output is true, I'd rather abort the test than continue with
> undefined data.

True, and with this regards, many EXPECT can become ASSERT in this test also.
But of course, not a duty in this CL

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1573: // Verify that output
audio is not written during muted mode. Other parameters
On 2016/05/12 07:44:40, hlundin-webrtc wrote:
> On 2016/05/11 11:29:54, minyue-webrtc wrote:
> > is this important?
> 
> Yes. One of the points with muted state is that we do not even spend cycles on
> writing zero-samples. However, the other parameters in AudioFrame must still
be
> populated.

But this is not sufficient in checking the efficiency of the implementation.
On the other hand, maintaining audio frame untouched when muted_output is not a
promise from the API perspective.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1659: sizeof(a.data_[0])) !=
0) {
On 2016/05/12 07:44:40, hlundin-webrtc wrote:
> On 2016/05/11 11:29:54, minyue-webrtc wrote:
> > weird wrapping
> 
> Better?

yes

[hlundin-webrtc, 2016-05-12 12:04:46]

Break out subfunctions in test

[hlundin-webrtc, 2016-05-12 12:05:08]

Add new tests

[hlundin-webrtc, 2016-05-12 12:06:20]

Restructured and added new tests. PTAL again.

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1965733002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1573: // Verify that output
audio is not written during muted mode. Other parameters
On 2016/05/12 10:56:07, minyue-webrtc wrote:
> On 2016/05/12 07:44:40, hlundin-webrtc wrote:
> > On 2016/05/11 11:29:54, minyue-webrtc wrote:
> > > is this important?
> > 
> > Yes. One of the points with muted state is that we do not even spend cycles
on
> > writing zero-samples. However, the other parameters in AudioFrame must still
> be
> > populated.
> 
> But this is not sufficient in checking the efficiency of the implementation.
> On the other hand, maintaining audio frame untouched when muted_output is not
a
> promise from the API perspective.

I updated the comment in neteq.h regarding this. See the diff between ps1 and
ps2. The comment says that the data_ (samples) may be left untouched when
muted==true.

[minyue-webrtc, 2016-05-12 12:28:00]

lgtm

[hlundin-webrtc, 2016-05-12 12:29:09]

The CQ bit was checked by henrik.lundin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-12 12:29:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1965733002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1965733002/140001

[commit-bot: I haz the power, 2016-05-12 12:36:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-12 12:36:26]

Dry run: Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/13253)

[hlundin-webrtc, 2016-05-12 20:49:04]

The CQ bit was checked by henrik.lundin@webrtc.org

[commit-bot: I haz the power, 2016-05-12 20:49:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1965733002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1965733002/140001

[commit-bot: I haz the power, 2016-05-12 20:51:33]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-05-12 20:51:41]

Description was changed from

==========
NetEq: Implement muted output

This CL implements the muted output functionality in NetEq. Tests are
added. The feature is currently off by default, and AcmReceiver makes
sure that the muted state is not engaged.

BUG=webrtc:5608
==========

to

==========
NetEq: Implement muted output

This CL implements the muted output functionality in NetEq. Tests are
added. The feature is currently off by default, and AcmReceiver makes
sure that the muted state is not engaged.

BUG=webrtc:5608

Committed: https://crrev.com/7a926812d8b6cba1e12f3d679c92052158a9e82f
Cr-Commit-Position: refs/heads/master@{#12711}
==========

[commit-bot: I haz the power, 2016-05-12 20:51:42]

Patchset 5 (id:??) landed as
https://crrev.com/7a926812d8b6cba1e12f3d679c92052158a9e82f
Cr-Commit-Position: refs/heads/master@{#12711}