GNOME Bugzilla – Bug 760918
omxvideodec : Use gstglmemoryegl for the RPi
Last modified: 2016-05-04 07:25:47 UTC
Created attachment 319477 [details] [review] patch Modified to use gstglmemoryegl to avoid texture creation/copy operations at the glupload.
As discussed in bug #760916, it would most likely make sense to remove EGLImageMemory... which means this patch would have to be updated.
Generally looks good but is this going to work right? Won't it make omxviddec accept all kinds of GL memory... while actually it will not work good at all on anything but EGLImages?
(In reply to Sebastian Dröge (slomo) from comment #2) > Generally looks good but is this going to work right? Won't it make Yes, :) > omxviddec accept all kinds of GL memory... while actually it will not work > good at all on anything but EGLImages? It checks the allocator's name to ensure whether is it a GLMemoryEGL or not before using it as a memory backend. Default fallback is a SystemMemory, so there is no change for the worst case scenario. I'll update #760916 with a clean up to remove EGLImageMemory soon
Review of attachment 319477 [details] [review]: Don't we need to keep a different caps feature to make this work ? glupload can then convert to GLMemory afterward.
(In reply to Nicolas Dufresne (stormer) from comment #4) > Review of attachment 319477 [details] [review] [review]: > > Don't we need to keep a different caps feature to make this work ? glupload > can then convert to GLMemory afterward. In ideal case, What I want to do is enabling passthrough at the glupload in this use case. Before this patch, glupload allocates additional GLMemory which has a gltexture, which is already created by EGLImageMemory. It is a waste of memory and memory bandwidth. Another nice point of this approach is, glupload don't have to create it's buffer pool because it will share same buffer with OMX video decoder. I'm not sure about a way to implement "converting", but it would means we need to make a additional memory to wrap and refs EGLImage, and synchronize its buffer pool with OMX's buffer, isn't it?
(In reply to Gwang Yoon Hwang from comment #5) > (In reply to Nicolas Dufresne (stormer) from comment #4) > > Review of attachment 319477 [details] [review] [review] [review]: > > > > Don't we need to keep a different caps feature to make this work ? glupload > > can then convert to GLMemory afterward. > > In ideal case, What I want to do is enabling passthrough at the glupload in > this use case. > > Before this patch, glupload allocates additional GLMemory which has a > gltexture, > which is already created by EGLImageMemory. > It is a waste of memory and memory bandwidth. Totally agree, it's not just a waste, it also kills the performance. > > Another nice point of this approach is, glupload don't have to create it's > buffer pool because it will share same buffer with OMX video decoder. > I'm not sure about a way to implement "converting", but it would means we > need to make a additional memory to wrap and refs EGLImage, and synchronize > its buffer pool with OMX's buffer, isn't it? We need buffer pools for the raw upload and dmabuf upload (at least). For the OMX case, we don't you are right, we can just pass-through the read-only EGLImage binded textures. In the original design, the sink was allocation a pool with EGLImage in it. This pool was placed in the propose allocation. This way, the omxdecoder would know if it should operated in system memory or using EGL acceleration. How do you signal this now without an EGL specific caps feature ?
(In reply to Nicolas Dufresne (stormer) from comment #6) > (In reply to Gwang Yoon Hwang from comment #5) > > (In reply to Nicolas Dufresne (stormer) from comment #4) > > > > Another nice point of this approach is, glupload don't have to create it's > > buffer pool because it will share same buffer with OMX video decoder. > > I'm not sure about a way to implement "converting", but it would means we > > need to make a additional memory to wrap and refs EGLImage, and synchronize > > its buffer pool with OMX's buffer, isn't it? > > We need buffer pools for the raw upload and dmabuf upload (at least). For > the OMX case, we don't you are right, we can just pass-through the read-only > EGLImage binded textures. > > In the original design, the sink was allocation a pool with EGLImage in it. > This pool was placed in the propose allocation. This way, the omxdecoder > would know if it should operated in system memory or using EGL acceleration. > How do you signal this now without an EGL specific caps feature ? I'm not sure I understand it correctly, but the omxvideodecoder can check the memory type whether it has an EGL backend or not via: checking the proposed pool's allocator with GST_IS_GL_MEMORY_EGL_ALLOCATOR or checking the memory type via gst_memory_is_type, isn't it enough?
(In reply to Nicolas Dufresne (stormer) from comment #4) > Review of attachment 319477 [details] [review] [review]: > > Don't we need to keep a different caps feature to make this work ? glupload > can then convert to GLMemory afterward. Technically, we don't need an extra caps feature for this specific case as it's either GLMemoryEGL (which is compatible with the GLMemory interface) or SystemMemory. However, I would prefer adding the caps feature as the allocation query may not always succeed (e.g. initial playbin/decodebin negotiation). I would suggest the following rules for omxviddec to choose the the GL allocator. GLMemoryEGL - require the correct allocator in the allocation query/pool GLMemory - use the egl allocator regardless glupload would just passthrough everything when converting from GLMemoryEGL to GLMemory. This may require some basetransform work to actually passthrough the e.g. allocation query correctly.
Created attachment 327262 [details] [review] omxvideodec: Use gstglmemoryegl for the RPi minor update to gst-indent the sources and port to the newest series in bug 760916.
commit 27d2cdd45dcf0bbd58c753e4b33058607153c4c1 Author: Gwang Yoon Hwang <yoon@igalia.com> Date: Wed Jan 20 03:10:38 2016 +0900 omxvideodec : Use gstglmemoryegl for the RPi Modified to use gstglmemoryegl to avoid texture creation/copy operations at the glupload. [Matthew Waters]: gst-indent the sources and port testegl to GstGLMemoryEGL https://bugzilla.gnome.org/show_bug.cgi?id=760918