GNOME Bugzilla – Bug 740671
aspectratiocrop: crop needs to be reset when video size changes
Last modified: 2014-12-01 14:28:03 UTC
Created attachment 291434 [details] [review] fix with continuous playback of multiple files, the transition to a file with a smaller video size can causes the negotiation problem. example (for aspect-ratio 3:2) first video: 1280x720, crop: l=160,r=160,b=0,t=0 second video: 320x320, crop: l=0,r=0,b=40,t=40 Caps negotiations take place before setting the crop, and for the second video crop will still l=160,r=160,b=0,t=0 320x320 => (crop l=160,r=160,b=0,t=0) => 0x320 => negotiation error
continuous playback still does not work!!! Bug 738285
crop can be reset only when you change the caps of the stream, if caps not changed, the new crop will not be set
Comment on attachment 291434 [details] [review] fix This looks wrong. You shouldn't have to worry about links to any other pads, and also the stream-start event is irrelevant for cropping. The only change that should be necessary is in the handling of the CAPS event. You will get a CAPS event for the new stream, and then would set up the new cropping values accordingly and then send the appropriate CAPS event downstream with the new crop values.
Negotiations are performed by videocrop filter (not aspectratiocrop). Stream-start does not perform cropping, only reset videocrop settings for successful negotiations in the future. The problem is that the new crop values are set after negotiations
A unit test addition to tests/check/elements/aspectratiocrop.c might be helpful, in case you're up for it.
... The problem is that the new crop values are set after negotiations *that will fail* if new_width - *old_crop* <= 1
Created attachment 291443 [details] [review] fix 2 even simpler
I'm not sure if a query should really change state in an element, or if an element should rely on a query for anything.
I will try to write the test
It shouldn't, no. This seems like a workaround for a deeper problem somewhere
Perhaps you are right and the problem is deeper. But I need to work, and not to have been canonically
GST_EVENT_CAPS should be a good starting point.
GST_EVENT_CAPS event is handled in the aspectratiocrop, in this handler is given a new crop vlaue. But the negotiations at this point already will fail
How do they fail? You might also need to adjust the values returned in the CAPS queries (but adjusting the query values, not element state!).
Created attachment 291462 [details] log Pay attention to the time of 10 seconds, then there is a transition to the next video
Created attachment 291527 [details] [review] fix the solution is simple. what was the reason to do so initially
Created attachment 291529 [details] figure Now xvimagesink cuts off the black borders
aspectratiocrop receives the caps-event and sends it downstream, and only then changes the value of crop, so to smaller video first used crop values from the previous video and then the desired crop.
comment 17 xvimage does not truncate the frame but sets the size of the window, corresponding to the new frame size
Review of attachment 291527 [details] [review]: Can you add a commit message that is a bit more descriptive, and that explains why the baseclass needs to be called after instead of before ?
example continuous playback: first video - 1280x720 second video - 320x240 aspect ratio - 4:3 pipeline: source/playbin -> aspectratio (videocrop) -> xvimagesink 1. first video, source send caps-event1(width=1280,height=720) 2. aspectratio receive caps-event1 3. -- aspectratio send caps-event1 downstream 4. -- videocrop (crop t=0,b=0,l=0,r=0) send caps-event1 downstream 5. -- xvimagesink receive caps-event1, set output window size *1280x720*!!! 6. aspectratio set new crop (t=0,b=0,l=160,r=160) 7. videocrop (t=0,b=0,l=160,r=160) 8. playing... 9. second video, source send caps-event2 (width=320, height=240) 10. aspectratio receive caps-event2 11. -- aspectratio send caps-event2 downstream 12. -- videocrop (crop t=0,b=0,l=160,r=160) changed caps-event2 to caps-event3 (width=320 - l - r, height=240 - t - b)=>(width:0, height:240) 13. -- xvimagesink receive caps-event3 (width=0,height=240) and throw error-message 14. aspectratio set new crop (t=0, b=0, r=0, l=0) too late if we switch the step 3 and 6, 11<->14 then videocrop get the freshest information on time, then: 8. xvimagesink receive caps-event (width=960,height=720) and 13. xvimagesink receive caps-event (width=320, height=240) I have bad speaking skills, change the commit message itself
comment 21 step 6 change to 2.1 step 14 change to 10.1
Created attachment 291700 [details] [review] fix When an caps-event is received, we must immediately change the crop to videocrop correctly changed caps-event dimension, otherwise the videocrop will first use the previous value of the crop that when resizing video to a smaller may cause an error.
Thanks for the patch. This one actually makes sense :) I like bugs that are fixed by removing code! commit 6348de195d1e29d43518b21a19659e852177e9f2 Author: Andrei Sarakeev <sarakusha@gmail.com> Date: Wed Nov 26 10:33:09 2014 +0300 aspectratiocrop: Handle resolution changes properly When an caps-event is received, we must immediately change the crop to videocrop correctly changed caps-event dimension, otherwise the videocrop will first use the previous value of the crop that when resizing video to a smaller resolution may cause an error. https://bugzilla.gnome.org/show_bug.cgi?id=740671