Bug 190018

Summary: Runtime flag and IDL for MediaRecorder
Product: WebKit Reporter: Wendy <yuhan_wu>
Component: WebRTCAssignee: Wendy <yuhan_wu>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, eric.carlson, ews-watchlist, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 85851    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Archive of layout-test-results from ews117 for mac-sierra
none
Archive of layout-test-results from ews103 for mac-sierra
none
Patch
none
Patch
none
Archive of layout-test-results from ews202 for win-future
none
Patch
none
Patch
none
Patch
none
Patch none

Description Wendy 2018-09-26 16:38:06 PDT
Add a runtime flag (experimental) and IDL file for the MediaRecorder.
Sepc: https://w3c.github.io/mediacapture-record/#mediarecorder-api
Comment 1 Wendy 2018-09-28 13:30:02 PDT
Created attachment 351103 [details]
Patch
Comment 2 youenn fablet 2018-09-28 13:57:57 PDT
Comment on attachment 351103 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Add an architechture of the IDL and the class for MediaRecorder and BlobEvent.

The patch looks good.
You might also need to:
- Enable the runtime flag for WebKitTestRunner/DumpRenderTree.
- Add a test to verify that BlobEvent, MediaRecorder have constructors and do work. This test might already be covered partially in imported/w3c/web-platform-tests.

> Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:5
> + * Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).

This copyright should only have the Apple line with 2018 as the year.
Since BlobEvent.cpp is not needed, I would not include it here in this patch.

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:27
> +#if ENABLE(MEDIA_STREAM)

Add a line here.

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:29
> +#include <wtf/text/AtomicString.h>

Ditto here.

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:35
> +

No need for this line.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:5
> + * Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).

Ditto for the copyright.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:33
> +

Two lines can be removed.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:42
> +Ref<MediaRecorder> MediaRecorder::create(ScriptExecutionContext& context, Ref<MediaStream>&& stream, Options options)

Should be Options&&

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:48
> +    : ActiveDOMObject(&context)

No need for this constructor, instead reuse the constructor below with a Options&& = { } in the declaration.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:34
> +class MediaRecorder : public ActiveDOMObject, public RefCounted<MediaRecorder>, public EventTargetWithInlineData {

"class MediaRecorder" final would be better.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:40
> +        unsigned long audioBitsPerSecond, videoBitsPerSecond, bitsPerSecond;

One line per member.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:44
> +    static Ref<MediaRecorder> create(ScriptExecutionContext&, Ref<MediaStream>&&, Options);

Options&& = { } should do the trick.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:53
> +    }

Do we need to define this destructor.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:29
> +    EnabledAtRuntime=MediaRecorder,

Sort the keywords lexically.

> Source/WebCore/Modules/mediastream/RTCRtpReceiver.cpp:35
> +#include "PeerConnectionBackend.h"

Is this change needed because of unified build issues?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-18787
> -				574F55DF204F3744002948C6 /* LocalAuthenticator.mm */,

Unrelated change?
Comment 3 Eric Carlson 2018-09-30 03:12:49 PDT
Comment on attachment 351103 [details]
Patch

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

> Source/WebCore/Sources.txt:184
> +Modules/mediarecorder/MediaRecorder.cpp
> +Modules/mediarecorder/BlobEvent.cpp

Nit: reverse the order of these lines so the "mediarecorder" group is sorted.
Comment 4 Wendy 2018-10-02 11:47:40 PDT
Created attachment 351422 [details]
Patch
Comment 5 Wendy 2018-10-02 12:27:10 PDT
Created attachment 351429 [details]
Patch
Comment 6 Wendy 2018-10-02 12:36:04 PDT
Created attachment 351431 [details]
Patch
Comment 7 Wendy 2018-10-02 13:07:04 PDT
Created attachment 351433 [details]
Patch
Comment 8 EWS Watchlist 2018-10-02 14:53:33 PDT
Comment on attachment 351433 [details]
Patch

Attachment 351433 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9431681

New failing tests:
fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html
Comment 9 EWS Watchlist 2018-10-02 14:53:35 PDT
Created attachment 351442 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 10 EWS Watchlist 2018-10-02 15:31:10 PDT
Comment on attachment 351433 [details]
Patch

