*** Bug 706069 has been marked as a duplicate of this bug. ***

This is also in bug #699517, should we continue this patch here or there? See my comments there (comment 35).

Isn't the real solution here to eliminate the custom bufferpool entirely ?

Would that mean copying the v4l2 driver allocated user space mapped buffers to freshly allocated memory?  Seems to be happening anyway implicitly at times via the GstBuffer API resize and map functions.  Is it not desirable to minimize alloc/copy/free in the pipeline to minimize CPU consumption?

Created attachment 251906 [details] [review]
Patch for v4l2src to put back replaced buffer...and fix race condition...

Through extended 24 hour testing, I found a race condition between dqbuf and qbuf functions that can undermine replacement of original buffer in the qbuf function.  This warranted the introduction of mutex protection in both the aforementioned functions.

Created attachment 251911 [details] [review]
Patch for v4l2src to put back replaced buffer...and fix race condition...

Created attachment 251912 [details] [review]
Patch for v4l2src to put back replaced buffer...and fix race condition...

Forgot about some short circuit returns...

Created attachment 251932 [details] [review]
Patch for v4l2src to put back replaced buffer...and fix race condition...

Moved mutex initialization...

Created attachment 251955 [details] [review]
Patch for v4l2src to put back replaced buffer...and fix race condition...

Still had mutex messed up...

Created attachment 252068 [details] [review]
Patch for v4l2src to put back replaced buffer...and fix race condition...

Do GstBuffer memory replacement with resized v4l2 buffer to bytes used wrapped GstMemory object so that it is not replaced with system memory GstMemory object.

Created attachment 252279 [details] [review]
Git formatted patch to address this bug...

Does this patch have a chance of making it in to the next release...?  :-)

*** Bug 699517 has been marked as a duplicate of this bug. ***

Review of attachment 252279 [details] [review]:

I think we should put something along those lines in 1.2.

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

This will break v4l2sink I think, it needs to know how much of the buffer is valid when it is queued.

@@ +675,3 @@
+  if (pool->buffers[index] != NULL) {
+      goto already_queued;
+  }

Why the extra {} ?

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

Didn't you already replace the memory when doing the qbuf ?

@@ +894,3 @@
 
+            /* copy the buffer with memory, but not the meta data */
+            copy = gst_buffer_copy_region (*buffer, GST_BUFFER_COPY_MEMORY, 0, -1);

Why not copy the metadata also ? Don't you want GST_BUFFER_COPY_ALL | GST_BUFFER_COPY_DEEP ?

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

What are you using from glib-compat-private ?

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

Why do you add a new lock here? Any reason to not use the GstObject lock ?

Review of attachment 252279 [details] [review]:

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

You are correct.  Don't know why I did that.

@@ +675,3 @@
+  if (pool->buffers[index] != NULL) {
+      goto already_queued;
+  }

The {} can be removed.

@@ +686,3 @@
   pool->buffers[index] = buf;
   pool->num_queued++;
+  GST_V4L2_BUFFER_POOL_UNLOCK (pool);

GST_V4L2_BUFFER_POOL_UNLOCK (pool) not needed.

@@ +736,3 @@
+  }
+
+  GST_V4L2_BUFFER_POOL_LOCK (pool);

GST_V4L2_BUFFER_POOL_LOCK (pool) not needed.

@@ +743,3 @@
   outbuf = pool->buffers[vbuffer.index];
