GNOME Bugzilla – Bug 779067
glupload: Add support for Vivante DirectTexture uploads
Last modified: 2017-02-27 08:43:35 UTC
See commit messages. This comes with a little library for sharing the memory with gstreamer-imx and similar code that shares a "physical memory location" with us somehow.
Created attachment 346440 [details] [review] allocators: Add GstPhysMemory abstraction This can be used in a generic way as a base class by all platforms that, in one way or another, pass around physical memory addresses.
Created attachment 346441 [details] [review] glupload: Add support for Vivante DirectTexture uploads Together with the upcoming gstreamer-imx patch, this allows zerocopy between imxvpudec and other elements and glimagesink. This is losely based on a patch by Haihua Hu <b55597@freescale.com> from https://github.com/Freescale/meta-freescale/blob/master/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/
Created attachment 346442 [details] [review] glupload: Add support for Vivante DirectTexture uploads Together with the upcoming gstreamer-imx patch, this allows zerocopy between imxvpudec and other elements and glimagesink. This is losely based on a patch by Haihua Hu <b55597@freescale.com> from https://github.com/Freescale/meta-freescale/blob/master/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/
(In reply to Sebastian Dröge (slomo) from comment #3) > Together with the upcoming gstreamer-imx patch, this allows zerocopy > between imxvpudec and other elements and glimagesink. https://github.com/Freescale/gstreamer-imx/pull/142
Review of attachment 346440 [details] [review]: Looks mostly ok to me. ::: gst-libs/gst/allocators/Makefile.am @@ +23,3 @@ +gir_cincludes=$(patsubst %,--c-include='gst/badallocators/%',$(libgstbadallocators_@GST_API_VERSION@_include_HEADERS)) + +GstAllocators-@GST_API_VERSION@.gir: $(INTROSPECTION_SCANNER) libgstbadallocators-@GST_API_VERSION@.la Doesn't this conflict with the GstAllocators-1.0 gir in -base?
Review of attachment 346442 [details] [review]: An open question is how do we know the GstPhysMemory is usable by the Vivante extension? ::: configure.ac @@ +892,3 @@ +dnl check for Vivante DirectVIV support +AC_CHECK_LIB(GLESv2, glTexDirectVIV, [HAVE_VIV_DIRECTVIV=yes], [HAVE_VIV_DIRECTVIV=no]) I think this is the wrong thing to be checking here as this needs to be checked at runtime by the GL function loading infrastructure. Do we even need to check for vivante specific things here? ::: gst-libs/gst/gl/Makefile.am @@ +147,3 @@ $(GST_ALL_LDFLAGS) \ + $(GST_LT_LDFLAGS) \ + $(top_builddir)/gst-libs/gst/allocators/libgstbadallocators-@GST_API_VERSION@.la Don't .la libraries go in _LIBADD? ::: gst-libs/gst/gl/gstglupload.c @@ +39,3 @@ +#if GST_GL_HAVE_VIV_DIRECTVIV +#include <gst/allocators/gstphysmemory.h> +#include <GLES2/gl2ext.h> This include should be in gstglapi.h @@ +1395,3 @@ + gl_format, (void **) &unmap_data->map.data, + &((GstPhysMemory *) in_mem)->phys_addr); + glTexDirectInvalidateVIV (GL_TEXTURE_2D); These should use the functions in GstGLFuncs in order to use the correct GL functions for the correct context/thread. Which results in, even if GST_GL_HAVE_VIV_DIRECTVIV is true (which I don't think we need at all), glTexDirectVIVMap and glTexDirectInvalidateVIV may not exist at runtime and should be a condition for choosing to upload with this uploader.
Comment on attachment 346440 [details] [review] allocators: Add GstPhysMemory abstraction (In reply to Matthew Waters (ystreet00) from comment #5) > Doesn't this conflict with the GstAllocators-1.0 gir in -base? Yes, thanks!
(In reply to Matthew Waters (ystreet00) from comment #6) > Review of attachment 346442 [details] [review] [review]: > > An open question is how do we know the GstPhysMemory is usable by the > Vivante extension? If you have hardware with that, you'll have that function in your GL library. And on that hardware the only kind of "physical memory" that will be available is one that can be used here :) > ::: configure.ac > @@ +892,3 @@ > > +dnl check for Vivante DirectVIV support > +AC_CHECK_LIB(GLESv2, glTexDirectVIV, [HAVE_VIV_DIRECTVIV=yes], > [HAVE_VIV_DIRECTVIV=no]) > > I think this is the wrong thing to be checking here as this needs to be > checked at runtime by the GL function loading infrastructure. > > Do we even need to check for vivante specific things here? If we check at runtime, then we would always compile it in although it's useless almost everywhere else. How would we include it in the function loading infrastructure, other than adding more functions to the GL prototypes? We could probably load it dynamically when the uploader is created? > ::: gst-libs/gst/gl/Makefile.am > @@ +147,3 @@ > $(GST_ALL_LDFLAGS) \ > + $(GST_LT_LDFLAGS) \ > + > $(top_builddir)/gst-libs/gst/allocators/libgstbadallocators- > @GST_API_VERSION@.la > > Don't .la libraries go in _LIBADD? Yes > ::: gst-libs/gst/gl/gstglupload.c > @@ +39,3 @@ > +#if GST_GL_HAVE_VIV_DIRECTVIV > +#include <gst/allocators/gstphysmemory.h> > +#include <GLES2/gl2ext.h> > > This include should be in gstglapi.h The problem is that gstglapi.h will include GLES3/*.h if available instead... and those don't declare the functions for whatever reason although they are available on work. Not a problem if we dynamically load them though. > @@ +1395,3 @@ > + gl_format, (void **) &unmap_data->map.data, > + &((GstPhysMemory *) in_mem)->phys_addr); > + glTexDirectInvalidateVIV (GL_TEXTURE_2D); > > These should use the functions in GstGLFuncs in order to use the correct GL > functions for the correct context/thread. > > Which results in, even if GST_GL_HAVE_VIV_DIRECTVIV is true (which I don't > think we need at all), glTexDirectVIVMap and glTexDirectInvalidateVIV may > not exist at runtime and should be a condition for choosing to upload with > this uploader. So would you suggest to a) check at configure if the symbol is there to decide whether to compile in that codepath or not, and b) load the symbol dynamically when the uploader is created?
(In reply to Sebastian Dröge (slomo) from comment #8) > (In reply to Matthew Waters (ystreet00) from comment #6) > > Review of attachment 346442 [details] [review] [review] [review]: > > > > An open question is how do we know the GstPhysMemory is usable by the > > Vivante extension? > > If you have hardware with that, you'll have that function in your GL > library. And on that hardware the only kind of "physical memory" that will > be available is one that can be used here :) As long as this is always true, it should be fine. > > ::: configure.ac > > @@ +892,3 @@ > > > > +dnl check for Vivante DirectVIV support > > +AC_CHECK_LIB(GLESv2, glTexDirectVIV, [HAVE_VIV_DIRECTVIV=yes], > > [HAVE_VIV_DIRECTVIV=no]) > > > > I think this is the wrong thing to be checking here as this needs to be > > checked at runtime by the GL function loading infrastructure. > > > > Do we even need to check for vivante specific things here? > > If we check at runtime, then we would always compile it in although it's > useless almost everywhere else. > > How would we include it in the function loading infrastructure, other than > adding more functions to the GL prototypes? We could probably load it > dynamically when the uploader is created? Yes, those are the two options :). Pick one, I don't mind which. > > ::: gst-libs/gst/gl/gstglupload.c > > @@ +39,3 @@ > > +#if GST_GL_HAVE_VIV_DIRECTVIV > > +#include <gst/allocators/gstphysmemory.h> > > +#include <GLES2/gl2ext.h> > > > > This include should be in gstglapi.h > > The problem is that gstglapi.h will include GLES3/*.h if available > instead... and those don't declare the functions for whatever reason > although they are available on work. Not a problem if we dynamically load > them though. It seems that GLES2/gl2ext.h vs GLES3/*.h is another header minefield as things that are in GLES2 aren't exposed in GLES3 headers. *sigh*. I'm not sure what to do here exactly but this is probably for another bug. > > @@ +1395,3 @@ > > + gl_format, (void **) &unmap_data->map.data, > > + &((GstPhysMemory *) in_mem)->phys_addr); > > + glTexDirectInvalidateVIV (GL_TEXTURE_2D); > > > > These should use the functions in GstGLFuncs in order to use the correct GL > > functions for the correct context/thread. > > > > Which results in, even if GST_GL_HAVE_VIV_DIRECTVIV is true (which I don't > > think we need at all), glTexDirectVIVMap and glTexDirectInvalidateVIV may > > not exist at runtime and should be a condition for choosing to upload with > > this uploader. > > So would you suggest to a) check at configure if the symbol is there to > decide whether to compile in that codepath or not, and b) load the symbol > dynamically when the uploader is created? Yes, exactly.
(In reply to Matthew Waters (ystreet00) from comment #9) > > > @@ +1395,3 @@ > > > + gl_format, (void **) &unmap_data->map.data, > > > + &((GstPhysMemory *) in_mem)->phys_addr); > > > + glTexDirectInvalidateVIV (GL_TEXTURE_2D); > > > > > > These should use the functions in GstGLFuncs in order to use the correct GL > > > functions for the correct context/thread. > > > > > > Which results in, even if GST_GL_HAVE_VIV_DIRECTVIV is true (which I don't > > > think we need at all), glTexDirectVIVMap and glTexDirectInvalidateVIV may > > > not exist at runtime and should be a condition for choosing to upload with > > > this uploader. > > > > So would you suggest to a) check at configure if the symbol is there to > > decide whether to compile in that codepath or not, and b) load the symbol > > dynamically when the uploader is created? > > Yes, exactly. Ok, will do, thanks for the review :) And the header mess is not really relevant anymore then as I don't need the declaration from the header but can just declare the function types locally then as function pointers.
Created attachment 346586 [details] [review] allocators: Add GstPhysMemory abstraction This can be used in a generic way as a base class by all platforms that, in one way or another, pass around physical memory addresses.
Created attachment 346587 [details] [review] glupload: Add support for Vivante DirectTexture uploads Together with the upcoming gstreamer-imx patch, this allows zerocopy between imxvpudec and other elements and glimagesink. This is losely based on a patch by Haihua Hu <b55597@freescale.com> from https://github.com/Freescale/meta-freescale/blob/master/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/
Review of attachment 346586 [details] [review]: No that I think about it, maybe the address retrieval should be in the allocator interface and not have a base memory struct containing a single guintptr. Rationale is that another memory type may want to provide a physical address but may have another memory type that it needs to provide be it's parent memory. Essentially allowing polymorphism of memory implementations. ::: gst-libs/gst/allocators/gstphysmemory.c @@ +35,3 @@ + * gst_is_phys_memory: + * @mem:a #GstMemory + *• What is this character? ;)
(In reply to Matthew Waters (ystreet00) from comment #13) > Review of attachment 346586 [details] [review] [review]: > > No that I think about it, maybe the address retrieval should be in the > allocator interface and not have a base memory struct containing a single > guintptr. > > Rationale is that another memory type may want to provide a physical address > but may have another memory type that it needs to provide be it's parent > memory. Essentially allowing polymorphism of memory implementations. That's a very good idea, thanks! It will be slower though, but that should be fine here
Created attachment 346643 [details] [review] allocators: Add GstPhysMemory abstraction This can be used in a generic way as a base class by all platforms that, in one way or another, pass around physical memory addresses.
Created attachment 346644 [details] [review] allocators: Add GstPhysMemoryAllocator abstraction This can be used in a generic way as common interface by all platforms that, in one way or another, pass around physical memory addresses.
Created attachment 346645 [details] [review] glupload: Add support for Vivante DirectTexture uploads Together with the upcoming gstreamer-imx patch, this allows zerocopy between imxvpudec and other elements and glimagesink. This is losely based on a patch by Haihua Hu <b55597@freescale.com> from https://github.com/Freescale/meta-freescale/blob/master/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/
Review of attachment 346644 [details] [review]: Looks good
Review of attachment 346645 [details] [review]: Looks good
Attachment 346644 [details] pushed as 5cdf3a3 - allocators: Add GstPhysMemoryAllocator abstraction Attachment 346645 [details] pushed as ed1e4c1 - glupload: Add support for Vivante DirectTexture uploads