I think compiling with "enable-glx=no" works around the problem.

gst_vaapi_create_display (gst/vaapi/gstvaapipluginutil.c) looks through compatible display types in display_maps[] with the following order:  GST_VAAPI_DISPLAY_TYPE_WAYLAND, GST_VAAPI_DISPLAY_TYPE_GLX, GST_VAAPI_DISPLAY_TYPE_X11.

So if GLX is also available on the system, GLX is picked over X11 (even if EGL is specified in GST_GL_PLATFORM).

display_type argument in gst_vaapi_create_display seems to always be GST_VAAPI_DISPLAY_TYPE_ANY. Shouldn't this argument be affected by env. variable GST_GL_PLATFORM or some other VAAPI user-controlled variable/property? 

A related question:  display_maps[] has no GST_VAAPI_DISPLAY_TYPE_EGL entry, shouldn't it have one for EGL non-X11 systems? 

Also, is GST_VAAPI_DISPLAY_TYPE_X11 enough for EGL X11-based systems (GST_GL_WINDOW=x11 GST_GL_PLATFORM=egl) ?

I'm not sure if GstVideoGLTextureUploadMeta is not just more generally broken. I can't get GstVideoGLTextureUploadMeta even with glx using git master.

filesrc location=test-h264.mov ! qtdemux ! vaapidecode ! 'video/x-raw(meta:GstVideoGLTextureUploadMeta)' ! glimagesink

yields not-negotiated.

It seems that gst_vaapidecode_update_src_caps() relies on decode->has_texture_upload_meta == TRUE, which is only set gst_vaapidecode_decide_allocation. gst_vaapidecode_decide_allocation() doesn't happen until after the decoder has already tried to set the wrong caps and gotten not-negotiated.

I am sure this worked last time I tried, so it's a regression since sometime in July.

I have also tried above pipeline that Jan provided and it doesn't work. format is selected as I420 in glimagesinkbin and it goes over CPU memory. Can anybody check that?

