Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699503002:
Change minimum DTMF event duration to be 40 milliseconds (Closed)
DescriptionChange minimum DTMF event duration to be 40 milliseconds
The current value of 100 milliseconds is the recommended default value
in https://w3c.github.io/webrtc-pc/#rtcdtmfsender; the actual minimum specified is 40 milliseconds.
BUG=webrtc:7163
Review-Url: https://codereview.webrtc.org/2699503002
Cr-Commit-Position: refs/heads/master@{#17430}
Committed: https://chromium.googlesource.com/external/webrtc/+/588101cb91468673c66cd84029c561e8ef7a9279
Patch Set 1 #
Total comments: 1
Patch Set 2 : Change minimum DTMF event duration to be 40 milliseconds #
Total comments: 7
Patch Set 3 : Change minimum DTMF event duration to be 40 milliseconds #
Messages
Total messages: 26 (12 generated)
dminor@webrtc.org changed reviewers: + solenberg@chromium.org
Description was changed from ========== Change kMinTelephoneEventDuration to be 40 milliseconds The current value of 100 milliseconds is the recommended default value in RFC 7874; the actual minimum specified is 40 milliseconds. BUG=webrtc:7163 ========== to ========== Change kMinTelephoneEventDuration to be 40 milliseconds The current value of 100 milliseconds is the recommended default value in RFC 7874; the actual minimum specified is 40 milliseconds. BUG=webrtc:7163 ==========
solenberg@chromium.org changed reviewers: + solenberg@webrtc.org - solenberg@chromium.org
https://codereview.webrtc.org/2699503002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2699503002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:105: const int kMinTelephoneEventDuration = 40; Refer to https://w3c.github.io/webrtc-pc/#rtcdtmfsender in the CL description (+a comment here). Also, you should update the limits in webrtc/pc/dtmfsender.cc and add the missing unit tests.
Description was changed from ========== Change kMinTelephoneEventDuration to be 40 milliseconds The current value of 100 milliseconds is the recommended default value in RFC 7874; the actual minimum specified is 40 milliseconds. BUG=webrtc:7163 ========== to ========== Change minimum DTMF event duration to be 40 milliseconds The current value of 100 milliseconds is the recommended default value in https://w3c.github.io/webrtc-pc/#rtcdtmfsender; the actual minimum specified is 40 milliseconds. BUG=webrtc:7163 ==========
Patchset #2 (id:20001) has been deleted
solenberg@webrtc.org changed reviewers: + deadbeef@webrtc.org
lgtm Adding Taylor for webrtc/pc/ ownership approval. https://codereview.webrtc.org/2699503002/diff/40001/webrtc/pc/dtmfsender.cc File webrtc/pc/dtmfsender.cc (right): https://codereview.webrtc.org/2699503002/diff/40001/webrtc/pc/dtmfsender.cc#n... webrtc/pc/dtmfsender.cc:42: // The duration cannot be more than 6000ms or less than 40ms. The gap between Add reference to spec.
https://codereview.webrtc.org/2699503002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2699503002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:106: const int kMaxTelephoneEventDuration = 60000; // Actual limit is 2^16 As long as we're changing the min to match the spec, should we change the max? This is 60 seconds instead of 6. https://codereview.webrtc.org/2699503002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2699503002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:256: // Check that we fail if we specify a too short duration nit: Period at end of comment, "duration that's too short" https://codereview.webrtc.org/2699503002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:257: EXPECT_FALSE(channel_->InsertDtmf(ssrc, 2, 39)); There should also be a test somewhere that "InsertDtmf" with a duration of exactly 40 works. https://codereview.webrtc.org/2699503002/diff/40001/webrtc/pc/dtmfsender_unit... File webrtc/pc/dtmfsender_unittest.cc (right): https://codereview.webrtc.org/2699503002/diff/40001/webrtc/pc/dtmfsender_unit... webrtc/pc/dtmfsender_unittest.cc:337: EXPECT_FALSE(dtmf_->InsertDtmf(tones, 39, inter_tone_gap)); There should also be a test somewhere that "InsertDtmf" with a duration of exactly 40 works.
https://codereview.webrtc.org/2699503002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2699503002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:106: const int kMaxTelephoneEventDuration = 60000; // Actual limit is 2^16 On 2017/02/16 18:18:22, Taylor Brandstetter wrote: > As long as we're changing the min to match the spec, should we change the max? > This is 60 seconds instead of 6. I don't think it makes sense to check ranges at all layers of the call chain, so these should just be removed. This is not an API boundary. DtmfSender::DoInsertDtmf() already ensures we're sticking to the WebRTC 1.0 spec, and AFAICT this is the only way we get to WebRtcVoiceMediaChannel::InsertDtmf(). Channel::SendTelephoneEventOutband() DCHECKs the input is within the bounds specified in RFC4733 (that's the limit of 65535 which somebody has rounded down to 60s above). Those checks should be in the module producing the DTMF payloads (RtpSenderAudio), however that's a story for a different CL. So I suggest we remove these four constants and their use in WebRtcVoiceMediaChannel::InsertDtmf().
https://codereview.webrtc.org/2699503002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2699503002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:106: const int kMaxTelephoneEventDuration = 60000; // Actual limit is 2^16 On 2017/02/16 19:54:58, the sun wrote: > On 2017/02/16 18:18:22, Taylor Brandstetter wrote: > > As long as we're changing the min to match the spec, should we change the max? > > This is 60 seconds instead of 6. > > I don't think it makes sense to check ranges at all layers of the call chain, so > these should just be removed. This is not an API boundary. > > DtmfSender::DoInsertDtmf() already ensures we're sticking to the WebRTC 1.0 > spec, and AFAICT this is the only way we get to > WebRtcVoiceMediaChannel::InsertDtmf(). > > Channel::SendTelephoneEventOutband() DCHECKs the input is within the bounds > specified in RFC4733 (that's the limit of 65535 which somebody has rounded down > to 60s above). Those checks should be in the module producing the DTMF payloads > (RtpSenderAudio), however that's a story for a different CL. > > So I suggest we remove these four constants and their use in > WebRtcVoiceMediaChannel::InsertDtmf(). Sounds good to me.
On 2017/02/21 14:44:30, dminor wrote: Anything more to see here?
I believe patch set 3 addresses all of the review comments to date. Please let me know if I've missed something, this is my first time using this code review tool and it's entirely possible I've missed something along the way. Thank you!
lgtm (sorry, didn't realize you were waiting for me)
The CQ bit was checked by dminor@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2699503002/#ps60001 (title: "Change minimum DTMF event duration to be 40 milliseconds")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by dminor@webrtc.org
The CQ bit was checked by dminor@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490723758857310,
"parent_rev": "3ac729a53b4604c920c923dbf71bec74ed8cb812", "commit_rev":
"588101cb91468673c66cd84029c561e8ef7a9279"}
Message was sent while issue was closed.
Description was changed from ========== Change minimum DTMF event duration to be 40 milliseconds The current value of 100 milliseconds is the recommended default value in https://w3c.github.io/webrtc-pc/#rtcdtmfsender; the actual minimum specified is 40 milliseconds. BUG=webrtc:7163 ========== to ========== Change minimum DTMF event duration to be 40 milliseconds The current value of 100 milliseconds is the recommended default value in https://w3c.github.io/webrtc-pc/#rtcdtmfsender; the actual minimum specified is 40 milliseconds. BUG=webrtc:7163 Review-Url: https://codereview.webrtc.org/2699503002 Cr-Commit-Position: refs/heads/master@{#17430} Committed: https://chromium.googlesource.com/external/webrtc/+/588101cb91468673c66cd8402... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/588101cb91468673c66cd8402... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
