After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 740671 - aspectratiocrop: crop needs to be reset when video size changes
aspectratiocrop: crop needs to be reset when video size changes
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.4.5
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2014-11-25 07:23 UTC by Andrei Sarakeev
Modified: 2014-12-01 14:28 UTC
See Also:
GNOME target: ---
GNOME version: ---

fix (3.53 KB, patch)
2014-11-25 07:23 UTC, Andrei Sarakeev
needs-work Details | Review
fix 2 (3.36 KB, patch)
2014-11-25 12:17 UTC, Andrei Sarakeev
none Details | Review
log (323.63 KB, text/plain)
2014-11-25 14:20 UTC, Andrei Sarakeev
fix (1.25 KB, patch)
2014-11-26 07:44 UTC, Andrei Sarakeev
needs-work Details | Review
figure (5.56 KB, image/png)
2014-11-26 08:02 UTC, Andrei Sarakeev
fix (2.28 KB, patch)
2014-11-28 06:29 UTC, Andrei Sarakeev
committed Details | Review

Description Andrei Sarakeev 2014-11-25 07:23:10 UTC
Created attachment 291434 [details] [review]

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
Comment 1 Andrei Sarakeev 2014-11-25 08:18:38 UTC
continuous playback still does not work!!!
Bug 738285
Comment 2 Andrei Sarakeev 2014-11-25 09:02:52 UTC
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 3 Sebastian Dröge (slomo) 2014-11-25 09:47:21 UTC
Comment on attachment 291434 [details] [review]

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.
Comment 4 Andrei Sarakeev 2014-11-25 11:08:57 UTC
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
Comment 5 Tim-Philipp Müller 2014-11-25 11:19:33 UTC
A unit test addition to tests/check/elements/aspectratiocrop.c might be helpful, in case you're up for it.
Comment 6 Andrei Sarakeev 2014-11-25 11:49:55 UTC
... The problem is that the new crop values are set after negotiations *that
will fail* if new_width - *old_crop* <= 1
Comment 7 Andrei Sarakeev 2014-11-25 12:17:32 UTC
Created attachment 291443 [details] [review]
fix 2

even simpler
Comment 8 Tim-Philipp Müller 2014-11-25 12:23:39 UTC
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.
Comment 9 Andrei Sarakeev 2014-11-25 12:27:51 UTC
I will try to write the test
Comment 10 Sebastian Dröge (slomo) 2014-11-25 12:28:49 UTC
It shouldn't, no. This seems like a workaround for a deeper problem somewhere
Comment 11 Andrei Sarakeev 2014-11-25 12:37:24 UTC
Perhaps you are right and the problem is deeper. But I need to work, and not to have been canonically
Comment 12 Tim-Philipp Müller 2014-11-25 12:45:01 UTC
GST_EVENT_CAPS should be a good starting point.
Comment 13 Andrei Sarakeev 2014-11-25 14:05:11 UTC
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
Comment 14 Sebastian Dröge (slomo) 2014-11-25 14:07:12 UTC
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!).
Comment 15 Andrei Sarakeev 2014-11-25 14:20:11 UTC
Created attachment 291462 [details]

Pay attention to the time of 10 seconds, then there is a transition to the next video
Comment 16 Andrei Sarakeev 2014-11-26 07:44:17 UTC
Created attachment 291527 [details] [review]

the solution is simple. what was the reason to do so initially
Comment 17 Andrei Sarakeev 2014-11-26 08:02:16 UTC
Created attachment 291529 [details]

Now xvimagesink cuts off the black borders
Comment 18 Andrei Sarakeev 2014-11-26 09:45:48 UTC
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 19 Andrei Sarakeev 2014-11-26 09:55:05 UTC
comment 17
xvimage does not truncate the frame but sets the size of the window, corresponding to the new frame size
Comment 20 Olivier Crête 2014-11-26 21:28:41 UTC
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 ?
Comment 21 Andrei Sarakeev 2014-11-27 05:56:31 UTC
example continuous playback:
first video - 1280x720
second video - 320x240
aspect ratio - 4:3
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)
13. xvimagesink receive caps-event (width=320, height=240)

I have bad speaking skills, change the commit message itself
Comment 22 Andrei Sarakeev 2014-11-27 06:15:15 UTC
comment 21
step 6 change to 2.1
step 14 change to 10.1
Comment 23 Andrei Sarakeev 2014-11-28 06:29:00 UTC
Created attachment 291700 [details] [review]

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.
Comment 24 Sebastian Dröge (slomo) 2014-11-28 10:21:12 UTC
Thanks for the patch. This one actually makes sense :) I like bugs that are fixed by removing code!

commit 6348de195d1e29d43518b21a19659e852177e9f2
Author: Andrei Sarakeev <>
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.