GNOME Bugzilla – Bug 783588
vaapidecoder: Add framework for codec-specific properties
Last modified: 2017-07-10 17:43:25 UTC
This is needed for Bug 732265, Bug 732266 and Bug 762509: These bugs need to add codec-specific properties ("base-only" and "low-latency") but there is no framework in place to properly support this. Matt has proposed a patch in Bug 762509 but this bug should help separate this issue with the original purpose of 762509.
Created attachment 353726 [details] [review] Framework to support decoder-type-specific properties Copy of my previous patch to bug 762509, but modified to use a generic, example property instead of one specific to that bug. (The expectation is that the example property would get removed before anything got committed, but it helps clarify how the framework works.)
Review of attachment 353726 [details] [review]: This patch shall be split in several patches because it mixes different unrelated things, such as the decoder properties handling and the moving preprocessor macros.
Review of attachment 353726 [details] [review]: There's a semantic boarder among gst/vaapi and gst-libs/gst/vaapi gst/vaapi is for gstreamer related stuff, like these properties gst-libs/gst/vaapi is for parsing and VA related stuff
Created attachment 353751 [details] [review] vaapidecode: add properties callbacks in decoders map
Created attachment 353752 [details] [review] gstvaapidecode_props: add skeleton for h264 decoder properties
Created attachment 353753 [details] [review] libs: decoder: h264: add getter/setter for low latency mode
Created attachment 353754 [details] [review] vaapidecode_props: h264: add low latency property
These patches are a Request For Comments: What do you think? IMO is much more simpler than any other approach, though it doesn't handle common properties, as done in the encoders, but I consider they are not required.
(In reply to Víctor Manuel Jáquez Leal from comment #2) > Review of attachment 353726 [details] [review] [review]: > > This patch shall be split in several patches because it mixes different > unrelated things, such as the decoder properties handling and the moving > preprocessor macros. Sorry, I didn't mean to include the preprocessor changes to gstencoder_h264.c. That was needed to get things compiling again after a recent git pull. It got checked into the same rev by accident. (Although, I think that's addressing a separate, recently introduced bug.) Was that the only unrelated item? If so, I'll go ahead and pull that out and re-submit.
Review of attachment 353754 [details] [review]: ::: gst/vaapi/gstvaapidecode_props.c @@ +49,3 @@ + GstVaapiDecoderH264 *decoder = + GST_VAAPI_DECODER_H264 (GST_VAAPIDECODE (object)->decoder); + Wouldn't object->decoder be NULL here if this is called prior to starting the pipeline (as happens when setting properties via gst-launch-1.0)? That's why my previous patch included the new 'props' object - it provides a way to get/set the values prior to the associated decoder object's creation.
(In reply to Víctor Manuel Jáquez Leal from comment #8) > These patches are a Request For Comments: > > What do you think? > > IMO is much more simpler than any other approach, though it doesn't handle > common properties, as done in the encoders, but I consider they are not > required. I like this organization. It nicely separates the roles of get/vaapi and get-libs/gst/vaapi. But see my review comments about the problems caused by the decoder object's getting constructed later than the initial property get/set calls are made. (See gst_vaapidecode_reset and gst_vaapidecode_create in gstvaapidecode.c) To that end, I like sreerenj's idea from comment #41 from bug 762509. It might require a bit of redesign, but the basic idea is that instead of delaying the construction of decoder objects until caps are known, construct them earlier and then 'activate' them once caps are known. That way the decoder objects are immediately available for property set/get, and we wouldn't need a separate object for holding property values in the interim.
Created attachment 355029 [details] [review] vaapidecode: properties callback in decoders map
Created attachment 355030 [details] [review] vaapidecode_props: add skeleton for h264 decoder properties
Created attachment 355031 [details] [review] vaapidecode_props: h264: add low latency property Adding support for private data.
Created attachment 355032 [details] [review] libs: decoder: h264: add getter/setter for low latency mode
Created attachment 355033 [details] [review] vaapidecode: set h264 low latency to decoder
Finally got some time to revisit this issue. The proposed strategy is quite the same as before (just a bit more self contained), but now a private structure is added using the gobject's magic.
Review of attachment 355030 [details] [review]: ::: gst/vaapi/gstvaapidecode_props.c @@ +1,2 @@ +/* + * gstvaapidecode_props.h - VA-API decoders specific properties typo :) _props.c
Created attachment 355109 [details] [review] vaapidecode: properties callback in decoders map
Created attachment 355110 [details] [review] vaapidecode_props: add skeleton for h264 decoder properties
Created attachment 355111 [details] [review] vaapidecode_props: h264: add low latency property Adding support for private data.
Created attachment 355112 [details] [review] libs: decoder: h264: add getter/setter for low latency mode
Created attachment 355113 [details] [review] vaapidecode: set h264 low latency to decoder
Review of attachment 355111 [details] [review]: ::: gst/vaapi/gstvaapidecode_props.c @@ +41,3 @@ + switch (prop_id) { + case GST_VAAPI_DECODER_H264_PROP_FORCE_LOW_LATENCY: + g_value_set_boolean (value, priv->is_low_latency); Although it's not necessary for the low-latency property, this doesn't currently handle dynamic changes to the property value once the h264 decoder object has been constructed. For future properties that might need it, is there a way to extend this to handle dynamic updates to an already-existing decoder object?
(In reply to Matt Staples from comment #24) > Review of attachment 355111 [details] [review] [review]: > > ::: gst/vaapi/gstvaapidecode_props.c > @@ +41,3 @@ > + switch (prop_id) { > + case GST_VAAPI_DECODER_H264_PROP_FORCE_LOW_LATENCY: > + g_value_set_boolean (value, priv->is_low_latency); > > Although it's not necessary for the low-latency property, this doesn't > currently handle dynamic changes to the property value once the h264 decoder > object has been constructed. For future properties that might need it, is > there a way to extend this to handle dynamic updates to an already-existing > decoder object? Yes, you have access to the object, and the object contains the decoder, you just need the proper casts and validate if it is already created like in the previous patch: GstVaapiDecoderH264 *decoder = GST_VAAPI_DECODER_H264 (GST_VAAPIDECODE (object)->decoder); if (decoder) gst_vaapi_decoder_h264_set_property (decoder, val);
Attachment 355109 [details] pushed as 207486a - vaapidecode: properties callback in decoders map Attachment 355110 [details] pushed as 484033d - vaapidecode_props: add skeleton for h264 decoder properties Attachment 355111 [details] pushed as f39b7e9 - vaapidecode_props: h264: add low latency property Attachment 355112 [details] pushed as 28d1d04 - libs: decoder: h264: add getter/setter for low latency mode Attachment 355113 [details] pushed as 551ae94 - vaapidecode: set h264 low latency to decoder Also pushed 11f461fb - vaapidecode_props: h264: set low-latency in decoder