GNOME Bugzilla – Bug 726299
avvideodec: Should reject buffer pools that cannot grow
Last modified: 2014-05-08 19:44:01 UTC
I found libav plugin will use the buffers from render to store the reference frames internally, but it doesn't tell the render the maximum buffers it needed. It will allocate buffers from buffer pool if it needed in run time. This will cause problem for some pre-allocated renders, such as V4l2 output. For this kind of driver, we need to specify how many buffers needed, then allocate them, we can't allocate buffers in run time. So shall the libav plugin set the maximun buffers to the buffer pool while in negotiation (gst_ffmpegviddec_decide_allocation)?
It should set the minimum (and maybe maximum) number of buffers on the allocation query, yes.
Just a minimum. Also, I wanted to fix the v4l2 side to detect missing CREATE_BUFS and set a maximum. In a way that if N is requestion, but v4l2 could only offer N-1, it would be possible for libav (and other decoders) to reject the buffer pool.
So is there a plan/patch in gst-libav to fix this issue? Say libav will provide the minimum buffers it needed for decoding, then gst-libav set this value to the buffer pool? (In reply to comment #1) > It should set the minimum (and maybe maximum) number of buffers on the > allocation query, yes.
*** Bug 727189 has been marked as a duplicate of this bug. ***
I didn't find anything in libav that could tell me how many buffers is needed. Also, v4l2 buffer pool does not support video alignment, which means that libav won't be doing direct rendering. Copying in libav, or copying at v4l2 element level won't make much difference. So I think the solution is to fix the buffer pool in v4l2 to clearly state if it's growable or not (max to 0 for growable), and avoid buffer pools that cannot grow in libav.
Created attachment 273126 [details] [review] [PATCH] videodec: Don't use non-growable pool As we don't know how many output buffer we need to operate, we need to avoid pool that can't grow. Otherwise the pipeline may stall, waiting for buffers. https://bugzilla.gnome.org/show_bug.cgi?id=726299 --- ext/libav/gstavviddec.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
For the v4l2 part, one cane poke at: http://cgit.collabora.com/git/user/nicolas/gst-plugins-good.git/commit/?h=v4l2-converter&id=01a88e88a4be9857f4824d2d61faaa02dfaf84d3 Though it's part of my v4l2-converter branch. I'll try and get that ready for review soon.
(In reply to comment #6) Shall it also reject the allocator, actually the pool can't grow as the allocator can't allocate more. The patch should be updated as: /* Don't use pool that can't grow, as we don't know how many buffer we'll * need, otherwise we may stall */ if (max != 0) { gst_object_unref (pool); pool = gst_video_buffer_pool_new (); + gst_object_unref (allocator); + allocator = NULL; max = 0; update_pool = TRUE; } > Created an attachment (id=273126) [details] [review] > [PATCH] videodec: Don't use non-growable pool > > > As we don't know how many output buffer we need to operate, we need to > avoid pool that can't grow. Otherwise the pipeline may stall, waiting > for buffers. > > https://bugzilla.gnome.org/show_bug.cgi?id=726299 > --- > ext/libav/gstavviddec.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-)
(In reply to comment #8) > (In reply to comment #6) > Shall it also reject the allocator, actually the pool can't grow as the > allocator can't allocate more. > > The patch should be updated as: > > /* Don't use pool that can't grow, as we don't know how many buffer we'll > * need, otherwise we may stall */ > if (max != 0) { > gst_object_unref (pool); > pool = gst_video_buffer_pool_new (); > + gst_object_unref (allocator); > + allocator = NULL; > max = 0; > update_pool = TRUE; > } I totally agree, thanks for pointing this out.
I've been rethinking this, and looking at how how many observation buffer is required to actually decode few formats. The worst seems 16 (h264), though at 16 you can't really push one out. We could fixate a number, and reject pools if they can't at least offer that (e.g. 20 or more as max, and growable, min < max). Would that be acceptable ?
I think add 6 extra buffer based on 16 (h264 max reference buffer count) can get better performance, 16 + 1 (for decoder output) + 3 (video queue held in playbin) + 2 (sink held). so it will do like: - if max >= 22 or max = 0, use this pool. - else reject the pool.
Right, so basically it's 17 + minmum that downstream wants right ? Having the queues and element with latency contributing to the allocation query (calculation of the min) is obviously the long term goal in order to better support fixed size pools.
commit 62a4d065ed7bd117d869fd8bcb61274c2870ddf5 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Thu Mar 27 18:53:53 2014 -0400 videodec: Don't use non-growable pool As we don't know how many output buffers we need to operate, we need to avoid pool that can't grow. Otherwise the pipeline may stall, waiting for buffers. For now, we require it to be able to grow to at least 32 buffers, which I think is a fair amount of buffers for decoders. https://bugzilla.gnome.org/show_bug.cgi?id=726299