Fix bug in DTMF generation where events with level > 36 would be ignored.

Fix bug in DTMF generation where events with level > 36 would be ignored.

BUG=webrtc:2795
Committed: https://crrev.com/99df6c03c333202b59be8a70fa561027d995b0c2
Cr-Commit-Position: refs/heads/master@{#14598}

[the sun, 2016-10-11 07:59:13]

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

[the sun, 2016-10-11 07:59:13]

According to section 2.3.4 of RFC 4733 the level is max 63 (yielding -63 dBm0).
Is there a reason our cap was set lower?

See: https://tools.ietf.org/html/rfc4733#page-9

[hlundin-webrtc, 2016-10-11 09:08:36]

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/dtmf_buffer.cc (right):

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/dtmf_buffer.cc:102: event.volume < 0 ||
event.volume > 63 ||
This is a bug that was introduced by yours truly in the NetEq3->NetEq4 rewrite.
The old implementation simply discarded low-amplitude events, without throwing
an error. You could argue that it is bad to do that too.

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/dtmf_tone_generator.cc (right):

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/dtmf_tone_generator.cc:91: const int
DtmfToneGenerator::kAmplitude[37] = {
You will have to extend this array with values for the newly accepted
attenuations. Or, scrap the array and calculate the amplitude each time.

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:391: // Event: 2, E
bit, Volume: 0x3F, Length: 0x10F0.
Please, spell out the volume and length in decimal notation too.

[the sun, 2016-10-11 09:44:53]

Why is the max amplitude 16141 and not 16383? Or 16195 (for -0.1 dB)?

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/dtmf_buffer.cc (right):

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/dtmf_buffer.cc:102: event.volume < 0 ||
event.volume > 63 ||
On 2016/10/11 09:08:36, hlundin-webrtc wrote:
> This is a bug that was introduced by yours truly in the NetEq3->NetEq4
rewrite.
> The old implementation simply discarded low-amplitude events, without throwing
> an error. You could argue that it is bad to do that too.

Acknowledged.

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/dtmf_tone_generator.cc (right):

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/dtmf_tone_generator.cc:91: const int
DtmfToneGenerator::kAmplitude[37] = {
On 2016/10/11 09:08:36, hlundin-webrtc wrote:
> You will have to extend this array with values for the newly accepted
> attenuations. Or, scrap the array and calculate the amplitude each time.

Done.

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/2404183003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:391: // Event: 2, E
bit, Volume: 0x3F, Length: 0x10F0.
On 2016/10/11 09:08:36, hlundin-webrtc wrote:
> Please, spell out the volume and length in decimal notation too.

Done.

[hlundin-webrtc, 2016-10-11 10:30:16]

I don't know why the max amplitude is set the way it is.

LGTM

[the sun, 2016-10-11 10:31:55]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2016-10-11 10:32:06]

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 11:35:36]

Committed patchset #3 (id:40001)

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

Description was changed from

==========
Fix bug in DTMF generation where events with level > 36 would be ignored.

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

to

==========
Fix bug in DTMF generation where events with level > 36 would be ignored.

BUG=webrtc:2795

Committed: https://crrev.com/99df6c03c333202b59be8a70fa561027d995b0c2
Cr-Commit-Position: refs/heads/master@{#14598}
==========

[commit-bot: I haz the power, 2016-10-11 11:35:45]

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