Voe::Channel: Turned GetPlayoutFrequency into GetRtpTimestampRateHz.

Voe::Channel: Turned GetPlayoutFrequency into GetRtpTimestampRateHz.

This gets rid of a bit of codec-specific code in VoE.

BUG=webrtc:5805
Committed: https://crrev.com/e280cdeb744c2e0c40fa1da24dce59436eda2e39
Cr-Commit-Position: refs/heads/master@{#14614}

[kwiberg-webrtc, 2016-09-20 15:34:53]

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

[kwiberg-webrtc, 2016-09-20 15:34:54]

https://codereview.webrtc.org/2355483003/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2355483003/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:3148: return current_receive_codec.plfreq;
CodecInst::plfreq is the actual sample rate of the codec, not the RTP timestamp
rate (which is what the old code was computing). Is this what you intended?

https://codereview.webrtc.org/2355483003/diff/1/webrtc/voice_engine/channel.h
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2355483003/diff/1/webrtc/voice_engine/channel.h...
webrtc/voice_engine/channel.h:443: int32_t GetPayloadFrequency() const;
Consider adding a comment that explains what this method does. :-)

Also, in the ACM we're trying to standardize on the names "SampleRateHz" and
"RtpTimestampRateHz", which IMHO are more descriptive. So consider calling it
"GetPayloadSampleRateHz".

Also, the return type should probably be plain int.

[ossu, 2016-09-20 15:43:51]

https://codereview.webrtc.org/2355483003/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2355483003/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:3148: return current_receive_codec.plfreq;
On 2016/09/20 15:34:54, kwiberg-webrtc wrote:
> CodecInst::plfreq is the actual sample rate of the codec, not the RTP
timestamp
> rate (which is what the old code was computing). Is this what you intended?

Hmm ... no. I was quite sure of this when I made this change, but it's been
sitting around as part of another set of CLs for quite a while. I'll re-examine
this one and get back to you. I seem to recall I wanted to get the value
directly from the SdpAudioFormat (in DecoderDatabase) but this doesn't look like
it does anymore.

https://codereview.webrtc.org/2355483003/diff/1/webrtc/voice_engine/channel.h
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2355483003/diff/1/webrtc/voice_engine/channel.h...
webrtc/voice_engine/channel.h:443: int32_t GetPayloadFrequency() const;
On 2016/09/20 15:34:54, kwiberg-webrtc wrote:
> Consider adding a comment that explains what this method does. :-)

I don't know!

> Also, in the ACM we're trying to standardize on the names "SampleRateHz" and
> "RtpTimestampRateHz", which IMHO are more descriptive. So consider calling it
> "GetPayloadSampleRateHz".

Alright.

> Also, the return type should probably be plain int.

Agreed.

[ossu, 2016-10-06 10:41:51]

Patchset #2 (id:20001) has been deleted

[ossu, 2016-10-06 11:12:36]

Alright. I believe I misunderstood CodecInst::plfreq to contain the clockrate
and not the samplerate. I've reimplemented it to do what I intended from the
start: get the clockrate from the SdpAudioFormat in the DecoderDatabase.

I've tested this manually, by changing the order of codecs in
webrtcvoiceengine.cc's kCodecPrefs, adding RTC_CHECKs to check that getting the
value from the SdpAudioFormat matches the calculation previously done in the
function and then running through all the peerconnection unittests (which is
where I identified this code as being excercised).

I've no automatic way of testing this, though. Do you have any idea where I
might put a test for this. I was hoping channel_unittests would have something
to start from, but it's completely empty. Since there are no tests for Channel
already, and we're aiming to get rid of it, I don't particularly want to set up
tests from scratch.

[ossu, 2016-10-06 13:51:03]

Description was changed from

==========
Voe::Channel: Turned GetPlayoutFrequency into GetPayloadFrequency.

If this is correct, it gets rid of a bit of codec-specific code in VoE.

BUG=webrtc:5805
==========

