Bug 192359 – [WPE] GStreamer/WPE Source element

• Status: RESOLVED
• Resolution: INVALID
• Component: WPE WebKit

[Philippe Normand]

The wpe element is used to produce a video texture representing a web page rendered off-screen by WPE.

[Philippe Normand]

Created attachment 356483 [details]
Patch

[Philippe Normand]

Created attachment 356484 [details]
Patch

[Thibault Saunier]

Comment on attachment 356484 [details]
Patch

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

Didn't review the WPEView specific code as I am clueless :-)

ooc, why in WebKit and not in -bad in the end?

> Source/ThirdParty/WPE/GstWPE/WPEThreadedView.cpp:2
> + * Copyright (C) <2018> Žan Doberšek <[e-mail]>

Bugzilla is screwing the UTF8 or the file is not using the right encoding?

> Source/ThirdParty/WPE/GstWPE/WPEThreadedView.cpp:25
> +#if 0

#if 0?

Oooc, why not use GStreamer debug system, it is a GStreamer src after all.

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:30
> + * gst-launch-1.0 -v wpesrc location="wpe+https://gstreamer.freedesktop.org" ! glimagesink

I would make the location accept `http://` itself as location. wpe+ should be used only for the URIHandler interface I think.

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:52
> + * - Expose an audio src pad (requires an AudioSession implementation in WebKit and a WPEBackend-fdo API for it)

It would be a bin on top of that I guess, you are implementing a subclass of GstPushSrc here, we won't be able to add a new source pad.

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:98
> +  gint64 timestamp_offset;      /* base offset */

I can't find any place where this is used.

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:140
> +    goto eos;

I do not understand the relation between the missing framerate and eos.

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:143
> +  g_return_val_if_fail (img != NULL, GST_FLOW_ERROR);

Aren't there valid case where WPE could not produce an image? If there is none, I have the impression the issue should be reported by WPE itself and then we should just relay it.

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:166
> +  GST_BUFFER_TIMESTAMP (buffer) = src->timestamp_offset + src->running_time;

Please use GST_BUFFER_PTS rather than the old compatibility variant.

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:230
> +    gst_object_unref (src->context);

g_clear_object? (In 1.16 will be gst_clear_object).

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:233
> +  delete src->view;

Reset to NULL? (Also why not a `std::unique_ptr` ?)

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:552
> +        gst_object_unref (src->other_context);

clear_object?

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:557
> +        gst_object_unref (src->display);

