Bug 209332 – [GStreamer] Lazy initialization support

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

As Zan pointed out, gst_init() can be useless in some contexts and thus have a negative impact on the WebProcess initialization time. Perhaps we should just call gst_init() before creating the first pipeline of the process.

[Charlie Turner]

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.

[Charlie Turner]

Created attachment 395258 [details]
Patch

[Charlie Turner]

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.

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

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

[Charlie Turner]

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

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

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.

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

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

[Philippe Normand]

Created attachment 414164 [details]
Patch

[Philippe Normand]

Comment on attachment 414164 [details]
Patch

This breaks several API tests, needs a few more changes.

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

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

[Philippe Normand]

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

[Philippe Normand]

Created attachment 414215 [details]
Patch

[Carlos Garcia Campos]

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?

[Xabier Rodríguez Calvar]

The GStreamer part lgtm

[Carlos Garcia Campos]

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.

[Philippe Normand]

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.

[Philippe Normand]

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

[Carlos Garcia Campos]

Why it can't be mutable for us?

[Philippe Normand]

(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

[Philippe Normand]

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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?

[Philippe Normand]

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

[Philippe Normand]

Created attachment 416930 [details]
Patch

[Carlos Garcia Campos]

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);

[Carlos Garcia Campos]

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

[Philippe Normand]

(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

[Philippe Normand]

Created attachment 417364 [details]
Patch

[Philippe Normand]

Created attachment 417369 [details]
Patch

[Carlos Garcia Campos]

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

[Philippe Normand]

Created attachment 417373 [details]
Patch

[Carlos Garcia Campos]

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

[Philippe Normand]

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

[Radar WebKit Bug Importer]

<rdar://problem/73033900>

[Philippe Normand]

https://github.com/WebKit/WebKit/commit/b0a620ef5d220d30a5c24b09ca873219be58f478