to

==========
Voe::Channel: Turned GetPlayoutFrequency into GetRtpTimestampRateHz.

If this is correct, it gets rid of a bit of codec-specific code in VoE.

BUG=webrtc:5805
==========

[kwiberg-webrtc, 2016-10-07 12:05:22]

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

[kwiberg-webrtc, 2016-10-07 12:05:23]

Looks good, provided you can answer "yes" to both questions.

Adding hlundin@ to the Reviewers list, since he knows about RTP clockrates and
sample rates.

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

https://codereview.webrtc.org/2355483003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:102:
RTC_DCHECK(last_audio_format_);
Are you sure this will always be the case? I seem to recall that if you use one
of the legacy ways of setting the decoders in NetEq, you get no SdpAudioFormat.

https://codereview.webrtc.org/2355483003/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2355483003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:3147: : audio_coding_->PlayoutFrequency();
Nice simplification! You always take the clockrate from ReceiveFormat() if it's
available, which isn't what the old code did, but I guess that's OK?

"gotttten" is usually spelled with an even number of t's.

[kwiberg-webrtc, 2016-10-07 12:06:47]

On 2016/10/06 11:12:36, ossu wrote:
> I've no automatic way of testing this, though. Do you have any idea where I
> might put a test for this. I was hoping channel_unittests would have something
> to start from, but it's completely empty. Since there are no tests for Channel
> already, and we're aiming to get rid of it, I don't particularly want to set
up
> tests from scratch.

No, I don't have a good sense of where such a test could find a good home.

[ossu, 2016-10-07 12:21:59]

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

https://codereview.webrtc.org/2355483003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/acm_receiver.cc:102:
RTC_DCHECK(last_audio_format_);
On 2016/10/07 12:05:23, kwiberg-webrtc wrote:
> Are you sure this will always be the case? I seem to recall that if you use
one
> of the legacy ways of setting the decoders in NetEq, you get no
SdpAudioFormat.

That is no longer true, however you can avoid getting one if there's no current
decoder, but since RtpHeaderToDecoder() got one, above, we should be fine.

