GNOME Bugzilla – Bug 769266
vaapi encoder should not accept all caps
Last modified: 2016-12-07 17:19:14 UTC
The VAAPI encoder should not announce caps that it can't support and then throw a runtime error: 0:00:22.650358794 32413 0x40e80f0 ERROR vaapi gstvaapiencoder.c:585:is_chroma_type_supported: We only support YUV 4:2:0 and YUV 4:2:2 for encoding. Please try to use vaapipostproc to convert the input format. 0:00:22.650384591 32413 0x40e80f0 ERROR vaapi gstvaapiencoder.c:625:set_context_info: failed to determine chroma type for format YUY2 0:00:22.650393900 32413 0x40e80f0 ERROR vaapi gstvaapiencoder.c:693:gst_vaapi_encoder_reconfigure_internal: failed to update VA context Instead, the sink pad caps query should filter out unsupported formats so that upstream can choose a sensible format to feed the encoder.
I think this is a bit related with bug 759533, because sink and encoders share the same code to upload raw buffers.
Marking this as blocker. This needs to be fixed or the encoder rank needs to be set to NONE, otherwise this just breaks with autoplugged encoders such as in camerabin and encodebin. Simple test pipeline: gst-launch-1.0 videotestsrc ! video/x-raw,format=YUY2 ! videoconvert ! vaapih264enc ! fakesink
Created attachment 332821 [details] [review] encoders: demote to RANK_NONE since not fit for autoplugging yet
Let me work on this next Monday. @Sree, what do you think to apply this? Are there known users of encodebin?
(In reply to Víctor Manuel Jáquez Leal from comment #4) > Let me work on this next Monday. > > @Sree, what do you think to apply this? Fine, but I hope we can get a fix before 1.10... >Are there known users of encodebin? No direct customers.
Thanks. Leaving bug open awaiting proper fix.
Before my vacations I worked on "a proper fix" for this issue. This are my findings: In Haswell, if we query the created VA context, we got these possible color formats to be uploaded: NV12, I420 and YV12 Nonetheless, it is impossible to get those values without creating the encoder context, and to create the encoder context it is mandatory to have a GstVaapiContextInfo with all the sink caps. This implies to have, already, the negotiated color format. Thus we have an "API level dead lock": we need the VA context to get the possible formats to upload, and we need to know the format to upload to create the VA context.
(In reply to Víctor Manuel Jáquez Leal from comment #7) > Before my vacations I worked on "a proper fix" for this issue. This are my > findings: > > In Haswell, if we query the created VA context, we got these possible color > formats to be uploaded: NV12, I420 and YV12 > > Nonetheless, it is impossible to get those values without creating the > encoder context, and to create the encoder context it is mandatory to have a > GstVaapiContextInfo with all the sink caps. This implies to have, already, > the negotiated color format. > > Thus we have an "API level dead lock": we need the VA context to get the > possible formats to upload, and we need to know the format to upload to > create the VA context. You can get the supported surface pixel formats with just a config, not necessarily a context. Like ensure_formats() in gstvaapifilter.c
*** Bug 772567 has been marked as a duplicate of this bug. ***
(In reply to Scott D Phillips from comment #8) > (In reply to Víctor Manuel Jáquez Leal from comment #7) > > Before my vacations I worked on "a proper fix" for this issue. This are my > > findings: > > > > In Haswell, if we query the created VA context, we got these possible color > > formats to be uploaded: NV12, I420 and YV12 > > > > Nonetheless, it is impossible to get those values without creating the > > encoder context, and to create the encoder context it is mandatory to have a > > GstVaapiContextInfo with all the sink caps. This implies to have, already, > > the negotiated color format. > > > > Thus we have an "API level dead lock": we need the VA context to get the > > possible formats to upload, and we need to know the format to upload to > > create the VA context. > > You can get the supported surface pixel formats with just a config, not > necessarily a context. Like ensure_formats() in gstvaapifilter.c True. Actually, the code that gets the formats uses the config, not the context, but still, the context and the config are coupled in GstVaapiContext object used by the encoder. The solution would be to create a temporal config before the context creation.
Created attachment 341130 [details] [review] libs: context: split context_create into context_create and context_create_config With decoupling between VAConfig and VAContext during creation, we can make it querying supported pixel surface without creation of VAContext.
Created attachment 341131 [details] [review] libs: encoder: add gst_vaapi_encoder_get_surface_formats Add new api gst_vaapi_encoder_get_surface_formats, so that encode plugin can get supported pixel surfaces.
Created attachment 341132 [details] [review] libs: context: add logic to decide to create VA Context or not If ContextInfo has just initial information without width and height, it doesn't create VA Context.
Created attachment 341133 [details] [review] vaapiencode: query VA to get supported pixel surfaces in get_caps
Review of attachment 341133 [details] [review]: It doesn't compile :) ::: gst/vaapi/gstvaapiencode.c @@ +355,3 @@ + goto normal; + + formats = gst_vaapi_encoder_get_surface_formats (encode->encoder); I guess you missed something in your patch gstvaapiencode.c:357:13: error: implicit declaration of function 'gst_vaapi_encoder_get_surface_formats' is invalid in C99 [-Werror,-Wimplicit-function-declaration] formats = gst_vaapi_encoder_get_surface_formats (encode->encoder); ^ gstvaapiencode.c:357:11: error: incompatible integer to pointer conversion assigning to 'GArray *' (aka 'struct _GArray *') from 'int' [-Werror,-Wint-conversion] formats = gst_vaapi_encoder_get_surface_formats (encode->encoder); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Sorry for inconvenience and noise. The second patch that i uploaded is wrong. It's different patch :(
Created attachment 341185 [details] [review] libs: context: split context_create into context_create and context_create_config With decoupling between VAConfig and VAContext during creation, we can make it querying supported pixel surface without creation of VAContext.
Created attachment 341186 [details] [review] libs: context: add logic to decide to create VA Context or not If ContextInfo has just initial information without width and height, it doesn't create VA Context.
Created attachment 341187 [details] [review] libs: encoder: add gst_vaapi_encoder_get_surface_formats Add new api gst_vaapi_encoder_get_surface_formats, so that encode plugin can get supported pixel surfaces.
Created attachment 341189 [details] [review] vaapiencode: query VA to get supported pixel surfaces in get_caps
Created attachment 341190 [details] [review] vaapiencode: release internal encoder at stop As an internal encoder is created at start, releasing this at stop to be consistency.
Review of attachment 341190 [details] [review]: ::: gst/vaapi/gstvaapiencode.c @@ +450,3 @@ + GstVaapiEncode *const encode = GST_VAAPIENCODE_CAST (venc); + + gst_vaapi_encoder_replace (&encode->encoder, NULL); why not call gst_vaapiencode_destroy () here and remove it from close() ???
Review of attachment 341189 [details] [review]: ::: gst/vaapi/gstvaapiencode.c @@ +372,3 @@ + gst_caps_append (out_caps, gst_caps_copy (raw_caps)); + + gst_caps_replace (&plugin->sinkpad_caps, out_caps); the idea of having the variable memeber sinkpad_caps is to avoids its calculation as much as possible. This should be moved to a function that validates it existence and return it, otherwise do this calculation.
Comment on attachment 341185 [details] [review] libs: context: split context_create into context_create and context_create_config committed with message log changed.
Comment on attachment 341186 [details] [review] libs: context: add logic to decide to create VA Context or not committed with changes.
Comment on attachment 341187 [details] [review] libs: encoder: add gst_vaapi_encoder_get_surface_formats split and committed with changes
Comment on attachment 341189 [details] [review] vaapiencode: query VA to get supported pixel surfaces in get_caps changed and committed.
Comment on attachment 332821 [details] [review] encoders: demote to RANK_NONE since not fit for autoplugging yet this has been reverted since the problem is fixed