Multi frequency DTMF support - receiver side

Support receiving DTMF for multiple RTP clock rates.

BUG=webrtc:2795
Committed: https://crrev.com/2779bab02a3c9e576cdb9927ef86d3fc40772bdc
Cr-Commit-Position: refs/heads/master@{#15128}

[the sun, 2016-09-12 14:19:45]

Description was changed from

==========
Support DTMF for multiple rates.

BUG=
==========

to

==========
Support DTMF for multiple rates.

BUG=
==========

[the sun, 2016-09-12 14:19:45]

solenberg@webrtc.org changed reviewers:
+ ossu@webrtc.org

[the sun, 2016-09-20 07:46:30]

Description was changed from

==========
Support DTMF for multiple rates.

BUG=
==========

to

==========
Support DTMF for multiple rates.

BUG=webrtc:2795
==========

[the sun, 2016-10-03 20:00:22]

Missing tests + a few TODOs left in the code, but can you PTAL. In particular,
is it really necessary to add all the extra enum entries for DTMF, or is there a
better way with the architecture we're moving towards?

[ossu, 2016-10-04 09:26:57]

If we want to support these new rates with rtpplay, then I don't see any option
but to add these enums. That tool will have to be rewritten to do without
NetEqDecoder before we can remove that enum completely.

If you don't, however, it should soon be able to push SdpAudioFormats all the
way down to the ACM and NetEq, completely bypassing the conversion to
NetEqDecoder. I expect that to be after
https://codereview.webrtc.org/2365653004/ and one more (future) CL has landed.

Otherwise, looks good. I've left a couple of comments about compatibility below.

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
File webrtc/media/engine/payload_type_mapper.cc (right):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
webrtc/media/engine/payload_type_mapper.cc:87: {{kDtmfCodecName,   48000, 1},
123},
I'm not sure these need to be added here, but maybe it's as well. Since they're
new, they're not necessary to ensure backwards compatibility. Could be nice that
they match the numbers in kCodecPrefs, although outer layers will ensure this
gets sorted out regardless.

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/rent_a_codec.h (left):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/rent_a_codec.h:129: kDecoderAVT,
Not sure someone outside uses this one, but maybe it would be prudent to have
kDecoderAVT = kDecoderAVT8kHz, just for backwards compatibility. If so, that
probably applies to kAVT, above, as well.

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (left):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:237: if (payload_type
== FLAGS_avt || payload_type == FLAGS_red)
Why could this be 0 before but not anymore?

[the sun, 2016-10-08 22:06:51]

I'm going to add a test in NetEq as well (that what's #if:d out in
neteq_impl_unittest.cc).

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
File webrtc/media/engine/payload_type_mapper.cc (right):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
webrtc/media/engine/payload_type_mapper.cc:87: {{kDtmfCodecName,   48000, 1},
123},
On 2016/10/04 09:26:56, ossu wrote:
> I'm not sure these need to be added here, but maybe it's as well. Since
they're
> new, they're not necessary to ensure backwards compatibility. Could be nice
that
> they match the numbers in kCodecPrefs, although outer layers will ensure this
> gets sorted out regardless.

If they're not there, we may map the PTs differently between send and receive,
which AFAIK isn't supported in the signaling layer yet - or did you fix that?

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/rent_a_codec.h (left):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/rent_a_codec.h:129: kDecoderAVT,
On 2016/10/04 09:26:57, ossu wrote:
> Not sure someone outside uses this one, but maybe it would be prudent to have
> kDecoderAVT = kDecoderAVT8kHz, just for backwards compatibility. If so, that
> probably applies to kAVT, above, as well.

I'm leaving kDecoderAVT and kAVT as is then - compiler yelled at me for having
multiple entries with the same value in the enum.

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (left):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:237: if (payload_type
== FLAGS_avt || payload_type == FLAGS_red)
On 2016/10/04 09:26:57, ossu wrote:
> Why could this be 0 before but not anymore?

It couldn't. Previously, a WAVE file with SR=0 would be produced if the stream
only contained DTMF.

[ossu, 2016-10-10 23:26:34]

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
File webrtc/media/engine/payload_type_mapper.cc (right):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
webrtc/media/engine/payload_type_mapper.cc:87: {{kDtmfCodecName,   48000, 1},
123},
On 2016/10/08 22:06:51, the sun wrote:
> On 2016/10/04 09:26:56, ossu wrote:
> > I'm not sure these need to be added here, but maybe it's as well. Since
> they're
> > new, they're not necessary to ensure backwards compatibility. Could be nice
> that
> > they match the numbers in kCodecPrefs, although outer layers will ensure
this
> > gets sorted out regardless.
> 
> If they're not there, we may map the PTs differently between send and receive,
> which AFAIK isn't supported in the signaling layer yet - or did you fix that?