However, now that you mention it, it is quite possible to get a zero clockrate
in that SdpAudioFormat (only when using an external decoder for a format we
don't support internally), so I'll have to deal with that in
GetRtpClockrateHz(). Thanks!

https://codereview.webrtc.org/2355483003/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2355483003/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:3147: : audio_coding_->PlayoutFrequency();
On 2016/10/07 12:05:23, kwiberg-webrtc wrote:
> Nice simplification! You always take the clockrate from ReceiveFormat() if
it's
> available, which isn't what the old code did, but I guess that's OK?

Yes, I believe the result is the same. If we weren't able to rely on the format
being correct before, then the G722 special-casing wouldn't have been reliable.
(Honestly, I can't say I know for a fact that it was, but running both these
solutions in parallel seemed to indicate that they returned equal results.)
 
> "gotttten" is usually spelled with an even number of t's.

Are you sure? Hmm ... I'll look into it.

[hlundin-webrtc, 2016-10-07 12:32:19]

I think this is sane. LGTM % comments (mine and kwiberg's).

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

https://codereview.webrtc.org/2355483003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/include/audio_coding_module.h:545: virtual
rtc::Optional<SdpAudioFormat> ReceiveFormat() const = 0;
A bit of a description wouldn't go amiss. In particular, this information
relates to the last incoming packet (not the last output audio), and under what
circumstances is it empty.

[ossu, 2016-10-07 12:41:33]

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

https://codereview.webrtc.org/2355483003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/include/audio_coding_module.h:545: virtual
rtc::Optional<SdpAudioFormat> ReceiveFormat() const = 0;
On 2016/10/07 12:32:19, hlundin-webrtc wrote:
> A bit of a description wouldn't go amiss. In particular, this information
> relates to the last incoming packet (not the last output audio), and under
what
> circumstances is it empty.

That's right! I remember thinking: "I'll comment this later". :)

[ossu, 2016-10-07 12:41:39]

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

[commit-bot: I haz the power, 2016-10-07 12:41:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2016-10-07 12:53:53]

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

https://codereview.webrtc.org/2355483003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/include/audio_coding_module.h:545: virtual
rtc::Optional<SdpAudioFormat> ReceiveFormat() const = 0;
On 2016/10/07 12:41:32, ossu wrote:
> On 2016/10/07 12:32:19, hlundin-webrtc wrote:
> > A bit of a description wouldn't go amiss. In particular, this information
> > relates to the last incoming packet (not the last output audio), and under
> what
> > circumstances is it empty.
> 
> That's right! I remember thinking: "I'll comment this later". :)

Then you're all set! It's the thought that counts.

[commit-bot: I haz the power, 2016-10-07 13:24:37]

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

[commit-bot: I haz the power, 2016-10-07 13:24:38]

Dry run: This issue passed the CQ dry run.

[ossu, 2016-10-11 00:15:04]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-10-11 00:15:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2355483003/#ps60001 (title:
"Handle zero clockrate, make better comments, fix bad spel")

[commit-bot: I haz the power, 2016-10-11 00:15:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[ossu, 2016-10-11 00:15:28]

The CQ bit was unchecked by ossu@webrtc.org

[ossu, 2016-10-11 00:15:52]

Description was changed from

==========
Voe::Channel: Turned GetPlayoutFrequency into GetRtpTimestampRateHz.

If this is correct, it gets rid of a bit of codec-specific code in VoE.

BUG=webrtc:5805
==========

to

==========
Voe::Channel: Turned GetPlayoutFrequency into GetRtpTimestampRateHz.

This gets rid of a bit of codec-specific code in VoE.

BUG=webrtc:5805
==========

[ossu, 2016-10-11 00:16:02]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-10-11 00:16:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-11 00:30:00]

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

[commit-bot: I haz the power, 2016-10-11 00:30:02]

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

[ossu, 2016-10-11 18:33:31]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-10-11 18:33:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-11 18:36:26]

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

[commit-bot: I haz the power, 2016-10-11 18:36:27]

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

[ossu, 2016-10-11 18:41:22]

On 2016/10/11 18:36:27, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   presubmit on master.tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9128)

Ah, now it's failing with a proper message. Karl, would you kindly give me an
LGTM? Or have I missed addressing one of your comments?

[kwiberg-webrtc, 2016-10-11 18:54:19]

lgtm

[ossu, 2016-10-11 18:56:41]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-10-11 18:56:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-11 19:25:42]

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

[commit-bot: I haz the power, 2016-10-11 19:25:43]

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

[ossu, 2016-10-12 17:12:35]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-10-12 17:12:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-12 18:04:12]

Description was changed from

==========
Voe::Channel: Turned GetPlayoutFrequency into GetRtpTimestampRateHz.

This gets rid of a bit of codec-specific code in VoE.

BUG=webrtc:5805
==========

to

==========
Voe::Channel: Turned GetPlayoutFrequency into GetRtpTimestampRateHz.

This gets rid of a bit of codec-specific code in VoE.

BUG=webrtc:5805
==========

[commit-bot: I haz the power, 2016-10-12 18:04:14]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-12 18:04:24]

Description was changed from

==========
Voe::Channel: Turned GetPlayoutFrequency into GetRtpTimestampRateHz.

This gets rid of a bit of codec-specific code in VoE.

BUG=webrtc:5805
==========

to

==========
Voe::Channel: Turned GetPlayoutFrequency into GetRtpTimestampRateHz.

This gets rid of a bit of codec-specific code in VoE.

BUG=webrtc:5805

Committed: https://crrev.com/e280cdeb744c2e0c40fa1da24dce59436eda2e39
Cr-Commit-Position: refs/heads/master@{#14614}
==========

[commit-bot: I haz the power, 2016-10-12 18:04:25]

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