Change NetEq::GetPlayoutTimestamp to return an rtc::Optional<uint32_t>

Change NetEq::GetPlayoutTimestamp to return an rtc::Optional<uint32_t>

This is in preparation for changes to when the playout timestamp is
valid.

BUG=webrtc:5669
Committed: https://crrev.com/9a410dd082a650a7ded02ac5b2bd3ca6f45b644a
Cr-Commit-Position: refs/heads/master@{#12256}

[hlundin-webrtc, 2016-04-04 15:39:07]

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

[hlundin-webrtc, 2016-04-04 15:39:07]

Please, take a look.

[minyue-webrtc, 2016-04-04 15:53:51]

good work! some minor comments.

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/acm_receiver.cc (right):

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:202: *playout_timestamp -
"*playout_timestamp -" may be moved to follow after "audio_frame->timestamp_ ="

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1527: return
playout_timestamp ? *playout_timestamp : 0;
consider using value_or()

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/t...
File webrtc/modules/audio_coding/test/APITest.cc (right):

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/t...
webrtc/modules/audio_coding/test/APITest.cc:704: estimDelay = (double)
((uint32_t)(inTimestamp - outTimestamp))
no value assigned to outTimestam

[hlundin-webrtc, 2016-04-04 21:02:54]

PTAL again.

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/acm_receiver.cc (right):

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:202: *playout_timestamp -
On 2016/04/04 15:53:50, minyue-webrtc wrote:
> "*playout_timestamp -" may be moved to follow after "audio_frame->timestamp_
="

Done. The extra line break is what git cl format gave me, but I do agree that it
is prettier to keep it together on one line.

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1527: return
playout_timestamp ? *playout_timestamp : 0;
On 2016/04/04 15:53:50, minyue-webrtc wrote:
> consider using value_or()

Nice. I didn't know about that one. Done.

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/t...
File webrtc/modules/audio_coding/test/APITest.cc (right):

https://codereview.webrtc.org/1853183002/diff/1/webrtc/modules/audio_coding/t...
webrtc/modules/audio_coding/test/APITest.cc:704: estimDelay = (double)
((uint32_t)(inTimestamp - outTimestamp))
On 2016/04/04 15:53:50, minyue-webrtc wrote:
> no value assigned to outTimestam

Ouch. My bad. Thanks for catching this.

[minyue-webrtc, 2016-04-05 07:58:03]

lgtm

[kwiberg-webrtc, 2016-04-05 14:04:03]

lgtm (with some suggestions for the tests)

This is exactly the sort of thing I had in mind when introducing rtc::Optional.

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

https://codereview.webrtc.org/1853183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:483:
*neteq_->GetPlayoutTimestamp());
IIRC, Optional::operator* does a DCHECK, not a CHECK. So consider writing this
instead:

EXPECT_EQ(rtc::Optional<uint32_t>(...), neteq_->GetPlayoutTimestamp());

https://codereview.webrtc.org/1853183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:742:
EXPECT_EQ(*timestamp, *last_timestamp + expected_timestamp_increment[i]);
To avoid dereferencing invalid Optionals (which will crash if DCHECKS are
enabled), consider using ASSERT_TRUE when checking that they are valid.

https://codereview.webrtc.org/1853183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/test/delay_test.cc (right):

https://codereview.webrtc.org/1853183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/test/delay_test.cc:214:
static_cast<double>(encoding_sample_rate_hz_);
Consider ASSERTing that the optional was valid, to avoid crashing.

[hlundin-webrtc, 2016-04-05 14:16:58]

Fixing kwiberg's comments

[hlundin-webrtc, 2016-04-05 14:19:45]

Fixed. Will commit this.

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

https://codereview.webrtc.org/1853183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:483:
*neteq_->GetPlayoutTimestamp());
On 2016/04/05 14:04:02, kwiberg-webrtc wrote:
> IIRC, Optional::operator* does a DCHECK, not a CHECK. So consider writing this
> instead:
> 
> EXPECT_EQ(rtc::Optional<uint32_t>(...), neteq_->GetPlayoutTimestamp());

Done.

