GNOME Bugzilla – Bug 760916
gl: implement GstGLMemoryEGL
Last modified: 2016-05-15 08:47:12 UTC
Created attachment 319474 [details] [review] patch Because current GstEGLImageMemory does not inherit GstGLMemory, GLUpload allocates additional GLMemory and upload the decoded contents from the decoder which uses EGLImage (e.g. gst-omx in RPi). This work adds GstGLMemoryEGL to avoid this overhead. Decoders allocate GstGLMemoryEGL and decode its contents to the EGLImage of GstGLMemoryEGL. And GLUpload uses this memory without allocation of additional textures and blit operations.
Review of attachment 319474 [details] [review]: This is precisely where I had planned for this to go. kudos. Do you also have some patches to gst-omx that use the new memory? Then we can remove gsteglimagememory from gst-omx.
I have a patch which modifies to use GLMemoryEGL instead of EGLImageMemory, but I'm thinking to do some more job to supports EGLImageMemory as a fallback. I'll upload it today. (In reply to Matthew Waters (ystreet00) from comment #1) > Review of attachment 319474 [details] [review] [review]: > > This is precisely where I had planned for this to go. kudos. > > Do you also have some patches to gst-omx that use the new memory? Then we > can remove gsteglimagememory from gst-omx.
(In reply to Gwang Yoon Hwang from comment #2) > I have a patch which modifies to use GLMemoryEGL instead of EGLImageMemory, > but I'm thinking to do some more job to supports EGLImageMemory as a > fallback. > I'll upload it today. I don't think it's really necessary to support both. With this new memory, GstEGLImageMemory will disappear.
(In reply to Matthew Waters (ystreet00) from comment #3) > (In reply to Gwang Yoon Hwang from comment #2) > > I have a patch which modifies to use GLMemoryEGL instead of EGLImageMemory, > > but I'm thinking to do some more job to supports EGLImageMemory as a > > fallback. > > I'll upload it today. > > I don't think it's really necessary to support both. With this new memory, > GstEGLImageMemory will disappear. I submit a patch for gst-omx as a user of this memory: https://bugzilla.gnome.org/show_bug.cgi?id=760918
As part of this bug, we should probably also remove EGLImageMemory while we're at it? Let's not carry useless cruft around.
Created attachment 319491 [details] [review] Updated patch Update a patch with a bugzilla number at commit log. For the removing GstEGLImageMemory, I found another user of GstEGLImageMemory. GstGLUpload uses GstEGLImageMemory if the src is a DMABuf. I think it would be better to remove GstEGLImageMemory after implementing GstGLMemoryDMABuf.
(In reply to Gwang Yoon Hwang from comment #6) > Created attachment 319491 [details] [review] > Updated patch > > Update a patch with a bugzilla number at commit log. > > For the removing GstEGLImageMemory, I found another user of > GstEGLImageMemory. > GstGLUpload uses GstEGLImageMemory if the src is a DMABuf. > > I think it would be better to remove GstEGLImageMemory after implementing > GstGLMemoryDMABuf. GstGLMemoryDMABuf makes very little sense, and it does not work the way you seem to think. In DMABuf you need to split the EGLImage from the GL texture. The EGLImage, for performance reason, need to be attached to the original memory containing the DMABuf, so it can be reused later. If needed though, we can simply add a ref-counted wrapper for EGLImage. I would not want to waste a textures and malloc just for having a ref-counted EGLImage.
(In reply to Nicolas Dufresne (stormer) from comment #7) > (In reply to Gwang Yoon Hwang from comment #6) > > Created attachment 319491 [details] [review] > > Updated patch > > > > Update a patch with a bugzilla number at commit log. > > > > For the removing GstEGLImageMemory, I found another user of > > GstEGLImageMemory. > > GstGLUpload uses GstEGLImageMemory if the src is a DMABuf. > > > > I think it would be better to remove GstEGLImageMemory after implementing > > GstGLMemoryDMABuf. > > GstGLMemoryDMABuf makes very little sense, and it does not work the way you > seem to think. In DMABuf you need to split the EGLImage from the GL texture. > The EGLImage, for performance reason, need to be attached to the original > memory containing the DMABuf, so it can be reused later. If needed though, > we can simply add a ref-counted wrapper for EGLImage. I would not want to > waste a textures and malloc just for having a ref-counted EGLImage. Well, GstGLMemoryDMABuf can do it itself, isn't it? We can implement GstGLMemoryDMABuf to allocate EGLImage itself to reference DMABuf memory and create a texture from a EGLImage to use it for GstGLMemory interface.
Let's me clarify how it works and you'll judge by yourself. For dmabuf, there is a buffer that comes into glupload, it's a GstBuffer with GstDMABufMemory inside of it. For each of the DMABuf, we will need to create a EGLImage which wraps the dmabuf FD. This EGLImage wrapper must be reused everything this specific FD comes back. For this reason, we want to attach (I use qdata here), the EGLImage to the GstDMBufMemory. Later, if I receive a dmabuf that always have an EGLImage, we'll reuse it. Because of the memory flushing taking place when an EGLImage is created, this is extremely important. On the src pad side of glupload, we will need to create buffer that contains memory with GL texture that are binded to the EGLImage. We don't need to keep a reference to the EGLImage, the specification defines the GL will take care or referencing the EGLImage. At this point, the notion of DMABuf is completly gone. That's why an GstGLMemoryDMABuf seems rather useless to me. We would have two owners of the EGLImage and could require wasting an FD by doing a dup3() on the dmabuf FD. Or, we'd have a GstGLMemoryDMABuf that does not know about dmabufs. The true fact is very little difference between an GstGLMemoryEGL (an EGL backed texture) whether it was used by omx decoder on the Pi, or backed by en EGLImage. The difference reside on who you create the EGLImage, the rest is abstracted by the concept of EGLImage exactly so you can share the same code. You'll notice this point has been missed a lot in glupload as we have a lot of duplicated EGLImage code.
Just double checking, but for dmabuf, the lifetime requirements are as follows: dmabuf -> EGLimage: requires the dmabuf to keep the EGLImage alive. EGLImage -> GL texture: can discard the EGLImage (for 2D textures). For external-oes textures from an EGLImage, I believe there are extra synchronisation requirements that require us to keep a reference to the EGLImage so that it doesn't get reused too early. Is the only problem that gsteglimagememory/gstglmemoryegl does not have a way to wrap existing EGLImage's? If so, that's easy to add. As you say it may need to be a refcounted EGLImage reference but still, dead simple to add. For GstGLMemoryEGL, it would need to optionally take a EGLImage to create the GL texture from and then discard it (or not) if that's allowed/not required anymore. That is entirely possible now with the GstGLVideoAllocationParams and you have a GL texture bufferpool for free! The exact semantics would have to be defined for GstGLMemoryEGL but it is entirely possible. If it ever makes sense to combine all three handles (dmabuf, EGLImage, GL texture) then a GstGLMemoryDMABUF would make sense (but I don't believe it does currently).
(In reply to Matthew Waters (ystreet00) from comment #10) > Just double checking, but for dmabuf, the lifetime requirements are as > follows: > > dmabuf -> EGLimage: requires the dmabuf to keep the EGLImage alive. > EGLImage -> GL texture: can discard the EGLImage (for 2D textures). For > external-oes textures from an EGLImage, I believe there are extra > synchronisation requirements that require us to keep a reference to the > EGLImage so that it doesn't get reused too early. Seems right, except that there is no external eos texture in those two use cases (but that may exist on Android). For the dmabuf case, we use GstParentBufferMeta with the buffer containing dmabuf to solve this one (the EGLImage is just a wrapper really, what you do with it is not so important, as long as you manage to reuse the same EGLImage for the same FD for performance reason). For the OMX case, nothing to be done I believe, in this new version, glupload will passthrough like it did a long time ago. If you transform something with an asynchronous PBO though, you should figure-out a way to keep a reference on the original, regardless if it's another texture or an EGLImage binded texture (if it's not already the case I mean). > > Is the only problem that gsteglimagememory/gstglmemoryegl does not have a > way to wrap existing EGLImage's? If so, that's easy to add. As you say it > may need to be a refcounted EGLImage reference but still, dead simple to add. Indeed. > > For GstGLMemoryEGL, it would need to optionally take a EGLImage to create > the GL texture from and then discard it (or not) if that's allowed/not > required anymore. That is entirely possible now with the > GstGLVideoAllocationParams and you have a GL texture bufferpool for free! > The exact semantics would have to be defined for GstGLMemoryEGL but it is > entirely possible. > > If it ever makes sense to combine all three handles (dmabuf, EGLImage, GL > texture) then a GstGLMemoryDMABUF would make sense (but I don't believe it > does currently). Nod.
Review of attachment 319491 [details] [review]: ::: gst-libs/gst/gl/egl/gstglmemoryegl.c @@ +67,3 @@ +{ + g_return_val_if_fail (gst_is_gl_memory_egl (GST_MEMORY_CAST (mem)), NULL); + return GST_GL_CONTEXT_EGL(_gl_mem_get_parent(mem))->egl_display; I see warnings (GLib-GObject-WARNING **: invalid uninstantiatable type '<unknown>' in cast to 'GstGLContextEGL') because get_parent() returns NULL. Not sure why, might be worth debugging :)
(In reply to Philippe Normand from comment #12) > Review of attachment 319491 [details] [review] [review]: > > ::: gst-libs/gst/gl/egl/gstglmemoryegl.c > @@ +67,3 @@ > +{ > + g_return_val_if_fail (gst_is_gl_memory_egl (GST_MEMORY_CAST (mem)), NULL); > + return GST_GL_CONTEXT_EGL(_gl_mem_get_parent(mem))->egl_display; > > I see warnings (GLib-GObject-WARNING **: invalid uninstantiatable type > '<unknown>' in cast to 'GstGLContextEGL') because get_parent() returns NULL. > Well, not NULL but the mem itself
Comment on attachment 319474 [details] [review] patch As far as I understand this patch has been updated, so I mark it as obsolete.
Created attachment 327260 [details] [review] gl: implement GstGLMemoryEGL Slightly modified patch running gst-indent, removing the critical mentioned and only adding the glmemoryegl allocator in glupload when we have an EGL context.
Created attachment 327261 [details] [review] gl: replace GstEGLImageMemory with an EGLImage wrapper Implements an EGLImage wrapper and uses it in the dmabuf uploader along with the GstGLMemoryEGL.
commit 5498e97a11c2627af22560cdc949d8f95883210e Author: Matthew Waters <matthew@centricular.com> Date: Wed May 4 12:17:59 2016 +1000 gl/egl: replace gsteglimagememory with an EGLImage wrapper That can be passed to GstGLMemoryEGL. This also ports the dmabuf uploader to GstEGLImage and GstGLMemoryEGL. commit c83fd26c80a9637803476f6605a4dae97e2fd529 Author: Gwang Yoon Hwang <yoon@igalia.com> Date: Thu Jan 21 22:18:17 2016 +0900 gl: implement GstGLMemoryEGL Because current GstEGLImageMemory does not inherit GstGLMemory, GLUpload allocates additional GLMemory and upload the decoded contents from the decoder which uses EGLImage (e.g. gst-omx in RPi). This work adds GstGLMemoryEGL to avoid this overhead. Decoders allocate GstGLMemoryEGL and decode its contents to the EGLImage of GstGLMemoryEGL. And GLUpload uses this memory without allocation of additional textures and blit operations. [Matthew Waters]: gst-indent the sources and fix a critical retreiving the egl display from the memory. https://bugzilla.gnome.org/show_bug.cgi?id=760916
*** Bug 753886 has been marked as a duplicate of this bug. ***