Refactor WVoE DTMF handling #2

- Remove calls to VoEDtmf from WVoE/MC.
- Flatten logic and make the relevant calls on VoE::Channel from AudioSendStream::SendTelephoneEvent().
- Store current payload type for telephone events in WVoMC, instead of setting it on the Channel. This should be refactored to be an AudioSendStream::Config parameter when we redo WVoMC::SetSendCodecs().

BUG=webrtc:4690
R=pthatcher@webrtc.org, tina.legrand@webrtc.org

Committed: https://crrev.com/b572768efbc1e52b97a5ad98932c667956aba4b8
Cr-Commit-Position: refs/heads/master@{#10895}

[the sun, 2015-12-02 15:13:02]

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

[the sun, 2015-12-02 15:16:38]

Description was changed from

==========
Refactor WVoE DTMF handling #2

BUG=webrtc:4690
==========

to

==========
- Remove calls to VoEDtmf from WVoE/MC.
- Flatten logic and make the relevant calls on VoE::Channel from
AudioSendStream::SendTelephoneEvent().
- Store current payload type for telephone events in WVoMC, instead of setting
it on the Channel. This should be refactored to be an AudioSendStream::Config
parameter when we redo WVoMC::SetCodec().

BUG=webrtc:4690
==========

[the sun, 2015-12-02 15:17:23]

Description was changed from

==========
- Remove calls to VoEDtmf from WVoE/MC.
- Flatten logic and make the relevant calls on VoE::Channel from
AudioSendStream::SendTelephoneEvent().
- Store current payload type for telephone events in WVoMC, instead of setting
it on the Channel. This should be refactored to be an AudioSendStream::Config
parameter when we redo WVoMC::SetCodec().

BUG=webrtc:4690
==========

to

==========
- Remove calls to VoEDtmf from WVoE/MC.
- Flatten logic and make the relevant calls on VoE::Channel from
AudioSendStream::SendTelephoneEvent().
- Store current payload type for telephone events in WVoMC, instead of setting
it on the Channel. This should be refactored to be an AudioSendStream::Config
parameter when we redo WVoMC::SetSendCodecs().

BUG=webrtc:4690
==========

[pthatcher1, 2015-12-02 18:45:37]

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/fakeweb...
File talk/media/webrtc/fakewebrtccall.h (right):

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/fakeweb...
talk/media/webrtc/fakewebrtccall.h:78: TelephoneEvent telephone_event_;
I think lastest_telephone_event_ would be a better name.

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1337: return stream_;
Rather than expose the stream underneath, it seems like we should just add a
SendTelephoneEvent method that calls the SendTelephoneEvent method on the
stream_.

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2306: }
Does this work?

audio it = ssrc ? send_streams_.find(ssrc) : send_streams_.begin();

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.h:268: rtc::Optional<int>
dtmf_payload_type_;
Why do we store this?  Why not just look it up from the list of send_codecs_
whenever we need it?  DTMF is sent so infrequently that perf isn't an issue. 
And the code would be cleaner, I think, if it just reads it on demand.

[the sun, 2015-12-02 19:40:23]

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.h:268: rtc::Optional<int>
dtmf_payload_type_;
On 2015/12/02 18:45:37, pthatcher1 wrote:
> Why do we store this?  Why not just look it up from the list of send_codecs_
> whenever we need it?  DTMF is sent so infrequently that perf isn't an issue. 
> And the code would be cleaner, I think, if it just reads it on demand.

Perhaps it would be a little cleaner if we did that, but note that it will need
to be done in two places: CanInsertDtmf() and InsertDtmf() and, going forward,
the payload type will be added to the AudioSendStream::Config so there will
nevertheless be code under SetSendCodecs() which figures out whether to enable
DTMF or not.

If it is ok with you I'd like to keep it as now and revisit when we refactor
codec setting to use RAC.

[pthatcher, 2015-12-02 19:42:46]

