Runtime flag and IDL for MediaRecorder

• Component: WebRTC
• Status: RESOLVED FIXED

[Wendy]

Add a runtime flag (experimental) and IDL file for the MediaRecorder.
Sepc: https://w3c.github.io/mediacapture-record/#mediarecorder-api

[Wendy]

Created attachment 351103 [details]
Patch

[youenn fablet]

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?

[Eric Carlson]

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.

[Wendy]

Created attachment 351422 [details]
Patch

[Wendy]

Created attachment 351429 [details]
Patch

[Wendy]

Created attachment 351431 [details]
Patch

[Wendy]

Created attachment 351433 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wendy]

Created attachment 351464 [details]
Patch

[Wendy]

Created attachment 351467 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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.

[Wendy]

Created attachment 351562 [details]
Patch

[Wendy]

Created attachment 351564 [details]
Patch

[Chris Dumez]

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."

[Wendy]

Created attachment 351576 [details]
Patch

[Chris Dumez]

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.

[Wendy]

Created attachment 351604 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351604 [details]
Patch

Clearing flags on attachment: 351604

Committed r236840: <https://trac.webkit.org/changeset/236840>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45018935>