Bug 132952 – [MediaStream] RTCDtmfSender default values need to be updated.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kiran]

The default ToneDuration and ToneGap values are modified in the updated spec.
So these values need to be updated.

According to spec [1]

The duration parameter indicates the duration in ms to use for each character passed in the tones parameters. The duration cannot be more than 6000 ms or less than 40 ms. The default duration is 100 ms for each tone.

The interToneGap parameter indicates the gap between tones. It MUST be at least 30 ms. The default value is 70 ms.

[1] http://dev.w3.org/2011/webrtc/editor/webrtc.html#methods-4

[Kiran]

Created attachment 231509 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

The default ToneDuration and ToneGap values are modified in the updated spec.

The patch updates the default values as per the spec.

[Eric Carlson]

Comment on attachment 231509 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

The changes look fine, but please add a test. A new test would be fine, or you should be able to modify LayoutTests/fast/mediastream/RTCPeerConnection-dtmf.html to test the min, max, and defaults.

[Kiran]

Created attachment 231512 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Updated RTCPeerConnection-dtmf.html to check for default values, as per the comments.

[Eric Carlson]

Comment on attachment 231512 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

View in context: https://bugs.webkit.org/attachment.cgi?id=231512&action=review

> LayoutTests/fast/mediastream/RTCPeerConnection-dtmf.html:47
>                      dtmfsender.insertDTMF("1");
>                      shouldBeEqualToString('dtmfsender.toneBuffer', "1");
> +                    shouldBeEqualToString('dtmfsender.duration', "100");
> +                    shouldBeEqualToString('dtmfsender.interToneGap', "70");

insertDTMF() has two optional parameters:

    [RaisesException] void insertDTMF(DOMString tones, optional long duration, optional long interToneGap);

so you can also test the tone duration and inter-tone gap maximum and minimum.

[Kiran]

Created attachment 231519 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Added tests to check min and max values for duration and min value for duration as specified in review comments.

[Eric Carlson]

Comment on attachment 231519 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

View in context: https://bugs.webkit.org/attachment.cgi?id=231519&action=review

> LayoutTests/fast/mediastream/RTCPeerConnection-dtmf.html:61
> +                    dtmfsender.insertDTMF("1", "40", "30");
> +                    shouldBeEqualToString('dtmfsender.toneBuffer', "1");
> +                    shouldBeEqualToString('dtmfsender.duration', "40");
> +                    shouldBeEqualToString('dtmfsender.interToneGap', "30");
> +
> +                    dtmfsender.insertDTMF("1", "6000");
> +                    shouldBeEqualToString('dtmfsender.toneBuffer', "1");
> +                    shouldBeEqualToString('dtmfsender.duration', "6000");
> +                    shouldBeEqualToString('dtmfsender.interToneGap', "70");
> +

You should also test to ensure that exceptions are thrown when the limits are exceeded. Something like this should work:

            testDOMException("dtmfsender.insertDTMF('1', '20', '30')", "DOMException.SYNTAX_ERR");
            testDOMException("dtmfsender.insertDTMF('1', '7000', '30')", "DOMException.SYNTAX_ERR");

[Kiran]

Created attachment 231558 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Added tests for testing min and max limits as per the review comments.

[Kiran]

Created attachment 231566 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Update the reviewers name in ChangeLog file

[WebKit Commit Bot]

Comment on attachment 231566 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Rejecting attachment 231566 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 231566, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
a/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: http://webkit-queues.appspot.com/results/5342596842913792

[Kiran]

Created attachment 231567 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Uploaded the patch again.

[Alberto Garcia]

Committed r168966: <http://trac.webkit.org/changeset/168966>

[Eric Carlson]

Reopened because no updated test results were landed. Please attach a patch with an updated RTCPeerConnection-dtmf-expected.txt.

[Kiran]

Hi,
The code without this patch is also not giving the expected data,
The reason for this is 
1. There is not actual implementation for DTMFSender.
2. Mock implementation is, by default, returning null.

So, until the code is added for DTMFHandler (either mock implementation or full implementation), we can not invoke insertDTMF and can not test this feature.

I would like to add a FIXME comment in the code, if reviewer is ok for adding it instead of checking for the expected_output_file.

[Eric Carlson]

(In reply to comment #13)
> Hi,
> The code without this patch is also not giving the expected data,
> The reason for this is 
> 1. There is not actual implementation for DTMFSender.
> 2. Mock implementation is, by default, returning null.
> 
> So, until the code is added for DTMFHandler (either mock implementation or full implementation), we can not invoke insertDTMF and can not test this feature.
> 
> I would like to add a FIXME comment in the code, if reviewer is ok for adding it instead of checking for the expected_output_file.

Why don't you open a bug about adding mock DTMFSender support and add a FIXME that references that bug. Ideally that would have been done with this patch, but that is my fault for not noticing :-)

[Kiran]

Created attachment 232111 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Added FIXME to implement mock DTMFSenderHandler inorder to test basic functionality of DTMFSender.

[Kiran]

Created attachment 232114 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Removed empty line in ChangeLog.

[Kiran]

(In reply to comment #14)
> (In reply to comment #13)
> > Hi,
> > The code without this patch is also not giving the expected data,
> > The reason for this is 
> > 1. There is not actual implementation for DTMFSender.
> > 2. Mock implementation is, by default, returning null.
> > 
> > So, until the code is added for DTMFHandler (either mock implementation or full implementation), we can not invoke insertDTMF and can not test this feature.
> > 
> > I would like to add a FIXME comment in the code, if reviewer is ok for adding it instead of checking for the expected_output_file.
> 
> Why don't you open a bug about adding mock DTMFSender support and add a FIXME that references that bug. Ideally that would have been done with this patch, but that is my fault for not noticing :-)

HI,
I raised a new bug for implementing RTCDTMFSenderHandlerMock (BUG: 133298).
Since this patch is already landed (comment 11), it seems we can close this bug and track this on 133298.