Bug 205802 – Implement MediaRecorder backend in GPUProcess

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Implement MediaRecorder backend in GPUProcess

[youenn fablet]

Created attachment 386846 [details]
Patch

[youenn fablet]

Created attachment 386848 [details]
Patch

[youenn fablet]

Created attachment 386851 [details]
Patch

[youenn fablet]

Created attachment 386853 [details]
Patch

[youenn fablet]

Created attachment 386936 [details]
Patch

[youenn fablet]

Created attachment 386942 [details]
Patch

[youenn fablet]

Created attachment 386946 [details]
Patch

[youenn fablet]

Created attachment 386957 [details]
Patch

[youenn fablet]

Created attachment 386961 [details]
Patch

[Eric Carlson]

Comment on attachment 386961 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:555
> +    page.mediaRecorderProvider().setUseGPUProcess(true);

Is this the correct default?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:153
> +#if PLATFORM(COCOA) && USE(LIBWEBRTC)

Should this be "#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM"?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:181
> +    if (decoder.messageReceiverName() == Messages::RemoteMediaRecorders::messageReceiverName()) {
> +        mediaRecorders().didReceiveMessageFromWebProcess(connection, decoder);
> +        return;
> +    }
> +    if (decoder.messageReceiverName() == Messages::RemoteMediaRecorder::messageReceiverName()) {

"RemoteMediaRecorders" is a bit subtle and easy to confuse with "RemoteMediaRecorder". Maybe something like "RemoteMediaRecorderManager" instead?

> Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:52
> +    // FIXME: We would better to throw an exception to JavaScript if writer creation fails.

s/We would better to throw an exception/We should throw an exception/

[youenn fablet]

(In reply to Eric Carlson from comment #10)
> Comment on attachment 386961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386961&action=review
> 
> > Source/WebCore/testing/Internals.cpp:555
> > +    page.mediaRecorderProvider().setUseGPUProcess(true);
> 
> Is this the correct default?

I think so, I am not sure we will enable MediaRecorder by default without GPUProcess.

> 
> > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:153
> > +#if PLATFORM(COCOA) && USE(LIBWEBRTC)
> 
> Should this be "#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM"?

Right.

> > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:181
> > +    if (decoder.messageReceiverName() == Messages::RemoteMediaRecorders::messageReceiverName()) {
> > +        mediaRecorders().didReceiveMessageFromWebProcess(connection, decoder);
> > +        return;
> > +    }
> > +    if (decoder.messageReceiverName() == Messages::RemoteMediaRecorder::messageReceiverName()) {
> 
> "RemoteMediaRecorders" is a bit subtle and easy to confuse with
> "RemoteMediaRecorder". Maybe something like "RemoteMediaRecorderManager"
> instead?

OK, will go with manager.

> > Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:52
> > +    // FIXME: We would better to throw an exception to JavaScript if writer creation fails.
> 
> s/We would better to throw an exception/We should throw an exception/

OK

[youenn fablet]

Comment on attachment 386961 [details]
Patch

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

>>> Source/WebKit/WebProcess/GPU/webrtc/MediaRecorderPrivate.cpp:52
>>> +    // FIXME: We would better to throw an exception to JavaScript if writer creation fails.
>> 
>> s/We would better to throw an exception/We should throw an exception/
> 
> OK

Actually, I implemented it now using m_errorCallback, will remove this FIXME.

[youenn fablet]

Created attachment 386965 [details]
Patch for landing

[youenn fablet]

Created attachment 386966 [details]
Patch for landing

[youenn fablet]

Created attachment 386968 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 386968 [details]
Patch for landing

Clearing flags on attachment: 386968

Committed r254132: <https://trac.webkit.org/changeset/254132>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58376117>

[Ross Kirsling]

This broke WinCairo, as indicated by EWS.

[youenn fablet]

(In reply to Ross Kirsling from comment #19)
> This broke WinCairo, as indicated by EWS.

I did not see it :(
Will fix it.

[Ross Kirsling]

(In reply to youenn fablet from comment #20)
> (In reply to Ross Kirsling from comment #19)
> > This broke WinCairo, as indicated by EWS.
> 
> I did not see it :(
> Will fix it.

No worries -- thanks Youenn!

[WebKit Commit Bot]

Re-opened since this is blocked by bug 205868

[youenn fablet]

Created attachment 387091 [details]
Build fixes

[WebKit Commit Bot]

Comment on attachment 387091 [details]
Build fixes

Clearing flags on attachment: 387091

Committed r254194: <https://trac.webkit.org/changeset/254194>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.