GNOME Bugzilla – Bug 730758
tee: Handle ALLOCATION query
Last modified: 2017-10-02 18:15:20 UTC
On a RPi with GStreamer 1.3.2, this pipeline fails: gst-launch-1.0 videotestsrc ! queue ! omxh264enc ! h264parse ! omxh264dec ! "video/x-raw(memory:EGLImage)" ! tee ! queue ! glimagesink ERROR: from element /GstPipeline:pipeline0/GstOMXH264Dec-omxh264dec:omxh264dec-omxh264dec0: Internal data stream error. Additional debug info: gstomxvideodec.c(1468): gst_omx_video_dec_loop (): /GstPipeline:pipeline0/GstOMXH264Dec-omxh264dec:omxh264dec-omxh264dec0: stream stopped, reason not-negotiated ERROR: pipeline doesn't want to preroll. But this one works: gst-launch-1.0 videotestsrc ! queue ! omxh264enc ! h264parse ! omxh264dec ! "video/x-raw(memory:EGLImage)" ! glimagesink
Others case where gl elements fail with tee: 1- omxh264dec ! tee ! glimagesink fails whereas it worked with eglglessink. It was useful to force not using EGLImage as tee currently does not forward query allocation. 2- videotestsrc ! gleffects ! tee ! glimagesink should force gleffects to download (and glimagesink to upload again). Currently I see black frames.
tee does not fordward the ALLOCATION query. I was sure that there was a bug about this already but can't find it now... Basically tee will have to query each srcpad, then take the intersection of all replies. And when dynamically adding srcpads things become very interesting :)
Might need more work in the gst value, just mentioning, but nothing seriously hard, just that is fails weirdly.
I was thinking about this yesterday, I think it would be best to force upstream allocation in case of tee. As the allocation cannot be replaced reliably, letting downstream pools being used will make it difficult, near impossible for dynamic pipelines. So in the end, what we need to aggregate is the min/max/size, for the allocation pools, but ignore the pools, the memory alignment for the param and ignore the allocators (? not certain) and then intersect the metas. Could have been nice to have some semantic for let's say OverlayComposition. The intersection will likely drop it if the displays report different size. While in practice it should have picked the biggest size in order to guaranty the highest sub-title / overlay quality.
After a grep, GST_VIDEO_OVERLAY_COMPOSITION_META_API_TYPE is the only meta that make use of the gst_query_add_allocation_meta() params argument. I think it's valid to just ignore the params in a first iteration. Later we can add maybe an option aggregate function to the GstMetaInfo structure, this would implement the logic needed.
Created attachment 357266 [details] [review] tee: Implement allocation query aggregation This will aggregate allocation params, pool and will keep all meta that has no parameters. https://bugzilla.gnome.org/show_bug.cgi?id=785680
Created attachment 357267 [details] [review] tee: Implement allocation query aggregation This will aggregate allocation params, pool and will keep all meta that has no parameters.
Created attachment 357284 [details] [review] tee: Implement allocation query aggregation [Update] Fixed an assert for the case downstream didn't set any allocation pools but succeeded the allocation query.
Review of attachment 357284 [details] [review]: Should you remove "alloc-pad" property ? (seems to be mark unused anyway) Maybe add a unit test in gstreamer/tests/check/elements/tee.c ? At least one and then we can add more. ::: plugins/elements/gsttee.c @@ +730,3 @@ + gst_allocation_params_init (&ctx.params); + + Should it ignore not linked pads if allow-not-linked is true ?
(In reply to Julien Isorce from comment #9) > Review of attachment 357284 [details] [review] [review]: > > Should you remove "alloc-pad" property ? (seems to be mark unused anyway) It seems that I forgot to attach it, but I have a patch that deprecate it instead as removing it would be an API break. > > Maybe add a unit test in gstreamer/tests/check/elements/tee.c ? At least one > and then we can add more. I'll do. > > ::: plugins/elements/gsttee.c > @@ +730,3 @@ > + gst_allocation_params_init (&ctx.params); > + > + > > Should it ignore not linked pads if allow-not-linked is true ? I think this is difficult to implement since gst_pad_peer_query() returns a boolean rather then GstFlowReturn. Though, GstPad return TRUE when it gets a query on unlinked pads, which makes the code behave like if allow-not-linked was set. I'm not sure if it should (I even doubt it should) behave differently for the other case. I'm tempted to leave it this way.
Review of attachment 357284 [details] [review]: ::: plugins/elements/gsttee.c @@ +592,3 @@ + gst_query_parse_allocation (ctx->query, &caps, NULL); + + * allocation would otherwise make dynamic pipelines much more complicated as Shouldn't we pass the need_pool from ctx->query as well? Currently by always passing need_pool=FALSE kmssink doesn't return its buffers requirements, see https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/sys/kms/gstkmssink.c#n979
Review of attachment 357284 [details] [review]: ::: plugins/elements/gsttee.c @@ +611,3 @@ + gst_query_parse_nth_allocation_param (query, i, NULL, ¶ms); + + guint count, i, size, min; Maybe display the GST_DEBUG_PAD_NAME as well to help debugging? Or at the start of the method?
(In reply to Guillaume Desmottes from comment #11) > Review of attachment 357284 [details] [review] [review]: > > ::: plugins/elements/gsttee.c > @@ +592,3 @@ > + gst_query_parse_allocation (ctx->query, &caps, NULL); > + > + * allocation would otherwise make dynamic pipelines much more complicated > as > > Shouldn't we pass the need_pool from ctx->query as well? > Currently by always passing need_pool=FALSE kmssink doesn't return its > buffers requirements, see > https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/sys/kms/ > gstkmssink.c#n979 Good point. It still a waste to create the pool downstream, maybe we should just fix sinks to set a NULL pool with their requirement if need-pool is FALSE ?
(In reply to Guillaume Desmottes from comment #12) > Review of attachment 357284 [details] [review] [review]: > > ::: plugins/elements/gsttee.c > @@ +611,3 @@ > + gst_query_parse_nth_allocation_param (query, i, NULL, ¶ms); > + > + guint count, i, size, min; > > Maybe display the GST_DEBUG_PAD_NAME as well to help debugging? Or at the > start of the method? Ok.
Created attachment 359234 [details] [review] tee: Deprecate alloc-pad property It has no effect, no implemented, and can lead to bad rendering.
Created attachment 359235 [details] [review] tee: Implement allocation query aggregation This will aggregate allocation params, pool and will keep all meta that has no parameters.
Created attachment 359236 [details] [review] tee: Add test for the allocation query
This new version provides: - Patch to deprecate alloc-pad - A unit test for the allocation query - Partial support for allow-not-linked (only works if directly not linked) - Added some debug logs with pad names - Added "ifndef GST_DISABLE_GST_DEBUG" to debug for loop For the need-pool, I think the tee implementation is correct. We need to go through element and setup buffer requirement if need-pool is false (setting a NULL pool). This is going to be quite a lot of patches, but nothing complicated. Another patch is coming to improve the overall performance. Whenever a tee has more then 1 pad, there is contention with the negotiated allocation that makes the queuing inefficient. We need to allocate one more buffer in this case to make sure we don't starve the threads.
Created attachment 359239 [details] [review] tee: Allocate one more buffer when multi-plexing This extra buffer ensure that the downstream threads are not starved when multiplexing a stream.
Created attachment 359240 [details] [review] plugins-bad: Request minimum buffer even if need_pool is FALSE When tee is used, it will not request a pool, but still it wants to know how many buffers are required.
Created attachment 359271 [details] [review] identity: Add a drop-allocation property When enabled, this property will make the allocation query fail. This is the same as one could have done using a tee before the tee started implementing the allocation query.
Thx for the unit test. (In reply to Nicolas Dufresne (stormer) from comment #21) > identity: Add a drop-allocation property > This is the same as one could have done using a tee before Great! I was about to ask for something like that. And just to clarify, the attached patch are only for the pools requirements right ? It does not allow to propose a pool ? Example: dec ! tee ! xvimagesink will prevent dec to use the downstream pool from xvimagesink ? I can see the answer in comment #4 and I can see the pool not being passed to the decoder when doing a test but I am asking for confirmation that this is expected.
That's correct, we have no optimization for the single pad tee case. Considering the purpose of tee, I think it's fair enough. It's quite difficult atm as element re-allocate their pool on every reconfigure event. If we start ignoring some reconfigure event we'll be happy that the tee didn't offer a downstream pool in the first place.
Created attachment 359294 [details] [review] plugins-bad: Request minimum buffer even if need_pool is FALSE When tee is used, it will not request a pool, but still it wants to know how many buffers are required.
Created attachment 359295 [details] [review] video-filter: Support allocation pool with pool object This is used to indicate upstream the requirement in buffers while no buffer pool can be provided. In this case, only configure the pool with caps/size/min/max if we have caps, which we only parsed when there was no allocation pool.
Created attachment 359296 [details] [review] plugins-base: Request minimum buffer even if need_pool is FALSE When tee is used, it will not request a pool, but still it wants to know how many buffers are required.
Created attachment 359298 [details] [review] vaapi: Request minimum buffer even if need_pool is FALSE When tee is used, it will not request a pool, but still it wants to know how many buffers are required.
Comment on attachment 359294 [details] [review] plugins-bad: Request minimum buffer even if need_pool is FALSE plugins-bad fixes pushed as 9b2e28d91d00638e76a39ec219e99aa33241b242
Comment on attachment 359298 [details] [review] vaapi: Request minimum buffer even if need_pool is FALSE vaapi fixes pushed as 2039eb882ea51b5e4fafb8f3cc0f89b64a4f5a19
Attachment 359295 [details] pushed as 5d6fbcb - video-filter: Support allocation pool with pool object And plugins-base fixes pushed as 2bf665486e02cf380175baae43308de7dc03e733
Attachment 359234 [details] pushed as 945322f - tee: Deprecate alloc-pad property Attachment 359235 [details] pushed as cf803ea - tee: Implement allocation query aggregation Attachment 359236 [details] pushed as 291400d - tee: Add test for the allocation query Attachment 359239 [details] pushed as 1cd0dd3 - tee: Allocate one more buffer when multi-plexing Attachment 359271 [details] pushed as 2cc5c53 - identity: Add a drop-allocation property
I wonder if we should also have a drop-allocation-query property on tee itself. If the new stuff causes problems in people's pipelines it's easier to set that on the existing tees than to add extra identity elements.
(In reply to Tim-Philipp Müller from comment #32) > I wonder if we should also have a drop-allocation-query property on tee > itself. If the new stuff causes problems in people's pipelines it's easier > to set that on the existing tees than to add extra identity elements. The identity property is just for debugging. I hope no one need that really. My view is that if we found a case where it is the best solution, then it will be a good moment to add that property.
The added test does not cover the case where the aggregated size and min/max buffers == 0. I think there is no point in adding a pool in that case, right? I have a local chance that fixes my app again, but would like to discuss first.
Created attachment 360772 [details] [review] don't create a 'bad' pool Here is my fix. I'd like to understand if a pool with size==0 is something that has a meaning and fix the docs+code accordingly. Right now we don't document what size=0 means, but we also don't prevent it.
Review of attachment 360772 [details] [review]: Can you split the re-factoring of the test from the new test, I wanted to look at the test to understand what you mean, but you have changed everything.
Review of attachment 360772 [details] [review]: That's a good catch, it's an unwanted side effect. I would probably suggest adding explicit mark, e.g. param_found/pool_found since some Opaque format may use 0 for pool size (in fact I have no idea, but explicit marker seems safer). ::: plugins/elements/gsttee.c @@ +800,3 @@ + /* If size==0, buffers would have no memory. */ + if (ctx.size > 0) { + gst_query_add_allocation_param (ctx.query, NULL, &ctx.params); That's incorrect, we should have a specific mark to know if we didn't find any allocation params. @@ +802,3 @@ + gst_query_add_allocation_param (ctx.query, NULL, &ctx.params); + gst_query_add_allocation_pool (ctx.query, NULL, ctx.size, + ctx.min_buffers, 0); That's correct, indeed we should not introduce a allocation_pool if there was none-downstream.
Created attachment 360780 [details] [review] don't create a 'bad' pool Sorry for having the test refactoring in there. Will push that in a minute. W.r.t the marker I separated the check for the params.
Review of attachment 360780 [details] [review]: Looks good. I'll merge with the refactoring, might need a no_params test while at it, if you don't have time, I'll take care, no worries.
I've fixed the unit test to properly test an downstream proposed allocation with no pool, and added the no params case. The fact that size was 0 is just a side effect of how we aggregate. Downstream should not propose a pool with size 0, that would be a bug.