GNOME Bugzilla – Bug 354007
[PLUGIN-MOVE] videocrop should be moved to -good
Last modified: 2007-06-12 20:17:04 UTC
I would like to move the videocrop element into gst-plugins-good, so it can be used by playbin/totem to implement 'zooming' (see bug #171916). Moving guidelines are here for reference: http://webcvs.freedesktop.org/gstreamer/gstreamer/docs/random/moving-plugins?view=markup I have ported/written the element and am willing to maintain it. There are docs and extensive unit tests and it derives from a base class (GstBaseTransform) and conforms to our coding style (valgrind unit test passes fine but takes eternities to complete, hence disabled in Makefile.am). I am looking for someone to review the code and sponsor the move. Any takers?
I should add that the code and the units tests can be found in gst-plugins-bad CVS: http://webcvs.freedesktop.org/gstreamer/gst-plugins-bad/gst/videocrop/gstvideocrop.c?view=markup http://webcvs.freedesktop.org/gstreamer/gst-plugins-bad/gst/videocrop/gstvideocrop.h?view=markup http://webcvs.freedesktop.org/gstreamer/gst-plugins-bad/tests/check/elements/videocrop.c?view=markup
Code itself looks largely good. In common with most of the rest of gstreamer's video handling, our inability to do anything useful with stride makes me cringe. The handling of odd-offsets in chroma-subsampled planes looks incorrect; we probably don't care about that enough to fix it right now (it's hard), but we should document it. I can't run the valgrind test; I wanted to add a test that cropped to 1x1, and make sure that ran correctly, but I couldn't. liboil screwing me over... Tim, if you can add that test, check that it runs successfully, and add something documenting that odd top or left offsets are handled incorrectly, I'm happy to sponsor this.
Thanks for the review. > In common with most of the rest of gstreamer's video handling, our inability to > do anything useful with stride makes me cringe. Yeah, don't really see us fixing that in 0.10 though (not in the near future at least). > The handling of odd-offsets in chroma-subsampled planes looks incorrect; we > probably don't care about that enough to fix it right now (it's hard), but we > should document it. Documented now. > I can't run the valgrind test; I wanted to add a test that cropped to 1x1, > and make sure that ran correctly, but I couldn't. liboil screwing me over... > > Tim, if you can add that test, check that it runs successfully, and add > something documenting that odd top or left offsets are handled incorrectly, > I'm happy to sponsor this. Added test that crops to 1x1 and checks the data matches the expected data for a small number of formats. Hope that's more or less what you had in mind. If not, let me know and I'll add more stuff. Runs fine in valgrind here. 2006-09-08 Tim-Philipp Müller <tim at centricular dot net> * configure.ac: Bump requirements of -base (videocrop test case needs this). * gst/videocrop/gstvideocrop.c: Document sloppy handling of subsampled chroma planes if left/top cropping is an odd number. * tests/check/elements/videocrop.c: (handoff_cb), (videocrop_test_cropping_init_context), (videocrop_test_cropping_deinit_context), (videocrop_test_cropping), (check_1x1_buffer), (GST_START_TEST), (videocrop_suite), (main): Add another unit test that crops the input to 1x1 (and checks that that pixel has the expected values in a number of formats). 2006-09-08 Tim-Philipp Müller <tim at centricular dot net> * gst/videocrop/Makefile.am: * gst/videocrop/gstvideocrop.c: (gst_video_crop_class_init), (gst_video_crop_transform_packed), (gst_video_crop_transform_planar): Some quick tests indicate that it doesn't make a great deal of sense to use liboil here, at least not for the memcpy()s we do, so remove liboil usage until there is clear evidence it actually makes a positive difference somewhere.
Just an update: this isn't quite ready yet, there are some problems with the UYVY-type formats (videotestsrc's output is also slightly dodgy for these formats). Will add an interactive test for this stuff.
Fixed all remaining issues: 2007-01-28 Tim-Philipp Müller <tim at centricular dot net> * gst/videocrop/gstvideocrop.c: (gst_video_crop_get_image_details_from_caps), (gst_video_crop_transform_packed_complex): Fix cropping for packed 4:2:2 formats YUYV/YUY2 and UYVY. * tests/icles/videocrop-test.c: (check_bus_for_errors), (test_with_caps), (main): Block streaming thread before changing filter caps while the pipeline is running so that we don't get random not-negotiated errors just because GStreamer can't handle that yet. Interactive test is in gst-plugins-bad/test/icles/test-videocrop run with or without --with-ffmpegcolorspace argument. This element is ready to be moved (see bug #352605 comment #7 for timing concerns though).
Marking as blocker for the upcoming -good release.
Done.