Bug 132952 - [MediaStream] RTCDtmfSender default values need to be updated.
Summary: [MediaStream] RTCDtmfSender default values need to be updated.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 124288
  Show dependency treegraph
 
Reported: 2014-05-15 05:54 PDT by Kiran
Modified: 2014-05-27 08:04 PDT (History)
10 users (show)

See Also:


Attachments
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. (1.60 KB, patch)
2014-05-15 06:01 PDT, Kiran
eric.carlson: review+
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. (2.90 KB, patch)
2014-05-15 07:38 PDT, Kiran
no flags Details | Formatted Diff | Diff
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. (4.27 KB, patch)
2014-05-15 08:55 PDT, Kiran
eric.carlson: commit-queue-
Details | Formatted Diff | Diff
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. (4.49 KB, patch)
2014-05-15 22:46 PDT, Kiran
kiran.guduru: commit-queue?
Details | Formatted Diff | Diff
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. (4.49 KB, patch)
2014-05-16 04:12 PDT, Kiran
commit-queue: commit-queue-
Details | Formatted Diff | Diff
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. (4.49 KB, patch)
2014-05-16 04:23 PDT, Kiran
kiran.guduru: commit-queue?
Details | Formatted Diff | Diff
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. (4.78 KB, patch)
2014-05-26 23:49 PDT, Kiran
kiran.guduru: commit-queue?
Details | Formatted Diff | Diff
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec. (4.78 KB, patch)
2014-05-26 23:57 PDT, Kiran
kiran.guduru: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kiran 2014-05-15 05:54:21 PDT
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
Comment 1 Kiran 2014-05-15 06:01:37 PDT
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.
Comment 2 Eric Carlson 2014-05-15 06:28:59 PDT
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.
Comment 3 Kiran 2014-05-15 07:38:42 PDT
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.
Comment 4 Eric Carlson 2014-05-15 08:26:41 PDT
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.
Comment 5 Kiran 2014-05-15 08:55:53 PDT
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.
Comment 6 Eric Carlson 2014-05-15 10:45:13 PDT
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");
Comment 7 Kiran 2014-05-15 22:46:22 PDT
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.
Comment 8 Kiran 2014-05-16 04:12:16 PDT
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
Comment 9 WebKit Commit Bot 2014-05-16 04:20:04 PDT
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
Comment 10 Kiran 2014-05-16 04:23:47 PDT
Created attachment 231567 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Uploaded the patch again.
Comment 11 Alberto Garcia 2014-05-16 04:56:52 PDT
Committed r168966: <http://trac.webkit.org/changeset/168966>
Comment 12 Eric Carlson 2014-05-16 11:51:24 PDT
Reopened because no updated test results were landed. Please attach a patch with an updated RTCPeerConnection-dtmf-expected.txt.
Comment 13 Kiran 2014-05-22 22:17:52 PDT
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.
Comment 14 Eric Carlson 2014-05-23 10:11:01 PDT
(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 :-)
Comment 15 Kiran 2014-05-26 23:49:11 PDT
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.
Comment 16 Kiran 2014-05-26 23:57:53 PDT
Created attachment 232114 [details]
The patch updates the default values for RTCDTMFSender tonegap and toneduration as per the spec.

Removed empty line in ChangeLog.
Comment 17 Kiran 2014-05-27 00:29:19 PDT
(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.