NetEq::GetPlayoutTimestamp returns empty during CNG

NetEq::GetPlayoutTimestamp returns empty during CNG

With this change, the return value from NetEq::GetPlayoutTimestamp is
empty if the latest call to NetEq::GetAudio resulted in comfort noise
(codec-internal or external) being played out. This is because the
playout timestamp is not updated during CNG, and can therefore not be
trusted.

A few unit tests were updated to reflect the change.

BUG=webrtc:5669
Committed: https://crrev.com/0d96ab7dc4053ae5793aac018379a860b2534358
Cr-Commit-Position: refs/heads/master@{#12268}

[hlundin-webrtc, 2016-04-06 09:54:11]

Check for empty timestamp during CNG (and PLC)

[hlundin-webrtc, 2016-04-06 09:56:19]

Patchset #1 (id:1) has been deleted

[hlundin-webrtc, 2016-04-06 11:13:58]

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

[hlundin-webrtc, 2016-04-06 11:13:58]

Minyue,

Please, review this final CL for fixing the CNG timestamp bug.

Thanks!

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:669:
EXPECT_CALL(mock_decoder, PacketDuration(_, kPayloadLengthBytes))
These new EXPEC_CALLs are unrelated to this CL, but I added them to silence a
noisy test output.

[minyue-webrtc, 2016-04-06 13:39:42]

Great work! See some small comments

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:729: int
expected_timestamp_increment[8] = {
Is there a better to change this.

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

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1500: out_frame_.timestamp_
+ out_frame_.samples_per_channel_);
Curious. Is it important that out_frame_.timestamp_ stays?

[hlundin-webrtc, 2016-04-06 15:25:46]

Consolidate the test code

[hlundin-webrtc, 2016-04-06 15:27:57]

PTAL again.

I did a smaller simplification in patch set 2. Then I went crazy and reduced
code duplication by creating a lambda in patch set 3. Let me know what you
think.

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:729: int
expected_timestamp_increment[8] = {
On 2016/04/06 13:39:42, minyue-webrtc wrote:
> Is there a better to change this.

How about now?

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

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1500: out_frame_.timestamp_
+ out_frame_.samples_per_channel_);
On 2016/04/06 13:39:42, minyue-webrtc wrote:
> Curious. Is it important that out_frame_.timestamp_ stays?

Since AudioFrame::timestamp_ is not an rtc::Optional, we must provide a value
also during CNG periods. (Changing the AudioFrame implementation is beyond the
scope of this work.) So, AudioFrame::timestamp_ retains the old behavior of
NetEq::GetPlayoutTimestamp(), i.e., the timstamp is not updated during CNG. This
test verifies that too.

[hlundin-webrtc, 2016-04-06 15:40:52]

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

[commit-bot: I haz the power, 2016-04-06 15:41:01]

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

[commit-bot: I haz the power, 2016-04-06 17:41:35]

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

[commit-bot: I haz the power, 2016-04-06 17:41:36]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[minyue-webrtc, 2016-04-06 18:13:08]

lgtm

LGTM + a couple of questions

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

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1500: out_frame_.timestamp_
+ out_frame_.samples_per_channel_);
On 2016/04/06 15:27:56, hlundin-webrtc wrote:
> On 2016/04/06 13:39:42, minyue-webrtc wrote:
> > Curious. Is it important that out_frame_.timestamp_ stays?
> 
> Since AudioFrame::timestamp_ is not an rtc::Optional, we must provide a value
> also during CNG periods. (Changing the AudioFrame implementation is beyond the
> scope of this work.) So, AudioFrame::timestamp_ retains the old behavior of
> NetEq::GetPlayoutTimestamp(), i.e., the timstamp is not updated during CNG.
This
> test verifies that too.

Should it not updated during CNG?  What Is that timestamp used for?

https://codereview.webrtc.org/1861303002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/1861303002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:741: // Lambda for
verifying the timestamps.
I like it

https://codereview.webrtc.org/1861303002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:760: SCOPED_TRACE("");
what does this do?

[hlundin-webrtc, 2016-04-06 18:21:40]

Thanks for the swift reviews! I'm going to land this when the last try bot is
done.

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

https://codereview.webrtc.org/1861303002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1500: out_frame_.timestamp_
+ out_frame_.samples_per_channel_);
On 2016/04/06 18:13:07, minyue-webrtc wrote:
> On 2016/04/06 15:27:56, hlundin-webrtc wrote:
> > On 2016/04/06 13:39:42, minyue-webrtc wrote:
> > > Curious. Is it important that out_frame_.timestamp_ stays?
> > 
> > Since AudioFrame::timestamp_ is not an rtc::Optional, we must provide a
value
> > also during CNG periods. (Changing the AudioFrame implementation is beyond
the
> > scope of this work.) So, AudioFrame::timestamp_ retains the old behavior of
> > NetEq::GetPlayoutTimestamp(), i.e., the timstamp is not updated during CNG.
> This
> > test verifies that too.
> 
> Should it not updated during CNG?  What Is that timestamp used for?

It is currently not used at all (iirc), but since the timestamp_ field is
available in the AudioFrame, and it has a clear meaning in this context, I
thought it was just as well to populate it. But currently it is not updated
during CNG, just like PlayoutTimestamp used not to be (now it is empty instead
during CNG).

https://codereview.webrtc.org/1861303002/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/1861303002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:760: SCOPED_TRACE("");
On 2016/04/06 18:13:07, minyue-webrtc wrote:
> what does this do?

If a failure happens in the lambda, a printout will be triggered from this
SCOPED_TRACE, too. Like this:

../../webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:750: Failure
Value of: ts
  Actual: false
Expected: true
i = 2
Google Test trace:
../../webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:762: 

That is, you get one line number from within the lambda (750 in this example),
but also a line number from this SCOPED_TRACE (762), which helps the user figure
out which invokation of the lambda failed. (You can also provide a string to
print to the SCOPED_TRACE, but I thought that didn't help in this case.)

[hlundin-webrtc, 2016-04-06 19:26:49]

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

[commit-bot: I haz the power, 2016-04-06 19:26:56]

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

[commit-bot: I haz the power, 2016-04-06 19:28:30]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-04-06 19:28:34]

Description was changed from

==========
NetEq::GetPlayoutTimestamp returns empty during CNG

With this change, the return value from NetEq::GetPlayoutTimestamp is
empty if the latest call to NetEq::GetAudio resulted in comfort noise
(codec-internal or external) being played out. This is because the
playout timestamp is not updated during CNG, and can therefore not be
trusted.

A few unit tests were updated to reflect the change.

BUG=webrtc:5669
==========

to

==========
NetEq::GetPlayoutTimestamp returns empty during CNG

With this change, the return value from NetEq::GetPlayoutTimestamp is
empty if the latest call to NetEq::GetAudio resulted in comfort noise
(codec-internal or external) being played out. This is because the
playout timestamp is not updated during CNG, and can therefore not be
trusted.

A few unit tests were updated to reflect the change.

BUG=webrtc:5669

Committed: https://crrev.com/0d96ab7dc4053ae5793aac018379a860b2534358
Cr-Commit-Position: refs/heads/master@{#12268}
==========

[commit-bot: I haz the power, 2016-04-06 19:28:35]

Patchset 3 (id:??) landed as
https://crrev.com/0d96ab7dc4053ae5793aac018379a860b2534358
Cr-Commit-Position: refs/heads/master@{#12268}