After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 769266 - vaapi encoder should not accept all caps
vaapi encoder should not accept all caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 772567 (view as bug list)
Depends on: 773546
Blocks: 774241
 
 
Reported: 2016-07-28 12:49 UTC by Jan Schmidt
Modified: 2016-12-07 17:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
encoders: demote to RANK_NONE since not fit for autoplugging yet (1.13 KB, patch)
2016-08-06 11:57 UTC, Tim-Philipp Müller
committed Details | Review
libs: context: split context_create into context_create and context_create_config (3.77 KB, patch)
2016-12-01 10:02 UTC, Hyunjun Ko
none Details | Review
libs: encoder: add gst_vaapi_encoder_get_surface_formats (6.09 KB, patch)
2016-12-01 10:03 UTC, Hyunjun Ko
none Details | Review
libs: context: add logic to decide to create VA Context or not (1.70 KB, patch)
2016-12-01 10:04 UTC, Hyunjun Ko
none Details | Review
vaapiencode: query VA to get supported pixel surfaces in get_caps (1.75 KB, patch)
2016-12-01 10:05 UTC, Hyunjun Ko
none Details | Review
libs: context: split context_create into context_create and context_create_config (3.78 KB, patch)
2016-12-02 00:41 UTC, Hyunjun Ko
committed Details | Review
libs: context: add logic to decide to create VA Context or not (1.30 KB, patch)
2016-12-02 00:43 UTC, Hyunjun Ko
committed Details | Review
libs: encoder: add gst_vaapi_encoder_get_surface_formats (2.78 KB, patch)
2016-12-02 00:44 UTC, Hyunjun Ko
committed Details | Review
vaapiencode: query VA to get supported pixel surfaces in get_caps (1.75 KB, patch)
2016-12-02 00:46 UTC, Hyunjun Ko
committed Details | Review
vaapiencode: release internal encoder at stop (1.80 KB, patch)
2016-12-02 00:48 UTC, Hyunjun Ko
committed Details | Review

Description Jan Schmidt 2016-07-28 12:49:49 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.
Comment 1 Víctor Manuel Jáquez Leal 2016-07-28 13:45:51 UTC
I think this is a bit related with bug 759533, because sink and encoders share the same code to upload raw buffers.
Comment 2 Tim-Philipp Müller 2016-08-06 10:55:17 UTC
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
Comment 3 Tim-Philipp Müller 2016-08-06 11:57:47 UTC
Created attachment 332821 [details] [review]
encoders: demote to RANK_NONE since not fit for autoplugging yet
Comment 4 Víctor Manuel Jáquez Leal 2016-08-06 13:16:21 UTC
Let me work on this next Monday.

@Sree, what do you think to apply this? Are there known users of encodebin?
Comment 5 sreerenj 2016-08-07 20:44:01 UTC
(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.
Comment 6 Tim-Philipp Müller 2016-08-08 17:03:36 UTC
Thanks. Leaving bug open awaiting proper fix.
Comment 7 Víctor Manuel Jáquez Leal 2016-09-06 10:52:05 UTC
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.
Comment 8 Scott D Phillips 2016-09-12 17:07:32 UTC
(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
Comment 9 Víctor Manuel Jáquez Leal 2016-10-08 09:57:31 UTC
*** Bug 772567 has been marked as a duplicate of this bug. ***
Comment 10 Víctor Manuel Jáquez Leal 2016-10-09 12:28:31 UTC
(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.
Comment 11 Hyunjun Ko 2016-12-01 10:02:41 UTC
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.
Comment 12 Hyunjun Ko 2016-12-01 10:03:49 UTC
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.
Comment 13 Hyunjun Ko 2016-12-01 10:04:50 UTC
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.
Comment 14 Hyunjun Ko 2016-12-01 10:05:29 UTC
Created attachment 341133 [details] [review]
vaapiencode: query VA to get supported pixel surfaces in  get_caps
Comment 15 Víctor Manuel Jáquez Leal 2016-12-01 12:25:00 UTC
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);                                               ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Comment 16 Hyunjun Ko 2016-12-02 00:35:15 UTC
Sorry for inconvenience and noise.
The second patch that i uploaded is wrong. It's different patch :(
Comment 17 Hyunjun Ko 2016-12-02 00:41:01 UTC
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.
Comment 18 Hyunjun Ko 2016-12-02 00:43:54 UTC
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.
Comment 19 Hyunjun Ko 2016-12-02 00:44:53 UTC
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.
Comment 20 Hyunjun Ko 2016-12-02 00:46:39 UTC
Created attachment 341189 [details] [review]
vaapiencode: query VA to get supported pixel surfaces in get_caps
Comment 21 Hyunjun Ko 2016-12-02 00:48:54 UTC
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.
Comment 22 Víctor Manuel Jáquez Leal 2016-12-05 15:43:34 UTC
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() ???
Comment 23 Víctor Manuel Jáquez Leal 2016-12-05 16:01:50 UTC
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 24 Víctor Manuel Jáquez Leal 2016-12-07 17:14:28 UTC
Comment on attachment 341185 [details] [review]
libs: context: split context_create into context_create and context_create_config

committed with message log changed.
Comment 25 Víctor Manuel Jáquez Leal 2016-12-07 17:17:02 UTC
Comment on attachment 341186 [details] [review]
libs: context: add logic to decide to create VA Context or not

committed with changes.
Comment 26 Víctor Manuel Jáquez Leal 2016-12-07 17:17:35 UTC
Comment on attachment 341187 [details] [review]
libs: encoder: add gst_vaapi_encoder_get_surface_formats

split and committed with changes
Comment 27 Víctor Manuel Jáquez Leal 2016-12-07 17:18:18 UTC
Comment on attachment 341189 [details] [review]
vaapiencode: query VA to get supported pixel surfaces in get_caps

changed and committed.
Comment 28 Víctor Manuel Jáquez Leal 2016-12-07 17:18:46 UTC
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