This looks to me quite similar to bug #745660

And it kinds of worries me, because it seems that different backends may support or not, different color formats, like if we need a helper that detects the backend and set the output format color accordingly.

(In reply to Víctor Manuel Jáquez Leal from comment #1)
> This looks to me quite similar to bug #745660

I do not know. For me it seems there are several problems around this but your call :)

> 
> And it kinds of worries me, because it seems that different backends may
> support or not, different color formats, like if we need a helper that
> detects the backend and set the output format color accordingly.

Is vaQueryImageFormats serves this purpose ? Which is used in gstvaapidisplay.c::ensure_image_formats. 

I did not know about that and actually mesa/st/va returns NV12, YV12, I420. Instead of only NV12.
I found it is a bug so I fixed it locally to only return NV12.

But now I get this error:

0:00:00.296754841  2130      0x1e410d0 INFO             vaapidecode gstvaapipluginbase.c:50:plugin_set_display:<vaapidecode0> set display 0x1c43390
Pipeline is PREROLLING ...
Got context from element 'vaapidecode0': gst.vaapi.Display=context, display=(GstVaapiDisplay)NULL;
0:00:00.298357285  2130      0x1e2e540 INFO             vaapidecode gstvaapidecode.c:168:gst_vaapidecode_update_sink_caps:<vaapidecode0> new sink caps = video/mpeg, mpegversion=(int)2, systemstream=(boolean)false, parsed=(boolean)true, width=(int)720, height=(int)480, framerate=(fraction)30000/1001, pixel-aspect-ratio=(fraction)8/9, codec_data=(buffer)000001b32d01e0240ea6238110111112121213131313141414141415151515151516161616161616171717171717171718181819181818191a1a1a1a191b1b1b1b1b1c1c1c1c1e1e1e1f1f21000001b5148200010000000001b5250505050b420f00000001b20087711b01016f721b01016f810025656e636f64656420627920544d5047456e6320287665722e20322e30312e33302e31313629, profile=(string)main, level=(string)main, interlace-mode=(string)mixed
0:00:00.298523851  2130      0x1e2e540 DEBUG            vaapidecode gstvaapipluginbase.c:463:ensure_sinkpad_buffer_pool: assume video buffer pool format is NV12
0:00:00.298599157  2130      0x1e2e540 DEBUG                  vaapi gstvaapiimage.c:261:gst_vaapi_image_new: format I420, size 720x480
0:00:00.298648893  2130      0x1e2e540 DEBUG                  vaapi gstvaapidisplay.c:715:ensure_image_formats: 1 image formats
0:00:00.298664395  2130      0x1e2e540 DEBUG                  vaapi gstvaapidisplay.c:717:ensure_image_formats:   NV12
0:00:00.298690761  2130      0x1e2e540 DEBUG                  vaapi gstvaapidisplay.c:763:ensure_subpicture_formats: 1 subpicture formats
0:00:00.298704822  2130      0x1e2e540 DEBUG                  vaapi gstvaapidisplay.c:765:ensure_subpicture_formats:   BGRA
0:00:00.298728741  2130      0x1e2e540 DEBUG                  vaapi gstvaapiimage.c:116:gst_vaapi_image_destroy: image 0xffffffff
0:00:00.298756639  2130      0x1e2e540 ERROR       vaapivideomemory gstvaapivideomemory.c:745:gst_vaapi_video_allocator_new: failed to allocate VA image pool
0:00:00.298774995  2130      0x1e2e540 ERROR         vaapivideopool gstvaapivideobufferpool.c:222:gst_vaapi_video_buffer_pool_set_config: failed to create GstVaapiVideoAllocator object
0:00:00.298791471  2130      0x1e2e540 ERROR            vaapidecode gstvaapipluginbase.c:488:ensure_sinkpad_buffer_pool: failed to reset buffer pool config

** (gst-launch-1.0:2130): CRITICAL **: gst_vaapi_decoder_parse: assertion 'decoder != NULL' failed
0:00:00.299029404  2130      0x1e2e540 ERROR            vaapidecode gstvaapidecode.c:840:gst_vaapidecode_parse_frame: parse error 11

