Add SplitUVPlanes and MergeUVPlanes

Add SplitUVPlanes and MergeUVPlanes

Add public methods SplitUVPlanes and MergeUVPlanes based on the
optimized assembly functions that already exists. Also, de-duplicate the
CPU dispatching code for these functions by moving them to helper
functions.

BUG=libyuv:629

[magjed_chromium, 2016-08-20 13:02:40]

Patchset #1 (id:1) has been deleted

[magjed_chromium, 2016-08-20 13:20:58]

Patchset #1 (id:20001) has been deleted

[magjed_chromium, 2016-08-20 15:59:40]

Description was changed from

==========
Add SplitUVPlanes and MergeUVPlanes

Add public methods SplitUVPlanes and MergeUVPlanes based on the
optimized assembly functions that already exists. Also, de-duplicate the
CPU dispatching code for these functions by moving them to helper
functions.

BUG=libyuv:629
==========

to

==========
Add SplitUVPlanes and MergeUVPlanes

Add public methods SplitUVPlanes and MergeUVPlanes based on the
optimized assembly functions that already exists. Also, de-duplicate the
CPU dispatching code for these functions by moving them to helper
functions.

BUG=libyuv:629
==========

[magjed_chromium, 2016-08-20 15:59:40]

magjed@chromium.org changed reviewers:
+ fbarchard@google.com

[magjed_chromium, 2016-08-20 16:00:22]

Frank - please take a look.

[fbarchard1, 2016-08-24 17:42:24]

https://codereview.chromium.org/2266533003/diff/40001/include/libyuv/convert.h
File include/libyuv/convert.h (right):

