Bug 209332

It sounds like a good idea to only initialise GStreamer if we're going to play media. TL;DR, it's fine to do this but it might break WebRTC. Currently, Gst can be initialised by either 1/ WebProcess initialization - so for every web view, there will be a gst_init (and potentially a registry rebuild...) 2/ MediaPlayerPrivateGStreamer::isAvailable - this is called when media engines are registered 1/ seems like the wrong thing to do, and 2/ is sensible. 2/ happens when you call an API like webkit_web_view_can_show_mime_type or load a page with media elements. Spinning up for the MIME APIs could be avoided if we maintain a static whitelist like other ports do, but we elect to dynamically answer this question, I guess to support use-cases like codec auto-install (which is busted as well unfortunately). I have WIP patch that I'll attach, it only spins Gst up if we really need it. After testing I realised that WebRTC tests for the GTK port are busted, as well as basic WebRTC test pages. Because I don't have time to fix WebRTC I will leave it here in case it's useful after WebRTC gets some love in the future.

Created attachment 395258 [details] Patch

The patch is WIP, there's a hack to boot Gst in the web process unconditionally for testing, since the Gst "mock" capturers actually do start Gst, the startup logic for WebRTC could be adjusted somehow to do this more on-demand, but due to the issues I saw in this area I had to move on.

