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 698837 - v4l2: cache CIDs until the device is opened
v4l2: cache CIDs until the device is opened
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-04-25 13:29 UTC by Michael Olbrich
Modified: 2013-05-29 18:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (2.43 KB, patch)
2013-04-25 13:29 UTC, Michael Olbrich
needs-work Details | Review
add a property for arbitrary v4l2 controls (6.25 KB, patch)
2013-05-20 16:47 UTC, Michael Olbrich
needs-work Details | Review
add a property for arbitrary v4l2 controls (6.52 KB, patch)
2013-05-29 09:38 UTC, Michael Olbrich
committed Details | Review

Description Michael Olbrich 2013-04-25 13:29:31 UTC
Created attachment 242416 [details] [review]
patch

This makes it possible to set CIDs on the gst-launch command-line.
Without this, setting these properties has no effect because the
properties are set before the device is opened.
Comment 1 Sebastian Dröge (slomo) 2013-04-25 13:44:57 UTC
Hmm, this makes it impossible to unset any CIDs later
Comment 2 Michael Olbrich 2013-04-25 13:53:39 UTC
True. Maybe we can only add to the list while the device is closed.
And clear the list after handling the CIDs after open.
Comment 3 Sebastian Dröge (slomo) 2013-04-25 14:05:02 UTC
Or adding a new property where a complete list of CIDs can be set, which allows the application to add/remove any from the list. This current API seems to be a bit weird to me
Comment 4 Michael Olbrich 2013-04-26 18:53:16 UTC
Interesting idea. It would make it possible to set arbitrary controls.
But what data type should be used? GstStructure is close, exept for the name.
And writing an extra parser for a command-line argument seems a bit overkill.
Or is there some other usefull type like that?
Comment 5 Sebastian Dröge (slomo) 2013-04-29 07:14:31 UTC
GstStructure sounds like a good choice here, we use it for similar things elsewhere (e.g. for passing custom cookies to souphttpsrc).
Comment 6 Sebastian Dröge (slomo) 2013-04-29 07:39:13 UTC
Some relevant information from IRC about this:

<slomo> wtay: what do you think about this bug https://bugzilla.gnome.org/show_bug.cgi?id=698837 ? i'm not sure if i'm thinking in the right direction there
<wtay> slomo, AFAIK you only know the possible CIDs after you open the device
<wtay> slomo, instead of only having a fixed number of properties, a structure sounds better..
<slomo> wtay: maybe in combination with a message to tell the app about all possible CIDs or something like that?
<wtay> slomo, yes, and a getter to get a structure with the current properties and values
<wtay> I don't know.. we don't really do those dynamic properties anywhere like that yet
<wtay> slomo, what we do a lot is keep the property around and set it later when we can
Comment 7 Michael Olbrich 2013-05-20 16:47:43 UTC
Created attachment 244836 [details] [review]
add a property for arbitrary v4l2 controls

How about this?
Comment 8 Sebastian Dröge (slomo) 2013-05-21 06:45:51 UTC
Review of attachment 244836 [details] [review]:

::: sys/v4l2/gstv4l2object.c
@@ +492,3 @@
           G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
