GNOME Bugzilla – Bug 698837
v4l2: cache CIDs until the device is opened
Last modified: 2013-05-29 18:19:08 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.
Hmm, this makes it impossible to unset any CIDs later
True. Maybe we can only add to the list while the device is closed. And clear the list after handling the CIDs after open.
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
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?
GstStructure sounds like a good choice here, we use it for similar things elsewhere (e.g. for passing custom cookies to souphttpsrc).
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
Created attachment 244836 [details] [review] add a property for arbitrary v4l2 controls How about this?
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
(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
Do you want to provide a new patch? :)
Created attachment 245536 [details] [review] add a property for arbitrary v4l2 controls new version of the patch
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.
> >+ * 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.
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