https://codereview.chromium.org/2266533003/diff/40001/include/libyuv/convert....
include/libyuv/convert.h:206: int SplitUVPlanes(const uint8* src_uv, int
src_stride_uv,
This belongs in planar_functions.h
Functions in convert.h are for FOURCC formats converted to I420.

https://codereview.chromium.org/2266533003/diff/40001/include/libyuv/convert....
include/libyuv/convert.h:206: int SplitUVPlanes(const uint8* src_uv, int
src_stride_uv,
src_uv is a single plane of packed 2 channels.  Suggest SplitUVPlane()
SplitUVRow() is name of row function.  Plane functions follow same name with Row
vs Plane.  Should be consistent naming.  Functions in libyuv are usually
singular.

https://codereview.chromium.org/2266533003/diff/40001/include/libyuv/convert_...
File include/libyuv/convert_from.h (right):

https://codereview.chromium.org/2266533003/diff/40001/include/libyuv/convert_...
include/libyuv/convert_from.h:165: int MergeUVPlanes(const uint8* src_u, int
src_stride_u,
move to planar_functions.h
functions in convert from are for I420To other formats.

https://codereview.chromium.org/2266533003/diff/40001/include/libyuv/row.h
File include/libyuv/row.h (right):

https://codereview.chromium.org/2266533003/diff/40001/include/libyuv/row.h#ne...
include/libyuv/row.h:847: uint8 *dst_v, int width);
remove. typedef'ed functions not conventional in libyuv.

https://codereview.chromium.org/2266533003/diff/40001/include/libyuv/row.h#ne...
include/libyuv/row.h:848: SplitUVRowFunction GetOptimizedSplitUVRowFunction(
prefer we dont do getter... inconsistent with rest of libyuv.
not needed if only one instance of code.  remove duplicate code.  use
SplitUVPlane from NV12ToI420.

https://codereview.chromium.org/2266533003/diff/40001/include/libyuv/row.h#ne...
include/libyuv/row.h:870: MergeUVRowFunction GetOptimizedMergeUVRowFunction(int
width);
ditto.

https://codereview.chromium.org/2266533003/diff/40001/source/convert.cc
File source/convert.cc (left):

https://codereview.chromium.org/2266533003/diff/40001/source/convert.cc#oldco...
source/convert.cc:230: // Width and height are plane sizes (typically half pixel
width).
suggest keeping the comments.

https://codereview.chromium.org/2266533003/diff/40001/source/convert.cc
File source/convert.cc (right):

https://codereview.chromium.org/2266533003/diff/40001/source/convert.cc#newco...
source/convert.cc:253: SplitUVRowFunction SplitUVRow =
GetOptimizedSplitUVRowFunction(
prefer not use getter function.

https://codereview.chromium.org/2266533003/diff/40001/source/convert.cc#newco...
source/convert.cc:256: for (int y = 0; y < height; ++y) {
declare y at top for c89 compatibility.

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/convert.h
File include/libyuv/convert.h (right):

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/convert....
include/libyuv/convert.h:206: int SplitUVPlane(const uint8* src_uv, int
src_stride_uv,
prefer SplitPlane function name function name for consistency with other planar
functions.

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/convert....
include/libyuv/convert.h:206: int SplitUVPlane(const uint8* src_uv, int
src_stride_uv,
Should be moved to planar_functions.h

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/convert_...
File include/libyuv/convert_from.h (right):

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/convert_...
include/libyuv/convert_from.h:165: int MergeUVPlanes(const uint8* src_u, int
src_stride_u,
Do we need this function?

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/convert_...
include/libyuv/convert_from.h:165: int MergeUVPlanes(const uint8* src_u, int
src_stride_u,
Move to planar_functions.h

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/convert_...
include/libyuv/convert_from.h:165: int MergeUVPlanes(const uint8* src_u, int
src_stride_u,
Prefer MergePlanes function name.  UV may be a misnomer.  It merges 2 planes
into 1, but they may contain any 2 bytes.

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/row.h
File include/libyuv/row.h (right):

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/row.h#ne...
include/libyuv/row.h:846: typedef void (*SplitUVRowFunction)(const uint8
*src_uv, uint8 *dst_u,
prefer no function types.  it would increase header size alot to do every row
function.  most only get used once.

https://codereview.chromium.org/2266533003/diff/60001/include/libyuv/row.h#ne...
include/libyuv/row.h:848: SplitUVRowFunction GetOptimizedSplitUVRowFunction(
prefer SplitPlane function name function name for consistency with other planar
functions.

https://codereview.chromium.org/2266533003/diff/60001/source/convert.cc
File source/convert.cc (left):

https://codereview.chromium.org/2266533003/diff/60001/source/convert.cc#oldco...
source/convert.cc:230: // Width and height are plane sizes (typically half pixel
width).
keep this comment.

https://codereview.chromium.org/2266533003/diff/60001/source/convert.cc
File source/convert.cc (right):

https://codereview.chromium.org/2266533003/diff/60001/source/convert.cc#newco...
source/convert.cc:256: for (int y = 0; y < height; ++y) {
int y needs to be declared at top of function for c89 compatibility.

https://codereview.chromium.org/2266533003/diff/60001/source/convert_from.cc
File source/convert_from.cc (right):

https://codereview.chromium.org/2266533003/diff/60001/source/convert_from.cc#...
source/convert_from.cc:375: CopyPlane(src_y, src_stride_y, dst_y, dst_stride_y,
width, height);
FYI CopyPlane has been made optional for I420ToNV12 upstream.

https://codereview.chromium.org/2266533003/diff/60001/source/convert_from.cc#...
source/convert_from.cc:404: MergeUVRowFunction MergeUVRow_ =
GetOptimizedMergeUVRowFunction(width);
remove function type def and getter.

https://codereview.chromium.org/2266533003/diff/60001/source/convert_from.cc#...
source/convert_from.cc:408: MergeUVRow_(src_u, src_v, dst_uv, width);
add comment about width being with of the UV plane, which is typically
halfwidth.

https://codereview.chromium.org/2266533003/diff/60001/source/planar_functions.cc
File source/planar_functions.cc (right):

https://codereview.chromium.org/2266533003/diff/60001/source/planar_functions...
source/planar_functions.cc:38: dst_stride_y = -dst_stride_y;
copy plane is really intended as a low level with minimal overhead.  i'm
thinking the planar functions shouldnt support negative height, the convert
higher level should?

https://codereview.chromium.org/2266533003/diff/60001/source/planar_functions...
source/planar_functions.cc:2550: memcpy(dst_uv, rows + awidth, awidth);
can you explain this change?  is there a bug?  The original code seems right to
me.

https://codereview.chromium.org/2266533003/diff/60001/source/planar_functions...
source/planar_functions.cc:2629: memcpy(dst_uv, rows + awidth, awidth);
ditto.  this seems like it would cause a UV buffer overflow on odd height.
You can set libyuv width/height via options and run valgrind or msan to check.

https://codereview.chromium.org/2266533003/diff/60001/source/row_common.cc
File source/row_common.cc (right):

https://codereview.chromium.org/2266533003/diff/60001/source/row_common.cc#ne...
source/row_common.cc:1804: SplitUVRowFunction GetOptimizedSplitUVRowFunction(
suggest removing.  common is not meant for this sort of function.
For each row function, there should be a _NEON, _SSE2, and _C version, which
goes here.
If we do getters, I think this belongs in its own file, or row_any.c

[magjed_chromium, 2016-08-25 14:03:46]

Now I finally get a review, but you have already taken my CL, changed it and
landed it without any input from me. Since I uploaded this CL, you have landed 4
CL:s that address the same issue:
'NV12ToI420 use SplitPlane function' https://codereview.chromium.org/2267303002
'NV12 allow NULL for Y' https://codereview.chromium.org/2276703002
'Allow NULL for dst_y in planar formats.'
https://codereview.chromium.org/2271053003
'Add SplitUVPlanes and MergeUVPlanes' https://codereview.chromium.org/2277603004
You have not even given me the chance to review any of these CL:s. I am not sure
how to collaborate with you.