I believe that'll be handled by the PeerConnection layer (my spidey-sense tells
me that mediasession.cc might be involved but I can look into it). At one point,
the send and receive lists will be joined and that's done the same way as the
offer and answer lists are joined. With that, a single payload type is picked
for identical formats.

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/acm2/rent_a_codec.h (left):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/acm2/rent_a_codec.h:129: kDecoderAVT,
On 2016/10/08 22:06:51, the sun wrote:
> On 2016/10/04 09:26:57, ossu wrote:
> > Not sure someone outside uses this one, but maybe it would be prudent to
have
> > kDecoderAVT = kDecoderAVT8kHz, just for backwards compatibility. If so, that
> > probably applies to kAVT, above, as well.
> 
> I'm leaving kDecoderAVT and kAVT as is then - compiler yelled at me for having
> multiple entries with the same value in the enum.

Oh? Silly compiler!

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (left):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:237: if (payload_type
== FLAGS_avt || payload_type == FLAGS_red)
On 2016/10/08 22:06:51, the sun wrote:
> On 2016/10/04 09:26:57, ossu wrote:
> > Why could this be 0 before but not anymore?
> 
> It couldn't. Previously, a WAVE file with SR=0 would be produced if the stream
> only contained DTMF.

Acknowledged.

[the sun, 2016-10-11 09:06:56]

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

[the sun, 2016-10-11 09:06:56]

Adding hlundin@ for a question on NetEq timestamps:

Which timestamp rate is used when converting to wall clock to figure out when to
e.g. trigger the tone generator?

[hlundin-webrtc, 2016-10-11 12:26:15]

There is no conversion of timestamps for DTMF. NetEq simply assumes that they
are in the same clockrate as the main payload.

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right):

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:398: {FLAGS_avt_16,
std::make_pair(NetEqDecoder::kDecoderAVT16kHz, "avt-16")},
Please, add similar mappings to webrtc/test/fuzzers/neteq_rtp_fuzzers.cc.

[hlundin-webrtc, 2016-10-11 12:32:34]

lgtm for neteq.

[the sun, 2016-10-11 15:22:53]

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right):

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:398: {FLAGS_avt_16,
std::make_pair(NetEqDecoder::kDecoderAVT16kHz, "avt-16")},
On 2016/10/11 12:26:15, hlundin-webrtc wrote:
> Please, add similar mappings to webrtc/test/fuzzers/neteq_rtp_fuzzers.cc. 

Done.

[the sun, 2016-10-11 17:55:20]

Description was changed from

==========
Support DTMF for multiple rates.

BUG=webrtc:2795
==========

to

==========
Support receiving DTMF for multiple RTP clock rates.

BUG=webrtc:2795
==========

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

lgtm with comments

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:182: const int
kSampleRateHz = 8000;
So DTMF sample rate doesn't depend on clock rate, but the clock rate NetEq runs
at (which is determined by the most recently used speech codec)? Am I
understanding this correctly?

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:207:
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_);
Do you want to check that there is actually audio generated as well? Like that
the frame isn't all zeroes or something? (I guess _actually_ validating the
waveform is a bit much?)

[the sun, 2016-10-11 19:17:17]

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
File webrtc/media/engine/payload_type_mapper.cc (right):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
webrtc/media/engine/payload_type_mapper.cc:87: {{kDtmfCodecName,   48000, 1},
123},
On 2016/10/10 23:26:34, ossu wrote:
> On 2016/10/08 22:06:51, the sun wrote:
> > On 2016/10/04 09:26:56, ossu wrote:
> > > I'm not sure these need to be added here, but maybe it's as well. Since
> > they're
> > > new, they're not necessary to ensure backwards compatibility. Could be
nice
> > that
> > > they match the numbers in kCodecPrefs, although outer layers will ensure
> this
> > > gets sorted out regardless.
> > 
> > If they're not there, we may map the PTs differently between send and
receive,
> > which AFAIK isn't supported in the signaling layer yet - or did you fix
that?
> 
> I believe that'll be handled by the PeerConnection layer (my spidey-sense
tells
> me that mediasession.cc might be involved but I can look into it). At one
point,
> the send and receive lists will be joined and that's done the same way as the
> offer and answer lists are joined. With that, a single payload type is picked
> for identical formats.

Because you gave the lgtm, I guess you still think it is right to leave the
"static" mapping in here though?

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:182: const int
kSampleRateHz = 8000;
On 2016/10/11 17:56:17, ossu wrote:
> So DTMF sample rate doesn't depend on clock rate, but the clock rate NetEq
runs
> at (which is determined by the most recently used speech codec)? Am I
> understanding this correctly?

