GNOME Bugzilla – Bug 786321
vaapi encoder: support bitrate reconfiguration in runtime
Last modified: 2017-10-20 14:35:15 UTC
I need to support a scenario where you the decoder side could inform encoder that they need a lower bitrate or a higher bitrate. I need to re-configure encoder's bitrate after the encoding was started. This is a common feature in other encoders (x264, IPP H.264, MSDK). There is any plan for this feature? Where can I give a look in order to provide a patch for this feature?
(In reply to Matteo Valdina from comment #0) > There is any plan for this feature? No plans, but it definitely would be a nice feature to have. > Where can I give a look in order to provide a patch for this feature? Change the property in run-time and reconfigure the encoder and see what happens.
Created attachment 358063 [details] [review] Handle format changes during encoding (e.g. resolution changes) I've experimented with resolution and bitrate changes while encoding. This patch handles the caps changes. I'm not sure if flushing and recreating the encoder is correct but I didn't find any better way. I don't have time right now to work on this, but maybe it's a good staring point.
Created attachment 358064 [details] [review] Handle bitrate changes during encoding This implements the bitrate changes. The other patch is needed, otherwise gst_vaapiencode_set_format() won't do anything useful.
@Michael Olbrich. Thank you so much for taking time for this issue. But IMHO, re-instantiation of encoder object entirely is not a good idea. I believe we can make it just by reconfiguring internally. See gst_vaapi_encoder_reconfigure_internal. And I found encoder plugins doesn't support to set property when PLAYING. We also need to support it.
Created attachment 359831 [details] [review] vaapiencode/libs: encoder: fix leaks of properties
Created attachment 359832 [details] [review] vaapiencode: allow to set property on runtime By this patch, we allow some properties that we want to be set on runtime. (eg. bitrate) Note that still all properties can be controlled by num_codedbuf_queued.
Created attachment 359833 [details] [review] libs: encoder: allow to set bitrate on runtime In case of streaming, controlling bitrate dynamically for encoder might be important to manage quality of the streaming. This patch is to support such a scenario.
Comment on attachment 358064 [details] [review] Handle bitrate changes during encoding Thanks Michael for the patch :) This patch is superseded by Hyunjun's ones.
Comment on attachment 358063 [details] [review] Handle format changes during encoding (e.g. resolution changes) As @Hyunjun said we should try to find a way to reconfigure the sink caps without re-instantiating the internal encoder.
Comment on attachment 359831 [details] [review] vaapiencode/libs: encoder: fix leaks of properties we should backport this patch to 1.10 and 1.12 branches
Review of attachment 359832 [details] [review]: ::: gst/vaapi/gstvaapiencode.c @@ +182,3 @@ + if (prop_value) { + g_value_copy (value, &prop_value->value); + return TRUE; I'm not sure about this. If I understand this correctly, we might copy the property value either whether the internal encoder is instantiated or not. Consider if the internal encoder shall be reconfigured, it will set the initial values.
Review of attachment 359833 [details] [review]: lgtm
Comment on attachment 359831 [details] [review] vaapiencode/libs: encoder: fix leaks of properties Attachment 359831 [details] pushed as 148f867 - vaapiencode/libs: encoder: fix leaks of properties
For branch 1.12 Attachment 359831 [details] pushed as 5e626fdb - vaapiencode/libs: encoder: fix leaks of properties
(In reply to Víctor Manuel Jáquez Leal from comment #11) > Review of attachment 359832 [details] [review] [review]: > > ::: gst/vaapi/gstvaapiencode.c > @@ +182,3 @@ > + if (prop_value) { > + g_value_copy (value, &prop_value->value); > + return TRUE; > > I'm not sure about this. > > If I understand this correctly, we might copy the property value either > whether the internal encoder is instantiated or not. > > Consider if the internal encoder shall be reconfigured, it will set the > initial values. If I understand what you mean correctly, we can take flush method in encoder for example. Every time the internal encoder is recreated, plugin(vaapiencode) calls ensure_encoder, which is doing set property with its own value (not default). Note that gst_vaapiencode_default_set_property is not called during this time.
Created attachment 361926 [details] [review] vaapiencode: allow to set property on runtime By this patch, we allow some properties that we want to be set on runtime. (eg. bitrate) Note that all properties are under control by num_codedbuf_queued.
(In reply to Víctor Manuel Jáquez Leal from comment #11) > Review of attachment 359832 [details] [review] [review]: > > ::: gst/vaapi/gstvaapiencode.c > @@ +182,3 @@ > + if (prop_value) { > + g_value_copy (value, &prop_value->value); > + return TRUE; > > I'm not sure about this. > > If I understand this correctly, we might copy the property value either > whether the internal encoder is instantiated or not. > > Consider if the internal encoder shall be reconfigured, it will set the > initial values. You're right, Victor. Thanks for correction.
Created attachment 361929 [details] [review] vaapiencode: allow to set property on runtime Tis patch, allows some properties that we want to be set on runtime. (eg. bitrate) Note that all properties are under control by num_codedbuf_queued.
Comment on attachment 358063 [details] [review] Handle format changes during encoding (e.g. resolution changes) this patch has been superseded by the other ones.
Attachment 359833 [details] pushed as acfabb3 - libs: encoder: allow to set bitrate on runtime Attachment 361929 [details] pushed as 8ef3bc3 - vaapiencode: allow to set property on runtime