GNOME Bugzilla – Bug 779765
GstUri: add function to parse uri and set properties to object
Last modified: 2018-11-03 12:40:03 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.
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?
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.
(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.
(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.
> 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);
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?
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.
Created attachment 347552 [details] [review] gsturi: add uri parser to set props to obj
Created attachment 347553 [details] [review] gsturi: add uri parser to set props to obj Sorry, attached the wrong file just now.
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.
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.
(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.
(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()
-- 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.