+  if (outbuf == NULL) {
+    GST_V4L2_BUFFER_POOL_UNLOCK (pool);

GST_V4L2_BUFFER_POOL_UNLOCK (pool) not needed and {} can be removed.

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

This resizes without replacing the dequeued buffer as does gst_buffer_resize().  I believe that there is work underway to prevent the replacement of wrapped memory by the GstBuffer class.  This is a temporary workaround.

@@ +784,1 @@
 

GST_V4L2_BUFFER_POOL_UNLOCK (pool) not needed and {} can be removed.

@@ +894,3 @@
 
+            /* copy the buffer with memory, but not the meta data */
+            copy = gst_buffer_copy_region (*buffer, GST_BUFFER_COPY_MEMORY, 0, -1);

Trying to be efficient...gst_buffer_copy() can be used instead as it was in the original source.

* To efficiently create a smaller buffer out of an existing one, you can
 * use gst_buffer_copy_region(). This method tries to share the memory objects
 * between the two buffers.

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

This resizes without replacing the dequeued buffer as does gst_buffer_resize().  I believe that there is work underway to prevent the replacement of wrapped memory by the GstBuffer class.  This is a temporary workaround.

@@ +1054,3 @@
 gst_v4l2_buffer_pool_init (GstV4l2BufferPool * pool)
 {
+  g_mutex_init(&pool->lock);

g_mutex_init (&pool->lock) not needed.

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

For the added lock which is not needed.

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

It is not needed.

@@ +93,3 @@
 
+#define GST_V4L2_BUFFER_POOL_LOCK(pool)     g_mutex_lock (&(pool)->lock)
+#define GST_V4L2_BUFFER_POOL_UNLOCK(pool)   g_mutex_unlock (&(pool)->lock)

These are not needed.

Any chance you can provide an updated patch ?

(In reply to comment #15)
> Review of attachment 252279 [details] [review]:
> @@ +775,3 @@
> +        gst_memory_new_wrapped (GST_MEMORY_FLAG_NO_SHARE,
> +            meta->mem, vbuffer.bytesused, 0, vbuffer.bytesused, NULL,
> +            NULL));
> 
> This resizes without replacing the dequeued buffer as does gst_buffer_resize().
>  I believe that there is work underway to prevent the replacement of wrapped
> memory by the GstBuffer class.  This is a temporary workaround.
> 
> @@ +894,3 @@
> 
> +            /* copy the buffer with memory, but not the meta data */
> +            copy = gst_buffer_copy_region (*buffer, GST_BUFFER_COPY_MEMORY, 0,
> -1);
> 
> Trying to be efficient...gst_buffer_copy() can be used instead as it was in the
> original source.

actually, gst_buffer_copy() won't do as it doesn't make a copy of the actual memory, you do want GST_BUFFER_COPY_DEEP

> * To efficiently create a smaller buffer out of an existing one, you can
>  * use gst_buffer_copy_region(). This method tries to share the memory objects
>  * between the two buffers.

You don't want to share the memory, as the next step is to give it back to the driver.

> @@ +1006,3 @@
> +                gst_memory_new_wrapped (GST_MEMORY_FLAG_NO_SHARE,
> +                    meta->mem, meta->vbuffer.length, 0, meta->vbuffer.length,
> NULL,
> +                    NULL));
> 
> This resizes without replacing the dequeued buffer as does gst_buffer_resize().
>  I believe that there is work underway to prevent the replacement of wrapped
> memory by the GstBuffer class.  This is a temporary workaround.

I also think that's the correct workaround as I don't think the rework in bug #707534 can land in time for 1.2

I will have one for you tomorrow morning after I have tested it, is that OK?

I am testing the revamped code and will generated a patch after letting it run for a couple of hours...

Created attachment 255308 [details] [review]
Git Formatted Patch

Here is a revised patch.  I left the lock in as I cannot confirm that it is not needed.  I took all the other suggestions from the patch review.

Review of attachment 255308 [details] [review]:

::: 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 ?

Review of attachment 255308 [details] [review]:

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

So, vbuffer.length is the size of the v4l2 buffer and vbuffer.bytesused the actual number of bytes used in the buffer.  So, IMHO, this should be valid.  The meta data obtained from the GstBuffer is used to obtain the length of the vbuffer.

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

Why not?

@@ +794,3 @@
+            meta->mem, vbuffer.bytesused, 0, vbuffer.bytesused, NULL,
+            NULL));
+  /* with vbuffer.bytesused as the size and replace the    */

Huh?  gst_buffer_resize() is the problem.  It can replace the wrapped memory object referenced by the GstBuffer with a new one that meets the new size constraint and then the previously wrapped memory object, which wraps a v4l2 buffer, is "lost in space".

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

Trying to protect the pool structure from being accessed on different thread times.  I am assuming that a buffer can get dequeued on another thread time.

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

O.K.

Review of attachment 255308 [details] [review]:

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

Maybe not, but the code would not build without it.  Please suggest an alternative.

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

GST_OBJECT_LOCK/UNLOCK is kind of a big mutex for this job.  I took this from the 0.10 v4l2src buffer pool source.

Actually, I'm pretty sure that you don't need to do any extra locking there.

OK...I will remove it then.  :-)  I am new to 1.xx framework as I have just recently moved to it after years of working in 0.10 framework.  Also, v4l2sink was not on my mind when I proposed this patch.

