[Bug 779067] New: glupload: Add support for Vivante DirectTexture uploads

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Bug ID: 779067
Summary: glupload: Add support for Vivante DirectTexture
uploads
Classification: Platform
Product: GStreamer
Version: unspecified
OS: All
Status: NEW
Severity: normal
Priority: Normal
Component: gst-plugins-bad
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@coaxion.net
QA Contact: gstreamer-***@lists.freedesktop.org
GNOME version: ---

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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

--- Comment #2 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Created attachment 346441
--> https://bugzilla.gnome.org/attachment.cgi?id=346441&action=edit
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 <***@freescale.com>
from
https://github.com/Freescale/meta-freescale/blob/master/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

--- Comment #1 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Created attachment 346440
--> https://bugzilla.gnome.org/attachment.cgi?id=346440&action=edit
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@coaxion.net

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346441|0 |1
is obsolete| |

--- Comment #3 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Created attachment 346442
--> https://bugzilla.gnome.org/attachment.cgi?id=346442&action=edit
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 <***@freescale.com>
from
https://github.com/Freescale/meta-freescale/blob/master/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

--- Comment #4 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to Sebastian Dröge (slomo) from comment #3)

Post by "GStreamer" (GNOME Bugzilla)
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

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346440|none |reviewed
status| |

--- Comment #5 from Matthew Waters (ystreet00) <***@gmail.com> ---
Review of attachment 346440:
--> (https://bugzilla.gnome.org/review?bug=779067&attachment=346440)

Looks mostly ok to me.

::: gst-libs/gst/allocators/Makefile.am
@@ +23,3 @@
+gir_cincludes=$(patsubst
%,--c-include='gst/badallocators/%',$(***@GST_API_VERSION@_include_HEADERS))
+
+GstAllocators-@***@.gir: $(INTROSPECTION_SCANNER)
libgstbadallocators-@***@.la

Doesn't this conflict with the GstAllocators-1.0 gir in -base?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346442|none |needs-work
status| |

--- Comment #6 from Matthew Waters (ystreet00) <***@gmail.com> ---
Review of attachment 346442:
--> (https://bugzilla.gnome.org/review?bug=779067&attachment=346442)

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-@***@.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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346440|reviewed |needs-work
status| |

--- Comment #7 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Comment on attachment 346440
--> https://bugzilla.gnome.org/attachment.cgi?id=346440
allocators: Add GstPhysMemory abstraction

(In reply to Matthew Waters (ystreet00) from comment #5)

Post by "GStreamer" (GNOME Bugzilla)
Doesn't this conflict with the GstAllocators-1.0 gir in -base?

Yes, thanks!

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

--- Comment #8 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to Matthew Waters (ystreet00) from comment #6)

Post by "GStreamer" (GNOME Bugzilla)
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 :)

Post by "GStreamer" (GNOME Bugzilla)
::: 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?

Post by "GStreamer" (GNOME Bugzilla)
::: gst-libs/gst/gl/Makefile.am
@@ +147,3 @@
$(GST_ALL_LDFLAGS) \
+ $(GST_LT_LDFLAGS) \
+
$(top_builddir)/gst-libs/gst/allocators/libgstbadallocators-
@***@.la
Don't .la libraries go in _LIBADD?

Yes

Post by "GStreamer" (GNOME Bugzilla)
::: 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.

Post by "GStreamer" (GNOME Bugzilla)
@@ +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?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

--- Comment #9 from Matthew Waters (ystreet00) <***@gmail.com> ---
(In reply to Sebastian Dröge (slomo) from comment #8)

Post by "GStreamer" (GNOME Bugzilla)
(In reply to Matthew Waters (ystreet00) from comment #6)

Post by "GStreamer" (GNOME Bugzilla)
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.

Post by "GStreamer" (GNOME Bugzilla)

Post by "GStreamer" (GNOME Bugzilla)
::: 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.

Post by "GStreamer" (GNOME Bugzilla)

Post by "GStreamer" (GNOME Bugzilla)
::: 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.

Post by "GStreamer" (GNOME Bugzilla)

Post by "GStreamer" (GNOME Bugzilla)
@@ +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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

--- Comment #10 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to Matthew Waters (ystreet00) from comment #9)

Post by "GStreamer" (GNOME Bugzilla)

Post by "GStreamer" (GNOME Bugzilla)

Post by "GStreamer" (GNOME Bugzilla)
@@ +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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Severity|normal |enhancement

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346442|needs-work |none
status| |
Attachment #346442|0 |1
is obsolete| |

--- Comment #12 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Created attachment 346587
--> https://bugzilla.gnome.org/attachment.cgi?id=346587&action=edit
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 <***@freescale.com>
from
https://github.com/Freescale/meta-freescale/blob/master/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346440|needs-work |none
status| |
Attachment #346440|0 |1
is obsolete| |

--- Comment #11 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Created attachment 346586
--> https://bugzilla.gnome.org/attachment.cgi?id=346586&action=edit
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com,
| |***@ndufresne.ca

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346586|none |needs-work
status| |

--- Comment #13 from Matthew Waters (ystreet00) <***@gmail.com> ---
Review of attachment 346586:
--> (https://bugzilla.gnome.org/review?bug=779067&attachment=346586)

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? ;)

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346587|none |accepted-commit_now
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

--- Comment #14 from Sebastian Dröge (slomo) <***@coaxion.net> ---
(In reply to Matthew Waters (ystreet00) from comment #13)

Post by "GStreamer" (GNOME Bugzilla)
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

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346586|needs-work |none
status| |
Attachment #346586|0 |1
is obsolete| |

--- Comment #15 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Created attachment 346643
--> https://bugzilla.gnome.org/attachment.cgi?id=346643&action=edit
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

--- Comment #16 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Created attachment 346644
--> https://bugzilla.gnome.org/attachment.cgi?id=346644&action=edit
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.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346587|accepted-commit_now |none
status| |
Attachment #346587|0 |1
is obsolete| |

--- Comment #17 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Created attachment 346645
--> https://bugzilla.gnome.org/attachment.cgi?id=346645&action=edit
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 <***@freescale.com>
from
https://github.com/Freescale/meta-freescale/blob/master/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346643|0 |1
is obsolete| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346644|none |accepted-commit_now
status| |

--- Comment #18 from Matthew Waters (ystreet00) <***@gmail.com> ---
Review of attachment 346644:
--> (https://bugzilla.gnome.org/review?bug=779067&attachment=346644)

Looks good

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Matthew Waters (ystreet00) <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346645|none |accepted-commit_now
status| |

--- Comment #19 from Matthew Waters (ystreet00) <***@gmail.com> ---
Review of attachment 346645:
--> (https://bugzilla.gnome.org/review?bug=779067&attachment=346645)

Looks good

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

--- Comment #20 from Sebastian Dröge (slomo) <***@coaxion.net> ---
Attachment 346644 pushed as 5cdf3a3 - allocators: Add GstPhysMemoryAllocator
abstraction
Attachment 346645 pushed as ed1e4c1 - glupload: Add support for Vivante
DirectTexture uploads

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346645|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #346644|accepted-commit_now |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=779067

Sebastian Dröge (slomo) <***@coaxion.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|git master |1.11.3