GNOME Bugzilla – Bug 727611
bufferpool: Add _set_flushing() and new (active,flushing) state
Last modified: 2014-05-26 17:41:54 UTC
On flush-start we are supposed to unblock everything that could have been blocked in our element. An existing, but rare, case we have right now is the when a pool that cannot grow has no more buffer to offer. acquire() will then block on a internal poll. To unblock this call, we need to call set_active(FALSE). While this will correctly unblock any call to acquire, it also has the side effect of freeing all the pre-allocated buffers and the memory that where attached to it. This means, that correctly implemented element using buffer pool would free and re-allocate all of its memory at every seek, even though the format and required amount of buffers may not have change. In order to give element a better control, I'm proposing to introduce a new state, moving the pool from 2 state, active and inactive to 3 states: inactive -> (!active, flushing) flushing -> (active, flushing) active -> (active, !flushing) The new methods would be called: gst_buffer_pool_set_flushing (pool, flushing) gst_buffer_pool_is_flushing (pool) This would share the same lock used for _set_active(), setting to non-flushing while not active will not be permitted.
Created attachment 276888 [details] Totem debug log This was from launching a video in totem and jumping way forward in the stream. Log is 'GST_DEBUG="3,v4l2*:7" totem'. The ERROR shows up at 0:00:08.74526783.
(In reply to comment #1) > Created an attachment (id=276888) [details] > Totem debug log > > This was from launching a video in totem and jumping way forward in the stream. > Log is 'GST_DEBUG="3,v4l2*:7" totem'. The ERROR shows up at 0:00:08.74526783. This should have it's own bug.
(In reply to comment #2) > > This should have it's own bug. Opened bug730476 for that issue.
I've started implementing this yesterday. It greatly helps in fixing flushing race in V4L2 pool, so it's not only a performance issue. I would like to make this blocker for 1.4, would it be fine ? I'll should be able to deliver that really soon.
Created attachment 277092 [details] [review] bufferpool: Add method and virtuals to set flushing state Here's the implementation. I've ported v4l2 code to test it, without porting GstBaseSrc to make sure it's backward compatible. It solve many race I was seeing in v4l2 as the flushing is not handle in sync with the pool state (and with the pool state lock). Still need to implement few unit tests, though I'm posting it to gather comments.
Created attachment 277132 [details] [review] V4L2 Port to the new API Attached the v4l2 port, it gives an idea. The only thing I'm unsure, is if I should add a gboolean return value to flush_start/flush_stop.
Review of attachment 277092 [details] [review]: Makes sense to me ::: gst/gstbufferpool.c @@ +493,2 @@ /* unset the flushing state now */ + do_set_flushing (pool, 0); s/0/FALSE/
Ok, let's give it a try then, I've fixed that, updated windows def and added a unit test. commit 103a40b6ce91497c2821da9738ceac882b060ece Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri May 23 15:26:59 2014 -0400 bufferpool: Add method and virtuals to set flushing state Currently there is no other way to unlock a buffer pool other then stopping it. This may have the effect of freeing all the buffers, which is too heavy for a seek. This patch add a method to enter and leave flushing state. As a convenience, flush_start/flush_stop virtual are added so pool implementation can also unblock their own internal poll atomically with the rest of the pool. This is fully backward compatible with doing stop/start to actually flush the pool (as being done in GstBaseSrc). https://bugzilla.gnome.org/show_bug.cgi?id=727611