I wonder how it has to be managed if we move all the gstreamer related bits into the GPUProcess (if it's possible).

(In reply to Víctor M. Jáquez L. from comment #4) > I wonder how it has to be managed if we move all the gstreamer related bits > into the GPUProcess (if it's possible). I haven't looked into the GPU process yet, but maybe that would fix this issue by side-effect, since initialisation from the web process POV would be async I presume?

Created attachment 399268 [details] WIP: webrtc/mediacapture/minibrowser changes This patch is a continuation of Charles' one. I decided to work on this in order to understand how GStreamer is currently initializated, and later move altogether to GPUProcess. My initial assumption was that all the GStreamer bits were contained in the WebProcess, but it is not the case of the media captures sources, which are called in the UIProcess (after the permission dialog). Also, the test application, the minibrowser, initialize GStreamer in the UIProcess. Hence, right now, there's no thread affinity with GStreamer, it is initialized and used from different processes. This patch removes the GStreamer initialization from test apps and ignores the unknown parameters so they can be processed by the WebProcess' auxiliary. Also calls the gstreamer initialization, which is idempotent, in the different entry points of media capture sources and webrtc codecs. The affinity is not fixed, but now GStreamer is lazy initialized, and apparently reduces the launch time of the minibrowsers.

Created attachment 400339 [details] WIP: media capture / webrtc / gstregistry initialization Just saw that https://trac.webkit.org/changeset/261922 also added a new gstreamer initialization point, which is not, afaik, contemplated for gpuprocess (I'd need to confirm it).

Created attachment 414164 [details] Patch

Comment on attachment 414164 [details] Patch This breaks several API tests, needs a few more changes.

(In reply to Philippe Normand from comment #9) > Comment on attachment 414164 [details] > Patch > > This breaks several API tests, needs a few more changes. Wow! What I've striving to do for several weeks you did it in a weekend :') Thanks!

The API test crashes on the RELEASE_ASSERT I added, triggered by the MIMETypeRegistry canShowMIMEType stuff, from the UIProcess, due mostly to the corresponding public WebView API :( I've tried to restrict this to the WebProcess, through a new IPC message but that doesn't work yet, because the WebProcess might not be started yet when this API is being called... For now I don't see how to nicely fix this without removing the ASSERT...

Created attachment 414215 [details] Patch

I think it's a good idea to restrict gst to the web process (we just need to make an exception for the registry scanner), but I'm not sure it's worth the lazy initialization in the web process. The MIME type registry is going to always initialize gst in the end when the first load happens (because of WebPage::canShowResponse()) in WebFrameLoaderClient::dispatchDecidePolicyForResponse(). Or do we want to delay the initialization until the first load? and not until media is going to be played?

The GStreamer part lgtm

Comment on attachment 414215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414215&action=review > Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:462 > +#if !USE(GLIB) why GLIB and not GSTREAMER? > Source/WebKit/UIProcess/glib/UserMediaPermissionRequestManagerProxyGLib.cpp:71 > + m_page.process().connection()->sendWithAsyncReply(Messages::UserMediaCaptureManager::ValidateUserMediaRequestConstraints(m_currentUserMediaRequest->userRequest(), deviceIDHashSalt), [validHandler, invalidHandler](Optional<String> invalidConstraint, Vector<WebCore::CaptureDevice> audioDevices, Vector<WebCore::CaptureDevice> videoDevices, Optional<String> deviceIdentifierHashSalt) { > + if (invalidConstraint.hasValue()) > + invalidHandler(*invalidConstraint); > + else > + validHandler(WTFMove(audioDevices), WTFMove(videoDevices), WTFMove(*deviceIdentifierHashSalt)); > + }); Instead of duplicating the whole function just for this change, maybe it's better to add a platformValidateUserMediaRequestConstraints() with this implementation for GStreamer based ports.

Comment on attachment 414215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414215&action=review >> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:462 >> +#if !USE(GLIB) > > why GLIB and not GSTREAMER? Because the Proxy impl I added is in the glib/ folder, for consistency. >> Source/WebKit/UIProcess/glib/UserMediaPermissionRequestManagerProxyGLib.cpp:71 >> + }); > > Instead of duplicating the whole function just for this change, maybe it's better to add a platformValidateUserMediaRequestConstraints() with this implementation for GStreamer based ports. There's one difference though, the invalidHandler needs to be mutable in cocoa ports and can't be for us.

Comment on attachment 414215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414215&action=review >>> Source/WebKit/UIProcess/glib/UserMediaPermissionRequestManagerProxyGLib.cpp:71 >>> + }); >> >> Instead of duplicating the whole function just for this change, maybe it's better to add a platformValidateUserMediaRequestConstraints() with this implementation for GStreamer based ports. > > There's one difference though, the invalidHandler needs to be mutable in cocoa ports and can't be for us. Same for the validHandler. So I'm not sure it's really worth to add a platform....() :)

Why it can't be mutable for us?

(In reply to Carlos Garcia Campos from comment #18) > Why it can't be mutable for us? Sorry, only the validHandler needs to be immutable for us. If I make it mutable here's the error: ../../Source/WebKit/UIProcess/glib/UserMediaPermissionRequestManagerProxyGLib.cpp:70:17: error: no matching function for call to object of type 'const (lambda at ../../Source/WebKit/UIProcess/glib/UserMediaPermissionRequestManagerProxyGLib.cpp:54:29)' validHandler(std::move<WTF::CheckMoveParameter>(audioDevices), std::move<WTF::CheckMoveParameter>(videoDevices), std::move<WTF::CheckMoveParameter>(*deviceIdentifierHashSalt)); ^~~~~~~~~~~~ ../../Source/WebKit/UIProcess/glib/UserMediaPermissionRequestManagerProxyGLib.cpp:54:29: note: candidate function not viable: 'this' argument has type 'const (lambda at ../../Source/WebKit/UIProcess/glib/UserMediaPermissionRequestManagerProxyGLib.cpp:54:29)', but method is not marked const auto validHandler = [this, request](Vector<CaptureDevice>&& audioDevices, Vector<CaptureDevice>&& videoDevices, String&& deviceIdentifierHashSalt) mutable { ^ 1 error generated. Compiler killed by signal 1

I can move the new WebKit files in a gstreamer folder instead of glib, if that's better.

Comment on attachment 414215 [details] Patch I would split this patch a bit. I would fist land a patch to allow creating a temp registry from the UI process to fill the media type in MIMEType registry. That would require the isInWebProcess() impl. Then we could bring back the assert in this patch to ensure only the registry is used from other processes.

Comment on attachment 414215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414215&action=review > Source/WebCore/platform/glib/RuntimeApplicationChecksGLib.cpp:35 > + return g_str_has_suffix(getCurrentExecutableName().data(), "WebProcess"); This could be cached in a static var, since it can't change. I find it a bit weak, I know it's unlikely that there's an application called FooWebProcess, but I think it would be better to at least check the actual name, even if we have to add an ifdef for GTK and WPE. There's still the possibility of having another program called WebKitWebProcess or WPEWebProcess at a different path, but it's even less unlikely. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:217 > +static Optional<Vector<String>> s_UIProcessCommandLineOptions { WTF::nullopt }; The initialization is not needed. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:218 > +void addGStreamerOptionsFromUIProcess(Vector<String>&& options) I would call this set instad of add > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:254 > - Vector<String> parameters = options.valueOr(extractGStreamerOptionsFromCommandLine()); > + Vector<String> parameters = s_UIProcessCommandLineOptions.valueOr(extractGStreamerOptionsFromCommandLine()); The command line options won't be used anymore, so maybe we can set them back to nullopt to free the Vector?

(In reply to Carlos Garcia Campos from comment #21) > Comment on attachment 414215 [details] > Patch > > I would split this patch a bit. I would fist land a patch to allow creating > a temp registry from the UI process to fill the media type in MIMEType > registry. That would require the isInWebProcess() impl. Then we could bring > back the assert in this patch to ensure only the registry is used from other > processes. A temp registry?

Created attachment 416930 [details] Patch

Comment on attachment 416930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416930&action=review After landing the patch attached to bug #220407, I think we could bring back the assert to ensureGStreamerInitialized(). > Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:177 > + ensureGStreamerInitialized(); After the patch attached to bug #220407, this would be: if (isInWebProcess()) ensureGStreamerInitialized(); else gst_init(nullptr, nullptr);

(In reply to Philippe Normand from comment #19) > (In reply to Carlos Garcia Campos from comment #18) > > Why it can't be mutable for us? > > Sorry, only the validHandler needs to be immutable for us. If I make it > mutable here's the error: > > ../../Source/WebKit/UIProcess/glib/ > UserMediaPermissionRequestManagerProxyGLib.cpp:70:17: error: no matching > function for call to object of type 'const (lambda at > ../../Source/WebKit/UIProcess/glib/ > UserMediaPermissionRequestManagerProxyGLib.cpp:54:29)' > > validHandler(std::move<WTF::CheckMoveParameter>(audioDevices), > std::move<WTF::CheckMoveParameter>(videoDevices), > std::move<WTF::CheckMoveParameter>(*deviceIdentifierHashSalt)); > ^~~~~~~~~~~~ > ../../Source/WebKit/UIProcess/glib/ > UserMediaPermissionRequestManagerProxyGLib.cpp:54:29: note: candidate > function not viable: 'this' argument has type 'const (lambda at > ../../Source/WebKit/UIProcess/glib/ > UserMediaPermissionRequestManagerProxyGLib.cpp:54:29)', but method is not > marked const > auto validHandler = [this, request](Vector<CaptureDevice>&& > audioDevices, Vector<CaptureDevice>&& videoDevices, String&& > deviceIdentifierHashSalt) mutable { > ^ > 1 error generated. > Compiler killed by signal 1 I think it's because you are not moving the handlers: m_page.process().connection()->sendWithAsyncReply(Messages::UserMediaCaptureManager::ValidateUserMediaRequestConstraints(m_currentUserMediaRequest->userRequest(), deviceIDHashSalt), [validHandler = WTFMove(validHandler), invalidHandler = WTFMove(invalidHandler)](Optional<String> invalidConstraint, Vector<WebCore::CaptureDevice> audioDevices, Vector<WebCore::CaptureDevice> videoDevices, Optional<String> deviceIdentifierHashSalt) mutable {

(In reply to Carlos Garcia Campos from comment #25) > Comment on attachment 416930 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416930&action=review > > After landing the patch attached to bug #220407, I think we could bring back > the assert to ensureGStreamerInitialized(). > > > Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:177 > > + ensureGStreamerInitialized(); > > After the patch attached to bug #220407, this would be: > > if (isInWebProcess()) > ensureGStreamerInitialized(); > else > gst_init(nullptr, nullptr); But we don't want GStreamer initialized in any other process, excepted the WebProcess, this was the intent in this patch, anyway :S

Created attachment 417364 [details] Patch

Created attachment 417369 [details] Patch

(In reply to Philippe Normand from comment #27) > (In reply to Carlos Garcia Campos from comment #25) > > Comment on attachment 416930 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=416930&action=review > > > > After landing the patch attached to bug #220407, I think we could bring back > > the assert to ensureGStreamerInitialized(). > > > > > Source/WebCore/platform/graphics/gstreamer/GStreamerRegistryScanner.cpp:177 > > > + ensureGStreamerInitialized(); > > > > After the patch attached to bug #220407, this would be: > > > > if (isInWebProcess()) > > ensureGStreamerInitialized(); > > else > > gst_init(nullptr, nullptr); > > But we don't want GStreamer initialized in any other process, excepted the > WebProcess, this was the intent in this patch, anyway :S We need it in the UI process as an exception because of the mime type registry. But in that case we don't want the web process initialization, that should assert if it's not in the web process, and gst_init should be enough.

Created attachment 417373 [details] Patch

Comment on attachment 417373 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417373&action=review > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:245 > -bool initializeGStreamer(Optional<Vector<String>>&& options) > +bool ensureGStreamerInitialized() > { > static std::once_flag onceFlag; > static bool isGStreamerInitialized; > - std::call_once(onceFlag, [options = WTFMove(options)] { > + std::call_once(onceFlag, [] { > isGStreamerInitialized = false; Please add a release assert to ensure this is only called in he web process

Committed r271396: <https://trac.webkit.org/changeset/271396>

<rdar://problem/73033900>