Also what needs to be done for that:

a) base classes don't allow to set the caps in the ALLOCATION query, resulting in them being the display/cropped caps. So the sink only gets them in the query, but later e.g. the decoder sets the uncropped caps in the buffer pool configuration. That was always the case in theoradec and mpeg2dec before, so not really a new problem.

b) theoradec (and possibly other code) assumes that passing a "random" video-info to GstVideoFrame API does something sensible. It now causes an assertion as it should Jan's patch should fix that, but apparently it breaks something else for some reason. Not sure why, it should make no difference to what we had before.

c) Nicolas thinks that by default basetransform passes through metas in the ALLOCATION query if the subclass runs in in_place mode. I didn't check this but I hope it's not true and something else was weird in his tests :) basetransform should drop all metas by default, that's why the filter_meta vfunc exists. Subclasses like GstVideoFilter will than retain GstVideoMeta but will drop everything else.

d) Many filters and textoverlay and things claim to support CROP meta, assuming that they don't need to do anything and in the worst case just also do their work on the bigger area and are just a bit slower because of that. That's wrong though, at least for elements that are positioning something. It will work as long as only cropping at the bottom or left is done, but otherwise the elements will have to translate.

With that explanation, what's the point of the cropmeta? To use it, you need a GstVideoMeta which contains the offset (giving you the top right corner), and you have the CAPS event (giving you the width/height). What does the CropMeta add to this?

Or is the area described by the crop meta a subset area described by the videometa ?

The GstVideoMeta contains the entire framebuffer, GstCropMeta gives you the subset area inside that, as well as a way for elements to negotiate that that's a supported way of operating.

(In reply to Jan Schmidt from comment #0)
> Currently GstVideoCropMeta implementation is problematic - caps width/height
> values and GstVideoMeta settings are a bit jumbled, and things need fixing
> and documenting across the board.
> 
> From slomo in bug #74010, it should work like this:
> 
> - CAPS event contains the display width/height (otherwise applications will
> become unhappy, GstDiscoverer reports wrong information, etc)
> - caps inside the ALLOCATION query contain the memory width/height

I'm not sure I agree with this one yet. Using caps to tell the pool what padding need to be allocated is weird. The padded width/height being in VideoMeta should be enough. I think we should be able to tell the pool the VideoAlignment we want, and by having CROP META option enable, we'd get frames that maps to the larger image instead of the cropped region.

> - GstVideoMeta contains the memory width/height

Agreed.

> - GstVideoCropMeta contains how much should be cropped relative to the
> values inside the video meta and the ALLOCATION query caps

I think the crop region should fit in the VideoMeta width/height. Allocation caps should remain the negotiated caps imho.

(In reply to Nicolas Dufresne (stormer) from comment #5)
> > - CAPS event contains the display width/height (otherwise applications will
> > become unhappy, GstDiscoverer reports wrong information, etc)
> > - caps inside the ALLOCATION query contain the memory width/height
> 
> I'm not sure I agree with this one yet. Using caps to tell the pool what
> padding need to be allocated is weird. The padded width/height being in
> VideoMeta should be enough. I think we should be able to tell the pool the
> VideoAlignment we want, and by having CROP META option enable, we'd get
> frames that maps to the larger image instead of the cropped region.

VideoAlignment is completely orthogonal to the CROP meta and should not be a requirement for using it.

Inside the ALLOCATION query in think we should have the memory width/height because downstream elements might decide to offer a buffer pool or not based on that, or to offer different buffer pools. E.g. think of xvimagesink which has a maximum allocation size for which it can provide shared memory.

Not really a blocker, it was broken since the beginnings of time

I wonder how the proposed changes here could have helpd solving recent issues we had with libav video decoding. Here's the situation:

What happens in libav is that the video decoder only know the memory size at the time of the allocation. As of now, because we still rely on sending caps (hence display size) before doing the allocation query, the decoder will create an internal (and tempory) pool with the requires alignments, and caps with the memory size as described here. Later the decoder is informed of the display size and negotiate (both the caps and the pool, though with display size). Later, we find that downstream pool is not suitable for our needs (e.g. missing VideoAlignment) but downstream do support the VideoMeta. We'd like to simply push from our internal pool, but that leads to later failure downstrea, because the width/height of the GstVideoMeta is larger. While this is acceptable for frame_map() this completly fails for video_frame_copy(). Vaapisink (unexpectedly) endup giving video_frame_copy() a src frame that is bigger (memory size) then the dest frame (display size).

So the question is how precisely this should be handled ? I understand we'd like avviddec to run the allocation query with the memory size in caps instead (in the future even run that before caps if ever possible), but as avviddec is not using downstream pool. It seems like a lot of work to for the the sink to remember the memory size in order to use that for frame_copies later. Shall we simply allow copying bigger to small in video_frame_copy ?

The problem here is that this would change the behaviour as existing code might assume that (if there is no crop meta) the width/height/etc in the video meta and caps are exactly the same.

One way to get around this would be to additionally negotiate a new meta like the crop meta for providing this information.

Ok, all this is complicated to implement in avviddec, mostly because of the mapped buffers. You still get to know the image size after you have configured/activated the pool. When you get this information, the buffer are already mapped (hence meta can no longer be added without copying). I think what we have implemented right now is the least worst solution, a remap would be nicer of course.

*** Bug 762543 has been marked as a duplicate of this bug. ***

See https://bugzilla.gnome.org/show_bug.cgi?id=762543#c17 for an implementation plan

Some heads up, in order to help fixing this, I've added crop meta support to videocrop element. This way we can generate any kind of crop meta for testing. As expected, it failed badly with pretentious elements that thinks they can handle crop meta without looking at it (videoconvert ;-P). For now, videoconvert no longer pretend that, a solution to that is required to figure-out how to control the allocation caps in GstBaseTransform, I'll file a sep bug. Fixing this will open up a large amount of use cases.

Second row of issues, sink sometimes need to copy images. But sinks tends to copy to a pool initialized from caps, which is not padded. That cause the gst_video_frame_copy() call to fail. The sink needs to do lazy allocation of internal pool, and base their pool on the GstVideoMeta width/height. As a side effect, it needs to check for any GstVideoMeta changes. This reminds me a bit the caps in 0.10 though. Note that looking at decide_allocation won't work, the pool used to allocate the buffers your be larger without coming from the sink pool.

https://bugzilla.gnome.org/show_bug.cgi?id=791449

I've create this one, https://bugzilla.gnome.org/show_bug.cgi?id=791764 for the basetransform limitation. Note that we still need a good use case for this, the videoconvert use case is a bit weird, because it seems less efficient, but there is corner cases were it might not be.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/172.