Bug 208814 – [GPUP][GTK] compile GPUProcess in GTK port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Víctor M. Jáquez L.]

Enable the compilation of GPUProcess infrastructure in GTK port

[Víctor M. Jáquez L.]

Created attachment 393044 [details]
WIP Patch

[Víctor M. Jáquez L.]

Right now the patch doesn't compile. But I would like to assure if I'm in the right direction. I'm quite newbie on these matters :)

[Carlos Alberto Lopez Perez]

Comment on attachment 393044 [details]
WIP Patch

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

> Source/cmake/WebKitFeatures.cmake:237
> +    WEBKIT_OPTION_DEFINE(ENABLE_GPU_PROCESS "Whether to enable the GPU process" PRIVATE ON)

Thats would enable it for all ports using CMake by default.
Perhaps you want to set this general default value to OFF, and then change the default to ON only for platform GTK in OptionsGTK.cmake

[Víctor M. Jáquez L.]

Created attachment 393170 [details]
WIP Patch

[Carlos Alberto Lopez Perez]

See https://trac.webkit.org/changeset/258253

[Michael Catanzaro]

Is the eventual goal to block the web process from accessing the GPU using the bubblewrap sandbox? Dropping the call to bindOpenGL in BubblewrapLauncher.cpp? If so, amazing! If not, what is the goal?

[Víctor M. Jáquez L.]

(In reply to Carlos Alberto Lopez Perez from comment #5)
> See https://trac.webkit.org/changeset/258253

I was strip of all glory, but it got easier my work :)

[Víctor M. Jáquez L.]

(In reply to Michael Catanzaro from comment #6)
> Is the eventual goal to block the web process from accessing the GPU using
> the bubblewrap sandbox? Dropping the call to bindOpenGL in
> BubblewrapLauncher.cpp? If so, amazing! If not, what is the goal?

The goal is to isolate multimedia in a process. It is not related with OpenGL or compositing, just multimedia and passing video frames to WebProcess. If I'm not getting it wrong.

[Víctor M. Jáquez L.]

Created attachment 394477 [details]
Patch

[Víctor M. Jáquez L.]

Created attachment 394479 [details]
Patch

[Víctor M. Jáquez L.]

Created attachment 394483 [details]
Patch

[Víctor M. Jáquez L.]

Created attachment 395701 [details]
Patch

[Víctor M. Jáquez L.]

The patch is ready for review :)

[Víctor M. Jáquez L.]

I spoke too fast

[Víctor M. Jáquez L.]

Created attachment 395704 [details]
Patch

[Don Olmstead]

Comment on attachment 395704 [details]
Patch

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

I feel like to start this should just be putting stub files in the directory and touching CMake files.

With regards to the #if PLATFORM(COCOA) stuff it really feels like its too early to guard things with COCOA. Once we have some implementations we should be able to figure out if LayerHostingContext should be shared or not.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:62
> +#if PLATFORM(COCOA)
> +#include "LayerHostingContext.h"

I added a LayerHostingContext.h to Source/WebKit/Platform/generic you can just include that and get rid of these COCOA changes.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.h:284
> +#if PLATFORM(COCOA)
>      std::unique_ptr<LayerHostingContext> m_inlineLayerHostingContext;
>      std::unique_ptr<LayerHostingContext> m_fullscreenLayerHostingContext;
> +#endif

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.messages.in:29
> +#if PLATFORM(COCOA)
>      PrepareForPlayback(bool privateMode, enum:uint8_t WebCore::MediaPlayerEnums::Preload preload, bool preservesPitch, bool prepareForRendering, float videoContentScale) -> (Optional<WebKit::LayerHostingContextID> inlineLayerHostingContextId, Optional<WebKit::LayerHostingContextID> fullscreenLayerHostingContextId) Async
> +#endif

Ditto

> Source/WebKit/SourcesGTK.txt:26
> +GPUProcess/media/gstreamer/RemoteMediaPlayerProxyGStreamer.cpp

You're missing a blank line here.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:44
> +#if PLATFORM(COCOA)
> +#include "LayerHostingContext.h"
> +#endif

Ditto

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:367
> +#if PLATFORM(COCOA)
>      Optional<LayerHostingContextID> m_fullscreenLayerHostingContextId;
> +#endif

Ditto

[Víctor M. Jáquez L.]

Created attachment 395785 [details]
Patch

[Víctor M. Jáquez L.]

Thanks Dom. I guess I've fixed what you commented.

I'm hesitant of VideoLayerRemoteGStreamer.cpp, since I'm not sure it is a GStreamer specific thing or rather something for the texturemapper or so.

[Víctor M. Jáquez L.]

Created attachment 395809 [details]
Patch

[Víctor M. Jáquez L.]

Created attachment 397179 [details]
Patch

[Víctor M. Jáquez L.]

Ping :)

Any comment on this patch?

[Don Olmstead]

Comment on attachment 397179 [details]
Patch

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