On 2015/12/02 19:40:23, the sun wrote:
>
https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
> File talk/media/webrtc/webrtcvoiceengine.h (right):
> 
>
https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.h:268: rtc::Optional<int>
> dtmf_payload_type_;
> On 2015/12/02 18:45:37, pthatcher1 wrote:
> > Why do we store this?  Why not just look it up from the list of send_codecs_
> > whenever we need it?  DTMF is sent so infrequently that perf isn't an issue.

> > And the code would be cleaner, I think, if it just reads it on demand.
> 
> Perhaps it would be a little cleaner if we did that, but note that it will
need
> to be done in two places: CanInsertDtmf() and InsertDtmf() and, going forward,
> the payload type will be added to the AudioSendStream::Config so there will
> nevertheless be code under SetSendCodecs() which figures out whether to enable
> DTMF or not.
> 
> If it is ok with you I'd like to keep it as now and revisit when we refactor
> codec setting to use RAC.

ok

[the sun, 2015-12-03 09:57:47]

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/fakeweb...
File talk/media/webrtc/fakewebrtccall.h (right):

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/fakeweb...
talk/media/webrtc/fakewebrtccall.h:78: TelephoneEvent telephone_event_;
On 2015/12/02 18:45:37, pthatcher1 wrote:
> I think lastest_telephone_event_ would be a better name.

Done.

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1337: return stream_;
On 2015/12/02 18:45:37, pthatcher1 wrote:
> Rather than expose the stream underneath, it seems like we should just add a
> SendTelephoneEvent method that calls the SendTelephoneEvent method on the
> stream_.

Done.

https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2306: }
On 2015/12/02 18:45:37, pthatcher1 wrote:
> Does this work?
> 
> audio it = ssrc ? send_streams_.find(ssrc) : send_streams_.begin();

Nice one! 
Except "audio" is not a recognized C++ keyword. ;)

[pthatcher1, 2015-12-03 18:13:52]

On 2015/12/03 09:57:47, the sun wrote:
>
https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/fakeweb...
> File talk/media/webrtc/fakewebrtccall.h (right):
> 
>
https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/fakeweb...
> talk/media/webrtc/fakewebrtccall.h:78: TelephoneEvent telephone_event_;
> On 2015/12/02 18:45:37, pthatcher1 wrote:
> > I think lastest_telephone_event_ would be a better name.
> 
> Done.
> 
>
https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
> File talk/media/webrtc/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.cc:1337: return stream_;
> On 2015/12/02 18:45:37, pthatcher1 wrote:
> > Rather than expose the stream underneath, it seems like we should just add a
> > SendTelephoneEvent method that calls the SendTelephoneEvent method on the
> > stream_.
> 
> Done.
> 
>
https://codereview.webrtc.org/1491743004/diff/40001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.cc:2306: }
> On 2015/12/02 18:45:37, pthatcher1 wrote:
> > Does this work?
> > 
> > audio it = ssrc ? send_streams_.find(ssrc) : send_streams_.begin();
> 
> Nice one! 
> Except "audio" is not a recognized C++ keyword. ;)

Obviously I type that word too often.

[pthatcher1, 2015-12-03 18:17:45]

lgtm

[the sun, 2015-12-03 18:46:59]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2015-12-03 18:47:05]

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

[commit-bot: I haz the power, 2015-12-03 18:51:04]

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

[commit-bot: I haz the power, 2015-12-03 18:51:05]

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

[the sun, 2015-12-03 19:27:12]

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

[the sun, 2015-12-03 19:27:39]

On 2015/12/03 18:51:05, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   presubmit on tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2148)

Adding tommi@, for webrtc/ and webrtc/test/

[the sun, 2015-12-04 08:56:23]

solenberg@webrtc.org changed reviewers:
+ tina.legrand@webrtc.org
- tommi@webrtc.org

[the sun, 2015-12-04 08:57:06]

