[Bug 693826] add dmabuf support in -good

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=693826
  GStreamer | gst-plugins-good | 1.0.5

Sebastian Dröge <slomo> changed:

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

--- Comment #12 from Sebastian Dröge <slomo at circular-chaos.org> 2013-02-16 11:14:54 UTC ---
Review of attachment 236253:
 --> (https://bugzilla.gnome.org/review?bug=693826&attachment=236253)

Please create the next patch against gst-plugins-base, adding a new library
called libgstallocators :)

::: configure.ac
@@ +1066,3 @@
 gst/deinterlace/Makefile
 gst/debugutils/Makefile
+gst/dmabuf/Makefile

Please check somehow if dmabuf is supported on this system before building that
part

::: gst/dmabuf/gstdmabuf.c
@@ +66,3 @@
+    g_warning ("Freeing memory still mapped");
+
+  close (dbmem->fd);

The documentation of gst_dmabuf_allocator_alloc() should mention that the
GstMemory takes ownership of the fd and closes it after usage

@@ +156,3 @@
+  GST_DEBUG ("%p: copy %" G_GSSIZE_FORMAT " %" G_GSIZE_FORMAT, mem, offset,
+      size);
+  return gst_dmabuf_allocator_alloc (mem->mem.allocator, dup (mem->fd), size);

I guess dup() can fail. You should catch that here and return NULL then

@@ +203,3 @@
+  static GstAllocator *dmabuf_allocator;
+
+  if (g_once_init_enter (&dmabuf_allocator)) {

This is not how g_once_init_{enter,leave}() should be used. It's for
initializing a gsize-sized variable, not a pointer (which can be of different
sizes). Better use GOnce here

@@ +214,3 @@
+
+GstAllocator *
+gst_dmabuf_allocator_new (void)

Should be called gst_dmabuf_allocator_obtain(). It's a singleton, no new
instance is returned here
Also add documentation

@@ +226,3 @@
+
+GstMemory *
+gst_dmabuf_allocator_alloc (GstAllocator * allocator, gint fd, gsize size)

Documentation.
Is a dmabuf always completely specified with the fd and size? No flags or
anything else, like for specifying a read-only dmabuf?

@@ +263,3 @@
+ */
+gint
+gst_dmabuf_memory_get_fd (GstMemory * mem)

Do a g_return_val_if_fail(gst_is_dmabuf_memory(mem), -1) here. It should IMHO
be the developers task to make sure it actually is dmabuf memory

@@ +282,3 @@
+{
+  gboolean ret = GST_IS_DMABUF_ALLOCATOR (mem->allocator);
+  ret &= g_strcmp0 (mem->allocator->mem_type, ALLOCATOR_NAME) == 0;

Only check the mem_type here, no the allocator. There could in theory be other
allocators in the future that return dmabuf memory (and for them we would need
to make part of the internal structs here public).

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.