ah yeah, the g_hash_table_destroy needs to be g_hash_table_unref

Any intention updating the patch (to apply on git master and fix the destroy/unref error) ?

Created attachment 347626 [details] [review]
v4l2: add GstUri handler to v4l2

(In reply to Nicolas Dufresne (stormer) from comment #2)
> Any intention updating the patch (to apply on git master and fix the
> destroy/unref error) ?

I am submitting my patches accumulated over time; it has been too long :-(

Depends on
https://bugzilla.gnome.org/show_bug.cgi?id=779765

Review of attachment 347626 [details] [review]:

::: sys/v4l2/gstv4l2object.c
@@ +588,3 @@
+          gst_uri_unref (v4l2object->uri);
+
+        v4l2object->uri = gst_uri_from_string (g_value_get_string (value));

This may fail.

@@ +590,3 @@
+        v4l2object->uri = gst_uri_from_string (g_value_get_string (value));
+
+        gst_object_set_properties_from_uri_query_parameters (G_OBJECT (v4l2object->element), v4l2object->uri);

It's nice to have a generic interface here, though there is some stuff I really don't like. I'll bring that to the respective bug. In this specific case, we MUST filter certain properties (name, parent, device, device-fd, io-mode, are the one I would deliberatly prevent from accessing from the URI). We should also find a way to validate the values in a way that it won't assert.

@@ +591,3 @@
+
+        gst_object_set_properties_from_uri_query_parameters (G_OBJECT (v4l2object->element), v4l2object->uri);
+        path = gst_uri_get_path(v4l2object->uri);

Coding style, need space before (.

@@ +710,3 @@
+        gchar *string = gst_uri_to_string (v4l2object->uri);
+
+        g_value_set_string (value, string);

g_value_take_string() please, you can probably make it a one liner.

::: sys/v4l2/gstv4l2sink.c
@@ +102,3 @@
+        gst_v4l2sink_video_orientation_interface_init);
+    G_IMPLEMENT_INTERFACE (GST_TYPE_URI_HANDLER,
+        gst_v4l2sink_uri_handler_init));

This is a non-sense, URI handler are strictly for sources.

btw, ignore the part about the sink, I learned later that it's usable to find and prepare sinks too using the gst-uri API. Sorry for the noise.

 
> @@ +710,3 @@
> +        gchar *string = gst_uri_to_string (v4l2object->uri);
> +
> +        g_value_set_string (value, string);
> 
> g_value_take_string() please, you can probably make it a one liner.

What is the rationale of preferring the g_value_take_string over g_value_set_string; is it the extra copy where take string takes the ownership and set string not?

(In reply to Marc Leeman from comment #8)
> What is the rationale of preferring the g_value_take_string over
> g_value_set_string; is it the extra copy where take string takes the
> ownership and set string not?

Yes, using g_value_take_string() saves an allocation and a copy.

Created attachment 374101 [details] [review]
v4l2: add GstUri handler to v4l2

Review of attachment 374101 [details] [review]:

::: sys/v4l2/gstv4l2object.c
@@ +542,3 @@
+    v4l2object->uri = gst_uri_from_string (DEFAULT_PROP_URI);
+  else
+    v4l2object->uri = DEFAULT_PROP_URI;

That is not great, DEFAULT_PROP_URI is both a string and a GstUri. You also add a runtime null check, where NULL is hard coded. If you think NULL is a the default, drop the default macro, it's not useful.

@@ +573,3 @@
+  if (v4l2object->uri) {
+    gst_uri_unref (v4l2object->uri);
+    v4l2object->uri = NULL;

No need, we g_free the v4l2object next.

@@ +628,3 @@
+        gst_object_set_properties_from_uri_query_parameters (G_OBJECT (v4l2object->element), v4l2object->uri);
+        path = gst_uri_get_path(v4l2object->uri);
+        if (0 != *(path))

Doc says the return of get_path is nullable, you are missing some checks here.

::: sys/v4l2/gstv4l2src.c
@@ +1047,1 @@
+  g_object_set (G_OBJECT (v4l2src->v4l2object->element), "uri", uri, NULL);

v4l2object->element should be v4l2src, why do you use this indirection ?

-- 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/gst-plugins-good/issues/176.