GNOME Bugzilla – Bug 774518
gl/egl: gstglcontext_egl.h removal fallout
Last modified: 2016-12-01 11:55:40 UTC
Right now, in gstreamer-vaapi, we are working on enabling dmabuf to downstream when it is possible. One way to do it is to querying the GstGL context in order to know if it supports the extension EGL_EXT_image_dma_buf_import: https://github.com/CapOM/gstreamer-vaapi/commit/5f0ac0fbb2bcf497e1182288b92d65b21e9b5b51 But now, that GstGLContextEGL is hidden, it is not possible to do this anymore. That's why we're requesting the revert of that commit in GstGL.
Use gst_gl_context_check_feature() for gl extension checking.
Matthew, I am sub-classing it: https://github.com/Samsung/ChromiumGStreamerBackend/blob/master/content/media/gstreamer/gpuprocess/gstglcontext_gpu_process.h#L52 Please revert!
Why? What does it give you that gstglcontext doesn't?
eglCreateImage
gst_gl_context_get_proc_address()
I cannot call gst_gl_context_egl_create_context because it does not have any sense when using a client/IPC gl API. So I feed these manually: See https://github.com/Samsung/ChromiumGStreamerBackend/blob/master/content/media/gstreamer/gpuprocess/gstglcontext_gpu_process.c . Please do not close it again until we find a consensus. It is a blocker for me at the moment.
Sure, and you can still feed them manually however you like. That doesn't force you to use gstglcontextegl as you can already override the relevant details of gstglcontext for this so the question still stands, why do you need gstglcontextegl and what doesn't gstglcontext provide that you need?
Thx. Sorry I do not understand :) How can I feed _GstGLContextEGL.egl_exts / .eglDestroyImage from GstGLContext only ?
You shouldn't need to. Those are internal details. If you need to, that's a bug elsewhere.
Created attachment 340010 [details] [review] gl/egl: check the feature in the extensions list
(In reply to Matthew Waters (ystreet00) from comment #9) > You shouldn't need to. Those are internal details. If you need to, that's > a bug elsewhere. In that case we need a common interface between GstGLContext and GstGLContextEGL for gst_egl_image_from_dmabuf::ctx_egl->eglCreateImageKHR call. Or split gstglcontext_egl.c::gst_gl_context_egl_create_context since it calls eglInitialize, eglGetDisplay, eglQueryString, to move egl->eglCreateImage feeding in another function that will be call generically from GstGLContext. Indeed I have no control on the init, I mean it is already done, I basically can only change the get_proc_addr.
gst_egl_image_from_dmabuf should use gst_gl_context_get_proc_address() then.
(In reply to Matthew Waters (ystreet00) from comment #12) > gst_egl_image_from_dmabuf should use gst_gl_context_get_proc_address() then. Good idea in general. But it will return null for me, also since it is done internally no need to change that part. In my chromium gst backend I do not call ctx_egl->eglCreateImageKHR neither, this is not the problem :) This is really how the gpu process works, I think you see it like a real gl api with shared libraries and everything. This is not the case. It is an IPC and only for GL, not for EGL. So I implemented IPC for eglCreateImage/Destroy but it does not make sense for eglGetDisplay/eglInitialize, also because it is not in the form ctx_egl->Initialize for example. It means that gst_gl_context_egl_create_context cannot be called for the client ipc that talks to the gpu process. I need to split gst_gl_context_egl_create_context to extract the egl->eglCreateImage feeding block. Do you think I can move it to gst_gl_context_egl_get_proc_address ? making sure it is called (maybe by doing the change you suggested in gst_egl_image_from_dmabuf ) I will still need to solve ->egl_exts, ->gl_api, ->egl_major, ->egl_minor though.
- egl_exts, you can implement yourself and has a function to check gl extensions with the above patch (and is overridable from the subclass). - gl_api, you can implement yourself and has a an overridable accessor. egl_major/minor, may need new API. whatever you're doing now with gst_gl_context_get_proc_address() will still continue to work and can be overridden. We just need to make sure that nobody uses the private GstGLContextEGL::egl*Image* functions anymore.
(In reply to Matthew Waters (ystreet00) from comment #14) > - egl_exts, you can implement yourself and has a function to check gl > extensions with the above patch (and is overridable from the subclass). > - gl_api, you can implement yourself and has a an overridable accessor. > egl_major/minor, may need new API. > > whatever you're doing now with gst_gl_context_get_proc_address() will still > continue to work and can be overridden. We just need to make sure that > nobody uses the private GstGLContextEGL::egl*Image* functions anymore. Thx for the suggestion. As you said I will need to change GstGL. I plan to do it next month. It will happen after the move to -base unless you plan to do the move later.
So... Can we close this bug after I push the proposed patch? Also, I would rename it to "expose egl extensions"
(In reply to Víctor Manuel Jáquez Leal from comment #16) > So... Can we close this bug after I push the proposed patch? > > Also, I would rename it to "expose egl extensions" If you rename it then yes :)
Attachment 340010 [details] pushed as 9ca4c8e - gl/egl: check the feature in the extensions list
Julien, this should get you 90% of where you need to be. commit 8c2118823b3d42840cc6f48cbdc0e1b342f90b80 Author: Matthew Waters <matthew@centricular.com> Date: Thu Nov 17 02:41:14 2016 +1100 gl/egl: remove EGLImage functions from egl context By adding the necessary GstEGLImage entry points to create a GstEGLImage from a GstGLMemory. https://bugzilla.gnome.org/show_bug.cgi?id=774518 commit f2e9190229094cd5b329dfb12aa8cd6468c19cbc Author: Matthew Waters <matthew@centricular.com> Date: Thu Nov 17 01:45:38 2016 +1100 glcontext: add vfunc to retrieve the OpenGL platform version i.e. the version of EGL, GLX, etc implemented. https://bugzilla.gnome.org/show_bug.cgi?id=774518 commit d42145e8c1ba3bc0445506b92bb7ac04ae98f4dd Author: Matthew Waters <matthew@centricular.com> Date: Thu Nov 17 01:38:32 2016 +1100 gl/egl: move get_error_string() into gstegl So others can use it without #include-ing a private header https://bugzilla.gnome.org/show_bug.cgi?id=774518 commit cdaada43009a008f1ba44a010f02ba005898b7da Author: Matthew Waters <matthew@centricular.com> Date: Thu Nov 27 15:50:04 2014 +1100 display/egl: implement getting the EGLDisplay of a specific platform https://bugzilla.gnome.org/show_bug.cgi?id=774518
(In reply to Matthew Waters (ystreet00) from comment #19) > Julien, this should get you 90% of where you need to be. Fantastic, thx!
On RPi these changes broke gst-gl (here at least): eglCreateImageKHR: failed to create image for buffer 0x7 target 12465 error 0x300c 0:00:00.623488333 4191 0x72504b80 ERROR glbasememory gstglbasememory.c:94:_mem_create_gl: Failed to create GL buffer: Failed to create EGLImage 0:00:00.623633281 4191 0x71b03a60 ERROR glbasememory gstglbasememory.c:168:gst_gl_base_memory_init: Could not create GL buffer with context:0x71a0d488 ** (WPEWebProcess:4191): CRITICAL **: gst_egl_image_get_image: assertion 'GST_IS_EGL_IMAGE (image)' failed eglCreateImageKHR: failed to create image for buffer 0x8 target 12465 error 0x300c 0:00:00.628313593 4191 0x72504b80 ERROR glbasememory gstglbasememory.c:94:_mem_create_gl: Failed to create GL buffer: Failed to create EGLImage 0:00:00.628405364 4191 0x71b03a60 ERROR glbasememory gstglbasememory.c:168:gst_gl_base_memory_init: Could not create GL buffer with context:0x71a0d488 ** (WPEWebProcess:4191): CRITICAL **: gst_egl_image_get_image: assertion 'GST_IS_EGL_IMAGE (image)' failed eglCreateImageKHR: failed to create image for buffer 0x9 target 12465 error 0x300c 0:00:00.630130676 4191 0x72504b80 ERROR glbasememory gstglbasememory.c:94:_mem_create_gl: Failed to create GL buffer: Failed to create EGLImage 0:00:00.630210104 4191 0x71b03a60 ERROR glbasememory gstglbasememory.c:168:gst_gl_base_memory_init: Could not create GL buffer with context:0x71a0d488 ** (WPEWebProcess:4191): CRITICAL **: gst_egl_image_get_image: assertion 'GST_IS_EGL_IMAGE (image)' failed eglCreateImageKHR: failed to create image for buffer 0xa target 12465 error 0x300c 0:00:00.631863020 4191 0x72504b80 ERROR glbasememory gstglbasememory.c:94:_mem_create_gl: Failed to create GL buffer: Failed to create EGLImage 0:00:00.631935156 4191 0x71b03a60 ERROR glbasememory gstglbasememory.c:168:gst_gl_base_memory_init: Could not create GL buffer with context:0x71a0d488 ** (WPEWebProcess:4191): CRITICAL **: gst_egl_image_get_image: assertion 'GST_IS_EGL_IMAGE (image)' failed 0:00:00.634486458 4191 0x71b03a60 ERROR omx gstomx.c:2048:gst_omx_port_populate_unlocked:<omxh264dec-omxh264dec0> Failed to pass buffer 0x6ee00e38 ((nil)) to egl_render port 221: Incorrect state operation (0x80001018) Reverting 8c2118823b3d42840cc6f48cbdc0e1b342f90b80 makes those errors disappear.
Created attachment 341074 [details] [review] gstglcontext: is_shared should return FALSE if no group
Comment on attachment 341074 [details] [review] gstglcontext: is_shared should return FALSE if no group (Please open a new bug for further issues)
Created attachment 341127 [details] [review] gstglupload: relax EGL context check Sorry I am using the same bug because this is really related to the gstglcontext_egl.h removal from public. And I had not time to check last time we closed that bug. Also this is the last problem I found. If I found other later I will open a new bug for sure. Thx for the reviews!
Comment on attachment 341074 [details] [review] gstglcontext: is_shared should return FALSE if no group commit 119955bea4acd1660b6a2403c956b686ac0d2d14 Author: Julien Isorce <j.isorce@samsung.com> Date: Wed Nov 30 09:22:17 2016 +0000 gstglcontext: is_shared should return FALSE if no group If a sub class of GstGLContext does not create a group then it currently crashes: 0 g_atomic_int_get (&share->refcount) 1 _context_share_group_is_shared (context->priv->sharegroup) 2 gst_gl_context_is_shared 3 _default_set_sync_gl https://bugzilla.gnome.org/show_bug.cgi?id=774518
Comment on attachment 341127 [details] [review] gstglupload: relax EGL context check commit f7b039e7df3bdcd950c65a74284f3735096d0ba1 Author: Julien Isorce <j.isorce@samsung.com> Date: Thu Dec 1 09:24:18 2016 +0000 gstglupload: relax EGL context check Check for GST_GL_PLATFORM_EGL enum instead of type GstGLContextEGL. https://bugzilla.gnome.org/show_bug.cgi?id=774518