Multi frequency DTMF support - sender side

Support multiple timestamp rates for sending DTMF.

We support multiple payload types, and one which matches the audio codec the closest, is picked (or the one with lowest clock rate, if no perfect match is found).

The exact clock rate is then ignored and DTMF packets are time stamped with the rate of the current audio codec. This is exactly the way the code has worked up to this point, but until now we have been under the impression that we were in fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will most likely find a payload type which matches the codec clock rate.

This CL also does a little cleaning of the DTMFQueue and RTPSenderAudio classes.
BUG=webrtc:2795
Committed: https://crrev.com/ffbbcac4c6111095a11536b96b600c6813444562
Cr-Commit-Position: refs/heads/master@{#15129}

[the sun, 2016-10-20 14:44:51]

Description was changed from

==========
Multi freq dtmf send side. WIP

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

to

==========
Support multiple timestamp rates for sending DTMF.

In reality, we only support multiple payload types, and one which matches the
audio codec the closest, is picked (or the one with lowest clock rate, if no
perfect match is found).

The exact clock rate is then ignored and DTMF packets are time stamped with the
rate of the current audio codec. This is exactly the way the code has worked up
to this point, but until now we have been under the impression that we were in
fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will
most likely find a payload type which matches the codec clock rate.

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

[the sun, 2016-10-20 14:44:51]

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

[the sun, 2016-10-20 14:45:58]

Description was changed from

==========
Support multiple timestamp rates for sending DTMF.

In reality, we only support multiple payload types, and one which matches the
audio codec the closest, is picked (or the one with lowest clock rate, if no
perfect match is found).

The exact clock rate is then ignored and DTMF packets are time stamped with the
rate of the current audio codec. This is exactly the way the code has worked up
to this point, but until now we have been under the impression that we were in
fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will
most likely find a payload type which matches the codec clock rate.

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

to

==========
Support multiple timestamp rates for sending DTMF.

In reality, we only support multiple payload types, and one which matches the
audio codec the closest, is picked (or the one with lowest clock rate, if no
perfect match is found).

The exact clock rate is then ignored and DTMF packets are time stamped with the
rate of the current audio codec. This is exactly the way the code has worked up
to this point, but until now we have been under the impression that we were in
fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will
most likely find a payload type which matches the codec clock rate.

This CL also does a little cleaning of the DTMFQueue and RTPSenderAudio classes.
BUG=webrtc:2795
==========

[the sun, 2016-10-20 14:55:06]

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

[the sun, 2016-10-20 14:55:55]

Description was changed from

==========
Support multiple timestamp rates for sending DTMF.

In reality, we only support multiple payload types, and one which matches the
audio codec the closest, is picked (or the one with lowest clock rate, if no
perfect match is found).

The exact clock rate is then ignored and DTMF packets are time stamped with the
rate of the current audio codec. This is exactly the way the code has worked up
to this point, but until now we have been under the impression that we were in
fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will
most likely find a payload type which matches the codec clock rate.

This CL also does a little cleaning of the DTMFQueue and RTPSenderAudio classes.
BUG=webrtc:2795
==========

to

==========
Support multiple timestamp rates for sending DTMF.

We support multiple payload types, and one which matches the audio codec the
closest, is picked (or the one with lowest clock rate, if no perfect match is
found).

The exact clock rate is then ignored and DTMF packets are time stamped with the
rate of the current audio codec. This is exactly the way the code has worked up
to this point, but until now we have been under the impression that we were in
fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will
most likely find a payload type which matches the codec clock rate.

This CL also does a little cleaning of the DTMFQueue and RTPSenderAudio classes.
BUG=webrtc:2795
==========

[the sun, 2016-10-24 20:11:31]

On 2016/10/20 14:55:06, the sun wrote:

ping Stefan...

[the sun, 2016-10-27 07:22:48]

On 2016/10/24 20:11:31, the sun wrote:
> On 2016/10/20 14:55:06, the sun wrote:
> 
> ping Stefan...

ping Henrik...

[the sun, 2016-10-27 16:10:51]

On 2016/10/27 07:22:48, the sun wrote:
> On 2016/10/24 20:11:31, the sun wrote:
> > On 2016/10/20 14:55:06, the sun wrote:
> > 
> > ping Stefan...
> 
> ping Henrik...

ping pong!

[hlundin-webrtc, 2016-10-28 08:12:37]

Mostly LG, but I have a few comments.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1835: // Note that we return if *any*
codec's PT is out of range.
This is more like an overall sanity check of the input. I think you should break
this out of the otherwise DTMF-specific for loop, and put it on top. Consider
using std::find_if_not.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1928: dtmf_payload_freq_ =
codec.clockrate;
Is there any reason to continue the for loop after finding a match?

You could use std::find_if, but I'm not sure the code gets any more readable:

const auto result = std::find_if(dtmf_codecs.begin(), dtmf_codecs.end(), 
  [&send_codec_spec](const AudioCodec& a) {return a.clockrate ==
send_codec_spec.codec_inst.plfreq;});
if (result != dtmf_codecs.end()) {
  dtmf_payload_type_ = rtc::Optional<int>(result->id);
  dtmf_payload_freq_ = result->clockrate;
}

Yeah, not very pretty...

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/dtmf_queue.cc (right):

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:12: #include
"webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h"
I'd rather you moved DTMF_OUTBAND_MAX to this file instead, since it is only
used here.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:19: int DTMFQueue::AddDTMF(const
Event& event) {
This looks like a bool-returning method to me.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:21: if (queue_.size() >=
DTMF_OUTBAND_MAX) {
Probably no longer needed as such, since queue_ is not limited in size as
before. Could of course be useful to keep it from growing indefinitely in case
of some weird error in the calling code.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:28: int DTMFQueue::NextDTMF(Event*
event) {
Wouldn't hurt to change this to return an rtc::Optional<event>.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/dtmf_queue.h (right):

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:19: class DTMFQueue {
I think it should even be DtmfQueue (style guide says "prefer to capitalize
acronyms as single words").

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:31: int AddDTMF(const Event& event);
AddDtmf

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:32: int NextDTMF(Event* event);
NextDtmf

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:33: bool PendingDTMF();
PendingDtmf

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:33: bool PendingDTMF();
const method.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h (right):

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h:67: Clock* const clock_ =
nullptr;
Not really needed, since this is set in the ctor initializer list.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h:68: RTPSender* const
rtp_sender_ = nullptr;
Ditto.

[the sun, 2016-11-07 13:33:31]

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1835: // Note that we return if *any*
codec's PT is out of range.
On 2016/10/28 08:12:36, hlundin-webrtc wrote:
> This is more like an overall sanity check of the input. I think you should
break
> this out of the otherwise DTMF-specific for loop, and put it on top. Consider
> using std::find_if_not.

Done.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1928: dtmf_payload_freq_ =
codec.clockrate;
On 2016/10/28 08:12:36, hlundin-webrtc wrote:
> Is there any reason to continue the for loop after finding a match?
> 
> You could use std::find_if, but I'm not sure the code gets any more readable:
> 
> const auto result = std::find_if(dtmf_codecs.begin(), dtmf_codecs.end(), 
>   [&send_codec_spec](const AudioCodec& a) {return a.clockrate ==
> send_codec_spec.codec_inst.plfreq;});
> if (result != dtmf_codecs.end()) {
>   dtmf_payload_type_ = rtc::Optional<int>(result->id);
>   dtmf_payload_freq_ = result->clockrate;
> }
> 
> Yeah, not very pretty...

Added a simple "break" instead.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/dtmf_queue.cc (right):

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:12: #include
"webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h"
On 2016/10/28 08:12:36, hlundin-webrtc wrote:
> I'd rather you moved DTMF_OUTBAND_MAX to this file instead, since it is only
> used here.

Agree.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:19: int DTMFQueue::AddDTMF(const
Event& event) {
On 2016/10/28 08:12:36, hlundin-webrtc wrote:
> This looks like a bool-returning method to me.

Done.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:21: if (queue_.size() >=
DTMF_OUTBAND_MAX) {
On 2016/10/28 08:12:36, hlundin-webrtc wrote:
> Probably no longer needed as such, since queue_ is not limited in size as
> before. Could of course be useful to keep it from growing indefinitely in case
> of some weird error in the calling code.

Yes.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:28: int DTMFQueue::NextDTMF(Event*
event) {
On 2016/10/28 08:12:36, hlundin-webrtc wrote:
> Wouldn't hurt to change this to return an rtc::Optional<event>.

Changed it at first, but decided against. since it would require a larger change
to the logic in the one place the method is being called.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/dtmf_queue.h (right):

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:19: class DTMFQueue {
On 2016/10/28 08:12:37, hlundin-webrtc wrote:
> I think it should even be DtmfQueue (style guide says "prefer to capitalize
> acronyms as single words").

Done.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:31: int AddDTMF(const Event& event);
On 2016/10/28 08:12:36, hlundin-webrtc wrote:
> AddDtmf

Done.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:32: int NextDTMF(Event* event);
On 2016/10/28 08:12:37, hlundin-webrtc wrote:
> NextDtmf

Done.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:33: bool PendingDTMF();
On 2016/10/28 08:12:36, hlundin-webrtc wrote:
> const method.

Done.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.h:33: bool PendingDTMF();
On 2016/10/28 08:12:37, hlundin-webrtc wrote:
> PendingDtmf

Done.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h (right):

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h:67: Clock* const clock_ =
nullptr;
On 2016/10/28 08:12:37, hlundin-webrtc wrote:
> Not really needed, since this is set in the ctor initializer list.

Done.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h:68: RTPSender* const
rtp_sender_ = nullptr;
On 2016/10/28 08:12:37, hlundin-webrtc wrote:
> Ditto.

Done.

[hlundin-webrtc, 2016-11-08 09:01:59]

LGTM with one nit.

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/dtmf_queue.cc (right):

https://codereview.webrtc.org/2392883002/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:28: int DTMFQueue::NextDTMF(Event*
event) {
On 2016/11/07 13:33:31, the sun wrote:
> On 2016/10/28 08:12:36, hlundin-webrtc wrote:
> > Wouldn't hurt to change this to return an rtc::Optional<event>.
> 
> Changed it at first, but decided against. since it would require a larger
change
> to the logic in the one place the method is being called.

Acknowledged.

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/dtmf_queue.cc (right):

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:14: constexpr size_t
DTMF_OUTBAND_MAX = 20;
Now, you'll only have to rename it kDtmfOutbandMax.

[stefan-webrtc, 2016-11-08 15:05:39]

lgtm, just two minor comments.

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1899: });
Sorting isn't necessary if you simply keep track of the lowest clockrate in the
loop on line 1892.

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h (right):

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h:95: uint8_t audio_level_dbov_
GUARDED_BY(send_audio_critsect_) = 0;
We've tended to prefer initializer lists as splitting initialization between
initializer lists and header files can be confusing (can't easily look at one
place to know if something is initialized).

[the sun, 2016-11-11 12:07:24]

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1899: });
On 2016/11/08 15:05:39, stefan-webrtc (holmer) wrote:
> Sorting isn't necessary if you simply keep track of the lowest clockrate in
the
> loop on line 1892.

Yeah, thanks. That's more nicer.

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/dtmf_queue.cc (right):

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/dtmf_queue.cc:14: constexpr size_t
DTMF_OUTBAND_MAX = 20;
On 2016/11/08 09:01:59, hlundin-webrtc wrote:
> Now, you'll only have to rename it kDtmfOutbandMax.

Done.

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h (right):

https://codereview.webrtc.org/2392883002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h:95: uint8_t audio_level_dbov_
GUARDED_BY(send_audio_critsect_) = 0;
On 2016/11/08 15:05:39, stefan-webrtc (holmer) wrote:
> We've tended to prefer initializer lists as splitting initialization between
> initializer lists and header files can be confusing (can't easily look at one
> place to know if something is initialized).

Ah yes, that's why I set clock_ and rtp_sender_ to nullptr even though they're
also set in the ctor. I'm reverting to that. Initializing at declaration is
preferable because then you only need to look in one place to see both what
members the class has and whether they're initialized. It also avoids needing to
duplicate init lists in case of multiple ctors (not the case here though).

[the sun, 2016-11-17 12:55:36]

The CQ bit was checked by solenberg@webrtc.org

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

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

[commit-bot: I haz the power, 2016-11-17 12:55: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 13:25:39]

Description was changed from

==========
Support multiple timestamp rates for sending DTMF.

We support multiple payload types, and one which matches the audio codec the
closest, is picked (or the one with lowest clock rate, if no perfect match is
found).

The exact clock rate is then ignored and DTMF packets are time stamped with the
rate of the current audio codec. This is exactly the way the code has worked up
to this point, but until now we have been under the impression that we were in
fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will
most likely find a payload type which matches the codec clock rate.

This CL also does a little cleaning of the DTMFQueue and RTPSenderAudio classes.
BUG=webrtc:2795
==========

to

==========
Support multiple timestamp rates for sending DTMF.

We support multiple payload types, and one which matches the audio codec the
closest, is picked (or the one with lowest clock rate, if no perfect match is
found).

The exact clock rate is then ignored and DTMF packets are time stamped with the
rate of the current audio codec. This is exactly the way the code has worked up
to this point, but until now we have been under the impression that we were in
fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will
most likely find a payload type which matches the codec clock rate.

This CL also does a little cleaning of the DTMFQueue and RTPSenderAudio classes.
BUG=webrtc:2795
==========

[commit-bot: I haz the power, 2016-11-17 13:25:40]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-11-17 13:25:54]

Description was changed from

==========
Support multiple timestamp rates for sending DTMF.

We support multiple payload types, and one which matches the audio codec the
closest, is picked (or the one with lowest clock rate, if no perfect match is
found).

The exact clock rate is then ignored and DTMF packets are time stamped with the
rate of the current audio codec. This is exactly the way the code has worked up
to this point, but until now we have been under the impression that we were in
fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will
most likely find a payload type which matches the codec clock rate.

This CL also does a little cleaning of the DTMFQueue and RTPSenderAudio classes.
BUG=webrtc:2795
==========

to

==========
Support multiple timestamp rates for sending DTMF.

We support multiple payload types, and one which matches the audio codec the
closest, is picked (or the one with lowest clock rate, if no perfect match is
found).

The exact clock rate is then ignored and DTMF packets are time stamped with the
rate of the current audio codec. This is exactly the way the code has worked up
to this point, but until now we have been under the impression that we were in
fact sending 8k DTMF.

In other words, this is an improvement over the current situation, since we will
most likely find a payload type which matches the codec clock rate.

This CL also does a little cleaning of the DTMFQueue and RTPSenderAudio classes.
BUG=webrtc:2795

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

[commit-bot: I haz the power, 2016-11-17 13:25:55]

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