Bug 202233 - MediaRecorder.start() Method is Ignoring the "timeslice" Parameter
Summary: MediaRecorder.start() Method is Ignoring the "timeslice" Parameter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari Technology Preview
Hardware: All All
: P2 Major
Assignee: youenn fablet
URL:
Keywords: InRadar
: 190291 (view as bug list)
Depends on:
Blocks: 85851
  Show dependency treegraph
 
Reported: 2019-09-25 13:33 PDT by Zachariah Moreno
Modified: 2020-07-03 05:58 PDT (History)
13 users (show)

See Also:


Attachments
test fails in safari 13.0 (3.96 MB, image/png)
2019-09-25 13:33 PDT, Zachariah Moreno
no flags Details
Patch (6.91 KB, patch)
2020-06-25 13:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Relanding (6.92 KB, patch)
2020-06-29 02:42 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zachariah Moreno 2019-09-25 13:33:56 PDT
Created attachment 379577 [details]
test fails in safari 13.0

Thank You for implementing the GetUserMedia & MediaRecorder interfaces!

We developed a test to verify GetUserMedia & MediaRecorder @ https://codepen.io/XachMoreno/pen/bGbeNWX.

The "timeslice" parameter within the MediaRecorder.start() method is being ignored & recording continues beyond the number in milliseconds.

To replicate...

 1. Open Safari
 2. Open the Develop Menu
 3. Hover over the Experimental Features list item
 4. Click on MediaRecorder to Enable it
 5. Navigate to https://codepen.io/XachMoreno/pen/bGbeNWX
 6. Click on the blue Start Test button at the bottom
 7. Allow access to Microphone & Camera
 8. Click on the Record button at the bottom
 9. Wait 5 seconds

If the test FAILS, an alert dialog will open communicating the lack of support for the timeslice parameter.

If the test PASSES, the 5 second timer will reset to 1 & continue recording without an alert.

Spec Reference - https://w3c.github.io/mediacapture-record/#dom-mediarecorder-start

Docs Reference - https://developer.mozilla.org/en-US/docs/Web/API/MediaRecorder/start
Comment 1 Radar WebKit Bug Importer 2019-09-25 15:06:54 PDT
<rdar://problem/55720555>
Comment 2 Eric Carlson 2019-09-25 16:41:44 PDT
We are aware that the "timeslice" parameter doesn't work yet, but we plan to implement it.

Thanks for the detailed bug report!
Comment 3 Octavian Naicu 2019-09-26 00:19:19 PDT
Eric, when's the MediaRecorder API planned to be pulled out of experimental and into production?
Comment 4 Zachariah Moreno 2019-11-12 12:07:47 PST
(In reply to Eric Carlson from comment #2)
> We are aware that the "timeslice" parameter doesn't work yet, but we plan to
> implement it.
> 
> Thanks for the detailed bug report!

Thank You Eric! Do timelines exist for...
 1. timeslice support?
 2. graduation from Experimental to Stable Feature?
Comment 5 majo 2020-05-11 08:50:38 PDT
I have started working on the bug
Comment 6 youenn fablet 2020-06-16 01:10:41 PDT
requestData should work as expected soon in WebKit ToT after https://bugs.webkit.org/show_bug.cgi?id=206929.

To implement the time slice parameter, the easiest might be to create a repeating timer, see Timer.h.

It would be interesting to know whether calling requestData is changing how time slice schedules fetches in other browsers implementing MediaRecorder.

We could either go with Timer::startRepeating, or have a one shot timer that is started in MediaRecorder::start and rescheduled in MediaRecorder::requestData lambda.
Comment 7 youenn fablet 2020-06-25 13:09:46 PDT
Created attachment 402801 [details]
Patch
Comment 8 Eric Carlson 2020-06-26 08:35:29 PDT
Comment on attachment 402801 [details]
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:129
> +ExceptionOr<void> MediaRecorder::startRecording(Optional<int> timeSlice)

`timeSlice` is `optional unsigned long` in the spec. Does it matter that this uses a signed int?
Comment 9 youenn fablet 2020-06-26 08:57:50 PDT
(In reply to Eric Carlson from comment #8)
> Comment on attachment 402801 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402801&action=review
> 
> > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:129
> > +ExceptionOr<void> MediaRecorder::startRecording(Optional<int> timeSlice)
> 
> `timeSlice` is `optional unsigned long` in the spec. Does it matter that
> this uses a signed int?

Ah, let's change the WebIDL and timeSlice signature as a follow-up.
It does not make sense to use a signed value here.
WPT media recorder tests are not very comprehensive...
Comment 10 Darin Adler 2020-06-26 09:31:13 PDT
Comment on attachment 402801 [details]
Patch

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

>>> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:129
>>> +ExceptionOr<void> MediaRecorder::startRecording(Optional<int> timeSlice)
>> 
>> `timeSlice` is `optional unsigned long` in the spec. Does it matter that this uses a signed int?
> 
> Ah, let's change the WebIDL and timeSlice signature as a follow-up.
> It does not make sense to use a signed value here.
> WPT media recorder tests are not very comprehensive...

Keep in mind that "unsigned long" in IDL corresponds to "unsigned" in C++ and "long" corresponds to "int". Although if we use the "wrong" type in C++ the symptom is mild.
Comment 11 EWS 2020-06-26 09:49:56 PDT
Committed r263565: <https://trac.webkit.org/changeset/263565>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402801 [details].
Comment 12 Jason Lawrence 2020-06-26 14:45:32 PDT
Reverted r263511, r263514, and r263565 for reason:

r263511 caused MediaRecorder test crashes on internal testers.

Committed r263588: <https://trac.webkit.org/changeset/263588>
Comment 13 youenn fablet 2020-06-29 02:42:51 PDT
Created attachment 403041 [details]
Relanding
Comment 14 youenn fablet 2020-06-29 03:05:55 PDT
*** Bug 190291 has been marked as a duplicate of this bug. ***
Comment 15 EWS 2020-06-29 03:10:06 PDT
Committed r263651: <https://trac.webkit.org/changeset/263651>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403041 [details].
Comment 16 Darin Adler 2020-06-29 09:42:27 PDT
Comment on attachment 403041 [details]
Relanding

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:188
> +    if (m_timeSliceTimer.isActive())
> +        m_timeSliceTimer.stop();

Is this more efficient than unconditionally calling stop?
Comment 17 Ryan Haddad 2020-07-02 12:35:06 PDT
Reverted r263633, r263651, and r263671 for reason:

Still seeing MediaRecorder test crashes after re-landing r263633

Committed r263854: <https://trac.webkit.org/changeset/263854>
Comment 18 youenn fablet 2020-07-03 05:49:13 PDT
(In reply to Darin Adler from comment #16)
> Comment on attachment 403041 [details]
> Relanding
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403041&action=review
> 
> > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:188
> > +    if (m_timeSliceTimer.isActive())
> > +        m_timeSliceTimer.stop();
> 
> Is this more efficient than unconditionally calling stop?

I think so, stop is unconditionally setting m_repeatInterval and calling setNextFireTime.
Comment 19 EWS 2020-07-03 05:58:28 PDT
Committed r263892: <https://trac.webkit.org/changeset/263892>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403041 [details].