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 740787 - videocrop: No longer apply the new crop if caps have not changed
videocrop: No longer apply the new crop if caps have not changed
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 1.5.1
Assigned To: Nicolas Dufresne (ndufresne)
GStreamer Maintainers
Depends on:
Reported: 2014-11-26 22:11 UTC by Nicolas Dufresne (ndufresne)
Modified: 2014-12-15 23:31 UTC
See Also:
GNOME target: ---
GNOME version: ---

videocrop: Make sure new crop is applied (3.11 KB, patch)
2014-11-26 22:11 UTC, Nicolas Dufresne (ndufresne)
reviewed Details | Review

Description Nicolas Dufresne (ndufresne) 2014-11-26 22:11:35 UTC
Created attachment 291602 [details] [review]
videocrop: Make sure new crop is applied

Since "basetransform: Fix caps equality check" commit a7f357,
set_info() will not be called anymore if crop didn't change
the caps. This patch fixes it by remembering if crop needed to
be applied. This check if made in transform_caps, so during
negotiation to avoid the race.

Some note about the fix, it does not fix the race that always existed when calling set_property() in between transform_caps() and set_info() (one need to use a pad probe to make it work). It also does not fix that this element may fail without using GST_ELEMENT_ERROR().
Comment 1 Sebastian Dröge (slomo) 2014-11-27 09:18:37 UTC
Comment on attachment 291602 [details] [review]
videocrop: Make sure new crop is applied

Seems like a good solution for now, it doesn't make things worse ;)

Might be good with some refactoring though, I don't see why videocrop should carry around the caps. It probably only uses the info anyway and only has the caps here because GstVideoFilter has that in the vfunc
Comment 2 Nicolas Dufresne (ndufresne) 2014-11-27 14:28:23 UTC
Caps are only used for debug printing indeed, I don't like storing it in the element either. Just a note, I didn't realize that the element is using both the OBJECT_LOCK and a custom lock, so it is likely I'm using it wrong. I have no idea why a custom lock is needed, neither what it could solve.
Comment 3 Sebastian Dröge (slomo) 2014-11-28 10:18:24 UTC
Probably because more mutexes are better ;)

From looking at the code it should just use the object lock. Would you mind updating your patch to do that and also get rid of the caps?
Comment 4 Nicolas Dufresne (ndufresne) 2014-12-15 23:31:26 UTC
commit 36f1a9bce1454701d15dbb616f69eb3bddc3c9c3
Author: Nicolas Dufresne <>
Date:   Mon Dec 15 18:19:05 2014 -0500

    videocrop: Make sure new crop is applied