+
+  g_object_class_install_property (gobject_class, PROP_EXTRA_CONTROLS,

Please add a gtk-doc documentation block above this to explain how it's meant to be used and what it does, also mention CID in the long description of g_param_spec_boxed()

::: sys/v4l2/v4l2_calls.c
@@ +276,3 @@
+      case V4L2_CTRL_TYPE_INTEGER_MENU:
+      case V4L2_CTRL_TYPE_BITMASK:
+      case V4L2_CTRL_TYPE_BUTTON:{

Are these all "int" typed? Not long, int64, some unsigned variant of these?

@@ +284,3 @@
+            control.name[i] = '_';
+        }
+        GST_WARNING_OBJECT (e, "adding generic controls '%s'", control.name);

This should be GST_DEBUG_OBJECT(), right? It's not a warning

@@ +855,3 @@
+  if (!G_VALUE_HOLDS (value, G_TYPE_INT)) {
+    GST_WARNING_OBJECT (v4l2object,
+        "'int' value expected for control '%s'.", g_quark_to_string (field_id));

Return from this function here, otherwise you call g_value_get_int() on something that is not an int
Comment 9 Michael Olbrich 2013-05-21 08:19:57 UTC
(In reply to comment #8)
> Review of attachment 244836 [details] [review]:
> 
> ::: sys/v4l2/gstv4l2object.c
> @@ +492,3 @@
>            G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> +
> +  g_object_class_install_property (gobject_class, PROP_EXTRA_CONTROLS,
> 
> Please add a gtk-doc documentation block above this to explain how it's meant
> to be used and what it does, also mention CID in the long description of
> g_param_spec_boxed()

Ok

> 
> ::: sys/v4l2/v4l2_calls.c
> @@ +276,3 @@
> +      case V4L2_CTRL_TYPE_INTEGER_MENU:
> +      case V4L2_CTRL_TYPE_BITMASK:
> +      case V4L2_CTRL_TYPE_BUTTON:{
> 
> Are these all "int" typed? Not long, int64, some unsigned variant of these?

Yes, these are all 32bit types. VIDIOC_S_CTRL only has a s32 for the value.
All other types require VIDIOC_S_EXT_CTRLS and that's a lot more complex and I
cannot test it.

> @@ +284,3 @@
> +            control.name[i] = '_';
> +        }
> +        GST_WARNING_OBJECT (e, "adding generic controls '%s'", control.name);
> 
> This should be GST_DEBUG_OBJECT(), right? It's not a warning

Of course. I changed it a few times while developing and commited the wrong version.
Maybe GST_INFO_OBJECT so the available contols are easier to find?

> 
> @@ +855,3 @@
> +  if (!G_VALUE_HOLDS (value, G_TYPE_INT)) {
> +    GST_WARNING_OBJECT (v4l2object,
> +        "'int' value expected for control '%s'.", g_quark_to_string
> (field_id));
> 
> Return from this function here, otherwise you call g_value_get_int() on
> something that is not an int

of course
Comment 10 Sebastian Dröge (slomo) 2013-05-28 16:49:07 UTC
Do you want to provide a new patch? :)
Comment 11 Michael Olbrich 2013-05-29 09:38:53 UTC
Created attachment 245536 [details] [review]
add a property for arbitrary v4l2 controls

new version of the patch
Comment 12 Michael Olbrich 2013-05-29 09:44:06 UTC
Comment on attachment 245536 [details] [review]
add a property for arbitrary v4l2 controls

>diff --git a/sys/v4l2/gstv4l2object.c b/sys/v4l2/gstv4l2object.c
>index b46b7e9..ec40c31 100644
>--- a/sys/v4l2/gstv4l2object.c
>+++ b/sys/v4l2/gstv4l2object.c
>@@ -490,6 +490,20 @@ gst_v4l2_object_install_properties_helper (GObjectClass * gobject_class,
>           "I/O mode",
>           GST_TYPE_V4L2_IO_MODE, DEFAULT_PROP_IO_MODE,
>           G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
>+
>+  /**
>+   * GstV4l2Src:extra-controls
>+   *
>+   * Additional v4l2 controls for the device. The controls are identified
>+   * by the control name (lowercase with '_' for any non-alphanumeric
>+   * characters).
>+   *
>+   * Since: 1.1

Is this correct? I wasn't sure what version to put there.

>+   */
>+  g_object_class_install_property (gobject_class, PROP_EXTRA_CONTROLS,
>+      g_param_spec_boxed ("extra-controls", "Extra Controls",
>+          "Extra v4l2 controls (CIDs) for the device",
>+          GST_TYPE_STRUCTURE, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> }
> 
> GstV4l2Object *
[...]
>diff --git a/sys/v4l2/v4l2_calls.c b/sys/v4l2/v4l2_calls.c
>index 6946e1b..61f442e 100644
>--- a/sys/v4l2/v4l2_calls.c
>+++ b/sys/v4l2/v4l2_calls.c
>@@ -269,6 +269,32 @@ gst_v4l2_fill_lists (GstV4l2Object * v4l2object)
>       GST_DEBUG_OBJECT (e, "skipping disabled control");
>       continue;
>     }
>+    switch (control.type) {
>+      case V4L2_CTRL_TYPE_INTEGER:
>+      case V4L2_CTRL_TYPE_BOOLEAN:
>+      case V4L2_CTRL_TYPE_MENU:
>+      case V4L2_CTRL_TYPE_INTEGER_MENU:
>+      case V4L2_CTRL_TYPE_BITMASK:
>+      case V4L2_CTRL_TYPE_BUTTON:{
>+        int i;
>+        control.name[31] = '\0';
>+        for (i = 0; control.name[i]; ++i) {
>+          control.name[i] = g_ascii_tolower (control.name[i]);
>+          if (!g_ascii_isalnum (control.name[i]))
>+            control.name[i] = '_';
>+        }
>+        GST_INFO_OBJECT (e, "adding generic controls '%s'", control.name);
>+        g_datalist_id_set_data (&v4l2object->controls,
>+            g_quark_from_string ((const gchar *) control.name),
>+            GINT_TO_POINTER (n));

I think GST_INFO_OBJECT is a good idea here, to make it easier to figure out
what controls are actually available.
Comment 13 Tim-Philipp Müller 2013-05-29 12:48:29 UTC
> >+   * Since: 1.1
> 
> Is this correct? I wasn't sure what version to put there.

Since: 1.2 or Since: 1.2.0 please :)

Normal users should never get to see 1.1.x versions.
Comment 14 Sebastian Dröge (slomo) 2013-05-29 18:19:08 UTC
Changed that one byte and pushed :)

commit 0fb59275b0e6d264fa971b077cdee11362f819e1
Author: Michael Olbrich <m.olbrich@pengutronix.de>
Date:   Mon May 20 16:45:37 2013 +0200

    v4l2: add a property for arbitrary v4l2 controls
    
    This makes it possible to set any controls that can be set with
    VIDIOC_S_CTRL.
    The controls are set when the property is set (if the device is open)
    and when the device is opened.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=698837