Ditto

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:574
> +  if (src->view) {

No need for `{` here.

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:748
> +      { "wpe+http", "wpe+https", "wpe+file", NULL };

Wouldn't it work to just do wpe and then have URIs as `wpe://https://` so we support any protocols WebKit handles?

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:795
> +      g_param_spec_boolean ("draws-background", "Draws the background",

If it is "whether to draw" shouldn't it be call "draw-background" ? (The 's' gives the impression it is informative and it is not writable).

> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:829
> +      G_SIGNAL_RUN_LAST, G_STRUCT_OFFSET (GstWpeSrcClass, configure_web_view),

You are not implementing the `config_web_view` vmethod, I think you can just remove it everywhere.

[Philippe Normand]

Comment on attachment 356484 [details]
Patch

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

>> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:30
>> + * gst-launch-1.0 -v wpesrc location="wpe+https://gstreamer.freedesktop.org" ! glimagesink
> 
> I would make the location accept `http://` itself as location. wpe+ should be used only for the URIHandler interface I think.

I'm not sure... Using http:// infers the raw data is injected into the pipeline, IMHO. This use-case is different, so it begs a different URI scheme, IMHO :)

>> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:140
>> +    goto eos;
> 
> I do not understand the relation between the missing framerate and eos.

Dunno either :) IIRC I copied this from gltestsrc.

>> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:143
>> +  g_return_val_if_fail (img != NULL, GST_FLOW_ERROR);
> 
> Aren't there valid case where WPE could not produce an image? If there is none, I have the impression the issue should be reported by WPE itself and then we should just relay it.

The threaded view blocks until a valid EGLImage has been produced.

>> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:233
>> +  delete src->view;
> 
> Reset to NULL? (Also why not a `std::unique_ptr` ?)

Not sure it's worth to use a unique_ptr?

>> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:748
>> +      { "wpe+http", "wpe+https", "wpe+file", NULL };
> 
> Wouldn't it work to just do wpe and then have URIs as `wpe://https://` so we support any protocols WebKit handles?

I don't follow :)

>> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:795
>> +      g_param_spec_boolean ("draws-background", "Draws the background",
> 
> If it is "whether to draw" shouldn't it be call "draw-background" ? (The 's' gives the impression it is informative and it is not writable).

ok!

[Philippe Normand]

Created attachment 356487 [details]
Patch

[Thibault Saunier]

(In reply to Philippe Normand from comment #4)
> Comment on attachment 356484 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356484&action=review
> 
> >> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:30
> >> + * gst-launch-1.0 -v wpesrc location="wpe+https://gstreamer.freedesktop.org" ! glimagesink
> > 
> > I would make the location accept `http://` itself as location. wpe+ should be used only for the URIHandler interface I think.
> 
> I'm not sure... Using http:// infers the raw data is injected into the
> pipeline, IMHO. This use-case is different, so it begs a different URI
> scheme, IMHO :)

The source is already selected at that point so the semantics of the property is what the source implements here. To me it is similar
to `filesrc` not requiring `file://` for its location property.

> >> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:140
> >> +    goto eos;
> > 
> > I do not understand the relation between the missing framerate and eos.
> 
> Dunno either :) IIRC I copied this from gltestsrc.

haha, could just remove I guess?

> >> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:143
> >> +  g_return_val_if_fail (img != NULL, GST_FLOW_ERROR);
> > 
> > Aren't there valid case where WPE could not produce an image? If there is none, I have the impression the issue should be reported by WPE itself and then we should just relay it.
> 
> The threaded view blocks until a valid EGLImage has been produced.

OK.

> >> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:233
> >> +  delete src->view;
> > 
> > Reset to NULL? (Also why not a `std::unique_ptr` ?)
> 
> Not sure it's worth to use a unique_ptr?

Well, would be simpler to me.

> >> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:748
> >> +      { "wpe+http", "wpe+https", "wpe+file", NULL };
> > 
> > Wouldn't it work to just do wpe and then have URIs as `wpe://https://` so we support any protocols WebKit handles?
> 
> I don't follow :)

Basically

``` c
protocols = { "wpe", NULL };
```

Then the uri would be `wpe://http://gstreamer.freedesktop.org/`. Not sure, just giving an idea here :-)

[Philippe Normand]

(In reply to Thibault Saunier from comment #6)
> (In reply to Philippe Normand from comment #4)
> > Comment on attachment 356484 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=356484&action=review
> > 
> > >> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:30
> > >> + * gst-launch-1.0 -v wpesrc location="wpe+https://gstreamer.freedesktop.org" ! glimagesink
> > > 
> > > I would make the location accept `http://` itself as location. wpe+ should be used only for the URIHandler interface I think.
> > 
> > I'm not sure... Using http:// infers the raw data is injected into the
> > pipeline, IMHO. This use-case is different, so it begs a different URI
> > scheme, IMHO :)
> 
> The source is already selected at that point so the semantics of the
> property is what the source implements here. To me it is similar
> to `filesrc` not requiring `file://` for its location property.
> 

Sounds fair but see below.

> > >> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:140
> > >> +    goto eos;
> > > 
> > > I do not understand the relation between the missing framerate and eos.
> > 
> > Dunno either :) IIRC I copied this from gltestsrc.
> 
> haha, could just remove I guess?
> 

OK!