(In reply to Dolevo from comment #3)
> Can anybody check that?

I intend to look at it next week.

(In reply to Víctor Manuel Jáquez Leal from comment #4)
> (In reply to Dolevo from comment #3)
> > Can anybody check that?
> 
> I intend to look at it next week.

Hi Victor, 
Did you have time to have a look at it so far? I desperately wait for an update.

Thanks.

Created attachment 313320 [details] [review]
vaapidecode: set format before decide allocation

There is a regression from commit 3d8e5e. It was expected the buffer pool
allocation occur before the caps negotiation, but it is not.

This patch fixes this regression: the caps negotiation is done regardless the
allocation query from downstream.

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>

Created attachment 313321 [details] [review]
vaapidecode: use caps to check the features

Instead of calling gst_vaapi_find_preferred_caps_feature(), which is
expensive, we check the caps from the allocation query, to check the
negotiated feature.

In order to do this verification a new utility function has been implemented:
gst_vaapi_caps_has_feature()

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>

(In reply to Víctor Manuel Jáquez Leal from comment #6)
> Created attachment 313320 [details] [review] [review]
> vaapidecode: set format before decide allocation
> 
> There is a regression from commit 3d8e5e. It was expected the buffer pool
> allocation occur before the caps negotiation, but it is not.
> 
> This patch fixes this regression: the caps negotiation is done regardless the
> allocation query from downstream.
> 
> Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>

This is only required to make the pipeline mentioned by Jan work. The second patch is just a simplification of the code.

> This is only required to make the pipeline mentioned by Jan work. The second
> patch is just a simplification of the code.

Could you give it a try and let me now if it works for you in order to push it to master, and add it to branch 0.6

Confirming that GstVideoGLTextureUploadMeta works here again with those patches.

Since these issues about caps negotiation are different from the original one (a crash when using glimagesink with opengl and egl), I'm going to create a new bug to track them.

(In reply to Víctor Manuel Jáquez Leal from comment #11)
> Since these issues about caps negotiation are different from the original
> one (a crash when using glimagesink with opengl and egl), I'm going to
> create a new bug to track them.

bug 756686

Hi,

After applying this patch to 0.6.1 version, I get the following error:

gst-launch-1.0 -vvv filesrc location=/store/1.mp4 ! qtdemux ! vaapidecode ! glimagesink
libva info: VA-API version 0.38.0
libva info: va_getDriverName() returns 0
libva info: Trying to open /usr/lib/va/fglrx_drv_video.so
libva info: Found init function __vaDriverInit_0_32
libva info: va_openDriver() returns 0
Setting pipeline to PAUSED ...
Pipeline is PREROLLING ...
Got context from element 'vaapidecode0': gst.vaapi.Display=context, display=(GstVaapiDisplay)NULL;
Got context from element 'sink': gst.gl.GLDisplay=context, gst.gl.GLDisplay=(GstGLDisplay)"\(GstGLDisplayX11\)\ gldisplayx11-0";
/GstPipeline:pipeline0/GstVaapiDecode:vaapidecode0.GstPad:sink: caps = "video/x-h264\,\ stream-format\=\(string\)avc\,\ alignment\=\(string\)au\,\ level\=\(string\)4.1\,\ profile\=\(string\)high\,\ codec_data\=\(buffer\)01640029ffe1001867640029ac34e5014016e84001974ec04c4b4023c60c458001000468eebcb0\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ framerate\=\(fraction\)24000/1001\,\ pixel-aspect-ratio\=\(fraction\)1/1"
Redistribute latency...
/GstPipeline:pipeline0/GstVaapiDecode:vaapidecode0.GstPad:src: caps = "video/x-raw\(meta:GstVideoGLTextureUploadMeta\)\,\ format\=\(string\)RGBA\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ colorimetry\=\(string\)sRGB\,\ framerate\=\(fraction\)24000/1001"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0.GstGhostPad:sink.GstProxyPad:proxypad0: caps = "video/x-raw\(meta:GstVideoGLTextureUploadMeta\)\,\ format\=\(string\)RGBA\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ colorimetry\=\(string\)sRGB\,\ framerate\=\(fraction\)24000/1001"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLUploadElement:gluploadelement0.GstPad:src: caps = "video/x-raw\(memory:GLMemory\,\ meta:GstVideoOverlayComposition\)\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ framerate\=\(fraction\)24000/1001\,\ format\=\(string\)RGBA\,\ colorimetry\=\(string\)sRGB"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLColorConvertElement:glcolorconvertelement0.GstPad:src: caps = "video/x-raw\(memory:GLMemory\,\ meta:GstVideoOverlayComposition\)\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ framerate\=\(fraction\)24000/1001\,\ format\=\(string\)RGBA\,\ colorimetry\=\(string\)sRGB"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLImageSink:sink.GstPad:sink: caps = "video/x-raw\(memory:GLMemory\,\ meta:GstVideoOverlayComposition\)\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ framerate\=\(fraction\)24000/1001\,\ format\=\(string\)RGBA\,\ colorimetry\=\(string\)sRGB"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLColorConvertElement:glcolorconvertelement0.GstPad:sink: caps = "video/x-raw\(memory:GLMemory\,\ meta:GstVideoOverlayComposition\)\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ framerate\=\(fraction\)24000/1001\,\ format\=\(string\)RGBA\,\ colorimetry\=\(string\)sRGB"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLUploadElement:gluploadelement0.GstPad:sink: caps = "video/x-raw\(meta:GstVideoGLTextureUploadMeta\)\,\ format\=\(string\)RGBA\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ colorimetry\=\(string\)sRGB\,\ framerate\=\(fraction\)24000/1001"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0.GstGhostPad:sink: caps = "video/x-raw\(meta:GstVideoGLTextureUploadMeta\)\,\ format\=\(string\)RGBA\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ colorimetry\=\(string\)sRGB\,\ framerate\=\(fraction\)24000/1001"
xvba_video: XVBA_GetSurface(): status 2
ERROR: from element /GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLUploadElement:gluploadelement0: Failed to upload buffer

** (gst-launch-1.0:5054): CRITICAL **: gst_vaapi_image_get_plane: assertion 'image != NULL' failed
Additional debug info:
gstgluploadelement.c(228): gst_gl_upload_element_prepare_output_buffer (): /GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLUploadElement:gluploadelement0

** (gst-launch-1.0:5054): CRITICAL **: gst_vaapi_image_get_pitch: assertion 'image != NULL' failed
ERROR: pipeline doesn't want to preroll.
Setting pipeline to NULL ...
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLImageSink:sink.GstPad:sink: caps = "NULL"

** (gst-launch-1.0:5054): CRITICAL **: gst_video_meta_unmap_vaapi_memory: assertion 'mem->surface' failed
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLColorConvertElement:glcolorconvertelement0.GstPad:src: caps = "NULL"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLColorConvertElement:glcolorconvertelement0.GstPad:sink: caps = "NULL"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLUploadElement:gluploadelement0.GstPad:src: caps = "NULL"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLUploadElement:gluploadelement0.GstPad:sink: caps = "NULL"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0.GstGhostPad:sink.GstProxyPad:proxypad0: caps = "NULL"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0.GstGhostPad:sink: caps = "NULL"
/GstPipeline:pipeline0/GstVaapiDecode:vaapidecode0.GstPad:src: caps = "NULL"
/GstPipeline:pipeline0/GstVaapiDecode:vaapidecode0.GstPad:sink: caps = "NULL"
/GstPipeline:pipeline0/GstQTDemux:qtdemux0.GstPad:video_0: caps = "NULL"
/GstPipeline:pipeline0/GstQTDemux:qtdemux0.GstPad:audio_0: caps = "NULL"





I receive the same error when using master. Could you please let me know if I am doing something wrong?
Thanks.

Export these variables 

GST_GL_API=opengl GST_GL_PLATFORM=glx

If it works, that means that you're hit by a couple bugs: bug 752958, bug 745660 and bug 753099.

Alternatively, you could try the patch posted in bug 754900.

Victor,

I get now, 

GST_GL_API=opengl GST_GL_PLATFORM=glx gst-launch-1.0 -vvv filesrc location=/store/1.mp4 ! qtdemux ! vaapidecode ! glimagesink


libva info: VA-API version 0.38.0
libva info: va_getDriverName() returns 0
libva info: Trying to open /usr/lib/va/fglrx_drv_video.so
libva info: Found init function __vaDriverInit_0_32
libva info: va_openDriver() returns 0
Setting pipeline to PAUSED ...
Pipeline is PREROLLING ...
Got context from element 'vaapidecode0': gst.vaapi.Display=context, display=(GstVaapiDisplay)NULL;
Got context from element 'sink': gst.gl.GLDisplay=context, gst.gl.GLDisplay=(GstGLDisplay)"\(GstGLDisplayX11\)\ gldisplayx11-0";
/GstPipeline:pipeline0/GstVaapiDecode:vaapidecode0.GstPad:sink: caps = "video/x-h264\,\ stream-format\=\(string\)avc\,\ alignment\=\(string\)au\,\ level\=\(string\)4.1\,\ profile\=\(string\)high\,\ codec_data\=\(buffer\)01640029ffe1001867640029ac34e5014016e84001974ec04c4b4023c60c458001000468eebcb0\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ framerate\=\(fraction\)24000/1001\,\ pixel-aspect-ratio\=\(fraction\)1/1"
/GstPipeline:pipeline0/GstVaapiDecode:vaapidecode0.GstPad:src: caps = "video/x-raw\(meta:GstVideoGLTextureUploadMeta\)\,\ format\=\(string\)RGBA\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ colorimetry\=\(string\)sRGB\,\ framerate\=\(fraction\)24000/1001"
Redistribute latency...
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0.GstGhostPad:sink.GstProxyPad:proxypad0: caps = "video/x-raw\(meta:GstVideoGLTextureUploadMeta\)\,\ format\=\(string\)RGBA\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ colorimetry\=\(string\)sRGB\,\ framerate\=\(fraction\)24000/1001"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLUploadElement:gluploadelement0.GstPad:src: caps = "video/x-raw\(memory:GLMemory\,\ meta:GstVideoOverlayComposition\)\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ framerate\=\(fraction\)24000/1001\,\ format\=\(string\)RGBA\,\ colorimetry\=\(string\)sRGB"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLColorConvertElement:glcolorconvertelement0.GstPad:src: caps = "video/x-raw\(memory:GLMemory\,\ meta:GstVideoOverlayComposition\)\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ framerate\=\(fraction\)24000/1001\,\ format\=\(string\)RGBA\,\ colorimetry\=\(string\)sRGB"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLImageSink:sink.GstPad:sink: caps = "video/x-raw\(memory:GLMemory\,\ meta:GstVideoOverlayComposition\)\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ framerate\=\(fraction\)24000/1001\,\ format\=\(string\)RGBA\,\ colorimetry\=\(string\)sRGB"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLColorConvertElement:glcolorconvertelement0.GstPad:sink: caps = "video/x-raw\(memory:GLMemory\,\ meta:GstVideoOverlayComposition\)\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ framerate\=\(fraction\)24000/1001\,\ format\=\(string\)RGBA\,\ colorimetry\=\(string\)sRGB"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0/GstGLUploadElement:gluploadelement0.GstPad:sink: caps = "video/x-raw\(meta:GstVideoGLTextureUploadMeta\)\,\ format\=\(string\)RGBA\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ colorimetry\=\(string\)sRGB\,\ framerate\=\(fraction\)24000/1001"
/GstPipeline:pipeline0/GstGLImageSinkBin:glimagesinkbin0.GstGhostPad:sink: caps = "video/x-raw\(meta:GstVideoGLTextureUploadMeta\)\,\ format\=\(string\)RGBA\,\ width\=\(int\)1280\,\ height\=\(int\)720\,\ interlace-mode\=\(string\)progressive\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ colorimetry\=\(string\)sRGB\,\ framerate\=\(fraction\)24000/1001"
X Error of failed request:  BadWindow (invalid Window parameter)
  Major opcode of failed request:  15 (X_QueryTree)
  Resource id in failed request:  0x600001
  Serial number of failed request:  45
  Current serial number in output stream:  45



I don't know which one of the bug I am now in :)

Is there any planned release in near future?

Thanks.

That's something related with XWindow, but I don't know what could be the cause. Try to apply the patch in bug 754900 without using the env vars.

There are plans for December or January, but nothing for sure.

Hi Victor,

The result is pretty much the same after applying patch 754900 and running the pipeline wihtout env vars. I guess I have nothing else than waiting for those issues to be fixed.

Thanks
Regards

As this bug is about a crash, I'm setting it as major

The story is more or less like this:

1\ When the pipeline is negotiated, the vaapidecode creates a VA display according to the g_display_map[] in gstvaapipluginutil.c (see comment #1). This is because, in that moment, the vaapidecode element doesn't have any clue about the running environment. Right now, in X11, the first one to try is GLX.

2\ The vaapidecode element creates its internal vaapidecoder object with its current VA display (let's assume it is GLX). The object vaapidecoder is who does the VA decoding. 

3\ When the gst_vaapi_plugin_base_decide_allocation() in gstvaapipluginbase.c is called, the vaapidecode element checks if the allocation meta has a GstGLContext. If the GstGLContext tells that the API or the Platform is different from what the vaapidecoder element has, a new VA display is created and it replaces the old one. In our assumpution, let's thing that the video sink uses EGL. So the GLX display is replaced by the EGL one. *BUT*, the internal vaapidecoder object _is never notified of that new VA display_.

4\ When a VA surfaces is created and exposed as an output GstBuffer, it is done with the VA display form the vaapidecoder object, in our case GLX. So, when the video sink tries to render it through the texture_upload, the functions used are the GLX ones, not the EGL, and everything crashes and burns.

The solution should be twofold:

1\ Notify the vaapidecode element to recreate (reset_full) its internal vaapidecoder object, when the VA display is replaced.

2\ Get, by the GstContext mechanism, an earlier the spread GstGLContext, avoiding future VA display replacement.

I have a patch to fix this... partially: all the pieces required for setep 1\ are there. But it is required to disable vaaapidecodebin.

If we want to use vaapidecodebin, step 2\ shall make it work.

I would prefer a *minimal* fix for this (0.7.0) release to stay in the safe side,  since
-- we planned for major rewrites gl-vaapi stuffs after the release
and
-- expecting 0.7 release by next week

How about lowering the vaapidecoebin ranklike we did for 0.6.0... ?

Created attachment 316415 [details] [review]
Revert "vaapidisplay: mark X11 display as compatible with EGL"

This reverts commit 200b1baabc066f8a4102f82f539655d588200ec9.

The reverted commit only disabled the GLTextureUploadMeta when the sink used
gles2/egl.

We need to enable it and configure the display to use the EGL backend.

Created attachment 316416 [details] [review]
Revert "Revert "Marking rank of vaapidecodebin as GST_RANK_MARGINAL for now.""

This reverts commit 18a8b87975baf7ad1eb6c02a5dde6889269b33fb.

The vaapidecode only can know if the next element has a GstGLContext, but the decodebin
place, whenever it can, the vaapipostproc, disabling the VA display configuring using the
GstGLContext information. This is why we should stop autoplugging vaapidecodebin.

Created attachment 316417 [details] [review]
WIP: vaapidecode: connect display_change() callback

When the base plugin changes the display, the decoder ought reconfigure
itself.

This patch connects the display_change() callback and reconfigures the decoder
accordingly.

ATENTION: this is a work-in-progres. Don't merge it since the decoder is
destroyed after parsing the first frame, losting the parsed information.

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>

With the fix for Bug 767203 and Bug 766206 this issue is fixed too, without the usage of display_change() callback.

This pipeline still crashes:

GST_GL_PLATFORM=egl gst-launch-1.0 filesrc location= ~/patterns/big_buck_bunny_1080p_h264.mov ! decodebin ! glimagesink

Because the decoder is instanciated first and creates it's own GstVaapiDisplay without knowing the render's GL platform.

Some notes that I found.

1) The case that uses decodebin (not playbin) in the pipeline.
- Couldn't get gl context earlier, because glsinkbin is not linked yet.
And then vaapi element gets gl context during preroll, which means that it should change vaapidisplay to get it working on gl.

2) The case that uses playbin.
- playbin handles autoplug-query signal, so vaapi element could get gl context before decoder start. And works fine.
( Note that the issue also happens on playbin3 because handling autoplug-query signal is not implemented yet)

3) Why is glx working even if using decodebin.
Because vaapi glx display is compatible with vaapiX11display.

Then I tried victor's patch for display_change callback, but it's not working for all media because it lost parse information during vaapidisplay change.
Some media is working, but not all as far as I tested.

IHMO, we should find another way rather than changing vaapi display dynamically.
Maybe we can make X11 or Wayland display first(by env variable?) if we don't know about gl thing , then when gl context is found, we can bind egl.

I'll try more :)

(In reply to Hyunjun Ko from comment #27)
> IHMO, we should find another way rather than changing vaapi display
> dynamically.
> Maybe we can make X11 or Wayland display first(by env variable?) if we don't
> know about gl thing , then when gl context is found, we can bind egl.
> 
> I'll try more :)

This requires many changes, even current VaapiDisplay design.
I think we can go forward once bug #768266 done by this way.
Or another solution. :)

Long time!

This time, I would like to propose new design for VaapiDisplay, not only to fix this issue, but also to improve current design, which is based on comment #27.

Current problems.
1. An instance of GstVaapiDisplayGLX/EGL should be created when gl context is found in pipeline, which means should remove the existed instance of Non-GL VaapiDisplay. Yes. That's what this bug says.

2. A bit weird design for GstVaapiDisplayEGL.
 GstVaapiDisplayEGL is a decendent of GstVaapiDisplay, and then has an instance of GstVaapiDisplayWayland/X11 as a private member again. IMHO, This desgin makes people confusing and less readable.

3. Hard to implement handling/drawing on GL/EGL window in vaapisink, which is not implemented properly.
 Currently, vaapisink has property "display", which is to be set specific display platform (glx/egl/drm), but not completed yet. 

So, basic idea of mine is below.
1. Cut the relation between GstVaapi(native)Display and GstVaapiDisplayGLX/EGL.
2. So we could create vaapi display for native display(X11/Wayland) first, then bind GL stuff once gl context is found.

To do this, I create GInterface for GstVaapiDisplayGLX/EGL and GstVaapiDisplay has an instance of this when GL things should be binded so that it could put surface to GL texture if needed.
This can fix this bug and makes handling display flexible, and I guess it could get easier to think about problem 3.

After some experiments, I got positive results.
I'm going to propose the patch, and want to listen to opinions!

Created attachment 338670 [details] [review]
libs: vaapidisplay_gl_iface: implement GstVaapiDisplayGlIface

Create new VaapiDisplay GL interface using GTypeInterface.
This interfaces should be specified by GstVaapiDisplayGLX/EGL.

Created attachment 338671 [details] [review]
vaapidisplay: refactor the current design by using new  GstVaapiDisplayGLIface

Cut the relation between GstVaapi(native)Display and GstVaapiDisplayGLX/EGL.
Then GstVaapiDisplayGLX/EGL use new GstVapiDisplayGLIface and
implements specific operation on its own platform.

So gst-vaapi elements could create vaapi display for native display(X11/Wayland) first, then bind GL stuff once gl context is found.

All these sound cool, though the second patch is huge. I have bad feeling when I have to code review huge patches.

As far as I understand, this patch includes the GstObjectifiation of GstVaapiDisplay (bug #768266) and adds the design change to use ginterfaces.

I would go step by step, it is possible: to the gstobjectification of gstvaapidisplay, and then this design change.

Created attachment 340104 [details] [review]
[PATCH 01/11] libs: display: gl_iface: Implement  GstVaapiDisplayGLIface

Create new VaapiDisplay GL interface using GTypeInterface,
This interface provides creating GL window and texture,
setting/getting GL context/display.

This interfaces should be specified by GstVaapiDisplayGLX/EGL.

Created attachment 340105 [details] [review]
[PATCH 02/11] libs: display: Refactor the current design by using new  GstVaapiDisplayGLIface

Cut the relation between GstVaapi(native)Display and GstVaapiDisplayGLX/EGL.
Then GstVaapiDisplayGLX/EGL implements specific operation on its own platform,
based on new GstVapiDisplayGLIface
  
So gst-vaapi elements could create vaapi display for native
display(X11/Wayland) first, then bind GL stuff once gl context is found.
  
Note that this removes compatiblity between X11 and EGL display type,
which doesn't make sense.

Created attachment 340106 [details] [review]
[PATCH 03/11] libs: display: egl: Implement methods in  GstVaapiDisplayGLIface

Cuts the relation with GstVaapiDisplay and implements egl-specific
methods based on GstVapiDisplayGLIface.

Note that this patch removes gst_vaapi_display_egl_new_with_native_display,
which is unnecessary any more.

Created attachment 340107 [details] [review]
[PATCH 04/11] libs: display: glx: Implement methods in  GstVaapiDisplayGLIface

Cuts the relation with GstVaapiDisplay and implements glx-specific
methods based on GstVapiDisplayGLIface.
  
Note that this patch removes gst_vaapi_display_glx_new_with_display,
which is unnecessary any more.

Created attachment 340108 [details] [review]
[PATCH 05/11] libs: window: glx/egl: changes according to new  GstVaapiDisplayGLIface

This patch modifies something need to be changed due to new
GstVaapiDisplayGLIface.

Note that this starts to use GST_VAAPI_DISPLAY_EGL/GLX_BIND instead of
GST_VAAPI_DISPLAY_EGL/GLX, to verify passed instance.

Created attachment 340109 [details] [review]
[PATCH 06/11] libs: texture: glx/egl: changes according to new  GstVaapiDisplayGLIface

This patch modifies something need to be changed due to new
GstVaapiDisplayGLIface.
  
Note that this starts to use GST_VAAPI_DISPLAY_EGL/GLX_BIND instead of
GST_VAAPI_DISPLAY_EGL/GLX, to verify passed instance.

Created attachment 340110 [details] [review]
[PATCH 07/11] libs: surface: egl: changes according to new  GstVaapiDisplayGLIface

This patch modifies something need to be changed due to new
GstVaapiDisplayGLIface.

Created attachment 340111 [details] [review]
[PATCH 08/11] pluginutil: Use new apis to create VaapiDisplay which  is binded GL stuff

Uses new api gst_vaapi_display_gl_bind to bind GL stuff when gl context is
found instead of creating new display.
  
So gst-vaapi elements could create vaapi display for native
display(X11/Wayland) first, then bind GL stuff.

Created attachment 340112 [details] [review]
[PATCH 09/11] tests: Changes test cases according to new  GstVaapiDisplayGLIface

This patch modifies something need to be changed due to deletion of some
apis and new apis gst_vaapi_display_gl_bind.
  
This also removes unnecessary test cases.

Created attachment 340113 [details] [review]
[PATCH 10/11] libs: object: Remove unnecessary macro method

Created attachment 340114 [details] [review]
[PATCH 11/11] libs: display: x11: Bind GLX as default

This patch avoids regression. (eg. running with cluttervideosink)
The native X11 display will be binded to GLX after creation.

I'm proposing new patch based on master.

Note that patches from 2 to 9 should be together to build successfully.
I just spilited those to review easier.

This is huge.

I'm not saying we don't need to redesign EGL display, but this is too much in my opinion. I don't know, I need more time to think about it.

But, I think we have to improve other issues regarding this bug before all this.

1. Rethink the GstGLDisplay context sharing, we could know the type of GL display much before if we "trap" that context info, hence we won't need to change the va display backend.

2. Instead of a list of try-and-error displays, we should query the environment to know what would be the best va display backend, only in case that we don't receive any context.

It seems that the patch in bug #777409 fixes this issue

(In reply to Víctor Manuel Jáquez Leal from comment #46)
> It seems that the patch in bug #777409 fixes this issue

Yes, I think so.
\o/

Victor,
IMHO, we can close this issue since commits are merged in bug #777409.

I tested on 
 - GLX/EGL on X11/Wayland with glimagesink
 - with cluttersink(only GLX on X11)
 - with kmssink

All test pipelines are working fine.

And probably the commit Revert "vaapidisplay: mark X11 display as compatible with EGL" can be landed.

(In reply to Hyunjun Ko from comment #49)
> And probably the commit Revert "vaapidisplay: mark X11 display as compatible
> with EGL" can be landed.

Yes, it make sense, since it was a workaround to avoid a crash. I'll push it and I'll close this bug.

Comment on attachment 316416 [details] [review]
Revert "Revert "Marking rank of vaapidecodebin as GST_RANK_MARGINAL for now.""

not required anymore

bug 777409 fixed this issue.