The sample rate is set when NetEq is constructed and defaults to 8000. This is
the output rate and has nothing to do with the rate of encoded audio (either
bit-, rtp clock-, or actual sample rate). Since DTMF is only encoded events, the
only rate that matters here is the output sample rate, so that the tone
generator can give us something sensible. Henrik already answered about the rtp
rate - IIUC it is duly ignored for DTMF, regardless of rate, and the rate of the
audio codec is used to align playout times.

https://codereview.webrtc.org/2337473002/diff/300001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:207:
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_);
On 2016/10/11 17:56:18, ossu wrote:
> Do you want to check that there is actually audio generated as well? Like that
> the frame isn't all zeroes or something? (I guess _actually_ validating the
> waveform is a bit much?)

Done.

[ossu, 2016-10-11 20:17:26]

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
File webrtc/media/engine/payload_type_mapper.cc (right):

https://codereview.webrtc.org/2337473002/diff/160001/webrtc/media/engine/payl...
webrtc/media/engine/payload_type_mapper.cc:87: {{kDtmfCodecName,   48000, 1},
123},
On 2016/10/11 19:17:17, the sun wrote:
> On 2016/10/10 23:26:34, ossu wrote:
> > On 2016/10/08 22:06:51, the sun wrote:
> > > On 2016/10/04 09:26:56, ossu wrote:
> > > > I'm not sure these need to be added here, but maybe it's as well. Since
> > > they're
> > > > new, they're not necessary to ensure backwards compatibility. Could be
> nice
> > > that
> > > > they match the numbers in kCodecPrefs, although outer layers will ensure
> > this
> > > > gets sorted out regardless.
> > > 
> > > If they're not there, we may map the PTs differently between send and
> receive,
> > > which AFAIK isn't supported in the signaling layer yet - or did you fix
> that?
> > 
> > I believe that'll be handled by the PeerConnection layer (my spidey-sense
> tells
> > me that mediasession.cc might be involved but I can look into it). At one
> point,
> > the send and receive lists will be joined and that's done the same way as
the
> > offer and answer lists are joined. With that, a single payload type is
picked
> > for identical formats.
> 
> Because you gave the lgtm, I guess you still think it is right to leave the
> "static" mapping in here though?

Yes, I've no strong feelings either way. It's probably safer to keep them
statically here, for now. We can deal with getting rid of all of these static
dynamic mappings at once, at a later time.

[the sun, 2016-10-12 08:32:16]

solenberg@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[the sun, 2016-10-12 08:32:49]

Adding kjellander@ for a look at neteq_rtp_fuzzer.cc

[kjellander_webrtc, 2016-10-13 00:26:57]

On 2016/10/12 08:32:49, the sun wrote:
> Adding kjellander@ for a look at neteq_rtp_fuzzer.cc

webrtc/test/fuzzers/neteq_rtp_fuzzer.cc: lgtm

[the sun, 2016-11-17 12:10:52]

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

[commit-bot: I haz the power, 2016-11-17 12:11:03]

Dry run: 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-11-17 12:27:12]

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

[commit-bot: I haz the power, 2016-11-17 12:27:14]

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

[the sun, 2016-11-17 12:35:37]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2016-11-17 12:35:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, ossu@webrtc.org, kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2337473002/#ps520001 (title:
"rebase")

[commit-bot: I haz the power, 2016-11-17 12:35:44]

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-11-17 12:45:21]

Description was changed from

==========
Support receiving DTMF for multiple RTP clock rates.

BUG=webrtc:2795
==========

to

==========
Support receiving DTMF for multiple RTP clock rates.

BUG=webrtc:2795
==========

[commit-bot: I haz the power, 2016-11-17 12:45:23]

Committed patchset #27 (id:520001)

[commit-bot: I haz the power, 2016-11-17 12:45:27]

Description was changed from

==========
Support receiving DTMF for multiple RTP clock rates.

BUG=webrtc:2795
==========

to

==========
Support receiving DTMF for multiple RTP clock rates.

BUG=webrtc:2795

Committed: https://crrev.com/2779bab02a3c9e576cdb9927ef86d3fc40772bdc
Cr-Commit-Position: refs/heads/master@{#15128}
==========

[commit-bot: I haz the power, 2016-11-17 12:45:28]

Patchset 27 (id:??) landed as
https://crrev.com/2779bab02a3c9e576cdb9927ef86d3fc40772bdc
Cr-Commit-Position: refs/heads/master@{#15128}