GNOME Bugzilla – Bug 784908
v4l2videodec: Implement stable naming scheme for elements
Last modified: 2017-07-14 17:34:38 UTC
Implement stable names for the v4l2videodec elements similar to the naming scheme of the v4l2videoenc elements. The first element for a codec will be named v4l2<codec>dec and the video node name will only be added, if there are multiple v4l2 devices that implement the same codecs. This naming scheme was suggested in https://bugzilla.gnome.org/show_bug.cgi?id=742918. There should be one subclass of v4l2videodec per codec, which would also allow to implement codec specific code in a subclass instead of mangling it into the v4l2videodec class. Right now, the new elements are only boilerplate and a function to check if the subclass should be registered for a v4l2 device. Note that multiple elements per v4l2 device might be registered if the v4l2 device supports multiple codecs. Moreover, there will still always be a v4l2videoXdec element to support codecs that don't have their own subclass, yet. It would be cool, if the fallback would only trigger if there are any unsupported codecs left, but all my implementations were really dirty.
Created attachment 355508 [details] [review] 0001-v4l2-prepare-video-decoder-for-stable-naming-scheme.patch
Created attachment 355509 [details] [review] 0002-v4l2-report-only-template-caps-in-v4l2-decoder-probe.patch
Created attachment 355510 [details] [review] 0003-v4l2-add-v4l2h264dec-element.patch
Created attachment 355511 [details] [review] 0004-v4l2-add-v4l2jpegdec-element.patch
Created attachment 355512 [details] [review] 0005-v4l2-add-v4l2mpegdec-element.patch
That's nice, I'll look at it soon, don't worry, just a little busy this week. Some general notes, instead of implementing subclasses, which seems all boiler plate, can we just add more items in the cdata ? For the metadata - longname - description The gst_v4l2_is_*_dec () are completly generic, and should not be needed really. Registration parameter is all covered already. Another thing, you are taking authorship of these element, I don't think this is fair, considering you are just wrapping.
(In reply to Nicolas Dufresne (stormer) from comment #6) > That's nice, I'll look at it soon, don't worry, just a little busy this > week. Some general notes, instead of implementing subclasses, which seems > all boiler plate, can we just add more items in the cdata ? > > For the metadata > - longname > - description > > The gst_v4l2_is_*_dec () are completly generic, and should not be needed > really. Registration parameter is all covered already. My first try was an implementation without subclasses and using the register function to decide which type to register, because the boilerplate seemed to much to me as well. However, in that case, having multiple elements for a single v4l2 device (e.g., v4l2h264dec and v4l2mpegdec using the same device) didn't work unless I would add a lot more logic to gstv4l2.c. To keep things separate, having multiple subclasses seemed a better fit. > Another thing, you > are taking authorship of these element, I don't think this is fair, > considering you are just wrapping. I was really hesitating to claim authorship, because there is nothing that I really added, but I didn't really know what else to do. I will happily put you as author, if you agree.
The split does not need to be visible in gstv4l2.c, you could have put all this into gst_v4l2_video_dec_register(). I think I'll have to take a look.
Created attachment 355532 [details] [review] 0001-v4l2-implement-stable-decoder-naming-scheme.patch (In reply to Nicolas Dufresne (stormer) from comment #8) > The split does not need to be visible in gstv4l2.c, you could have put all > this into gst_v4l2_video_dec_register(). I think I'll have to take a look. As I started with putting everything in gst_v4l2_video_dec_register(), I still have a patch for this, which I added as an attachment. It wasn't really tested, though, and doesn't handle multiple codecs per v4l2 device. This probably could be fixed by registering multiple elements in the register function by iterating over the caps.
Created attachment 355536 [details] [review] v4l2videodec: Implement stable element names Before that, each m2m node would be wrapped as a single, multi-format decoder element. As a unique name was needed, we where using the device name, which changes between re-boots. This led to unpredictable element names. In this patch, we generate an element per codec, using v4l2<codec>dec name. If for some reason there is multiple decoder for the same format, the following elements will be named v4l2<codec><node>dec.
Sorry, I had started writing my own from scratch. The reason I want this is for maintenance, the new files had no purpose unlike the encoder which implement format specific caps negotiation (and eventually property to CID bindings). Note that we should hack the caps probing now, so it only probe for 1 format, will speedup the start (same apply to encoder).
Review of attachment 355536 [details] [review]: ::: sys/v4l2/gstv4l2videodec.c @@ +919,3 @@ "V4L2 Video Decoder"); + Oops, I left some empty lines overhear, will fix in later update. @@ +977,3 @@ + "Codec/Decoder/Video", + cdata->description, + "Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>"); My email is not .com, will fix too.
Created attachment 355538 [details] [review] v4l2videodec: Implement stable element names Updated base on my own review, no major change.
Review of attachment 355538 [details] [review]: ::: sys/v4l2/gstv4l2videodec.c @@ +1034,3 @@ + } else { + gchar *s_str = gst_structure_to_string (s); + SET_META ("MPEG2"); I'm not sure if the warning is better than falling back to v4l2<node>dec. You are handling a lot more codecs that I did and adding a new mapping is easy, so this is probably unlikely. However, having a fallback that works with playbin and doesn't require a change here sounds reasonable to me. Maybe we want both, the warning and the fallback. @@ +1042,3 @@ + if (g_type_from_name (type_name) != 0) { + g_free (type_name); + } else if (gst_structure_has_name (s, "video/x-h263")) { This leads to v4l2<node><codec>dec names, which conflicts with my expectation and your commit message.
(In reply to Michael Tretter from comment #14) > Review of attachment 355538 [details] [review] [review]: > > ::: sys/v4l2/gstv4l2videodec.c > @@ +1034,3 @@ > + } else { > + gchar *s_str = gst_structure_to_string (s); > + SET_META ("MPEG2"); > > I'm not sure if the warning is better than falling back to v4l2<node>dec. > You are handling a lot more codecs that I did and adding a new mapping is > easy, so this is probably unlikely. However, having a fallback that works > with playbin and doesn't require a change here sounds reasonable to me. > Maybe we want both, the warning and the fallback. I thought about that, but if there is two unsupported formats, we'll have a type_name clash. I have made sure to cover all formats marked has codecs in the GstV4l2Object, so hitting this warning would be a programming error. I'll improve the warning message. > > @@ +1042,3 @@ > + if (g_type_from_name (type_name) != 0) { > + g_free (type_name); > + } else if (gst_structure_has_name (s, "video/x-h263")) { > > This leads to v4l2<node><codec>dec names, which conflicts with my > expectation and your commit message. I'll double check, I flip it over during implementation to match the encoder fallback. Thanks for the review.
About the v4l2<node><codec>dec, here's what we already have on the encoder side as a fallback: type_name = g_strdup_printf ("v4l2%s%senc", basename, codec); I will just stick with what was there and fix my broken comment. All I care is that being consistent.
Created attachment 355606 [details] [review] v4l2videodec: Implement stable element names Before that, each m2m node would be wrapped as a single, multi-format decoder element. As a unique name was needed, we where using the device name, which changes between re-boots. This led to unpredictable element names. In this patch, we generate an element per codec, using v4l2<codec>dec name. If there is multiple decoder for the same format, the following elements will be named v4l2<node><codec>dec.
I've also pushed this slightly related optimization, this way we don't probe all formats eventhough the subtype only support 1 format. This should help with startup time really, and reduce the overhead when dealing with operation logs. Attachment 355606 [details] pushed as 724a3ce - v4l2videodec: Implement stable element names
commit 06424c438e122480dd87504b8d1f5989442fedbe Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Jul 14 11:54:57 2017 -0400 v4l2: Don't probe for unneeded format For v4l2videodec/enc, we generate elements per formats, and in this case we can speed up the start up by only probing the format we care about.