(In reply to comment #21)
> Review of attachment 255308 [details] [review]:
> 
> ::: sys/v4l2/gstv4l2bufferpool.c
> @@ +681,2 @@
>    index = meta->vbuffer.index;
> +  meta->vbuffer.bytesused = meta->vbuffer.length;
> 
> So, vbuffer.length is the size of the v4l2 buffer and vbuffer.bytesused the
> actual number of bytes used in the buffer.  So, IMHO, this should be valid. 
> The meta data obtained from the GstBuffer is used to obtain the length of the
> vbuffer.

In this case, the buffer length is what the application put into it (for v4l2sink), which may be smaller than the allocated buffer.

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

Again in the v4l2sink case, the application has written something in the memory that's in the buffer, so you can't just ignore it.


> @@ +794,3 @@
> +            meta->mem, vbuffer.bytesused, 0, vbuffer.bytesused, NULL,
> +            NULL));
> +  /* with vbuffer.bytesused as the size and replace the    */
> 
> Huh?  gst_buffer_resize() is the problem.  It can replace the wrapped memory
> object referenced by the GstBuffer with a new one that meets the new size
> constraint and then the previously wrapped memory object, which wraps a v4l2
> buffer, is "lost in space".

In this case, it will only shrink the buffer, so that should never happen.

> @@ +800,2 @@
>    *buffer = outbuf;
> +  GST_V4L2_BUFFER_POOL_UNLOCK (pool);
> 
> Trying to protect the pool structure from being accessed on different thread
> times.  I am assuming that a buffer can get dequeued on another thread time.

The same buffers[x] member will only be accessed once you got the buffer back from the kernel, so only that thread owns it at this point.

>> @@ +794,3 @@
>> +            meta->mem, vbuffer.bytesused, 0, vbuffer.bytesused, NULL,
>> +            NULL));
>> +  /* with vbuffer.bytesused as the size and replace the    */
>> 
>> Huh?  gst_buffer_resize() is the problem.  It can replace the wrapped memory
>> object referenced by the GstBuffer with a new one that meets the new size
>> constraint and then the previously wrapped memory object, which wraps a v4l2
>> buffer, is "lost in space".
>
>In this case, it will only shrink the buffer, so that should never happen.

With all do respect, gst_buffer_resize() does replace the memory object.  I have followed the code path and put a ton of debug print statements in the code.  Without this change, there is no point in going through with a patch for this bug.

I am submitting another patch shortly...:-)

Created attachment 255345 [details] [review]
Git Formatted Patch Revised

I have revised the patch again...

...you are right about the code that I put in that bashes v4l2sink...sorry about that...

Created attachment 255355 [details] [review]
v4l2bufferpool: Restore original GstMemory in buffer if it has been changed

I simplified your patch a little, fixed the coding style. I also removed the playback (sink) bits, since we're not actually fixing it now.

Thanks...I will test it tonight.  Sorry for the original patch being so convoluted.  :-)

For my own edification, I thought that using gst_buffer_replace_memory() would kill two birds with one stone versus gst_buffer_remove_all_memory() and gst_buffer_append_memory() as you used or am I missing something?

+  if (obj->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+    gst_buffer_remove_all_memory (outbuf);
+    gst_buffer_append_memory (outbuf,
+        gst_memory_new_wrapped (GST_MEMORY_FLAG_NO_SHARE,
+            meta->mem, vbuffer.length, 0, vbuffer.bytesused, NULL, NULL));

My assumption was that there is only one memory object in the GstBuffer, which may be a bad assumption.  Using the two functions to remove all memory and then append makes sense to me as all of the memory objects are removed.  Good call!

Thanks...I will test it tonight.  Sorry for the original patch being so convoluted.  :-)

Review of attachment 255355 [details] [review]:

Looks good in general to me, please push after Robert has tested it and:

::: sys/v4l2/gstv4l2bufferpool.c
@@ +897,3 @@
+            /* copy the buffer */
+            copy = gst_buffer_copy_region (*buffer,
+                GST_BUFFER_COPY_ALL | GST_BUFFER_COPY_DEEP, 0, -1);

Why are you doing a deep copy here? Please add at least a comment :)

Robert, any news on testing? :)

Sorry, our gateway went down.  Testing just started...I am going to let it run for a few hours.

Testing has went well and CPU consumption by v4l2src is low.  Looks good so far.

Pushed

commit 141a1fc296b9d98179f09aaca578b4c1fccbaef7
Author: Robert Krakora <rob.krakora@messagenetsystems.com>
Date:   Thu Sep 19 17:11:34 2013 -0400

    v4l2bufferpool: Restore original GstMemory in buffer if it has been changed
    
    https://bugzilla.gnome.org/show_bug.cgi?id=706083