GNOME Bugzilla – Bug 687655
videocrop: Add support for automatic cropping (GST 0.10)
Last modified: 2012-11-07 10:50:01 UTC
The videocrop element does not currently allow automatic cropping. The attached patched enables automatic cropping by using -1 on the left, top, right or bottom properties. The cropping is done equally on both side in both are set to -1.
Created attachment 228127 [details] [review] videocrop: Add support for automatic cropping This change enable automatic cropping using -1 set to left, top, right or bottom property. In the case both side are set to automatic cropping, the croping will be done equally on both side (in the odd case, right and bottom cropping will be 1 pixel more).
I'll also provide a 1.0 version soon ...
Review of attachment 228127 [details] [review]: It would be lovely to have unit tests. Also, I think the caps negotiation may be broken if you crop on left and have it dynamic on the rightside.
Created attachment 228234 [details] [review] videocrop: Style fix
Created attachment 228235 [details] [review] videocrop: Add support for automatic cropping This change enable automatic cropping using -1 set to left, top, right or bottom property. In the case both side are set to automatic cropping, the croping will be done equally on both side (in the odd case, right and bottom cropping will be 1 pixel more).
Review of attachment 228235 [details] [review]: The actual patch looks good, the test has a little issue. ::: tests/check/elements/videocrop.c @@ +183,3 @@ + ctx->filter2 = gst_element_factory_make ("capsfilter", "filter2"); + fail_unless (ctx->filter != NULL, + "Failed to create second capsfilter element"); Is that filter2 element used anywhere? It seems to be never added to a pipeline and never linked? You only set the outcaps on it, so they're in effect ignored.
Created attachment 228299 [details] [review] videocrop: Add support for automatic cropping This change enable automatic cropping using -1 set to left, top, right or bottom property. In the case both side are set to automatic cropping, the croping will be done equally on both side (in the odd case, right and bottom cropping will be 1 pixel more).
(In reply to comment #6) > Review of attachment 228235 [details] [review]: > > The actual patch looks good, the test has a little issue. > > ::: tests/check/elements/videocrop.c > @@ +183,3 @@ > + ctx->filter2 = gst_element_factory_make ("capsfilter", "filter2"); > + fail_unless (ctx->filter != NULL, > + "Failed to create second capsfilter element"); > > Is that filter2 element used anywhere? It seems to be never added to a pipeline > and never linked? You only set the outcaps on it, so they're in effect ignored. My bad, updated in both bugs, the check should be on filter2, and the element should be added and linked. The caps are being set on the element for each tests.
Created attachment 228349 [details] [review] videocrop: Add support for automatic cropping This change enable automatic cropping using -1 set to left, top, right or bottom property. In the case both side are set to automatic cropping, the croping will be done equally on both side (in the odd case, right and bottom cropping will be 1 pixel more).
commit 42ec3552f2dfd77e0e83663369e78838c0463ab6 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Nov 6 11:53:25 2012 +0100 videocrop: Add support for automatic cropping This change enable automatic cropping using -1 set to left, top, right or bottom property. In the case both side are set to automatic cropping, the croping will be done equally on both side (in the odd case, right and bottom cropping will be 1 pixel more). https://bugzilla.gnome.org/show_bug.cgi?id=687655 commit 73a478b02fcb71e2fe9ed104fa2b1125b84c5460 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Nov 6 11:49:56 2012 +0100 videocrop: Style fix https://bugzilla.gnome.org/show_bug.cgi?id=687655