Enable the compilation of GPUProcess infrastructure in GTK port
Created attachment 393044 [details] WIP Patch
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 :)
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
Created attachment 393170 [details] WIP Patch
See https://trac.webkit.org/changeset/258253
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?
(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 :)
(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.
Created attachment 394477 [details] Patch
Created attachment 394479 [details] Patch
Created attachment 394483 [details] Patch
Created attachment 395701 [details] Patch
The patch is ready for review :)
I spoke too fast
Created attachment 395704 [details] Patch
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
Created attachment 395785 [details] Patch
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.
Created attachment 395809 [details] Patch
Created attachment 397179 [details] Patch
Ping :) Any comment on this patch?
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
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
(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?
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.
Created attachment 397849 [details] Patch
(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
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.
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+.
Committed r260899: <https://trac.webkit.org/changeset/260899> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397849 [details].
(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!
(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.