(In reply to sreerenj from comment #0)
> The xvimagesink seems to create a pool in set_caps() and which is always
> using as sink's internal pool irrespective of whether we create new pool in
> propose_allocation() or not.
> This is wrong when handling multi-resolution video rendering. T
Err, not about mutli-resolution video, but when handing buffers with GstVideoCropMeta attached

It would be good to use the pool independent of propose_allocation() I guess, and if upstream provides different buffers we would copy the buffers into the ones from our pool.

The resolution in propose_allocation() would be the allocation resolution, the one in set_caps() is the display resolution. And then for each buffer you need to check the crop meta to decide how much should be cropped and how. And you can get a propose_allocation() that is completely unrelated to the current CAPS and buffers you receive (maybe upstream has more buffers queued up in some old format, maybe upstream decided to not use your pool and do things differently, ...).

(In reply to Sebastian Dröge (slomo) from comment #2)
> It would be good to use the pool independent of propose_allocation() I
> guess, and if upstream provides different buffers we would copy the buffers
> into the ones from our pool.
> 
> The resolution in propose_allocation() would be the allocation resolution,
> the one in set_caps() is the display resolution. And then for each buffer
> you need to check the crop meta to decide how much should be cropped and
> how. And you can get a propose_allocation() that is completely unrelated to
> the current CAPS and buffers you receive (maybe upstream has more buffers
> queued up in some old format, maybe upstream decided to not use your pool
> and do things differently, ...).

This is exactly the case here, how to handle gst_video_frame_map()/gst_video_frame_copy() in gst_xv_image_sink_show_frame..

We can't use the GstVideoInfo received in set_caps() , The actual video_info should be the one derived from propose_allocation.

We only do pool allocation in propose_allocation() now btw, is this bug still valid ?

For the reference:

commit df313a6dbc563cd4922f9897316516b760738ba4
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Fri Jun 5 14:30:12 2015 -0400

    xvimagesink: Don't share internal pool
    
    Sharing the internal pool results in situation where the pool may have
    two upstream owners. This creates a race upon deactivation. Instead,
    always offer a new pool, and keep the internal pool internal in case
    we absolutely need it.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=748344

So the pool created in set_caps() is only used for when upstream buffer are not using the proposed pool. I'm not sure how you get a miss-match in video_info here. Are you using an upstream pool with a size that does not match the one allocated in the caps ?

(In reply to Nicolas Dufresne (stormer) from comment #5)
> For the reference:
> 
> commit df313a6dbc563cd4922f9897316516b760738ba4
> Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
> Date:   Fri Jun 5 14:30:12 2015 -0400
> 
>     xvimagesink: Don't share internal pool
>     
>     Sharing the internal pool results in situation where the pool may have
>     two upstream owners. This creates a race upon deactivation. Instead,
>     always offer a new pool, and keep the internal pool internal in case
>     we absolutely need it.
>     
>     https://bugzilla.gnome.org/show_bug.cgi?id=748344
> 
> So the pool created in set_caps() is only used for when upstream buffer are
> not using the proposed pool. I'm not sure how you get a miss-match in
> video_info here. Are you using an upstream pool with a size that does not
> match the one allocated in the caps ?

The reason by xvimagesink can work with vaapidecode is that vaaapidecode is always reporting uncropped values to xvimagesink->set_caps(),
but I am going to change this to report cropped dimension (display size)...

Now the issue is:
vaapidecode will report the cropped values, and xvimagesink will maintain itz own pool created in setcaps based on this cropped resolution.
But then vaapidecode will create itz own pool with uncropped values and those values will be only available in xvimagesink->propose_allocation.
In this scenario, vaapidecode and xvimagesink have their own pools and xvimagesink is trying to copy the frames with uncropped values which will
simply break the playback.

issues:
1: gst_video_frame_map(src_buffer) with wrong resolution which is saved in set_caps
2: gst_video_frame_copy(dst_buffer, src_buffer) where dst_buffer and src_buffer have different resolution..

Can you provide examples of the crop values ? I believe these are UV aligned, and that the problem here is not what xvimagesink (and literally all other sink) are doing, but the fact that you should implement VideoAlignment, so the crop get buried into VideoMeta. You can always keep your own meta if you need to remember the padded dimension.

Created attachment 322223 [details]
sample video created for testing

original dimension 330x240
cropped dimension 336x240

(In reply to sreerenj from comment #8)
> Created attachment 322223 [details]
> sample video created for testing
> 
> original dimension 330x240
> cropped dimension 336x240

Do you mean padded dimension ? You cannot crop bigger then the original. What is the x and y of your cropping rectangle ? If it's (0, 0), then this is VideoCropMeta is clearly the wrong mechanism for you. Instead you should expose the display width/height in the caps and use VideoMeta + GstVideoAlignment, so the crop is expressed in term of strides and offset.

(In reply to Nicolas Dufresne (stormer) from comment #9)
> (In reply to sreerenj from comment #8)
> > Created attachment 322223 [details]
> > sample video created for testing
> > 
> > original dimension 330x240
> > cropped dimension 336x240

Aha sorry, typo
> > original dimension 336x240
> > cropped dimension 330x240

> 
> Do you mean padded dimension ? You cannot crop bigger then the original.
> What is the x and y of your cropping rectangle ? If it's (0, 0), then this
> is VideoCropMeta is clearly the wrong mechanism for you. Instead you should
> expose the display width/height in the caps and use VideoMeta +
> GstVideoAlignment, so the crop is expressed in term of strides and offset.

(In reply to Nicolas Dufresne (stormer) from comment #7)
> Can you provide examples of the crop values ? I believe these are UV
> aligned, and that the problem here is not what xvimagesink (and literally
> all other sink) are doing, but the fact that you should implement
> VideoAlignment, so the crop get buried into VideoMeta. You can always keep
> your own meta if you need to remember the padded dimension.

Wondering what is the use case of GstVideoCropMeta then?? :)

GstVideoAlignment and GstVideoCropMeta are completely different things that only overlap slightly in some use cases. And if you use the crop meta correctly, things should work correctly nonetheless. Correctly means: caps contain display size, allocation query contains memory size, crop meta contains cropping that has to be done to get from memory size to display size.

(In reply to Sebastian Dröge (slomo) from comment #12)
>. And if you use the crop meta
> correctly, things should work correctly nonetheless. Correctly means: caps
> contain display size, allocation query contains memory size, crop meta
> contains cropping that has to be done to get from memory size to display
> size.

This is the point i was talking too, xvimagesink need a fix in this scenario..
Am i missing something??

(In reply to sreerenj from comment #11)
> Wondering what is the use case of GstVideoCropMeta then?? :)

It was introduced I believe to try and support theora specification. In theora, encoder can decide a x,y,width,height crop, regardless of the uv alignment. The spec says the encoder SHOULD respect the alignment though. Our encoder always set x/y to 0, which is always aligned with the sub-sampling. The problem is that the crop meta is badly defined. Currently, it is only usable if the caps are the GstBuffer original size. Otherwise you don't know what VideoMeta to pass when mapping the frame. In the original VideoFrame implementation, it would simply ignore the width/height from the caps, and use the one from VideoMeta, though this would lead to certain issues with x/y is not zero. Some assertions preventing that were added since, as this would lead to buffer oveflow. Maybe this was the wrong fix, it remains that using VideoMeta + VideoAlignment is the most tested, and most natural thing to do. This kind of padding is meant to be stored as stride and offset array. Though, with sub-sampling, this impose a certain alignment, which you have here.

(In reply to Sebastian Dröge (slomo) from comment #12)
> GstVideoAlignment and GstVideoCropMeta are completely different things that
> only overlap slightly in some use cases. And if you use the crop meta
> correctly, things should work correctly nonetheless. Correctly means: caps
> contain display size, allocation query contains memory size, crop meta
> contains cropping that has to be done to get from memory size to display
> size.

Not in this case. The proposed pool from xvimagesink is not used, the sink does not save the caps from the allocation query. It only works if you use the pool atm. The caps in the query where never meant to be a second set_format() imho. This case is fixable using the video alignment, which means keeping the cropped width/height in caps and setting the VideoMeta properly. Meanwhile, VideoCropMeta is not usable here, also nothing in the design or doc seems to help with that. There is a bug about it, but until it's fixed, it's better to avoid it when not required.

(In reply to Nicolas Dufresne (stormer) from comment #14)
>
> It was introduced I believe to try and support theora specification

It's also for actually cropping video but having the sink do that for you more efficiently. You could e.g. think of a container that has crop information and only wants to show a 320x320 part of the center of a bigger video.

(In reply to Sebastian Dröge (slomo) from comment #15)
> (In reply to Nicolas Dufresne (stormer) from comment #14)
> >
> > It was introduced I believe to try and support theora specification
> 
> It's also for actually cropping video but having the sink do that for you
> more efficiently. You could e.g. think of a container that has crop
> information and only wants to show a 320x320 part of the center of a bigger
> video.

gstreamer-vaapi depends heavily on GstVideoCropMeta too :)

Summary from IRC (this should become a duplicate of the other crop meta bug):

<ndufresne> slomo: using crop meta won't work for the scenario where downstream pool is not used, this would require all video sink that supports crop meta to interpret propose_allocation as being a set_padded_caps() or something
<slomo> ndufresne: why would they have to interprete and allocation query as some kind of set_caps()?
<ndufresne> slomo: the problem is that the sink need to copy that frame, because it's wrong memory type, for that it uses GstVideoFrame, and the only video information we have is that caps
<ndufresne> Because this is how everything is supposed to work
<ndufresne> but now, the incoming buffers have a width/height in it's GstVideoMeta that matchs the caps passed in proposed_allocation
<ndufresne> so frame_map() fails, no video sink is expecting that atm
<slomo> i see
<slomo> that sounds solveable though?
<slomo> everything supporting crop meta should handle that
<slomo> videoinfo from caps to frame_map() with display size, buffer contains GstVideoInfo with memory size... and then things should do something sensible ;)
<ndufresne> slomo: we could, it makes everything more complex then it should imho
<ndufresne> I believe the original idea, was that sink don't need to extract the VideoMeta and make a decision
<ndufresne> That's why video_frame_map() was overriding width/height silently before
<ndufresne> slomo: do you remember exactly why you added this restriction ?
<ndufresne> cause, an element that expect CropMeta, would expect larger width/height in the VideoInfo structure after mapping the VideoFrame
<slomo> ndufresne: yes, i didn't consider crop meta ;)
<slomo> ndufresne: i think if crop meta is on the buffer, gst_video_frame_map() must check different restrictions. that the meta+crop resolution is equal to the one from the video info
<ndufresne> I think what I'd like, is to be able to say that we can map a frame without having to extract the VideoMeta ourself, this is the main feature of using VideoFrame after all
<slomo> ndufresne: i added this restriction because there were elements that forgot to update their caps/video info and then just crashed in interesting ways
<ndufresne> slomo: that is a good option
<ndufresne> Now, it's not enought
<slomo> why not enough?
<ndufresne> yes, we need to improve video_frame_copy() too
<slomo> oh, sure
<ndufresne> it does not support odd crops
<slomo> but frame_map() should be happy with that so far
<ndufresne> so my conclusion, correct me if wrong, is that what xvimagesink (and all others) do, blindly so frame_copy() is correct, and we need to check the presence of a crop meta in frame_map
<slomo> yes
<slomo> and adjust the sanity check for width/height accordingly
<ndufresne> sounds to be like something I can work on, as a first step, I'll try and also detect odd crops (that could cause UV shift) and warn
<slomo> can you put our discussion in the bug for future reference?
<ndufresne> yes
<slomo> isn't there also another bug about the general crop meta problem somewhere?
<ndufresne> yes, there is I believe, need to find it, with this conclusion, it's now a duplicate
<ndufresne> slomo: if I implement CropMeta in videocrop, what would the downstream caps be btw ?
<slomo> the same as they are now
<slomo> the caps event has the caps it would have now too
<ndufresne> ... ! video/x-raw,width=100,height=100 ! videocrop top=10 bottom=10 left=10 right=10 ! video/x-raw,...
<slomo> only difference is the allocation query, it would have the sinkpad caps
<slomo> 80x80 in your example
<ndufresne> ok, 80x80, with a crop meta on buffers, that we agree then, great
<ndufresne> for a long time, I believed that crop meta was something we could add to a buffer, without affecting the caps
<slomo> so that in frame_map() you can calculate 80+10+10==100 and fail otherwise ;)
<slomo> yes the documentation about this is not existing
<ndufresne> well, the crop meta is using a rectangle (x,y,width,height), but we need to check that it's within the original size of course

Marking as a duplicate now.

*** This bug has been marked as a duplicate of bug 746087 ***

(In reply to Sebastian Dröge (slomo) from comment #17)
> Summary from IRC (this should become a duplicate of the other crop meta bug):

Sorry I was not in the irc,

So If I understood correctly from the chat above, I can survive with GstVideoCropMeta in gstreamer-vaapi as it is now (but of course , have to advertise the cropped values in set_caps)...right?

(In reply to sreerenj from comment #19)
> (In reply to Sebastian Dröge (slomo) from comment #17)
> > Summary from IRC (this should become a duplicate of the other crop meta bug):
> 
> Sorry I was not in the irc,
> 
> So If I understood correctly from the chat above, I can survive with
> GstVideoCropMeta in gstreamer-vaapi as it is now (but of course , have to
> advertise the cropped values in set_caps)...right

This depends on fixing some issues in VideoFrame API first. I still recommend you setting cropped width/height everywhere and setup you strides and offset appropriately. Crop here makes everything much more complex.

Also, it makes gstreamer-vaapi works with software video filters, most of them don't support the crop meta. I even believe some advertise it, but will pick the wrong sub-picture for doing the actual work.

(In reply to Nicolas Dufresne (stormer) from comment #20)
> (In reply to sreerenj from comment #19)
> > (In reply to Sebastian Dröge (slomo) from comment #17)
> > > Summary from IRC (this should become a duplicate of the other crop meta bug):
> > 
> > Sorry I was not in the irc,
> > 
> > So If I understood correctly from the chat above, I can survive with
> > GstVideoCropMeta in gstreamer-vaapi as it is now (but of course , have to
> > advertise the cropped values in set_caps)...right
> 
> This depends on fixing some issues in VideoFrame API first. I still
> recommend you setting cropped width/height everywhere and setup you strides
> and offset appropriately. Crop here makes everything much more complex.

Through GstVideoAlignment?
I am not sure whether it is safe to do for next release (if we plan to release a 1.8 together with official release).. Required regression testing over all codecs...

If you can use it (no cropping in sub-subsampling pixel granularity) and there's a pool that supports it and downstream supports it, then it is a possibly more efficient solution than the crop meta. As a fallback crop meta should work fine too though, we just need to fix $things