** (gst-launch-1.0:2130): CRITICAL **: gst_vaapi_decoder_parse: assertion 'decoder != NULL' failed
0:00:00.299096151  2130      0x1e2e540 ERROR            vaapidecode gstvaapidecode.c:840:gst_vaapidecode_parse_frame: parse error 11

** (gst-launch-1.0:2130): CRITICAL **: gst_vaapi_decoder_parse: assertion 'decoder != NULL' failed
0:00:00.299150872  2130      0x1e2e540 ERROR            vaapidecode gstvaapidecode.c:840:gst_vaapidecode_parse_frame: parse error 11
ERROR: from element /GstPipeline:pipeline0/GstVaapiDecode:vaapidecode0: No valid frames decoded before end of stream
Additional debug info:
gstvideodecoder.c(1207): gst_video_decoder_sink_event_default (): /GstPipeline:pipeline0/GstVaapiDecode:vaapidecode0:
no valid frames found
ERROR: pipeline doesn't want to preroll.
Setting pipeline to NULL ...
Freeing pipeline ...


What is worrying me are these 2 lines:
0:00:00.298523851  2130      0x1e2e540 DEBUG            vaapidecode gstvaapipluginbase.c:463:ensure_sinkpad_buffer_pool: assume video buffer pool format is NV12
0:00:00.298599157  2130      0x1e2e540 DEBUG                  vaapi gstvaapiimage.c:261:gst_vaapi_image_new: format I420, size 720x480

NV12 vs I420

Are you writing a vaapi backend for nouveau? Did I understand correctly? wow, great!

I this this commit might clarify the issue

https://github.com/01org/gstreamer-vaapi/commit/dac20cecb

I talked with Gwenole about it some time ago, that sw sinks works better with I420 formats, though there are backends which cannot deal with it.