https://codereview.webrtc.org/1853183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:742:
EXPECT_EQ(*timestamp, *last_timestamp + expected_timestamp_increment[i]);
On 2016/04/05 14:04:02, kwiberg-webrtc wrote:
> To avoid dereferencing invalid Optionals (which will crash if DCHECKS are
> enabled), consider using ASSERT_TRUE when checking that they are valid.

Yes, I learned that the hard way in the next CL after this (where I already
corrected it). Now I will get merge conflicts...

https://codereview.webrtc.org/1853183002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/test/delay_test.cc (right):

https://codereview.webrtc.org/1853183002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/test/delay_test.cc:214:
static_cast<double>(encoding_sample_rate_hz_);
On 2016/04/05 14:04:02, kwiberg-webrtc wrote:
> Consider ASSERTing that the optional was valid, to avoid crashing.

Done.

[hlundin-webrtc, 2016-04-05 14:20:00]

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

[hlundin-webrtc, 2016-04-05 14:20:00]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org,
minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1853183002/#ps40001 (title:
"Fixing kwiberg's comments")

[commit-bot: I haz the power, 2016-04-05 14:20:04]

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

[commit-bot: I haz the power, 2016-04-05 14:31:55]

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

[commit-bot: I haz the power, 2016-04-05 14:31:56]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4608)

[hlundin-webrtc, 2016-04-05 14:52:56]

Adding back the old PlayoutTimestamp method, now DEPRECATED

[hlundin-webrtc, 2016-04-05 14:54:24]

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

[hlundin-webrtc, 2016-04-05 14:54:25]

kwiberg: Please, look at patch set 4. I had to hang on to the old interface to
avoid breaking downstream clients.

solenberg: Please, review voice_engine/channel.cc.

[hlundin-webrtc, 2016-04-05 14:54:41]

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

[commit-bot: I haz the power, 2016-04-05 14:54:45]

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

[commit-bot: I haz the power, 2016-04-05 16:55:21]

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

[commit-bot: I haz the power, 2016-04-05 16:55:23]

Dry run: Try jobs failed on following builders:
  android_arm64_rel 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)

[kwiberg-webrtc, 2016-04-05 17:27:23]

lgtm

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

https://codereview.webrtc.org/1853183002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:481: // Wrap the
expected value in an rtc::Optional to compare them as such.
Not sure you need the comment, but OK.

[the sun, 2016-04-06 08:04:24]

lgtm

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

https://codereview.webrtc.org/1853183002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1527: return
playout_timestamp.value_or(0);
Why value_or() when you already expect the optional to contain a value?

[hlundin-webrtc, 2016-04-06 08:15:48]

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

https://codereview.webrtc.org/1853183002/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1527: return
playout_timestamp.value_or(0);
On 2016/04/06 08:04:24, the sun wrote:
> Why value_or() when you already expect the optional to contain a value?

EXPECT does not inhibit the execution of the test, so if it did not contain a
value, a simple "dereferencing" of playout_timestamp would crash the test binary
(there is a DCHECK inside rtc::Optional for that).

I'm keeping it this way for now, since I will be changing it in an upcoming CL.

[hlundin-webrtc, 2016-04-06 08:37:18]

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

[hlundin-webrtc, 2016-04-06 08:37:18]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1853183002/#ps60001 (title:
"Adding back the old PlayoutTimestamp method, now DEPRECATED")

[commit-bot: I haz the power, 2016-04-06 08:37:28]

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

[commit-bot: I haz the power, 2016-04-06 08:39:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-06 08:39:34]

Description was changed from

==========
Change NetEq::GetPlayoutTimestamp to return an rtc::Optional<uint32_t>

This is in preparation for changes to when the playout timestamp is
valid.

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

to

==========
Change NetEq::GetPlayoutTimestamp to return an rtc::Optional<uint32_t>

This is in preparation for changes to when the playout timestamp is
valid.

BUG=webrtc:5669

Committed: https://crrev.com/9a410dd082a650a7ded02ac5b2bd3ca6f45b644a
Cr-Commit-Position: refs/heads/master@{#12256}
==========

[commit-bot: I haz the power, 2016-04-06 08:39:36]

Patchset 4 (id:??) landed as
https://crrev.com/9a410dd082a650a7ded02ac5b2bd3ca6f45b644a
Cr-Commit-Position: refs/heads/master@{#12256}