GNOME Bugzilla – Bug 725227
gstegl / eglglessink: EGLImage improvements
Last modified: 2014-03-24 12:43:32 UTC
Created attachment 270398 [details] [review] draft patch for A Now that https://bugzilla.gnome.org/show_bug.cgi?id=706054 has been done, there are still some improvements we can do before to move to gst-plugins-gl (and even before other improvements https://bugzilla.gnome.org/show_bug.cgi?id=703343) The improvements we can do in egllib / eglglessink are: -A: refactor gst_egl_image_allocator_alloc_eglimage ( http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/eglgles/gstegladaptation_egl.c#n426 ) Some part of the function should be moved from eglglessink to gstegl lib. Draft patch will follow :p -B: attach a GstVideoGLTextureUploadMeta to each buffer. The meta callback will call everything around glEGLImageTargetTexture2DOES (i.e. http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/eglgles/gsteglglessink.c#n1369). So that lines 1369 etc will be removed and line 1360 will be used instead. A will allow to instantiate the GstEGLImageBufferPool in omxvideodec (through gst_egl_image_buffer_pool_new (NULL, NULL, NULL), see draft patch for A) or instantiate GstEGLImageBufferPool in our external video sink directly. (ex: webkitvideosink) B will allow to use this code path http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp#L329.
Created attachment 270880 [details] [review] eglglessink: extract GL calls from gst_egl_image_allocator_alloc_eglimage
Created attachment 270881 [details] [review] eglglessink: move gst_egl_image_allocator_alloc_eglimage to gstegl lib
Created attachment 270898 [details] [review] gstegl: add support for GstVideoGLTextureUploadMeta
Created attachment 270904 [details] [review] gstegl: add support for GstVideoGLTextureUploadMeta Just changed a comment
Created attachment 270907 [details] [review] eglglessink: move gst_gl_generate_texture to gsteglglessink.c
Waiting for review before to go further :)
And I suggest to create a gsteglgles2-1.0 lib (header gst/eglgles2/eglgles2.h) in gst-plugins-bad to put "gst_gl_generate_texture" and "gst_eglimage_to_gl_texture_upload_meta". So that we can use them elsewhere (i.e. webkitgtk) and merged later with gst-plugins-gl.
Why not just working on integrating it into gst-plugins-gl instead of adding yet another library to -bad that is going to be removed soon? :)
(In reply to comment #8) > Why not just working on integrating it into gst-plugins-gl instead of adding > yet another library to -bad that is going to be removed soon? :) I suggested it to avoid making eglglessink depends on gst-plugins-gl but I'm actually not against that :)
Well, and merge eglglessink into glimagesink :)
Created attachment 271457 [details] [review] egl: release EGLImage pb also present in master before the GstEGLImageBufferPool had been moved to egl lib
Comment on attachment 271457 [details] [review] egl: release EGLImage Sorry my patch was totally wrong. Should have been in free instead of release. And it would have been also wrong because the allocator is attached to the memory so this is automatically called when the buffer is freed. Note that there is another problem: eglDestroyImageKHR must be called in the gl thread so we need to do something like in alloc_buffer with the blocking func.
(In reply to comment #12) > > Note that there is another problem: eglDestroyImageKHR must be called in the gl > thread so we need to do something like in alloc_buffer with the blocking func. Really? That seems completely suboptimal for the use case of EGLImage...
(In reply to comment #13) > (In reply to comment #12) > > > > Note that there is another problem: eglDestroyImageKHR must be called in the gl > > thread so we need to do something like in alloc_buffer with the blocking func. > > Really? That seems completely suboptimal for the use case of EGLImage... eglDestroyImageKHR is now called since this patch https://bugzilla.gnome.org/show_bug.cgi?id=726337 (when using eglglessink with omxvideodec). Then I checked the return value, it was ok and no egl error so it seems to be ok but not after some time: Indeed, thanks to Josep who added drain support http://cgit.freedesktop.org/~adn770/gst-omx/commit/?h=drain&id=9e06a95be205d6bdaa7f12280ab336dbea6fdaf1, using an adaptive stream it's possible to have the use case where there are several resolution changes in a row. I remember there were still errors around eglimage after the 4 or 5th change. So I guessed it was like gltextures that need to be deleted in the gl thread. But you make me doubt about eglimage :). I thought it only depends if EGLimage has been created with GL_NO_CONTEXT or not. But maybe the errors are related to other stuffs. I have to check. Anyway I'll do a unit test like creating / deleting lot of large EGLimages using GstEGLImageBufferPool and see if there are any error / leak, when both deleting in a different thread or in the glthread.
Also when deleting an EGLImage created with a gltexture as the input EGLClientBuffer (http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/eglgles/gstegladaptation_egl.c#n499) the gltexture needs also to be deleted (http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/ext/eglgles/gstegladaptation_egl.c#n419). This call is here: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/egl/egl.c#n157 So it has to been done in the gl thread (or having the gl context current in that thread just during the delete could be another solution) like here http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglutils.c#n144
I close this bug as I end up with a problem in the gst_eglimage_memory. It does not have a ref on the gl context. Only the display. And to handle a lot of scenario it needs to make current the gl context in order to delete the gl sources of eglimages. Because of not holding a ref on the gl context, in scenarios like with omxvideodec, the glcontext get deleted before the decoder release the eglimages. So I decided to move directly to this bug https://bugzilla.gnome.org/show_bug.cgi?id=703343 where the goal is to add EGLImage support directly to gstgl.