Bug 192359 - [WPE] GStreamer/WPE Source element
Summary: [WPE] GStreamer/WPE Source element
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-04 03:44 PST by Philippe Normand
Modified: 2018-12-05 06:50 PST (History)
6 users (show)

See Also:


Attachments
Patch (52.00 KB, patch)
2018-12-04 03:56 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (52.00 KB, patch)
2018-12-04 04:03 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (51.38 KB, patch)
2018-12-04 05:28 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (51.02 KB, patch)
2018-12-04 06:22 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (51.08 KB, patch)
2018-12-04 06:31 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (1.11 MB, application/zip)
2018-12-04 08:30 PST, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2018-12-04 03:44:41 PST
The wpe element is used to produce a video texture representing a web page rendered off-screen by WPE.
Comment 1 Philippe Normand 2018-12-04 03:56:13 PST
Created attachment 356483 [details]
Patch
Comment 2 Philippe Normand 2018-12-04 04:03:35 PST
Created attachment 356484 [details]
Patch
Comment 3 Thibault Saunier 2018-12-04 04:33:31 PST
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 <zdobersek@igalia.com>

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.
Comment 4 Philippe Normand 2018-12-04 05:27:25 PST
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!
Comment 5 Philippe Normand 2018-12-04 05:28:03 PST
Created attachment 356487 [details]
Patch
Comment 6 Thibault Saunier 2018-12-04 05:43:30 PST
(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 :-)
Comment 7 Philippe Normand 2018-12-04 06:12:08 PST
(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
Comment 8 Philippe Normand 2018-12-04 06:21:02 PST
Ah, forgot to update the get_uri implementation :)
Comment 9 Philippe Normand 2018-12-04 06:22:30 PST
Created attachment 356491 [details]
Patch
Comment 10 Thibault Saunier 2018-12-04 06:29:33 PST
> 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 :-)
Comment 11 Philippe Normand 2018-12-04 06:31:30 PST
Created attachment 356492 [details]
Patch
Comment 12 Michael Catanzaro 2018-12-04 08:12:43 PST
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.
Comment 13 Philippe Normand 2018-12-04 08:21:50 PST
(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 :(
Comment 14 EWS Watchlist 2018-12-04 08:30:05 PST
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
Comment 15 EWS Watchlist 2018-12-04 08:30:06 PST
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
Comment 16 Philippe Normand 2018-12-04 10:10:01 PST
(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 <zdobersek@igalia.com>
> 
> Bugzilla is screwing the UTF8 or the file is not using the right encoding?
> 

The former, for the past 10 years
Comment 17 Michael Catanzaro 2018-12-04 11:21:33 PST
(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.
Comment 18 Michael Catanzaro 2018-12-04 11:29:22 PST
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?
Comment 19 Philippe Normand 2018-12-05 06:50:12 PST
The goal is now to upstream this in GStreamer. Interested people can follow https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/merge_requests/75