GNOME Bugzilla – Bug 795391
vaapi: problems when playing with glimagesink with egl
Last modified: 2018-06-14 02:29:56 UTC
Here are problems I found. $ gst-play-1.0 a.mp4 b.mp4 c.mp4 --videosink=glimagesink 1. Crash on Wayland. Not 100% but a crash is very likely to happen when jumping to next media. 2. Doesn't display anything from the 2nd media. You can see this issue on X11/EGL and Wayland(if the 1st one is fixed). If you can't reproduce these issues, please let me know because it means that my environment is wrong.
Confirmed. There's a problem when dmabuf is negotiated. Perhaps we're not resetting something properly. The crash I got in wayland (weston) is:
+ Trace 238570
(In reply to Víctor Manuel Jáquez Leal from comment #1) > Confirmed. > > There's a problem when dmabuf is negotiated. Perhaps we're not resetting > something properly. > > The crash I got in wayland (weston) is: Thanks for confirmation. I have a patch for the crash, and now I'm thinking about the solution for the 2nd problem. I'm going to propose some patches.
BTW, I guess this is a cause of the issue https://bugs.webkit.org/show_bug.cgi?id=184574 But I'm not sure yet.
(In reply to Víctor Manuel Jáquez Leal from comment #1) > Confirmed. > > There's a problem when dmabuf is negotiated. Perhaps we're not resetting > something properly. > > The crash I got in wayland (weston) is: Weird. That's different from mine. Mine is the following:
+ Trace 238580
Created attachment 371313 [details] [review] libs: display: avoid VA initialization twice Since the commit dcf135e2 landed, it tries to initalize VA display twice in case of creating GstVaapiDisplayEGL. This is fine on X11 at least but leads to crash in the driver on Wayland.
(In reply to Hyunjun Ko from comment #5) > Created attachment 371313 [details] [review] [review] > libs: display: avoid VA initialization twice > > Since the commit dcf135e2 landed, it tries to initalize VA display > twice in case of creating GstVaapiDisplayEGL. > > This is fine on X11 at least but leads to crash in the driver on Wayland. I would rather to resurrect the parenthood logic (and document it). It is clearer than the proposed semantics in this patch.
Created attachment 371315 [details] [review] RFC: display: egl: fix to create VaapiDisplayEGL with native EGL display and call it when creation gst_vaapi_display_egl_new_with_native_display has been broken since no one uses it. Currently it needs to call this api to create display with provided EGL display so that it could avoid duplicated calls for the native display (eg. eglTerminate).
The 2nd problems happens because gst-vaapi calls eglTerminate even though the egl display is still used in gst-gl. I'm requesting comments for the patch so that we could find better solution.
Created attachment 371316 [details] [review] libs: utils: egl: mark is_wraaped TRUE if the context is wrapped
Review of attachment 371316 [details] [review]: lgtm we should also backport it to 1.14 and 1.12
Review of attachment 371315 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapidisplay_egl.c @@ +51,3 @@ guint display_type; guint gles_version; + gboolean is_native; why not just add gpointer egl_display instead of juggling with different semantics in one single variable? @@ +117,3 @@ #endif #if USE_WAYLAND + if (!native_vaapi_display) we could use params->display_type to choose what vaapi display instantiates, besides the compilation dependencies ::: gst/vaapi/gstvaapipluginutil.c @@ +211,3 @@ + GstGLDisplayEGL *egl_display; + + egl_display = gst_gl_display_egl_from_gl_display (gl_display); why not call here too gst_gl_display_get_handle() thus you could get a common guintptr variable, which could be null by default. @@ +259,3 @@ + + if (gl_display) + gst_object_unref (gl_display); I don't see the reason for this unref since gl_display was unrefed already.
Review of attachment 371313 [details] [review]: I'm going to mark this as rejected, because it's just a proof-of-concept.
(In reply to Víctor Manuel Jáquez Leal from comment #6) > (In reply to Hyunjun Ko from comment #5) > > Created attachment 371313 [details] [review] [review] [review] > > libs: display: avoid VA initialization twice > > > > Since the commit dcf135e2 landed, it tries to initalize VA display > > twice in case of creating GstVaapiDisplayEGL. > > > > This is fine on X11 at least but leads to crash in the driver on Wayland. > > I would rather to resurrect the parenthood logic (and document it). It is > clearer than the proposed semantics in this patch. I don't understand what you mean. The parent thing is related to display-cache, which we removed. See commit ec3e10f. Which one do you want to resurrect?
Review of attachment 371315 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapidisplay_egl.c @@ +51,3 @@ guint display_type; guint gles_version; + gboolean is_native; Well, that's what I thought for a while. Adding another gpointer is fine to me, too. @@ +118,3 @@ #if USE_WAYLAND + if (!native_vaapi_display) + native_vaapi_display = gst_vaapi_display_wayland_new (NULL); This is not about this patch, isn't it? Anyway it still needs "#if USE_WAYLAND" since the header is included under the macro. ::: gst/vaapi/gstvaapipluginutil.c @@ +211,3 @@ + GstGLDisplayEGL *egl_display; + + egl_display = gst_gl_display_egl_from_gl_display (gl_display); Ah i didn't realize it. Thanks. @@ +259,3 @@ + + if (gl_display) + gst_object_unref (gl_display); That unref of gl_display is removed in this patch :)
(In reply to Hyunjun Ko from comment #14) > ::: gst/vaapi/gstvaapipluginutil.c > @@ +211,3 @@ > + GstGLDisplayEGL *egl_display; > + > + egl_display = gst_gl_display_egl_from_gl_display (gl_display); > > Ah i didn't realize it. Thanks. > Hmm, Victor, as far as I understand, it has to get GLDisplayEGL so that it retrieve EGLDisplay finally from it. Could you explain what you mean in detail please?
Created attachment 371373 [details] [review] libs: egl: utils: fix usage of GstGL macros Include gl.h for the required GstGL symbols.
Created attachment 371389 [details] [review] libs: egl: utils: fix usage of GstGL macros Include gl.h for the required GstGL symbols.
Created attachment 371390 [details] [review] libs: egl: utils: mark context as wrapped when it is The returning egl context may be null, so we should check the return value.
Created attachment 371391 [details] [review] meson: fix USE_GLES_VERSION_MASK 1. The macro in the code is USE_GLES_VERSION_MASK 2. glesv3 is provided by glesv2 pkg-config, then it's required to check headers
Attachment 371389 [details] pushed as 9fde93f - libs: egl: utils: fix usage of GstGL macros Attachment 371390 [details] pushed as 4af46f0 - libs: egl: utils: mark context as wrapped when it is Attachment 371391 [details] pushed as 785efdb - meson: fix USE_GLES_VERSION_MASK
branch 1.14 * 56b63720 meson: fix USE_GLES_VERSION_MASK * 5de03007 libs: egl: utils: mark context as wrapped when it is * b50e4225 libs: egl: utils: fix usage of GstGL macros
branch 1.12 * cf67d6d5 meson: fix USE_GLES_VERSION_MASK * 1660bd0d libs: egl: utils: mark context as wrapped when it is
Review of attachment 371315 [details] [review]: By the way, this patch will need to be rebased after bug 793643 is merged ::: gst-libs/gst/vaapi/gstvaapidisplay_egl.c @@ +118,3 @@ #if USE_WAYLAND + if (!native_vaapi_display) + native_vaapi_display = gst_vaapi_display_wayland_new (NULL); yes, we still need the macro. Just we could create efficiently the internal display by identifying the environment instead of try-and-error
(In reply to Víctor Manuel Jáquez Leal from comment #23) > Review of attachment 371315 [details] [review] [review]: > > By the way, this patch will need to be rebased after bug 793643 is merged > Okay, I'll be working on it after those patches are merged
Created attachment 371465 [details] [review] plugins: handle EGL when creating VAAPI display from gl
(In reply to Víctor Manuel Jáquez Leal from comment #25) > Created attachment 371465 [details] [review] [review] > plugins: handle EGL when creating VAAPI display from gl I cook this patch to create a EGL VAAPI display when the gl display is EGL. But it brings problems when playing a second video, as those expressed here.
(In reply to Víctor Manuel Jáquez Leal from comment #23) > Review of attachment 371315 [details] [review] [review]: > > By the way, this patch will need to be rebased after bug 793643 is merged > > ::: gst-libs/gst/vaapi/gstvaapidisplay_egl.c > @@ +118,3 @@ > #if USE_WAYLAND > + if (!native_vaapi_display) > + native_vaapi_display = gst_vaapi_display_wayland_new (NULL); > > yes, we still need the macro. Just we could create efficiently the internal > display by identifying the environment instead of try-and-error I see, but in case of GST_VAAPI_DISPLAY_TYPE_EGL, we have to do try-and-error.
(In reply to Hyunjun Ko from comment #27) > (In reply to Víctor Manuel Jáquez Leal from comment #23) > > Review of attachment 371315 [details] [review] [review] [review]: > > > > By the way, this patch will need to be rebased after bug 793643 is merged > > > > ::: gst-libs/gst/vaapi/gstvaapidisplay_egl.c > > @@ +118,3 @@ > > #if USE_WAYLAND > > + if (!native_vaapi_display) > > + native_vaapi_display = gst_vaapi_display_wayland_new (NULL); > > > > yes, we still need the macro. Just we could create efficiently the internal > > display by identifying the environment instead of try-and-error > > I see, but in case of GST_VAAPI_DISPLAY_TYPE_EGL, we have to do > try-and-error. Agree. That's the case with attachment 371465 [details] [review]
(In reply to Víctor Manuel Jáquez Leal from comment #11) > > ::: gst/vaapi/gstvaapipluginutil.c > @@ +211,3 @@ > + GstGLDisplayEGL *egl_display; > + > + egl_display = gst_gl_display_egl_from_gl_display (gl_display); > > why not call here too gst_gl_display_get_handle() thus you could get a > common guintptr variable, which could be null by default. > There are different cases. If there's a gldisplaywayland in glcontextegl, in this case, we should call gst_gl_display_egl_from_gl_display to retrieve EGLDisplay. (This is the case that's using gst-play or totem.) But if gldisplayegl in glcontextegl, we can use just gst_gl_display_get_handle. (This is the case that's in WebkitGTK.)
(In reply to Hyunjun Ko from comment #29) > (In reply to Víctor Manuel Jáquez Leal from comment #11) > > > > ::: gst/vaapi/gstvaapipluginutil.c > > @@ +211,3 @@ > > + GstGLDisplayEGL *egl_display; > > + > > + egl_display = gst_gl_display_egl_from_gl_display (gl_display); > > > > why not call here too gst_gl_display_get_handle() thus you could get a > > common guintptr variable, which could be null by default. > > > > There are different cases. > If there's a gldisplaywayland in glcontextegl, in this case, we should call > gst_gl_display_egl_from_gl_display to retrieve EGLDisplay. > (This is the case that's using gst-play or totem.) > > But if gldisplayegl in glcontextegl, we can use just > gst_gl_display_get_handle. > (This is the case that's in WebkitGTK.) You can see my patch in attachment 371465 [details] [review] and use the variable display_type to decide which API call use.
(In reply to Víctor Manuel Jáquez Leal from comment #30) > (In reply to Hyunjun Ko from comment #29) > > (In reply to Víctor Manuel Jáquez Leal from comment #11) > > > > > > ::: gst/vaapi/gstvaapipluginutil.c > > > @@ +211,3 @@ > > > + GstGLDisplayEGL *egl_display; > > > + > > > + egl_display = gst_gl_display_egl_from_gl_display (gl_display); > > > > > > why not call here too gst_gl_display_get_handle() thus you could get a > > > common guintptr variable, which could be null by default. > > > > > > > There are different cases. > > If there's a gldisplaywayland in glcontextegl, in this case, we should call > > gst_gl_display_egl_from_gl_display to retrieve EGLDisplay. > > (This is the case that's using gst-play or totem.) > > > > But if gldisplayegl in glcontextegl, we can use just > > gst_gl_display_get_handle. > > (This is the case that's in WebkitGTK.) > > You can see my patch in attachment 371465 [details] [review] [review] and use the > variable display_type to decide which API call use. hmm.. that wouldn't be simple imo. (just using gst_gl_display_egl_from_gl_display VS using 2 apis by conditional statement)
Comment on attachment 371315 [details] [review] RFC: display: egl: fix to create VaapiDisplayEGL with native EGL display and call it when creation ping? @hyunjun: what's the status of this patch?
(In reply to Víctor Manuel Jáquez Leal from comment #32) > Comment on attachment 371315 [details] [review] [review] > RFC: display: egl: fix to create VaapiDisplayEGL with native EGL display and > call it when creation > > ping? > > @hyunjun: what's the status of this patch? I have no plan on this for now. As we discussed before, it needs refactroing, doesn't it? If so, you'd better do that than me.
(In reply to Hyunjun Ko from comment #33) > (In reply to Víctor Manuel Jáquez Leal from comment #32) > > Comment on attachment 371315 [details] [review] [review] [review] > > RFC: display: egl: fix to create VaapiDisplayEGL with native EGL display and > > call it when creation > > > > ping? > > > > @hyunjun: what's the status of this patch? > > I have no plan on this for now. > As we discussed before, it needs refactroing, doesn't it? > If so, you'd better do that than me. Ah you talked about the 2nd patch. Sorry about misunderstanding. For the second patch, you'd better do that too imo.:) BTW, the crash still happens as I told you before, we need to focus on it because it's crash. You couldn't really reproduce the crash any more? Please 20 times next/prev on gst-play.
Regarding the problem #1, I proposed patches on the bug #796470, which is about a bit of refactoring GstVaapiDisplay.
Created attachment 372503 [details] [review] display: egl: fix to create VaapiDisplayEGL with native EGL display gst_vaapi_display_egl_new_with_native_display has been broken since no one uses it. Currently it needs to call this api to create display with provided EGL display so that it could avoid duplicated calls for the native display (eg. eglTerminate). ----------------------------------- I rebased on the patches on bug #796470.
I have pushed these commit in branch 1.14 * fdf7f65c libs: display: resurrect parent private memember * 55a14e39 libs: display: egl: initialize params structure * fa54262a plugins: handle EGL when creating VAAPI display from gl * 67d386d9 display: egl: fix to create VaapiDisplayEGL with native EGL display @hyunjun commit 67d386d9 is a modified version of yours. Also, I suspect we really need to resurrect the parent member in private structure (commit fdf7f65c), because of the locks to the wrapped vaDisplay.
Reopen to fix this issue in master
Created attachment 372637 [details] [review] display: egl: create VaapiDisplayEGL with native EGL display gst_vaapi_display_egl_new_with_native_display() has been broken since it wasn't used. Currently it's needed to call this API to create a display providing the EGL display, so it could avoid duplicated calls to the native display (eg. eglTerminate). Signed-off-by: Victor Jaquez <vjaquez@igalia.com>
Created attachment 372638 [details] [review] plugins: handle EGL when creating VAAPI display from gl
Created attachment 372639 [details] [review] libs: display: egl: initialize params structure Statically initialise the internal params structure.
Created attachment 372640 [details] [review] libs: display: resurrect parent private member This is, practically, a revert of commit dcf135e2. The parent logic is useful for the EGL display, which is a decorator of the real windowing subsystem (X11 or Wayland). Thus it is avoided calling vaInitialize() and vaTerminate() twice.
Created attachment 372653 [details] [review] display: egl: create VaapiDisplayEGL with native EGL display gst_vaapi_display_egl_new_with_native_display() has been broken since it wasn't used. Currently it's needed to call this API to create a display providing the EGL display, so it could avoid duplicated calls to the native display (eg. eglTerminate). Signed-off-by: Victor Jaquez <vjaquez@igalia.com>
Created attachment 372654 [details] [review] plugins: handle EGL when creating VAAPI display from gl If GstGL reports a EGL platform force to create a EGL display using the native EGL display.
Created attachment 372655 [details] [review] libs: display: egl: initialize params structure Statically initialise the internal params structure.
Created attachment 372656 [details] [review] libs: display: resurrect parent private member This is, practically, a revert of commit dcf135e2. The parent logic is useful for the EGL display, which is a decorator of the real windowing subsystem (X11 or Wayland). Thus it is avoided calling vaInitialize() and vaTerminate() twice.
Attachment 372653 [details] pushed as 50470ae - display: egl: create VaapiDisplayEGL with native EGL display Attachment 372654 [details] pushed as e62f3d7 - plugins: handle EGL when creating VAAPI display from gl Attachment 372655 [details] pushed as ad974b4 - libs: display: egl: initialize params structure Attachment 372656 [details] pushed as b0bebeb - libs: display: resurrect parent private member
I didn't focus on this issue enough recently, sorry. And thank you for your work with much better patches.