> > >> Source/ThirdParty/WPE/GstWPE/gstwpesrc.cpp:748
> > >> +      { "wpe+http", "wpe+https", "wpe+file", NULL };
> > > 
> > > Wouldn't it work to just do wpe and then have URIs as `wpe://https://` so we support any protocols WebKit handles?
> > 
> > I don't follow :)
> 
> Basically
> 
> ``` c
> protocols = { "wpe", NULL };
> ```
> 
> Then the uri would be `wpe://http://gstreamer.freedesktop.org/`. Not sure,
> just giving an idea here :-)

I tried this suggestion and
gst-play-1.0 "wpe://https://gstreamer.freedesktop.org" --videosink=glimagesink


GStreamer-CRITICAL **: 14:10:34.020: gst_uri_handler_get_uri: assertion 'gst_uri_is_valid (ret)' failed

[Philippe Normand]

Ah, forgot to update the get_uri implementation :)

[Philippe Normand]

Created attachment 356491 [details]
Patch

[Thibault Saunier]

> I tried this suggestion and gst-play-1.0 "wpe://https://gstreamer.freedesktop.org" --videosink=glimagesink
>
> GStreamer-CRITICAL **: 14:10:34.020: gst_uri_handler_get_uri: assertion 'gst_uri_is_valid (ret)' failed

Too bad :-)

[Philippe Normand]

Created attachment 356492 [details]
Patch

[Michael Catanzaro]

Ugh can we please not add it to ThirdParty? Is there a *really* strong reason for doing this? Since it's a standalone product, you can just depend on it, right? We don't currently have a dependencies policy for WPE, so you can add whatever dependencies you want.

[Philippe Normand]

(In reply to Michael Catanzaro from comment #12)
> Ugh can we please not add it to ThirdParty? Is there a *really* strong
> reason for doing this? Since it's a standalone product, you can just depend
> on it, right? We don't currently have a dependencies policy for WPE, so you
> can add whatever dependencies you want.

WPE is currently poorly packaged in distros, which makes life difficult for 3rd party projects like gstwpe :(

[EWS Watchlist]

Comment on attachment 356492 [details]
Patch

Attachment 356492 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10264758

New failing tests:
http/tests/security/contentSecurityPolicy/same-origin-plugin-document-blocked-in-child-window-report.php
http/tests/misc/error404.pl

[EWS Watchlist]

Created attachment 356499 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Philippe Normand]

(In reply to Thibault Saunier from comment #3)
> Comment on attachment 356484 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356484&action=review
> 
> Didn't review the WPEView specific code as I am clueless :-)
> 
> ooc, why in WebKit and not in -bad in the end?

I suppose it could be an option too :)

> 
> > Source/ThirdParty/WPE/GstWPE/WPEThreadedView.cpp:2
> > + * Copyright (C) <2018> Žan Doberšek <[e-mail]>
> 
> Bugzilla is screwing the UTF8 or the file is not using the right encoding?
> 

The former, for the past 10 years

[Michael Catanzaro]

(In reply to Philippe Normand from comment #13)
> WPE is currently poorly packaged in distros, which makes life difficult for
> 3rd party projects like gstwpe :(

How does adding this to WebKit's codebase make this any simpler? I don't understand what problem this is trying to solve. Obviously if you're building WPE, you can also build GstWPE.

[Michael Catanzaro]

Comment on attachment 356492 [details]
Patch

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

> Tools/jhbuild/jhbuildrc_common.py:97
> +    __uninstalled_path = os.environ.get("WEBKIT_UNINSTALLED_PATH", "")
> +    if __uninstalled_path:
> +        __lib_path = os.path.join(__uninstalled_path, "lib")
> +        addpath('GST_PLUGIN_PATH_1_0', __lib_path)
> +        addpath('WEBKIT_EXEC_PATH', os.path.join(__uninstalled_path, "bin"))
> +        addpath('WEBKIT_INJECTED_BUNDLE_PATH', __lib_path)

What is this for?

[Philippe Normand]

The goal is now to upstream this in GStreamer. Interested people can follow https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/merge_requests/75