GNOME Bugzilla – Bug 687964
videocrop: Add NV12/NV21 support
Last modified: 2012-11-10 01:39:47 UTC
Videocrop does not support NV12/21 bi-planar format. There format are commonly found on Android and iOS platforms.
Created attachment 228528 [details] [review] videocrop: Add NV12/NV21 support
Review of attachment 228528 [details] [review]: ::: gst/videocrop/gstvideocrop.c @@ +392,3 @@ + /* UV plane */ + uv_in = y_in + GST_VIDEO_FRAME_COMP_OFFSET (in_frame, 1); + uv_out = y_out + GST_VIDEO_FRAME_COMP_OFFSET (out_frame, 1); Use GST_VIDEO_FRAME_PLANE_DATA() here too. The components are always in order Y, U, V, A while what you want is the offset to the second plane (no matter if U or V is the first component in memory). Also the Y and UV planes could be in different memory areas. @@ +404,3 @@ + } + + uv_in += (vcrop->crop_top / 2) * GST_VIDEO_FRAME_PLANE_STRIDE (in_frame, 1); Round down this one too maybe? @@ +408,3 @@ + dx = GST_ROUND_UP_2 (width); + + for (i = 0; i < GST_ROUND_UP_2 (height) / 2; i++) { Maybe put this round-divide calculation out of the loop ::: gst/videocrop/gstvideocrop.h @@ +40,3 @@ VIDEO_CROP_PIXEL_FORMAT_PACKED_COMPLEX, /* UYVY, YVYU */ + VIDEO_CROP_PIXEL_FORMAT_PLANAR, /* I420, YV12 */ + VIDEO_CROP_PIXEL_FORMAT_BIPLANAR /* NV12, NV21 */ Maybe call this semi-planar instead... first read bipolar here ;)
(In reply to comment #2) > Review of attachment 228528 [details] [review]: > > ::: gst/videocrop/gstvideocrop.c > @@ +392,3 @@ > + /* UV plane */ > + uv_in = y_in + GST_VIDEO_FRAME_COMP_OFFSET (in_frame, 1); > + uv_out = y_out + GST_VIDEO_FRAME_COMP_OFFSET (out_frame, 1); > > Use GST_VIDEO_FRAME_PLANE_DATA() here too. The components are always in order > Y, U, V, A while what you want is the offset to the second plane (no matter if > U or V is the first component in memory). > > Also the Y and UV planes could be in different memory areas. Good point, this is a porting error. > > @@ +404,3 @@ > + } > + > + uv_in += (vcrop->crop_top / 2) * GST_VIDEO_FRAME_PLANE_STRIDE (in_frame, 1); > > Round down this one too maybe? Dividing by two already round down, also you don't need (vcrop->crop_top / 2) to be even. Unless I don't understand what you mean ? > > @@ +408,3 @@ > + dx = GST_ROUND_UP_2 (width); > + > + for (i = 0; i < GST_ROUND_UP_2 (height) / 2; i++) { > > Maybe put this round-divide calculation out of the loop > > ::: gst/videocrop/gstvideocrop.h > @@ +40,3 @@ > VIDEO_CROP_PIXEL_FORMAT_PACKED_COMPLEX, /* UYVY, YVYU */ > + VIDEO_CROP_PIXEL_FORMAT_PLANAR, /* I420, YV12 */ > + VIDEO_CROP_PIXEL_FORMAT_BIPLANAR /* NV12, NV21 */ > > Maybe call this semi-planar instead... first read bipolar here ;) Biplanar was from VLC doc I have read, but semi-planar seems more common. I'll change that.
Created attachment 228559 [details] [review] videocrop: Add NV12/NV21 support
Comment on attachment 228559 [details] [review] videocrop: Add NV12/NV21 support Looks good now, thanks :)
commit e111068f7bd348c34082da2513f527185547e892 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Nov 9 13:27:16 2012 +0100 videocrop: Add NV12/NV21 support https://bugzilla.gnome.org/show_bug.cgi?id=687964