[Bug 738102] v4l2bufferpool: cleanly handle streamon failure for output device

[GStreamer (bugzilla.gnome.org)]

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

Nicolas Dufresne (stormer) <nicolas.dufresne> changed:

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

--- Comment #8 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> 2014-10-09 15:34:30 UTC ---
Review of attachment 287980:
 --> (https://bugzilla.gnome.org/review?bug=738102&attachment=287980)

I think this unref() is completly wrong, the buffer is queued

::: sys/v4l2/gstv4l2bufferpool.c
@@ +1705,3 @@
+               * so it should not fail */
+              g_assert_not_reached ();
+            }

This check isn't useful, _qbuf would have failed if it was not the case.

@@ +1711,1 @@
             gst_buffer_unref (to_queue);

The problem here is that it's not correct to call unref() in the first place,
the buffer is no longer owned by this code since qbuf() have succeed. But as
you mention there is no logic that would let us go through flush_stop() logic,
cause we'll never reach streamon state. That means we need to restore the "not
streaming" state, which is assumed to have no buffer queued. Performing the
flushing logic should be the way forward imho.

Considering this is OUTPUT, and there is only 1 process thread allowed, order
does not matter, even though the convention is to set the pointer to null and
then decrements the counter. Now, you are missing an important step, the
allocator still thinks the buffer is queued, gst_v4l2_allocator_flush
(pool->vallocator) will fix that. Finally, to avoid breaking
USERPTR/DMABUF-IMPORT, cleanup the qdata:

gst_mini_object_set_qdata (GST_MINI_OBJECT (buffer),
              GST_V4L2_IMPORT_QUARK, NULL, NULL);

See gst_v4l2_buffer_pool_flush_stop() for how this cleanup shall be done, take
note this case is simpler, you are guarantied to only have 1 buffer queued. So
basically that code is right, but you also need to cleanup quarks and allocator
state.

Good catch btw.

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