Summary: | [WPE] GStreamer/WPE Source element | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||||
Component: | WPE WebKit | Assignee: | Philippe Normand <pnormand> | ||||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||||
Severity: | Normal | CC: | bugs-noreply, ews-watchlist, mcatanzaro, rniwa, tsaunier, zan | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Philippe Normand
2018-12-04 03:44:41 PST
Created attachment 356483 [details]
Patch
Created attachment 356484 [details]
Patch
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 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! Created attachment 356487 [details]
Patch
(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 :-) (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 Ah, forgot to update the get_uri implementation :) Created attachment 356491 [details]
Patch
> 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 :-) Created attachment 356492 [details]
Patch
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. (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 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 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
(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 (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 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? The goal is now to upstream this in GStreamer. Interested people can follow https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/merge_requests/75 |