[Bug 784847] omxvideodec: add dmabuf support for zynqultrascaleplus

[GStreamer (GNOME Bugzilla)]

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

--- Comment #3 from Julien Isorce <julien.isorce at gmail.com> ---
Review of attachment 355431:
 --> (https://bugzilla.gnome.org/review?bug=784847&attachment=355431)

Looks clean ! I wrote a few questions.

::: configure.ac
@@ +159,1 @@


Like this instead:
https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/configure.ac#n837 ?

::: omx/gstomxbufferpool.c
@@ +27,3 @@
 #include "gstomxbufferpool.h"

+#include <gst/allocators/gstdmabuf.h>

Like this
https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/gl/gstglupload.c#n36
instead ?

@@ +395,3 @@
+      gint fd;
+
+      fd = GPOINTER_TO_INT (omx_buf->omx_buf->pBuffer);

I see that you do not see the memory:DMABuf caps features, so does it mean that
you assume this fd is always mappable ? Like if you do a gst_memory_map(mem,
FLAG_READ) will it succeed ? Same question with flag READWRITE ?

@@ +518,2 @@
     /* If it's our own memory we have to set the sizes */
+    if (!pool->other_pool && !pool->dmabuf) {

Maybe adjust the comment above. Like "... set the sizes except for dmabuf
because ..."

::: omx/gstomxvideodec.c
@@ +199,3 @@
   self->dec_out_port = gst_omx_component_add_port (self->dec, out_port_index);

+#ifdef USE_OMX_TARGET_ZYNQ_USCALE_PLUS

&& defined(OMX_ALG_BUF_DMA) ? Was it defined in older zyq-uscale versions ?
Just asking maybe you do not want to support older versions ?

@@ +217,3 @@
+      GST_ERROR_OBJECT (self, "Failed to set output buffer mode: %s (0x%08x)",
+          gst_omx_error_to_string (err), err);
+    buffer_mode.eMode = OMX_ALG_BUF_DMA;

Just to make, this is something you always want ? I.e. no fallback if
setparam(do_dmabuf) fails ?
I was thinking to just self->dmabuf = FALSE instead of return FALSE, something
like that. And GST_INFO instead of GST_ERROR. But this is your decision,
depends what you want.

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