GNOME Bugzilla – Bug 527951
[new element] aspectratiocrop
Last modified: 2009-01-25 18:21:18 UTC
This element crop's the video to the requested aspect-ratio of the user.
Created attachment 109199 [details] [review] aspectratio.diff Crop to the aspect ratio requested by the property
IMHO this should get a different name. I would expect an "aspectratio" element to scale the images to a given aspect ratio. This is more an "aspectratiocrop" element ;) Other than that the code looks good...
Created attachment 120253 [details] [review] aspectratiocrop element Renamed the element to aspectratiocrop. :) I will commit if there are no further objections ...
I was going to ask you to document that this element cropped to a specifed display aspect ratio... but then I looked at the code, and it doesn't - it crops to a specified image aspect ratio. Is this actually what anyone wants? It seems unlikely. Please make it crop to the specified DAR instead, unless you can explain why doing what it currently does is sensible. Also, please rename incomming_ar to incoming_ar - it'll annoy me less! :-)
(In reply to comment #4) > I was going to ask you to document that this element cropped to a specifed > display aspect ratio... but then I looked at the code, and it doesn't - it > crops to a specified image aspect ratio. > > Is this actually what anyone wants? It seems unlikely. Please make it crop to > the specified DAR instead, unless you can explain why doing what it currently > does is sensible. No, I'm really talking about (image) aspect ratio here. I'm using this in a transcoding pipeline with sources from TV, DVD and internet, where it is not uncommen that they add (by mistake or incopetence) black borders. But ofcourse it doesn't make sence to transcode these black borders. So here it has nothing todo with DAR but I think this is still a usefull element. Cropping to DAR can also be usefull but it's not the goal here... And I'm happy that you review in such de > > Also, please rename incomming_ar to incoming_ar - it'll annoy me less! :-) > I'm happy that you review in such detail, will fix it ;)
I think Mike's point is that it doesn't take pixel aspect ratio into account. In particular, it will incorrectly crop SD NTSC video (i.e., a DVD rip), which is 704x480 with 10/11 pixels, giving a display aspect ratio of 4:3. (We'll ignore the fact that 704x480 is actually just the clean area of 720x480 video. Bonus points for implementing clean area in gstreamer.)
Created attachment 126023 [details] [review] aspectratiocrop element Fixed the typo and take pixel aspect ratio into account. Integration of clean area needs probably more info on the stream.
committed: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=c1cb1e0c5bf6b3b9d71cf68f7cb269a568ac3255
Please add documentation (gtk-doc blurb) for this element and at least a minimal unit test.
ok, committed! gtk-doc: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=56b3efb077bba2189f5e128782683b75055360d5 unit test: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=27093c19a4578456cebe2523092c468d23614431