After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 779765 - GstUri: add function to parse uri and set properties to object
GstUri: add function to parse uri and set properties to object
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
1.10.4
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 738608 746762 746818 748042 779850 779855 780033 781441 781442
 
 
Reported: 2017-03-08 19:44 UTC by Marc Leeman
Modified: 2018-11-03 12:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
pick up properties and set to object (5.52 KB, patch)
2017-03-08 19:44 UTC, Marc Leeman
none Details | Review
gsturi: add uri parser to set props to obj (5.52 KB, patch)
2017-03-09 12:54 UTC, Marc Leeman
none Details | Review
gsturi: add uri parser to set props to obj (5.59 KB, patch)
2017-03-09 12:55 UTC, Marc Leeman
none Details | Review
gsturi: add uri parser to set props to obj (5.61 KB, patch)
2017-03-09 13:00 UTC, Marc Leeman
needs-work Details | Review

Description Marc Leeman 2017-03-08 19:44:08 UTC
Created attachment 347499 [details] [review]
pick up properties and set to object

As some core developers sometimes claim, we (Barco) do 'crazy' stuff with URIs.

This patch adds the functionality to pick up the key value pairs from a URI and set them to an object.

This allows to encode element properties into an URI and have an easy way to set it onto self.

e.g. 

rtsp://.......?debug=true&protocols=2

This combines with a number of patches we've submitted in the past; I will refresh them in the following days based on this submission.
Comment 1 Matthew Waters (ystreet00) 2017-03-09 05:31:06 UTC
Doesn't this conflict with URI's that actually have parameters that need to be passed to the server in order to connect correctly.  What happens if one of those parameters is also present as a property on the element?
Comment 2 Sebastian Dröge (slomo) 2017-03-09 06:10:01 UTC
Review of attachment 347499 [details] [review]:

::: gst/gsturi.c
@@ +2814,3 @@
+ */
+static gboolean
+gst_uri_query_to_boolean (gchar * value)

const I guess, safes us the cast below. Also this does not seem to be required as a public function (and the documentation uses an old name of the function)

@@ +2840,3 @@
+ */
+void
+gst_uri_parse_uri (const GstUri *uri, GObject * obj)

This needs a more descriptive name, and especially needs to be distinguished somehow from normal URI functions. This has nothing to do with URIs per se.

Maybe gst_object_set_properties_from_uri_query_parameters() or something.


