GNOME Bugzilla – Bug 791453
videocrop: Add GstVideoCropMeta support
Last modified: 2017-12-18 00:46:26 UTC
This is to avoid copying the data if the crop can be handled by a downstream element, like xvimagesink, ximagesink or kmssink. I've implemented this specifically to be able to test this often untested feature in sinks (kmssink for my case). Note that it won't apply any crop meta added by let's say a decoder. The reason is that it will go passthrough and we have nothing in the caps that would let us know. This felt like a project by itself, and I didn't really needed it. If someone needs it, a short workaround is to add a disable-passthrough property (or something), but I didn't want to commit to any API.
Created attachment 365331 [details] [review] videocrop: Add GstVideoCropMeta support If downstream supports this meta, it will add or update it from the GstBuffer in-place rather then copying.
Review of attachment 365331 [details] [review]: Looks good to me except for the fixate_caps() hack. Is there no better way, maybe by adding some new API to basetransform? :) ::: gst/videocrop/gstvideocrop.c @@ +704,3 @@ + goto done; + + if (!gst_pad_push_event (trans->srcpad, gst_event_new_caps (othercaps))) gst_base_transform_update_src_caps()? Sending a caps event behind basetransform's back is probably not a good idea. @@ +708,3 @@ + + query = gst_query_new_allocation (othercaps, FALSE); + if (!gst_pad_peer_query (trans->srcpad, query)) And this would cause two allocation queries to be done, with the result of this one being ignored more or less?
(In reply to Sebastian Dröge (slomo) from comment #2) > Review of attachment 365331 [details] [review] [review]: > > Looks good to me except for the fixate_caps() hack. Is there no better way, > maybe by adding some new API to basetransform? :) Probably, I can open a bug, but I'd like to keep that separate, as it's not related and this will raise a lot of questions, which we may prefer fixing by implementing GstAsyncBaseTransform (which will be able to work synchroneously). > > ::: gst/videocrop/gstvideocrop.c > @@ +704,3 @@ > + goto done; > + > + if (!gst_pad_push_event (trans->srcpad, gst_event_new_caps (othercaps))) > > gst_base_transform_update_src_caps()? Sending a caps event behind > basetransform's back is probably not a good idea. What's undocumented about this helper is that it will mark the srcpad for reconfigure, so it goes through full renegotiation next time. If you do that in this case you'll have an infinite recursion. > > @@ +708,3 @@ > + > + query = gst_query_new_allocation (othercaps, FALSE); > + if (!gst_pad_peer_query (trans->srcpad, query)) > > And this would cause two allocation queries to be done, with the result of > this one being ignored more or less? The FALSE means I don't need a pool, which reduces the amount of extra work, and the second query will be faster because the pipeline will already be drained. It's the only way for now, same thing happens for textoverlay.
Created attachment 365399 [details] [review] basetransform: Introduce decide_meta virtual function This virtual function is useful for sub-class that needs to decide if they are going to do passthrough or in-place transformation base on the supported meta API discovered through the allocation query. When this virtual method is set, the GstBaseTransform will first perform an allocation without requesting a pool. Then, if the element is neither passthrough or in-place, it will perform a normal allocation query and call decide_allocation() as usual.
Created attachment 365400 [details] [review] videocrop: Add GstVideoCropMeta support If downstream supports this meta, it will add or update it from the GstBuffer in-place rather then copying.
So I just changed my mind ;-P And added a new virtual method to GstBaseTransform, that's make the code straightforward. Now time to do what we all like to do, decide if that new method name is correct. GstBaseTransform::decide_meta I'm not yet convince to be fair, I didn't think about it much. I feel like it's not really deciding metas but in fact deciding base on the meta. For now, it's being used for passthrough/in-place decision, it's likely the only use case, as for anything not going passthrough or in-place, we can read that information from decide_allocation.
Review of attachment 365399 [details] [review]: ::: libs/gst/base/gstbasetransform.c @@ +916,3 @@ + "doing allocation query without requesting a pool"); + + query = gst_query_new_allocation (outcaps, FALSE); Can't the same be done on the allocation query that is already sent below? In theory both could result in different answers, and the one below is what we configure later
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 365399 [details] [review] [review]: > > ::: libs/gst/base/gstbasetransform.c > @@ +916,3 @@ > + "doing allocation query without requesting a pool"); > + > + query = gst_query_new_allocation (outcaps, FALSE); > > Can't the same be done on the allocation query that is already sent below? > In theory both could result in different answers, and the one below is what > we configure later Can you clarify your thought, I don't understand.
There are two allocation queries being done now, or not? And the results to both could be different, and the second allocation query is the one we actually configure for so anything we got from the first could be invalid.
(In reply to Sebastian Dröge (slomo) from comment #9) > There are two allocation queries being done now, or not? If the sub-class decides to passthrough or in-place, it will have run only 1 allocation query. > And the results to > both could be different, and the second allocation query is the one we > actually configure for so anything we got from the first could be invalid. For no reason in this context the first or the second allocation query should be invalid. If this happens for you in some of your code, you'll get random issues really. Remember there is no acknowledgment in this mechanism. The reason I'm proposing it this way, is to avoid the creation of a pool, so I optimized for the passthrough/in-place mode. I guess I could try and move the passthrough/in-place check after the allocation query, the uneeded bufferpool/allocator will simply be unreffed. What I wanted to avoid is to steel the buffer pool for single pool downstream elements (like v4l2 and OMX). But rethinking it, it will just be unreffed, without being activated.
Created attachment 365512 [details] [review] basetransform: Allow going passthrough inside decide_allocation Sub-class may want to decide to go passthrough/in-place by inspecting the support meta APIs. This patch duplicates the check for this mode, so we still don't do uneeded allocation query while we allow sub-classes to switch the behaviour during it's own decide_allocation call. Notice that such sub-class need to reset the class to non-passthrough in set_caps() in order for decide_allocation to be called again. This is needed otherwise we'd be doing an allocation query in element in which it make no sense (notably capsfilter).
Created attachment 365513 [details] [review] basetransform: Allow going passthrough inside decide_allocation Sub-class may want to decide to go passthrough/in-place by inspecting the support meta APIs. This patch duplicates the check for this mode, so we still don't do uneeded allocation query while we allow sub-classes to switch the behaviour during it's own decide_allocation call. Notice that such sub-class need to reset the class to non-passthrough in set_caps() in order for decide_allocation to be called again. This is needed otherwise we'd be doing an allocation query in element in which it make no sense (notably capsfilter).
Created attachment 365514 [details] [review] videocrop: Add GstVideoCropMeta support If downstream supports this meta, it will add or update it from the GstBuffer in-place rather then copying.
This revision does not introduce a new API anymore. The only side effect is that to get decide_allocation to be called upon renegotiation, the sub-class must reset passthrough/in-place to FALSE. Not a big deal, but clearly something we might forget about often, and which does not immediately causes issues. And before you ask, I already thought about just moving the checks, but then you get an allocation query in capsfilter, which is not causing any issues, but isn't nice.
Comment on attachment 365513 [details] [review] basetransform: Allow going passthrough inside decide_allocation Attachment 365513 [details] pushed as 443221c - basetransform: Allow going passthrough inside decide_allocation
Attachment 365514 [details] pushed as 3a17f52 - videocrop: Add GstVideoCropMeta support