Attachment 351433 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9431709

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor.html
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html
Comment 11 EWS Watchlist 2018-10-02 15:31:12 PDT
Created attachment 351446 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 12 youenn fablet 2018-10-02 15:33:03 PDT
(In reply to Build Bot from comment #8)
> Comment on attachment 351433 [details]
> Patch
> 
> Attachment 351433 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: https://webkit-queues.webkit.org/results/9431681
> 
> New failing tests:
> fast/mediacapturefromelement/CanvasCaptureMediaStream-
> imagebitmaprenderingcontext.html
> fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
> fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-
> element.html

These errors seem pretty nice as they seem like improvements.
You might need to update the corresponding -expected.txt files.
Comment 13 EWS Watchlist 2018-10-02 15:46:45 PDT
Comment on attachment 351433 [details]
Patch

Attachment 351433 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9432434

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/BlobEvent-constructor.html
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html
media/remote-control-command-seek.html
Comment 14 EWS Watchlist 2018-10-02 15:46:47 PDT
Created attachment 351447 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 15 Wendy 2018-10-02 18:25:10 PDT
Created attachment 351464 [details]
Patch
Comment 16 Wendy 2018-10-02 19:12:44 PDT
Created attachment 351467 [details]
Patch
Comment 17 EWS Watchlist 2018-10-02 21:12:01 PDT
Comment on attachment 351467 [details]
Patch

Attachment 351467 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9435513

New failing tests:
fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
Comment 18 EWS Watchlist 2018-10-02 21:12:12 PDT
Created attachment 351475 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 19 youenn fablet 2018-10-03 01:19:54 PDT
Comment on attachment 351467 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by Youenn Fablet.

At this stage of the patch process, it should probably be marked OOPS.

> Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:30
> +namespace WebCore {

This file does not define anything.
It is probably fine if we are we sure we will need it in the future.
If we are not sure, maybe we should remove it for now.

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:43
> +    static Ref<BlobEvent> create(const AtomicString& type, const Init& init)

If BlobEvent is expected to keep a RefPtr<Blob>, we might want to change const Init& to Init&&.
That can be done later on when actually implementing BlobEvent.

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:48
> +    ~BlobEvent()

Can we remove this destructor?

> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:31
> +    Constructor(DOMString type, BlobEventInit eventInitDict)

Maybe we can sort the keywords (Conditional, Constructor, Enabled, Exposed)

> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:100
> +    macro(BlobEvent) \

Should be moved up for sorting.

> LayoutTests/platform/mac-wk1/TestExpectations:635
> +# MediaRecorder is not supporrted on WK1

s/supporrted/supported/
Canvas capture is supported in WK1 so we should not skip the Canvas Capture tests.
As of Media Recorder, we might consider adding support for it since it should not be WK2 specific.
Maybe all that is needed is to update enableExperimentalFeatures() function in DumpRenderTree.mm to activate the runtime flag.

You might also want to skip these tests for the win platform.
Comment 20 Chris Dumez 2018-10-03 08:56:15 PDT
Comment on attachment 351467 [details]
Patch

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

>> Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:30
>> +namespace WebCore {
> 
> This file does not define anything.
> It is probably fine if we are we sure we will need it in the future.
> If we are not sure, maybe we should remove it for now.

Yes, I think it should be removed. It can always be added back later if and when it is needed.

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:30
> +#include <wtf/text/AtomicString.h>

Not needed.

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:36
> +class BlobEvent : public Event {

Class should be marked as final since Event is virtual.

>> Source/WebCore/Modules/mediarecorder/BlobEvent.h:43
>> +    static Ref<BlobEvent> create(const AtomicString& type, const Init& init)
> 
> If BlobEvent is expected to keep a RefPtr<Blob>, we might want to change const Init& to Init&&.
> That can be done later on when actually implementing BlobEvent.

I believe our current pattern in other event classes is also to have a third parameter like so:
, IsTrusted isTrusted = IsTrusted::No

Allowing the call site to construct an event that is trusted (trusted means that it is issued by the browser rather than the script on the page).

>> Source/WebCore/Modules/mediarecorder/BlobEvent.h:48
>> +    ~BlobEvent()
> 
> Can we remove this destructor?

+1

> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:33
> +

Why are we missed the attributes. If you plan to add them in a follow-up, could you please add a FIXME comment?

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:53
> +    return false;

Please add a FIXME comment and follow-up. Returning false here is rarely OK as it prevents page cache.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:30
> +#include "MediaStream.h"

Can this be forward declared instead? (You mean need an out-of-line destructor).

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:40
> +        unsigned long audioBitsPerSecond;

unsigned long -> unsigned (unsigned long in Web IDL maps to unsigned in C++).

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:41
> +        unsigned long videoBitsPerSecond;

ditto

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:42
> +        unsigned long bitsPerSecond;

ditto.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:31
> +    ConstructorCallWith=ScriptExecutionContext,

You could use ConstructorCallWith=Document since this is not exposed to workers. Then use Document& instead of ScriptExecutionContext& in your C++ implementation.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:35
> +    readonly attribute RecordingState state;

Could you please add a FIXME comment about the missing API?

> Source/WebCore/Sources.txt:2774
> +JSBlobEvent.cpp

Bad sorting
Comment 21 Chris Dumez 2018-10-03 16:31:31 PDT
Comment on attachment 351467 [details]
Patch

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

>>> Source/WebCore/Modules/mediarecorder/BlobEvent.cpp:30
>>> +namespace WebCore {
>> 
>> This file does not define anything.
>> It is probably fine if we are we sure we will need it in the future.
>> If we are not sure, maybe we should remove it for now.
> 
> Yes, I think it should be removed. It can always be added back later if and when it is needed.

Yes, I think it should be removed. It can always be added back later if and when it is needed.

>> Source/WebCore/Modules/mediarecorder/BlobEvent.h:30
>> +#include <wtf/text/AtomicString.h>
> 
> Not needed.

Not needed.

>> Source/WebCore/Modules/mediarecorder/BlobEvent.h:36
>> +class BlobEvent : public Event {
> 
> Class should be marked as final since Event is virtual.

Class should be marked as final since Event is virtual.

>>> Source/WebCore/Modules/mediarecorder/BlobEvent.h:43
>>> +    static Ref<BlobEvent> create(const AtomicString& type, const Init& init)
>> 
>> If BlobEvent is expected to keep a RefPtr<Blob>, we might want to change const Init& to Init&&.
>> That can be done later on when actually implementing BlobEvent.
> 
> I believe our current pattern in other event classes is also to have a third parameter like so:
> , IsTrusted isTrusted = IsTrusted::No
> 
> Allowing the call site to construct an event that is trusted (trusted means that it is issued by the browser rather than the script on the page).

I believe our current pattern in other event classes is also to have a third parameter like so:
, IsTrusted isTrusted = IsTrusted::No

Allowing the call site to construct an event that is trusted (trusted means that it is issued by the browser rather than the script on the page).

> Source/WebCore/Modules/mediarecorder/BlobEvent.h:45
> +        return adoptRef(*new BlobEvent(type, init, IsTrusted::No));

auto blobEvent = adoptRef(*new BlobEvent(type, init, isTrusted));
blobEvent->suspendIfNeeded();
return blobEvent;

Otherwise, you'll get crashes. You need to call suspendIfNeeded() on ActiveDOMObjects.

>>> Source/WebCore/Modules/mediarecorder/BlobEvent.h:48
>>> +    ~BlobEvent()
>> 
>> Can we remove this destructor?
> 
> +1

+1

>> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:33
>> +
> 
> Why are we missed the attributes. If you plan to add them in a follow-up, could you please add a FIXME comment?

Why are we missed the attributes. If you plan to add them in a follow-up, could you please add a FIXME comment?

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:53
>> +    return false;
> 
> Please add a FIXME comment and follow-up. Returning false here is rarely OK as it prevents page cache.

Please add a FIXME comment and follow-up. Returning false here is rarely OK as it prevents page cache.

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:30
>> +#include "MediaStream.h"
> 
> Can this be forward declared instead? (You mean need an out-of-line destructor).

Can this be forward declared instead? (You mean need an out-of-line destructor).

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:40
>> +        unsigned long audioBitsPerSecond;
> 
> unsigned long -> unsigned (unsigned long in Web IDL maps to unsigned in C++).

unsigned long -> unsigned (unsigned long in Web IDL maps to unsigned in C++).

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:41
>> +        unsigned long videoBitsPerSecond;
> 
> ditto

ditto

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:42
>> +        unsigned long bitsPerSecond;
> 
> ditto.

ditto.

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:31
>> +    ConstructorCallWith=ScriptExecutionContext,
> 
> You could use ConstructorCallWith=Document since this is not exposed to workers. Then use Document& instead of ScriptExecutionContext& in your C++ implementation.

You could use ConstructorCallWith=Document since this is not exposed to workers. Then use Document& instead of ScriptExecutionContext& in your C++ implementation.

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:35
>> +    readonly attribute RecordingState state;
> 
> Could you please add a FIXME comment about the missing API?

Could you please add a FIXME comment about the missing API?

>> Source/WebCore/Sources.txt:2774
>> +JSBlobEvent.cpp
> 
> Bad sorting

Bad sorting
Comment 22 Chris Dumez 2018-10-03 16:39:49 PDT
Comment on attachment 351467 [details]
Patch

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

>> Source/WebCore/Modules/mediarecorder/BlobEvent.h:45
>> +        return adoptRef(*new BlobEvent(type, init, IsTrusted::No));
> 
> auto blobEvent = adoptRef(*new BlobEvent(type, init, isTrusted));
> blobEvent->suspendIfNeeded();
> return blobEvent;
> 
> Otherwise, you'll get crashes. You need to call suspendIfNeeded() on ActiveDOMObjects.

This comment was meant for the MediaRecorder create() function, sorry.
Comment 23 Wendy 2018-10-03 16:59:27 PDT
Created attachment 351562 [details]
Patch
Comment 24 Wendy 2018-10-03 17:15:37 PDT
Created attachment 351564 [details]
Patch
Comment 25 Chris Dumez 2018-10-03 19:07:50 PDT
Comment on attachment 351564 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Reviewed by Youenn Fablet (OOPS!).

Please drop the OOPS.

> Source/WebCore/ChangeLog:12
> +        Add an architechture of the IDL and the class for MediaRecorder and BlobEvent.

Typo: architechture

> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:33
> +    // FIXME: The current patch is only for architecture of the BlobEvent

Please use period at the end of comments, as per coding style.

> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:34
> +    // FIXME: It will be implemented in follow-up patches

ditto.

Also, here is what we would usually do for such comments (we rarely use "Will be implemented in follow-up patches" in code comments):

// FIXME: Implement these:
// [ SameObject] readonly attribute Blob data;
// readonly attribute DOMHighResTimeStamp timecode;

Then you only have to uncomment them later on.

> Source/WebCore/Modules/mediarecorder/BlobEvent.idl:36
> +dictionary BlobEventInit : EventInit {

I would add a blank line before this.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:47
> +    , m_state(RecordingState::Inactive)

This could be inline in the header.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:58
> +    return false; // FIXME: This function will be implemented in a follow-up patch

Please use a period at the end of the comment.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:49
> +    RecordingState state() const

I think we usually put these on one line:
RecordingState state() const { return m_state; }

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:72
> +    RecordingState m_state;

RecordingState m_state { RecordingState::Inactive };

> Source/WebCore/Modules/mediarecorder/MediaRecorder.idl:37
> +    // FIXME: It will be implemented in follow-up patches

Same comments as above for the comments.

> LayoutTests/platform/mac-wk1/TestExpectations:633
> +webkit.org/b/189908 imported/w3c/web-platform-tests/resource-timing/resource_timing.worker.html [ Failure ]

Looks like this file has no real changes and should not be part of this patch.

> LayoutTests/platform/win/TestExpectations:4216
> +# MediaRecorder only enalbed for wk1 and wk2

Typo: enalbed.
Also missing a period at the end.
Finally, the comment does not really make sense because Windows uses WebKit1. Maybe it should be "# MediaRecorder is not enabled on Windows."
Comment 26 Wendy 2018-10-03 19:34:13 PDT
Created attachment 351576 [details]
Patch
Comment 27 Chris Dumez 2018-10-04 08:59:51 PDT
Comment on attachment 351576 [details]
Patch

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

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:57
> +    return false; // FIXME: Implement this function to solve the crash issue.

This comment is wrong, this should NOT cause any crashes, this is merely inefficient as it prevents PageCache. The comment should be something like.
// FIXME: We should do better here as this prevents entering PageCache.
Comment 28 Wendy 2018-10-04 11:05:19 PDT
Created attachment 351604 [details]
Patch
Comment 29 WebKit Commit Bot 2018-10-04 12:27:21 PDT
Comment on attachment 351604 [details]
Patch

Clearing flags on attachment: 351604

Committed r236840: <https://trac.webkit.org/changeset/236840>
Comment 30 WebKit Commit Bot 2018-10-04 12:27:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Radar WebKit Bug Importer 2018-10-04 12:58:02 PDT
<rdar://problem/45018935>