Bug 209332

Summary: [GStreamer] Lazy initialization support
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: Charlie Turner <cturner>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, calvaris, cgarcia, cturner, eric.carlson, ews-watchlist, glenn, gustavo, gyuyoung.kim, hta, jer.noble, menard, mkwst, philipj, ryuan.choi, sergio, tommyw, vjaquez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=220542
Bug Depends on: 220407    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
WIP: webrtc/mediacapture/minibrowser changes
none
WIP: media capture / webrtc / gstregistry initialization
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch cgarcia: review+

Description Philippe Normand 2020-03-20 03:56:13 PDT
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.
Comment 1 Charlie Turner 2020-04-02 04:01:35 PDT
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.
Comment 2 Charlie Turner 2020-04-02 04:03:07 PDT
Created attachment 395258 [details]
Patch
Comment 3 Charlie Turner 2020-04-02 04:04:56 PDT
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.
Comment 4 Víctor M. Jáquez L. 2020-04-02 04:50:01 PDT
I wonder how it has to be managed if we move all the gstreamer related bits into the GPUProcess (if it's possible).
Comment 5 Charlie Turner 2020-04-02 05:30:02 PDT
(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?
Comment 6 Víctor M. Jáquez L. 2020-05-13 09:11:06 PDT
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.
Comment 7 Víctor M. Jáquez L. 2020-05-27 09:14:26 PDT
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).
Comment 8 Philippe Normand 2020-11-15 07:05:42 PST
Created attachment 414164 [details]
Patch
Comment 9 Philippe Normand 2020-11-15 09:07:37 PST
Comment on attachment 414164 [details]
Patch

This breaks several API tests, needs a few more changes.
Comment 10 Víctor M. Jáquez L. 2020-11-16 00:20:08 PST
(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!
Comment 11 Philippe Normand 2020-11-16 03:21:11 PST
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...
Comment 12 Philippe Normand 2020-11-16 03:58:30 PST
Created attachment 414215 [details]
Patch
Comment 13 Carlos Garcia Campos 2020-11-17 02:50:47 PST
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?
Comment 14 Xabier Rodríguez Calvar 2020-11-17 04:19:08 PST
The GStreamer part lgtm
Comment 15 Carlos Garcia Campos 2020-11-17 06:15:19 PST
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 16 Philippe Normand 2020-11-18 01:25:58 PST
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 17 Philippe Normand 2020-11-18 08:22:28 PST
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....() :)
Comment 18 Carlos Garcia Campos 2020-11-19 00:38:02 PST
Why it can't be mutable for us?
Comment 19 Philippe Normand 2020-11-19 04:33:36 PST
(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
Comment 20 Philippe Normand 2020-11-20 02:20:48 PST
I can move the new WebKit files in a gstreamer folder instead of glib, if that's better.
Comment 21 Carlos Garcia Campos 2020-12-09 06:05:32 PST
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 22 Carlos Garcia Campos 2020-12-09 06:11:20 PST
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?
Comment 23 Philippe Normand 2021-01-04 02:08:33 PST
(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?
Comment 24 Philippe Normand 2021-01-04 02:49:41 PST
Created attachment 416930 [details]
Patch
Comment 25 Carlos Garcia Campos 2021-01-07 04:47:00 PST
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);
Comment 26 Carlos Garcia Campos 2021-01-07 05:00:43 PST
(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 {
Comment 27 Philippe Normand 2021-01-11 02:09:10 PST
(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
Comment 28 Philippe Normand 2021-01-11 02:58:01 PST
Created attachment 417364 [details]
Patch
Comment 29 Philippe Normand 2021-01-11 03:26:52 PST
Created attachment 417369 [details]
Patch
Comment 30 Carlos Garcia Campos 2021-01-11 03:54:11 PST
(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.
Comment 31 Philippe Normand 2021-01-11 06:10:20 PST
Created attachment 417373 [details]
Patch
Comment 32 Carlos Garcia Campos 2021-01-12 02:32:21 PST
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
Comment 33 Philippe Normand 2021-01-12 02:55:45 PST
Committed r271396: <https://trac.webkit.org/changeset/271396>
Comment 34 Radar WebKit Bug Importer 2021-01-12 02:56:14 PST
<rdar://problem/73033900>