Add new method AcmReceiver::last_packet_sample_rate_hz()

Add new method AcmReceiver::last_packet_sample_rate_hz()

This change allows us to delete AcmReceiver::last_audio_codec_id().

BUG=webrtc:3520
Committed: https://crrev.com/057fb89f83f4ba51d4f0151aa8f8cfa5d5bb0add
Cr-Commit-Position: refs/heads/master@{#10756}

[hlundin-webrtc, 2015-11-23 11:56:16]

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

[hlundin-webrtc, 2015-11-23 11:56:16]

Karl,

Please, take a look. In particular, comment on whether or not this is fair use
of rtc::Optional.

[kwiberg-webrtc, 2015-11-23 13:03:50]

lgtm with some nits

In particular, this is an excellent place to use rtc::Optional.

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/acm_receiver.cc (right):

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver.cc:132:
last_packet_sample_rate_hz_() {
No need to call the default constructor explicitly.

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/acm_receiver.h (right):

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver.h:158: // Returns the sample
rate of of the codec associated with the last incoming
Even number of ofs; odd number expected.

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver.h:160: // return value is
empty. Also, if the codec was unregistered since the last
"codec" -> "decoder", maybe?

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc
(right):

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:348:
EXPECT_EQ(c.inst.plfreq, *receiver_->last_packet_sample_rate_hz());
Optional has an operator== function, so you should be able to just assert
equality to rtc::Optional<int>(c.inst.plfreq).

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:360:
EXPECT_EQ(c.inst.plfreq, *receiver_->last_packet_sample_rate_hz());
Same.

[hlundin-webrtc, 2015-11-23 13:48:01]

Updates after review

[hlundin-webrtc, 2015-11-23 13:49:54]

Updates after review

[hlundin-webrtc, 2015-11-23 13:50:03]

Patchset #2 (id:20001) has been deleted

[hlundin-webrtc, 2015-11-23 13:50:45]

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/acm_receiver.cc (right):

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver.cc:132:
last_packet_sample_rate_hz_() {
On 2015/11/23 13:03:50, kwiberg-webrtc wrote:
> No need to call the default constructor explicitly.

Done.

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/acm_receiver.h (right):

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver.h:158: // Returns the sample
rate of of the codec associated with the last incoming
On 2015/11/23 13:03:50, kwiberg-webrtc wrote:
> Even number of ofs; odd number expected.

Done.

Took care of of of off-by-one error.

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver.h:160: // return value is
empty. Also, if the codec was unregistered since the last
On 2015/11/23 13:03:50, kwiberg-webrtc wrote:
> "codec" -> "decoder", maybe?

Done.

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc
(right):

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:348:
EXPECT_EQ(c.inst.plfreq, *receiver_->last_packet_sample_rate_hz());
On 2015/11/23 13:03:50, kwiberg-webrtc wrote:
> Optional has an operator== function, so you should be able to just assert
> equality to rtc::Optional<int>(c.inst.plfreq).

Acknowledged, but that won't give the same behavior as my code. I'm ASSERTing
that I get a value, but only EXPECTing it to be identical to plfreq. So, yes, I
could write the second line as 
EXPECT_EQ(rtc::Optional<int>(c.inst.plfreq),
receiver_->last_packet_sample_rate_hz());
but I don't think it makes the code any better.

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:360:
EXPECT_EQ(c.inst.plfreq, *receiver_->last_packet_sample_rate_hz());
On 2015/11/23 13:03:50, kwiberg-webrtc wrote:
> Same.

Will it make the code better?
EXPECT_EQ(rtc::Optional<int>(c.inst.plfreq),
receiver_->last_packet_sample_rate_hz());
vs. what I have now.

[kwiberg-webrtc, 2015-11-23 14:03:35]

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc
(right):

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:348:
EXPECT_EQ(c.inst.plfreq, *receiver_->last_packet_sample_rate_hz());
On 2015/11/23 13:50:45, hlundin-webrtc wrote:
> On 2015/11/23 13:03:50, kwiberg-webrtc wrote:
> > Optional has an operator== function, so you should be able to just assert
> > equality to rtc::Optional<int>(c.inst.plfreq).
> 
> Acknowledged, but that won't give the same behavior as my code. I'm ASSERTing
> that I get a value, but only EXPECTing it to be identical to plfreq. So, yes,
I
> could write the second line as 
> EXPECT_EQ(rtc::Optional<int>(c.inst.plfreq),
> receiver_->last_packet_sample_rate_hz());
> but I don't think it makes the code any better.

Well, no, what I suggested won't do exactly what you currently do, so if you
want exactly that, don't change it.

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:360:
EXPECT_EQ(c.inst.plfreq, *receiver_->last_packet_sample_rate_hz());
On 2015/11/23 13:50:45, hlundin-webrtc wrote:
> On 2015/11/23 13:03:50, kwiberg-webrtc wrote:
> > Same.
> 
> Will it make the code better?
> EXPECT_EQ(rtc::Optional<int>(c.inst.plfreq),
> receiver_->last_packet_sample_rate_hz());
> vs. what I have now.

In this case, arguably yes, since Optional's operator* will behave badly if the
Optional is unset. Alternatively, you could test separately if it's empty like
you did above.

[hlundin-webrtc, 2015-11-23 14:11:32]

Updates after second review

[hlundin-webrtc, 2015-11-23 14:13:35]

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
File webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc
(right):

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:348:
EXPECT_EQ(c.inst.plfreq, *receiver_->last_packet_sample_rate_hz());
On 2015/11/23 14:03:35, kwiberg-webrtc wrote:
> On 2015/11/23 13:50:45, hlundin-webrtc wrote:
> > On 2015/11/23 13:03:50, kwiberg-webrtc wrote:
> > > Optional has an operator== function, so you should be able to just assert
> > > equality to rtc::Optional<int>(c.inst.plfreq).
> > 
> > Acknowledged, but that won't give the same behavior as my code. I'm
ASSERTing
> > that I get a value, but only EXPECTing it to be identical to plfreq. So,
yes,
> I
> > could write the second line as 
> > EXPECT_EQ(rtc::Optional<int>(c.inst.plfreq),
> > receiver_->last_packet_sample_rate_hz());
> > but I don't think it makes the code any better.
> 
> Well, no, what I suggested won't do exactly what you currently do, so if you
> want exactly that, don't change it.

Right. I activated my brain now and realized that I don't need the ASSERT_TRUE
anymore. Your suggestion beats mine.

https://codereview.webrtc.org/1467183002/diff/1/webrtc/modules/audio_coding/m...
webrtc/modules/audio_coding/main/acm2/acm_receiver_unittest_oldapi.cc:360:
EXPECT_EQ(c.inst.plfreq, *receiver_->last_packet_sample_rate_hz());
On 2015/11/23 14:03:35, kwiberg-webrtc wrote:
> On 2015/11/23 13:50:45, hlundin-webrtc wrote:
> > On 2015/11/23 13:03:50, kwiberg-webrtc wrote:
> > > Same.
> > 
> > Will it make the code better?
> > EXPECT_EQ(rtc::Optional<int>(c.inst.plfreq),
> > receiver_->last_packet_sample_rate_hz());
> > vs. what I have now.
> 
> In this case, arguably yes, since Optional's operator* will behave badly if
the
> Optional is unset. Alternatively, you could test separately if it's empty like
> you did above.

Done.

[hlundin-webrtc, 2015-11-23 15:15:58]

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

[hlundin-webrtc, 2015-11-23 15:15:59]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1467183002/#ps60001 (title:
"Updates after second review")

[commit-bot: I haz the power, 2015-11-23 15:16:03]

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

[commit-bot: I haz the power, 2015-11-23 16:19:55]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-11-23 16:20:08]

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