GNOME Bugzilla – Bug 752363
appsrc: add support for pushing buffer lists
Last modified: 2017-12-09 11:17:54 UTC
Created attachment 307395 [details] [review] appsrc: extract buffer for a buffer list in a sample This is the first patch, I'll try to do another patch to keep compatibility with existing applications (1.0->1.4) that does not expect a buffer list in a sample pulled from appsink
regarding appsink it does not support buffer lists so no change is needed, now that gst_sample can have bufferlist maybe we should document that the sample pulled from appsink will not contain buffer list but only buffers
What is the reason of not having buffer list support in appsink?
The main problem is that this needs to be opt-in for appsink in one way or another, otherwise existing appsink users will suddenly stop working if there is a buffer list. Apart from that it "just" has to be implemented. For appsrc we only need API for bufferlists in basesrc, and then if the sample contains a buffer list we just use that to pass it downstream in one piece (the attached patch does not make sense as it will tear apart the buffer list). For appsrc there is no concern about backwards compatibility here. basesrc support for buffer lists is here: https://bugzilla.gnome.org/show_bug.cgi?id=750241
Maybe I'm missing something here but it's just a question of implementing render_buffer_list function in the appsink. It's a bug today that if a pipeline contains elements that generate and push buffer lists downstream to the appsink, this list is unfortunately split into single buffers again by the appsink. The logic of handling buffer list is already implemented in basesink and adding an additional buffer-list property for appsink seems like a work-around to the existing problem.
It's not a bug, buffer lists would just be transparently converted to single buffers currently and it would all work without the application having to know. If you add buffer list support, you need to ensure to only pass buffer lists to the application if it opted in (e.g. by a property) and otherwise split the buffer list into individual buffers again. Otherwise you break applications that work now (because buffer lists are transparently split now).
But why is this case handled in a different way in for example multiudpsink or fdsink. There is no buffer-list property in these elements. They just implement render_buffer_list func.
With those elements there's no external consumer who might expect different behaviour.
Ok. I got your point. Thanks for your explanation but I still believe that the old behavior in 0.10 was less complicated and more clear - having pull_buffer() and pull_buffer_list() callback functions in appsink set by the external consumers: based on the value of the appsink-callback function if was possible to detect the current support/mode of the appsink: buffer mode or buffer-list mode. Would you accept a patch that introduces a new buffer-list property in appsink?
Yes, that seems like the way forward. It should be FALSE by default though
Created attachment 334430 [details] [review] appsink: add support for buffer lists
Hi. Is the patch correct? Do you have time to review the code? Actually, this patch and the one solving https://bugzilla.gnome.org/show_bug.cgi?id=771525 improves performance while streaming RTP over RTSP. Thank you in advance!
This will have to wait until after 1.10 now, sorry :)
Hi. I wonder if the suggested patch is ok. Do I need to change something? Thanks.
Review of attachment 334430 [details] [review]: Looks good to me. Backwards-compatibility is maintained in the default case - degenerating to extracting buffers from buffer lists.
Review of attachment 307395 [details] [review]: As mentioned by Sebastian, we should finalise the API in bug #750241 to let basesrc create buffer lists and then just push them. The logic in core will take care of tearing them apart to buffers if downstream can't handle that.
Don't know if the new API proposal in bug #750241 is a good fit for this particular problem, since we can't pass through the GstBufferList as-is with that but would have to steal buffers and then let basesrc create a new buffer list. One solution might be to come up with an API that doesn't pass a bufferlist to appsrc, but N buffers, but if the intention is to pass through stuff coming from appsink that doesn't help.
The intention is to handle the app pushing in a GstSample, which holds either a GstBufferList. appsrc doesn't really have a lot of choice without an API to push that directly to basesrc.
Retitling since the appsink part was fixed ages ago.
Created attachment 359931 [details] [review] appsrc: add support for pushing buffer lists And samples that carry buffer lists.
commit b60cc0274c7d16e62f2ab52e2e679d3ce3b6afea (HEAD -> master) Author: Tim-Philipp Müller <tim@centricular.com> Date: Thu Aug 31 11:12:12 2017 +0100 appsrc: add support for pushing buffer lists And samples that carry buffer lists. https://bugzilla.gnome.org/show_bug.cgi?id=752363