Is there a specific reason you decided to add PLATFORM(COCOA) guards? If all that needs to be done is fill out some stubs for gstreamer then I think you should do that. I'm just being a stickler with it because once there's PLATFORM guards they never really go away.

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:76
> +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM)

Is there a reason why this should be Cocoa specific?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:367
> +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM)

Ditto

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:512
> +#if PLATFORM(COCOA)
>      userMediaCaptureManagerProxy().setOrientation(orientation);
> +#endif

Ditto

[Víctor M. Jáquez L.]

Comment on attachment 397179 [details]
Patch

Thanks for the review :) 


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

>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:76
>> +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM)
> 
> Is there a reason why this should be Cocoa specific?

Because UserMediaCaptureManagerProxy.h is Cocoa specific: https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.h

Thus compilation breaks:

../../Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:77:10: fatal error: 'UserMediaCaptureManagerProxy.h' file not found
#include "UserMediaCaptureManagerProxy.h"

>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:367
>> +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM)
> 
> Ditto

Because it is part of UserMediaCaptureManagerProxy

[Don Olmstead]

(In reply to Víctor M. Jáquez L. from comment #23)
> Comment on attachment 397179 [details]
> Patch
> 
> Thanks for the review :) 
> 
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397179&action=review
> 
> >> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:76
> >> +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM)
> > 
> > Is there a reason why this should be Cocoa specific?
> 
> Because UserMediaCaptureManagerProxy.h is Cocoa specific:
> https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/UIProcess/Cocoa/
> UserMediaCaptureManagerProxy.h
> 
> Thus compilation breaks:
> 
> ../../Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:77:10: fatal
> error: 'UserMediaCaptureManagerProxy.h' file not found
> #include "UserMediaCaptureManagerProxy.h"
> 
> >> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:367
> >> +#if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM)
> > 
> > Ditto
> 
> Because it is part of UserMediaCaptureManagerProxy

Sorry my bad the platforms I enabled on don't enable media stream. I talked some with Eric Carlson and I opened https://bugs.webkit.org/show_bug.cgi?id=211085 for this since UserMediaCaptureManagerProxy doesn't seem to be Cocoa specific at all.

If you want to take that bug before landing that's fine if not can you just add FIXMEs referencing that bug around those changes?

[Don Olmstead]

Comment on attachment 397179 [details]
Patch

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

r=me

Your call on how you want to address the UserMediaCaptureManagerProxy.

> Source/WebKit/ChangeLog:38
> +        New function that raise noImplemented()

Typo.

New function that raises notImplemented.

[Víctor M. Jáquez L.]

Created attachment 397849 [details]
Patch

[Víctor M. Jáquez L.]

(In reply to Don Olmstead from comment #25)
> Comment on attachment 397179 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397179&action=review
> 
> r=me
> 
> Your call on how you want to address the UserMediaCaptureManagerProxy.

I added a FIXME :)

> 
> > Source/WebKit/ChangeLog:38
> > +        New function that raise noImplemented()
> 
> Typo.
> 
> New function that raises notImplemented.

Fixed

[Xabier Rodríguez Calvar]

Comment on attachment 397849 [details]
Patch

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

LGTM but I would wait for the review of something with more knowledge of interprocess comms.

> Source/WebKit/ChangeLog:15
> +        (WebKit::GPUConnectionToWebProcess::dispatchMessage): guard

Sentences begin with a capital. Please check this in the rest of the changelo

> Source/cmake/OptionsGTK.cmake:-191
> -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS_PAINTING_API PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES})
> -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS_TYPED_OM PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES})
> -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS_CONIC_GRADIENTS PRIVATE ON)
> -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_RESOURCE_LOAD_STATISTICS PRIVATE ON)

Nit, I would leave these changes out of this patch, but won't be strongly against it.

[Don Olmstead]

Comment on attachment 397849 [details]
Patch

r=me

In the future if you get a r+ with nits then you can just replace the NOBODY (OOPS!) with the reviewers name and then ask for cq+.

[EWS]

Committed r260899: <https://trac.webkit.org/changeset/260899>

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

[Víctor M. Jáquez L.]

(In reply to Don Olmstead from comment #29)
> Comment on attachment 397849 [details]
> Patch
> 
> r=me
> 
> In the future if you get a r+ with nits then you can just replace the NOBODY
> (OOPS!) with the reviewers name and then ask for cq+.

Sorry. I should ask about it before uploading.

Thanks!

[Don Olmstead]

(In reply to Víctor M. Jáquez L. from comment #31)
> (In reply to Don Olmstead from comment #29)
> > Comment on attachment 397849 [details]
> > Patch
> > 
> > r=me
> > 
> > In the future if you get a r+ with nits then you can just replace the NOBODY
> > (OOPS!) with the reviewers name and then ask for cq+.
> 
> Sorry. I should ask about it before uploading.
> 
> Thanks!

No worries I had no idea that you could do that when I started committing so figured I'd mention it to you.