Switching reviewer for webrtc/ and webrtc/test/ to tlegrand@ since tommi@ might
be out today.

[tlegrand-webrtc, 2015-12-04 11:58:19]

I'm trying to think where it could make sense to prepare for the multiple
payload types that will be added. Can we discuss offline?

https://codereview.webrtc.org/1491743004/diff/100001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1491743004/diff/100001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:268: rtc::Optional<int>
dtmf_payload_type_;
We will support four different payload types shortly, one for each of the clock
rates 8000, 16000, 32000 and 48000. Does that mean that we will need four
parameters here?

[tlegrand-webrtc, 2015-12-04 13:32:03]

On 2015/12/04 11:58:19, tlegrand-webrtc wrote:
> I'm trying to think where it could make sense to prepare for the multiple
> payload types that will be added. Can we discuss offline?
> 
>
https://codereview.webrtc.org/1491743004/diff/100001/talk/media/webrtc/webrtc...
> File talk/media/webrtc/webrtcvoiceengine.h (right):
> 
>
https://codereview.webrtc.org/1491743004/diff/100001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvoiceengine.h:268: rtc::Optional<int>
> dtmf_payload_type_;
> We will support four different payload types shortly, one for each of the
clock
> rates 8000, 16000, 32000 and 48000. Does that mean that we will need four
> parameters here?

LGTM

[the sun, 2015-12-04 14:04:34]

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

[commit-bot: I haz the power, 2015-12-04 14:04:37]

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

[commit-bot: I haz the power, 2015-12-04 14:15:26]

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

[commit-bot: I haz the power, 2015-12-04 14:15:27]

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

[the sun, 2015-12-04 14:22:35]

Description was changed from

==========
- Remove calls to VoEDtmf from WVoE/MC.
- Flatten logic and make the relevant calls on VoE::Channel from
AudioSendStream::SendTelephoneEvent().
- Store current payload type for telephone events in WVoMC, instead of setting
it on the Channel. This should be refactored to be an AudioSendStream::Config
parameter when we redo WVoMC::SetSendCodecs().

BUG=webrtc:4690
==========

to

==========
- Remove calls to VoEDtmf from WVoE/MC.
- Flatten logic and make the relevant calls on VoE::Channel from
AudioSendStream::SendTelephoneEvent().
- Store current payload type for telephone events in WVoMC, instead of setting
it on the Channel. This should be refactored to be an AudioSendStream::Config
parameter when we redo WVoMC::SetSendCodecs().

BUG=webrtc:4690
R=pthatcher@webrtc.org, tina.legrand@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/b572768efbc1e52b97a5ad989...
==========

[the sun, 2015-12-04 14:22:38]

Committed patchset #7 (id:120001) manually as
b572768efbc1e52b97a5ad98932c667956aba4b8 (presubmit successful).

[commit-bot: I haz the power, 2015-12-04 14:22:40]

Description was changed from

==========
- Remove calls to VoEDtmf from WVoE/MC.
- Flatten logic and make the relevant calls on VoE::Channel from
AudioSendStream::SendTelephoneEvent().
- Store current payload type for telephone events in WVoMC, instead of setting
it on the Channel. This should be refactored to be an AudioSendStream::Config
parameter when we redo WVoMC::SetSendCodecs().

BUG=webrtc:4690
R=pthatcher@webrtc.org, tina.legrand@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/b572768efbc1e52b97a5ad989...
==========

to

==========
- Remove calls to VoEDtmf from WVoE/MC.
- Flatten logic and make the relevant calls on VoE::Channel from
AudioSendStream::SendTelephoneEvent().
- Store current payload type for telephone events in WVoMC, instead of setting
it on the Channel. This should be refactored to be an AudioSendStream::Config
parameter when we redo WVoMC::SetSendCodecs().

BUG=webrtc:4690
R=pthatcher@webrtc.org, tina.legrand@webrtc.org

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

[commit-bot: I haz the power, 2015-12-04 14:22:41]

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