Bug 213720 - Support MediaRecorder.onstart
Summary: Support MediaRecorder.onstart
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 85851
  Show dependency treegraph
 
Reported: 2020-06-29 02:59 PDT by youenn fablet
Modified: 2020-07-03 07:55 PDT (History)
12 users (show)

See Also:


Attachments
Patch (7.30 KB, patch)
2020-06-29 03:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (7.32 KB, patch)
2020-06-29 05:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for relanding (7.80 KB, patch)
2020-07-03 06:16 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for relanding (7.79 KB, patch)
2020-07-03 07:26 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 youenn fablet 2020-06-29 02:59:18 PDT
Support MediaRecorder.onstart
Comment 1 youenn fablet 2020-06-29 03:01:36 PDT
Created attachment 403043 [details]
Patch
Comment 2 youenn fablet 2020-06-29 05:46:01 PDT
Created attachment 403052 [details]
Patch
Comment 3 Darin Adler 2020-06-29 10:04:13 PDT
Comment on attachment 403052 [details]
Patch

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

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

It’s not really great to change the type here, but then store the value in m_timeSlice, an Optional<int>. Basically it’s the same incorrect conversion of large values to negative numbers, just done at a slightly later time.
Comment 4 youenn fablet 2020-06-29 10:42:38 PDT
Comment on attachment 403052 [details]
Patch

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

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:129
>> +ExceptionOr<void> MediaRecorder::startRecording(Optional<unsigned> timeSlice)
> 
> It’s not really great to change the type here, but then store the value in m_timeSlice, an Optional<int>. Basically it’s the same incorrect conversion of large values to negative numbers, just done at a slightly later time.

This patch also updates m_timeSlice to be an Optional<unsigned>.
Comment 5 Darin Adler 2020-06-29 10:49:36 PDT
Comment on attachment 403052 [details]
Patch

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

>>> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:129
>>> +ExceptionOr<void> MediaRecorder::startRecording(Optional<unsigned> timeSlice)
>> 
>> It’s not really great to change the type here, but then store the value in m_timeSlice, an Optional<int>. Basically it’s the same incorrect conversion of large values to negative numbers, just done at a slightly later time.
> 
> This patch also updates m_timeSlice to be an Optional<unsigned>.

Oops, how did I overlook that?
Comment 6 EWS 2020-06-29 11:32:15 PDT
Committed r263671: <https://trac.webkit.org/changeset/263671>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403052 [details].
Comment 7 Radar WebKit Bug Importer 2020-06-29 11:33:14 PDT
<rdar://problem/64899889>
Comment 8 Ryan Haddad 2020-07-02 12:35:08 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 9 youenn fablet 2020-07-03 06:16:47 PDT
Created attachment 403455 [details]
Patch for relanding
Comment 10 youenn fablet 2020-07-03 06:18:08 PDT
(In reply to youenn fablet from comment #9)
> Created attachment 403455 [details]
> Patch for relanding

Relanding, with the modification of enqueuing a task to dispatch error/start events given the startLoading callback can be called synchronously.
Comment 11 EWS 2020-07-03 07:14:41 PDT
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Comment 12 youenn fablet 2020-07-03 07:26:41 PDT
Created attachment 403457 [details]
Patch for relanding
Comment 13 EWS 2020-07-03 07:55:17 PDT
Committed r263896: <https://trac.webkit.org/changeset/263896>

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