GNOME Bugzilla – Bug 750241
[API] GstBaseSrc: add support for buffer list
Last modified: 2017-12-09 11:16:17 UTC
Created attachment 304380 [details] [review] WIP: basesrc: factor out do_sync and clock status For packet-based transports (UDP) or formats (RTP) every packet needs to be in a separate buffer, but for high-bitrate streams the overhead of pushing possibly up to a hundred thousand single buffers individually through the pipeline gets very high. GStreamer provides a solution to this in form of GstBufferList and gst_pad_push_list(), but there is currently no way to create a buffer list in a GstBaseSrc-derived element. The following patches add this functionality. They are still work-in-progress. One thing I would like to see supported is the ability to have a sub class decide whether to produce a buffer list or single buffers at run-time (e.g. based on configuration or such).
Created attachment 304381 [details] [review] WIP: basesrc: add ::create_list() vfunc to create a buffer list
The specific use case I have in mind for this is udpsrc recvmmsg() btw.
Would you perhaps make the new basesrc create API be a vfunc that takes both a pointer to GstBuffer * and GstBufferList * and the subclass can decide which one to fill each time, or perhaps a create function that takes GstMiniObject **out as an arg for it to fill?
No, what we came up with at the hackfest was something else, which is much simpler and better. Let me dig up the patch.
Created attachment 339946 [details] [review] basesrc: add buffer list support This simplifies things tremendously, both in GstBaseSrc and for subclasses. There's some overhead from the intermediary array, but we can optimise that further in future if it turns out to be a problem, since it's just an internal implementation detail.
We could additionally also add a _submit_buffer_list() function that works similarly but can only be called once before returning from create. This would address the push-bufferlist-into-appsrc use case from bug #752363
Created attachment 339948 [details] [review] WIP: basesrc: add gst_base_src_submit_buffer_list() API Something like this perhaps? (Completely untested of course) Reminder to self: Also needs 'transfer full' annotations in submit_buffers() or a (skip).
(In reply to Tim-Philipp Müller from comment #7) > Created attachment 339948 [details] [review] [review] > WIP: basesrc: add gst_base_src_submit_buffer_list() API > > Something like this perhaps? (Completely untested of course) > > Reminder to self: Also needs 'transfer full' annotations in submit_buffers() > or a (skip). I like that better than the 'submit_buffers()' approach.
> I like that better than the 'submit_buffers()' approach. It wasn't really meant as an either-or, I was suggesting both ;)
Created attachment 359928 [details] [review] basesrc: minor code readability improvement (useful for next pach)
Created attachment 359929 [details] [review] basesrc: add buffer list support Add a gst_base_src_submit_buffer_list() function that allows subclasses to produce a bufferlist containing multiple buffers in the ::create() function. The buffers in the buffer list will then also be pushed out in one go as a GstBufferList. This can reduce push overhead significantly for sources with packetised inputs (such as udpsrc) in high-throughput scenarios. The _submit_buffer_list() approach was chosen because it is fairly straight-forward, backwards-compatible, bindings-friendly (as opposed to e.g. making the create function return a mini object instead), and it allows the subclass maximum control: the subclass can decide dynamically at runtime whether to return a list or a single buffer (which would be messier if we added a create_list virtual method).
Created attachment 359930 [details] [review] basesrc: add buffer list support [v2] Same as above, but remove the g_return_if_fail (GST_PAD_MODE(pad) == GST_PAD_PUSH); guard in _submit_buffer_list(), since the pad might get shut down while the streaming thread is still doing work, in which case the warning would wrongly be triggered.
Review of attachment 359930 [details] [review]: Looks good to me
*** Bug 791059 has been marked as a duplicate of this bug. ***
Any hesitation you'd like to share ?
commit 18fe36a286d3dd806cfe645d643d9cdd8ea19237 (HEAD -> master, origin/master, origin/HEAD) Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed Aug 30 13:03:28 2017 +0100 basesrc: add buffer list support Add a gst_base_src_submit_buffer_list() function that allows subclasses to produce a bufferlist containing multiple buffers in the ::create() function. The buffers in the buffer list will then also be pushed out in one go as a GstBufferList. This can reduce push overhead significantly for sources with packetised inputs (such as udpsrc) in high-throughput scenarios. The _submit_buffer_list() approach was chosen because it is fairly straight-forward, backwards-compatible, bindings-friendly (as opposed to e.g. making the create function return a mini object instead), and it allows the subclass maximum control: the subclass can decide dynamically at runtime whether to return a list or a single buffer (which would be messier if we added a create_list virtual method). https://bugzilla.gnome.org/show_bug.cgi?id=750241 commit 880c573e8df17185f0773a2bbe412242e870907b Author: Tim-Philipp Müller <tim@centricular.com> Date: Thu Aug 31 01:18:28 2017 +0100 basesrc: minor code readability improvement