[Bug 706083] v4l2src: UVC Allocated buffers wrapped in GstBuffer get orphaned by GstBuffer API

[GStreamer (bugzilla.gnome.org)]

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

Olivier Crete (Tester) <olivier.crete> changed:

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

--- Comment #20 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-09-19 19:15:54 UTC ---
Review of attachment 255308:
 --> (https://bugzilla.gnome.org/review?bug=706083&attachment=255308)

::: sys/v4l2/gstv4l2bufferpool.c
@@ +681,2 @@
   index = meta->vbuffer.index;
+  meta->vbuffer.bytesused = meta->vbuffer.length;

Again, this won't work for v4l2sink, you have to leave bytesused to the current
buffer size, this is what the application has filled.

@@ +695,3 @@
+    gst_memory_new_wrapped (GST_MEMORY_FLAG_NO_SHARE,
+        meta->mem, meta->vbuffer.length, 0, meta->vbuffer.length, NULL,
+        NULL));

This won't work for v4l2sink if the memory has been changed., but it is broken
anyway right now, so it won't hurt more

@@ +794,3 @@
+            meta->mem, vbuffer.bytesused, 0, vbuffer.bytesused, NULL,
+            NULL));
+  }

This shouldn't be needed, the GstMemory should be writable at this point.

@@ +800,2 @@
   *buffer = outbuf;
+  GST_V4L2_BUFFER_POOL_UNLOCK (pool);

Why do you release the lock this late? why not just after decrementing
num_queued ?

@@ +912,3 @@

+            /* copy the buffer with memory */
+            copy = gst_buffer_copy_region (*buffer, GST_BUFFER_COPY_DEEP, 0,
-1);

You want GST_BUFFER_COPY_ALL | GST_BUFFER_COPY_DEEP, the metadata is also
important

@@ -994,3 @@
-            /* reset to the full length, in case it was changed */
-            gst_buffer_resize (buffer, 0, meta->vbuffer.length);
-

In the playback case, why do you remove the resize? The size may have changed..

::: sys/v4l2/gstv4l2bufferpool.h
@@ +34,2 @@
 #include "gstv4l2object.h"
+#include <gst/glib-compat-private.h>

Really, this is not needed even if you add a lock.

@@ +67,2 @@
   GstBuffer **buffers;
+  GMutex lock;

Why not just use GST_OBJECT_LOCK/UNLOCK instead of adding an extra mutex ?

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