(In reply to Víctor Manuel Jáquez Leal from comment #3)
> Are you writing a vaapi backend for nouveau? Did I understand correctly?
> wow, great!

vaapi backend already exists in mesa for any gallium driver. Mesa defines its own decoding/encoding api (see http://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/include/pipe/). nouveau, radeon, etc ... implements it.
Then mesa provides backend for vdpau, vaapi, openmax (http://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers) using this PIPE api or else.

Currently vdpau state tracker works for nouveau, but not vaapi because nouveau does not provide chunck decoding.

I did an attempt here: https://bugs.freedesktop.org/show_bug.cgi?id=89969 (there is a patch attached).
It seems I am on the right track but it still does not work. I need to debug it.

> 
> I this this commit might clarify the issue
> 
> https://github.com/01org/gstreamer-vaapi/commit/dac20cecb
> 
> I talked with Gwenole about it some time ago, that sw sinks works better
> with I420 formats, though there are backends which cannot deal with it.

Even if in the general caps has I420, it should be intersected during caps negotiation with what "vaQueryImageFormats" (gstvaapidisplay.c::ensure_image_formats) returns. See comment #2 for the debug trace.

I got more inputs so let me clarify.

When using vaapidecode ! notvaapisink  then gst-vaapi internally creates VAImages to read back the output VASurface.
I think it is the responsibility of vaapi backend to convert buffer from surface format to image format.

In our case gst-vaapi creates VAImages in I420 format. So it expects mesa vaapi backend to convert NV12 to I420. Because nouveau driver output NV12.
But mesa backend does not support this conversion and even if it would do we still want to avoid this software conversion.

So we should create the VAImages in the same format as what the output surface is. To query this format there is:
vaQuerySurfaceAttributes which can returns the VASurfaceAttribPixelFormat (i.e., VA_FOURCC_NV12, VA_FOURCC_I420 ... ) given an input config (i.e. a profile).

See its implementation for intel driver: http://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c#n4824

Mesa va backend needs to provides an implementation for vaQuerySurfaceAttributes. I plan to make one.

So even if vaapipostproc uses this vaQuerySurfaceAttributes, I think it should also be used by vaapidecode when outputting software buffers (i.e. when not outputting either vasurface or gluploadmeta)
Or create the va surface first and then just create VAImages that match the same format. What do you think ?

OK. Let' move this.

I crossed some emails with @Sree. He explained it quite nice:

"""
* The libgstvaapi decoder always creates a surface without specifying a VideoFormat (neither NV12 nor I420) and only specify the chroma subsampling (for us, yuv420)

* The driver selects whichever format supports internally, even some encrypted format in theory. For intel driver it is always NV12.

- So we always get the decoded surface in NV12, and these all happen in libgstvaapi
 
[STEP-X] 

* Now, in gst/vaapi, we create memory which has NV12 surface format, but we also create an vaimage having format requested by downstream:

** if we use hardware renderer (vaapisink), everything will be smooth and format is NV12 always

** if we use software renderer (xvimagesink), it try to download the surface content by using vaGetImage

*** Here the hardware doing implicit colorspace coversion from NV12 (that is the format of decoded surface attached to memory) to I420 (which is what supported by xvimagesink)

IMHO, whole these steps are correct. what you proposing is to change [STEP-X], in such a way that the vaimageformat should be always NV12 irrespective of what
> we get from downstream via allocation queries. right?
 
How about this at STEP-X:

1: Create a 64x64 surface of format nv12  (surface)
2: Create a 64x64 image of format i420 first (or wh atever we get from downstream) (image)
3: do a vaGeImage (surface , image) ,
4: if 3 is success, set vaImageFormat as I420 or what ever and exit.
4: if 3 fails (non intel driver),  Create 64x64 image of format NV12
5. do a vaGetImage (surface ,image), if success, set vaImageformat as NV12 irrespective of what we requested by  downstream.
""""

(In reply to Víctor Manuel Jáquez Leal --actually, Sree-- from comment #6)
>
> IMHO, whole these steps are correct. what you proposing is to change
> [STEP-X], in such a way that the vaImageFormat should be always NV12
> irrespective of what we get from downstream via allocation queries. 
> Right?

Not exactly.

What Julien proposed is to use vaQuerySurfaceAttributes() to know the internal color format (commonly, NV12) and use it as color format for software renderers.

That means to "ignore" the vaGetImage() capability of color conversion (which not all the backends support).

>  
> How about this at STEP-X:
> 
> 1: Create a 64x64 surface of format nv12  (surface)
> 2: Create a 64x64 image of format i420 first (or wh atever we get from
> downstream) (image)
> 3: do a vaGeImage (surface , image) ,
> 4: if 3 is success, set vaImageFormat as I420 or what ever and exit.
> 4: if 3 fails (non intel driver),  Create 64x64 image of format NV12
> 5. do a vaGetImage (surface ,image), if success, set vaImageformat as NV12
> irrespective of what we requested by  downstream.
> """"

I like this approach. But instead of using NV12, use what vaQuerySurfaceAttributes() returns.

@Julien, what do you think?

(In reply to Víctor Manuel Jáquez Leal from comment #7)
> @Julien, what do you think?

Ideally you intersect vaQuerySurfaceAttributes() && vaQueryImageFormats().
So that gstvaapidecode(not bin) can output memory:VASurface but also normal system memory.

If intersection is empty then just use any format returned by vaQuerySurfaceAttributes, (because in in case hw decoder will only support to output to one of this format. But that's fine, vaapipostproc should help in this case)

That's great you are having a look at it :)

Because of time pressure, we have to move this to release 0.8.0 :(

Created attachment 321609 [details] [review]
libs: move get_surface_formats to utils_core

The query of all the supported formats for a VA config were only used by the
postprocessor (vaapifilter). But, in order to enable the vaapidecoder to
negotiate a suitable raw format with downstream, we need to query these
formats against the decoder's config.

This patch is the first step: moves the code in filter's ensure_image() to a
generic gst_vaapi_get_surface_formats() in vaapiutils_core, so it can be
shared later by the decoder.

Created attachment 321610 [details] [review]
libs: context: ensure context formats

This patch ensures to get the formats, as filter does, available in the
decoder / encoder context.

The context fills up the array as soon it is created, otherwise the pipeline
could get stalled (perhaps this is a bug in my HSW backend).

After sketching these patches, if found out that the context's available formats (vaQuerySurfaceAttributes()) in the intel backend is also NV12 only.

Intersecting with vaQueryImageFormats() won't help.

I guess this means that we've to proceed as Sree proposed: testing vaGetImage of predefined surfaces.

The other option is to revert commit dac20cecb (plugins: expose I420 format for interop with SW elements) assuming that NV12 can be handled by the software elements.

I have checked a couple of backends (intel, mesa and the old vdpau):

* mesa VA tracker - it always returns NV12
* vdpau backend - it fallbacks the default VA implementation which always returns NV12
* intel backend: https://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c#n5356 (decoders only)
 
|----------+-------+------------------------------------------|
| G4X      | MPEG2 | I420                                     |
|----------+-------+------------------------------------------|
| ILK      | MPEG2 | I420                                     |
|          | H264  | I420                                     |
|----------+-------+------------------------------------------|
| GEN6     | *     | NV12                                     |
|----------+-------+------------------------------------------|
| GEN7     | JPEG  | IMC3, IMC1, Y800, 411P, 422H, 422V, 444P |
|          | HEVC  | P010                                     |
|          | rest  | NV12                                     |
|----------+-------+------------------------------------------|
| GEN{8,9} | JPEG  | IMC3, IMC1, Y800, 411P, 422H, 422V, 444P |
|          | rest  | NV12                                     |
|----------+-------+------------------------------------------|

(In reply to Víctor Manuel Jáquez Leal from comment #12)
> After sketching these patches, if found out that the context's available
> formats (vaQuerySurfaceAttributes()) in the intel backend is also NV12 only.
> 
> Intersecting with vaQueryImageFormats() won't help.

Hi Victor, that's great that you looked at this bug. But would you mind to clarify your 2 last comments (#12 and #13). It looks to me that in first hand you are saying the intersection is not useful because vaQuerySurfaceAttributes returns always NV12 with intel vaapi backend. But then in your comment #13 you are pointing the function that actually shows that intel vaapi backend does not always return NV12 since it can return I420 depending on the hardware. Which would make the intersection actually useful. What am I missing ? :)

(In reply to Julien Isorce from comment #14)
> (In reply to Víctor Manuel Jáquez Leal from comment #12)
> > After sketching these patches, if found out that the context's available
> > formats (vaQuerySurfaceAttributes()) in the intel backend is also NV12 only.
> > 
> > Intersecting with vaQueryImageFormats() won't help.
> 
> Hi Victor, that's great that you looked at this bug. But would you mind to
> clarify your 2 last comments (#12 and #13). It looks to me that in first
> hand you are saying the intersection is not useful because
> vaQuerySurfaceAttributes returns always NV12 with intel vaapi backend. But
> then in your comment #13 you are pointing the function that actually shows
> that intel vaapi backend does not always return NV12 since it can return
> I420 depending on the hardware. Which would make the intersection actually
> useful. What am I missing ? :)

D'oh! Yes, sorry.

Comment #12 was the result of my frustration after coded the posted patches and found that only NV12 was used in my setup (Haswell).

But today I saw the guts of vaQuerySurfaceAttributes(). And the results are in comment #13. So, yes, the intersection looks interesting.

But I dig also in the vaGetImage():

* Intel backend can perform several color transformations according to the hardware, and also supports a few by software as fallback https://cgit.freedesktop.org/vaapi/intel-driver/tree/src/i965_drv_video.c#n4550
* mesa tracker only supports from NV12 to YV12
* old vdpau backend, supports from its native format to I420 and NV12

But those chroma transformation capabilities are not be "queried", as far as know.

This leads me to a try-and-fail algorithm to check if I420 is supported.

As a related topic, I wondering whether it is good to delay the src caps setting (set_output_state()) until we finish the decoding of first frame.
So that we can extract the format of the decoded surfaces..
otherwise we need some hook code again for non yuv420 format, at least a utiity method to get chroma-subsamplinng-type from decoder and corresponding manipulations. For the formats like P010 (any format having more than 8 bits per component), I won't prefer to use I420 as the default surface download format.

Created attachment 321820 [details] [review]
vaapidecode: Delay the output format setting until we have a decoded surface

This will help to consolidate the out caps negotiation to a single place,which will make the code simpler, allows to get the exact decoded format if needed and the selected chroma type too.

We need a proper regression testing for this since it may seriously affect many pipelines especially when it decode multi resolution videos.

This will help to simplify the fixes for #753914 and #759181...

Added the patch here since Victor's  upcoming patch might needs to rebase on top of this :)

https://cgit.freedesktop.org/~sree/gstreamer-vaapi/commit/?h=P010&id=995a8a6bf42e3d9ba083588aecb9e98244af67f1

will set the default vaapidecode src format to nv12..

The patches from Sree, setting the default format to NV12, and the patches in bug 765223, somewhat mitigates this issue.

Nonetheless, it looks hard to fix thoroughly, since we cannot before hand what color conversion the driver can perform. We can go with a try-and-fail, but there are combination that are directly caught by asserts in the driver (segmentation faults).

IMO, after pushing the patches in 765223, we can close this bug since it is in a "managable" state.

Julien, I wonder if what we have right now in master works for you on this regard. It is still far from being optimal, but IMO, it is a good step forward.

Hi Victor, great, I'll have a go this week and let you know. I also plan to rebase my branch https://github.com/CapOM/gstreamer-vaapi/commits/dmabuf_with_and_without_caps_feature about dmabuf.

(In reply to Julien Isorce from comment #21)
> Hi Victor, great, I'll have a go this week and let you know. I also plan to
> rebase my branch
> https://github.com/CapOM/gstreamer-vaapi/commits/
> dmabuf_with_and_without_caps_feature about dmabuf.

About that, I filed bug 765435 in order to remove the "ugly hack".

(In reply to Víctor Manuel Jáquez Leal from comment #20)
> Julien, I wonder if what we have right now in master works for you on this
> regard. It is still far from being optimal, but IMO, it is a good step
> forward.

I confirm "mesa gallium vaapi" backend works with latest gstreamer-vaapi master without any hack (that was manually forcing the underlying surface to use nv12).

(In reply to Julien Isorce from comment #23)
> I confirm "mesa gallium vaapi" backend works with latest gstreamer-vaapi
> master without any hack (that was manually forcing the underlying surface to
> use nv12).

Yuhu!!! \o/

Shall we close this issue?

I mean, there is still the open problem to detect and expose dynamically the possible color spaces to download into a raw buffer depending on the VA context (but also the postprocessor has a lot of nasty asserts to reject some conversion).

But perhaps this should be handled in other issue.

(In reply to Víctor Manuel Jáquez Leal from comment #24)
> (In reply to Julien Isorce from comment #23)
> > I confirm "mesa gallium vaapi" backend works with latest gstreamer-vaapi
> > master without any hack (that was manually forcing the underlying surface to
> > use nv12).
> 
> Yuhu!!! \o/
> 
> Shall we close this issue?

Yes please close it.

> 
> I mean, there is still the open problem to detect and expose dynamically the
> possible color spaces to download into a raw buffer depending on the VA
> context (but also the postprocessor has a lot of nasty asserts to reject
> some conversion).
> 
> But perhaps this should be handled in other issue.

Yes feel free to open a new bug, this one was only about making work other backends with at least default negotiation. Thx

Fixed in master.

Attachment 321609 [details] pushed as d1581ba - libs: move get_surface_formats to utils_core
Attachment 321610 [details] pushed as 71264ed - libs: context: ensure context formats