[Bug 743345] glupload: Add support for dmabuf

[GStreamer (GNOME Bugzilla)]

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

Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> changed:

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

--- Comment #78 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> ---
Review of attachment 316599:
 --> (https://bugzilla.gnome.org/review?bug=743345&attachment=316599)

::: gst-libs/gst/gl/egl/gsteglimagememory.c
@@ +327,3 @@
+ */
+static int
+_drm_fourcc_from_info (GstVideoInfo * info)

The mapping are really weird and does not work for Big Endian. I believe we
could make it better by assuming a direct mapping for 32bits RGB formats. This
way the converter will passthrough for those formats.

Another way, similar to what you did, would have a fallback to importing
everything as RGBA/RGBx (DRM_FORMAT_ABGR8888/XBGR8888 in little endian). The
last would at least ensure that our shaders matches. This mostly seems like
what you have done, except some weirdness described lower.

@@ +337,3 @@
+      return DRM_FORMAT_RGB565;
+    case GST_VIDEO_FORMAT_RGBA:
+      return DRM_FORMAT_ARGB8888;

That's odd, RGBA should be 1-1 match with BGRA. I believe this should cause the
red and blue to be swapped.

@@ +343,3 @@
+      return DRM_FORMAT_XBGR8888;
+    case GST_VIDEO_FORMAT_xRGB:
+      return DRM_FORMAT_ABGR8888;

This is really weird, why use an alpha channel enabled format for the opaque
format, and use opaque for for BGRA ? We never know if the sampler will
optimize out the alpha on certain target. I think the best it so stick with
ABGR for everything, our shaders do set the alpha to opaque already.

@@ +345,3 @@
+      return DRM_FORMAT_ABGR8888;
+    case GST_VIDEO_FORMAT_GRAY8:
+      return fourcc_code ('R', '8', ' ', ' ');

Isn't there any defined for those ?

@@ +348,3 @@
+    case GST_VIDEO_FORMAT_YUY2:
+    case GST_VIDEO_FORMAT_UYVY:
+      return fourcc_code ('G', 'R', '8', '8');

Same, note, you have can use at the start a define like:

#ifndef DRM_FORMAT_GR88
#define ...
#endif

To avoid having to ifdef the code.

@@ +448,3 @@
+
+  for (int i = 0; i < meta->n_planes; i++)
+    GST_DEBUG ("meta: offset[%d]: %ld, stride[%d]: %d",

I had a warning on 32bit platform with that %ld. I believe this is a gsize, for
which you should use %" G_GSIZE_FORMAT "...

@@ +462,3 @@
+    out_buffer = gst_buffer_new ();
+    gst_buffer_insert_memory (out_buffer, 0, (GstMemory *) gl_mem);
+  } else if (n_mem == 1 && meta->n_planes > 1) {

Instead of forcing having 1 n_mem, you could use the same technique as in
map_default found in GstVideoMeta. This way you can support both single and
multiple memory cases without having two code path.

::: gst-libs/gst/gl/egl/gsteglimagememory.h
@@ +66,3 @@

+#if GST_GL_HAVE_DMABUF
+GstBuffer *

Remove newline here.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.