Should also get a "filter" that gives properties that should be ignored, and might make sense to have a "value transform" function per property that converts the query string to the proper value.
Comment 3 Marc Leeman 2017-03-09 08:12:28 UTC
(In reply to Matthew Waters (ystreet00) from comment #1)
> Doesn't this conflict with URI's that actually have parameters that need to
> be passed to the server in order to connect correctly.  What happens if one
> of those parameters is also present as a property on the element?

I do not quite understand what you mean: is your point that one might pass different values in the uri than you would pass while setting the object?

Of course, that can cause a conflict; but in essence the last one wins; just as if one would set properties twice on the same object.

This function would typically be called when you in the set_property while handling the PROP_URI.
Comment 4 Matthew Waters (ystreet00) 2017-03-09 08:55:32 UTC
(In reply to Marc Leeman from comment #3)
> (In reply to Matthew Waters (ystreet00) from comment #1)
> > Doesn't this conflict with URI's that actually have parameters that need to
> > be passed to the server in order to connect correctly.  What happens if one
> > of those parameters is also present as a property on the element?
> 
> I do not quite understand what you mean: is your point that one might pass
> different values in the uri than you would pass while setting the object?

Yes.

> Of course, that can cause a conflict; but in essence the last one wins; just
> as if one would set properties twice on the same object.

That presents a usability problem and would be a change in behaviour if added to all elements that support uris.

With an object that has a debug boolean property.

g_object_set (o, "uri", "prot://location?debug=1", "debug", FALSE, NULL);
behaves differently to
g_object_set (o, "debug", FALSE, "uri", "prot://location?debug=1", NULL);
when they really shouldn't.
Comment 5 Marc Leeman 2017-03-09 09:01:20 UTC

> That presents a usability problem and would be a change in behaviour if
> added to all elements that support uris.
> 
> With an object that has a debug boolean property.
> 
> g_object_set (o, "uri", "prot://location?debug=1", "debug", FALSE, NULL);
> behaves differently to
> g_object_set (o, "debug", FALSE, "uri", "prot://location?debug=1", NULL);
> when they really shouldn't.

Well, it is the same as saying that:

g_object_set(o, "prop", TRUE, NULL);
g_object_set(o, "prop", FALSE, NULL);

is behaving differently as

g_object_set(o, "prop", FALSE, NULL);
g_object_set(o, "prop", TRUE, NULL);
Comment 6 Matthew Waters (ystreet00) 2017-03-09 09:08:34 UTC
That's not the problem, the problem is the behaviour change when adding this to already existing elements.  What's the plan for managing this change in behaviour?
Comment 7 Marc Leeman 2017-03-09 10:28:01 UTC
I would need to look it up; but as far as I know, there are already elements (e.g. filesrc) that already do their custom parsing and have a uri handler and use a property (location) for the same data.

This patch generalises this parsing into a common location for more easy re-use.
Comment 8 Marc Leeman 2017-03-09 12:54:20 UTC
Created attachment 347552 [details] [review]
gsturi: add uri parser to set props to obj
Comment 9 Marc Leeman 2017-03-09 12:55:40 UTC
Created attachment 347553 [details] [review]
gsturi: add uri parser to set props to obj

Sorry, attached the wrong file just now.
Comment 10 Marc Leeman 2017-03-09 13:00:52 UTC
Created attachment 347554 [details] [review]
gsturi: add uri parser to set props to obj

Forgot to adjust the header file for the rename
Reordered the parameters: if the function is named gst_object, use the object as first param, uri second.
Comment 11 Nicolas Dufresne (ndufresne) 2017-03-10 21:40:24 UTC
Review of attachment 347554 [details] [review]:

By default, g_object_set() will assert on bad values, bad ranges. This is not acceptable from an URI. You need to pre-validate all the values, and their ranges, so it can warn and fail cleanly.

::: gst/gsturi.h
@@ +251,3 @@
 const gchar * gst_uri_get_fragment     (const GstUri * uri);
 gboolean gst_uri_set_fragment          (GstUri * uri, const gchar * fragment);
+void gst_object_set_properties_from_uri_query_parameters (GObject *obj, const GstUri *uri);

This is just way too long. One thing I notice, while reviewing, is that this is way too generic. We need a way to filter certain properties to be binded.
Comment 12 Sebastian Dröge (slomo) 2017-03-11 09:28:51 UTC
(In reply to Nicolas Dufresne (stormer) from comment #11)
> Review of attachment 347554 [details] [review] [review]:
> 
> By default, g_object_set() will assert on bad values, bad ranges. This is
> not acceptable from an URI. You need to pre-validate all the values, and
> their ranges, so it can warn and fail cleanly.

Good catch, missed that above

> ::: gst/gsturi.h
> 
> This is just way too long.

Do you have any suggestions? I suggested this new name in comment 2 because the previous one was just too generic. This function is doing something very specific, unrelated to general URI semantics but adding a specific meaning on top.

> One thing I notice, while reviewing, is that this
> is way too generic. We need a way to filter certain properties to be binded.

As you're the third to mention that, definitely ;) How about adding a parameter of type "const gchar *filter[]" that is NULL terminated and contains property names for this?

Also IMHO we also need a way for transforming the query parameters to their respective types, i.e. a set of function pointers that do any custom conversion (+ validation) when the default one does not apply.
Comment 13 Niels De Graef 2018-03-19 10:46:24 UTC
(In reply to Sebastian Dröge (slomo) from comment #12)
> (In reply to Nicolas Dufresne (stormer) from comment #11)
> > Review of attachment 347554 [details] [review] [review] [review]:
> > One thing I notice, while reviewing, is that this
> > is way too generic. We need a way to filter certain properties to be binded.
> 
> As you're the third to mention that, definitely ;) How about adding a
> parameter of type "const gchar *filter[]" that is NULL terminated and
> contains property names for this?
> 
> Also IMHO we also need a way for transforming the query parameters to their
> respective types, i.e. a set of function pointers that do any custom
> conversion (+ validation) when the default one does not apply.
Random suggestion: maybe we can save a flag inside a GParamSpec to save that it can be set using URI query parameters?

Say that we use for example a wrapper around g_object_class_install_property() (e.g. gst_object_class_install_uri_compatible_property). It basically calls g_param_spec_set_qdata() to set the flag (and possibly a default transform function). Then it is both clear from the class_init() which properties are supported for URI handling, and it's easily checkable from gst_parse_uri (or whatever it will be called eventually).


(In reply to Sebastian Dröge (slomo) from comment #12)
> (In reply to Nicolas Dufresne (stormer) from comment #11)
> > Review of attachment 347554 [details] [review] [review] [review]:
> > 
> > By default, g_object_set() will assert on bad values, bad ranges. This is
> > not acceptable from an URI. You need to pre-validate all the values, and
> > their ranges, so it can warn and fail cleanly.
> 
> Good catch, missed that above
> 
> > ::: gst/gsturi.h
> > 
> > This is just way too long.
> 
> Do you have any suggestions? I suggested this new name in comment 2 because
> the previous one was just too generic. This function is doing something very
> specific, unrelated to general URI semantics but adding a specific meaning
> on top.
Maybe drop the _query_parameters suffix? i.e. gst_object_set_properties_from_uri()
Comment 14 GStreamer system administrator 2018-11-03 12:40:03 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/226.