GNOME Bugzilla – Bug 742918
v4l2: Add decoder and converter element with RW device property
Last modified: 2017-07-14 17:40:05 UTC
Created attachment 294529 [details] [review] generic converter patch Instead of only creating device-specific convert elements as subclasses of GstV4l2Transform, allow instantiating former abstract base class GstV4l2Transform directly as a generic convert element. The specific device can be passed using the 'device' property. For this purpose the base classes 'device' property was given the read/write flag and device-specific subclasses overwrite this property with a read-only 'device' property set to the respecitve device path. This eases constructing pipelines in scripts and allows to use fixed '/dev/video/by-name/..' device paths. Example usage: gst-launch [..] ! v4l2convert device=/dev/video1 ! [..]
Created attachment 294530 [details] [review] generic decoder patch This patch creates a generic decoder element based on and similar to the generic convert element Example usage: gst-launch [..] ! v4l2decoder device=/dev/video5 ! [..]
(In reply to comment #1) > Created an attachment (id=294530) [details] [review] > generic decoder patch > > This patch creates a generic decoder element based on and similar to the > generic convert element > > Example usage: > > gst-launch [..] ! v4l2decoder device=/dev/video5 ! [..] v4l2videodecoder, or v4l2videodec would be the right name for this element. There is a lot of video specific code in there.
I think if we go this way (as opposed to having fixed name through udev), it will be required to also implement the GstDeviceMonitor part.
Any update / interest ?
ping ?
I can make the generic v4l2convert element work using this patch. (Not tested the decoder patch) However, this element accepts all formats in its src and sink caps. Caps should be redefined more precisely once the device property is set, so that GStreamer can negotiate the right format with other elements instead of just pick one and try.
The caps should be updated when the element is set to ready state. At least this is how it should work, and how v4l2src works.
Review of attachment 294529 [details] [review]: Except for the code duplication, this look ok. I'll give it a try to make sure nothing breaks. Can you add bug reference to your commit message when re-submitting. ::: sys/v4l2/gstv4l2object.c @@ +414,3 @@ + g_object_class_install_property (gobject_class, PROP_DEVICE, + g_param_spec_string ("device", "Device", "Device location", + NULL, G_PARAM_READABLE | G_PARAM_STATIC_STRINGS)); I don't like copy paste here, specially that we clearly need to improve the description, which will have to be done twice. Maybe add an argument to previous call or something ? Or just find a better way ?
Review of attachment 294530 [details] [review]: Please add bug reference to your commit message while resubmitting. Also, it says 5/5, where are the 3 other patches ?
Created attachment 299019 [details] [review] v4l2: Add gst_v4l2_object_install_device_property_helper v4l2: Add gst_v4l2_object_install_device_property_helper To avoid duplication of the device property definition, move it out into its own function and have it called from gst_v4l2_object_install_properties_helper and gst_v4l2_object_install_m2m_properties_helper.
Created attachment 299020 [details] [review] v4l2: Add generic v4l2convert element v4l2: Add generic v4l2convert element Instead of only creating device-specific convert elements as subclasses of GstV4l2Transform, allow instantiating former abstract base class GstV4l2Transform directly as a generic convert element. The specific device can be passed using the 'device' property. For this purpose the base classes 'device' property was given the read/write flag and device-specific subclasses overwrite this property with a read-only 'device' property set to the respecitve device path. This eases constructing pipelines in scripts and allows to use fixed '/dev/video/by-name/..' device paths. Example usage: gst-launch [..] ! v4l2convert device=/dev/video1 ! [..]
Created attachment 299021 [details] [review] v4l2: Add generic v4l2videodec element v4l2: Add generic v4l2videodec element This patch creates a generic decoder element based on and similar to the generic convert element Example usage: gst-launch [..] ! v4l2videodec device=/dev/video5 ! [..]
(In reply to Nicolas Dufresne (stormer) from comment #10) > ping ? Unfortunately Enrico is temporarily out of order. Just so this doesn't get forgotten, would splitting out the device property installation like this be preferred?
Any comments on this and what is still missing? Is the device monitor part still mandatory?
(In reply to Enrico Jorns from comment #15) > Any comments on this and what is still missing? I was on holiday, that's the reason I didn't yet look at this. It adds quite some "API", so I hope you wont be offended if I wait for the next development cycle to commit to that. > Is the device monitor part still mandatory? I think it is something one would like to make this usable in a more generic way. But I'd agree that this can be handled separately. In general though, adding such API instead of implementing proper auto-plugging looks like the lazy approach. The overall benefit is small, making this patch-set a bit of a low priority for me. Meanwhile, feel free to use them though, the code seems correct. The general goal with these decoders and transform element is tat they should just work, using decodebin and other dynamic elements.
Created attachment 342939 [details] [review] v4l2: Add gst_v4l2_object_install_device_property_helper
Created attachment 342941 [details] [review] v4l2: Add generic v4l2convert element
Created attachment 342942 [details] [review] v4l2: Add generic v4l2videodec element
I rebased the patches on current master. The point of this patch series is to make gst-launch pipelines more readable. For example, the following pipeline uses the added property to find the v4l2 by name: gst-launch [...] ! v4l2videodec device=/dev/v4l/by-name/coda-decoder ! [...] With the existing mechanism, the same pipeline would look as follows, which I consider less readable. gst-launch [...] ! v4l2video9dec ! [...] Of course, auto-detection is still preferred. However, decodebin, playbin, etc. will continue to use the v4l2videoCdec elements. Therefore, I do not see, why this mechanism should use the device monitor to be more generic unless you would want to remove the sub-classing mechanism.
Any comments?
Nothing special on my end, was just trying to find some time to properly review. Quick question, "/dev/v4l/by-name/" does not exist on my kernel here, I have by-id and by-path. Where is that from ?
(In reply to Nicolas Dufresne (stormer) from comment #22) > Nothing special on my end, was just trying to find some time to properly > review. Thanks :) > Quick question, "/dev/v4l/by-name/" does not exist on my kernel > here, I have by-id and by-path. Where is that from ? I have a udev rule that creates these symlinks using the name attribute.
Could you have another look on these patches?
Created attachment 353246 [details] [review] v4l2videoenc: verify outcaps with peer before setting the format If the subclass specifies multiple supported codecs or if the v4l2videoenc element is directly instantiated, the outcaps are not fixated and can contain codecs that are not supported by the downstream element. Ask the peer for the available caps and fixate them before setting the format. This patch is especially required for the generic encoder element.
Created attachment 353247 [details] [review] v4l2: Add generic v4l2videoenc element This patch creates a generic encoder element based on and similar to the generic convert element. Example usage: gst-launch [..] ! v4l2videoenc device=/dev/video2 ! [..] If this element is combined with udev rules to create named device nodes for the v4l2 devices, you can define stable pipelines with speaking names for the encoder elements, e.g. gst-launch [..] ! v4l2videoenc device=/dev/v4l/coda-encoder ! [..]
Review of attachment 353246 [details] [review]: ::: sys/v4l2/gstv4l2videoenc.c @@ +326,3 @@ outcaps = gst_caps_make_writable (outcaps); + outcaps = gst_pad_peer_query_caps (encoder->srcpad, outcaps); + outcaps = gst_caps_fixate (outcaps); This is obscure change. Each subclass of GstV4l2VideoEnc should have been created with a single template. So for the H264 subclass the template caps should simply be "video/x-h264,stream-format=byte-stream". The downstream query is also redundant, because this will be done few lines later, when we call gst_video_encoder_negotiated() (see negotiate() virtual method). The actual set format will happen at the start of decide_allocation().
Review of attachment 353247 [details] [review]: This is no longer valid. In the design, a generic encoder is not acceptable. You will need a generic H264, MPEG4, and so on encoder, a subclass the appropriate encoder.
Comment on attachment 353246 [details] [review] v4l2videoenc: verify outcaps with peer before setting the format As discussed over IRC, encoders have fixed name, no need for this patch anymore. We will likely drop all the other by implementing similar naming method (first found have a fixed named, then others are numbered using the node name).
This is obsoleted by the new approach, which is to give the first driver found, implementing a specific feature a fixed name. On most SoC only on such element will be found. As a result we'll have: v4l2h264dec v4l2h264enc etc. For encoder and decoders this is all implemented now, only transform element are missing. There is a proposed patch, which I should have a look and make a decision soon (kernel lacks some information). https://bugzilla.gnome.org/show_bug.cgi?id=784958