yuv use a shared table for windows, posix and C...

Make C, MMX and SSE2 all output the same results
BUG=19113, 41045
TEST=none.  just cleanup and making all platforms produce the same results


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44591

[fbarchard, 2010-04-14 02:43:39]

A single table for YUV conversions.
Work in progress... needs arm version

[fbarchard, 2010-04-14 17:49:29]

attempting to make C (for ARM), MMX and SSE2 all behave the same.

[Sergey Ulanov, 2010-04-14 18:14:41]

http://codereview.chromium.org/1638008/diff/16001/4004
File media/base/yuv_row_posix.cc (left):

http://codereview.chromium.org/1638008/diff/16001/4004#oldcode1133
media/base/yuv_row_posix.cc:1133: static inline uint32 clip(int32 value) {
Isn't clip() used in YuvPixel()?

[fbarchard, 2010-04-14 18:27:08]

Compiles on windows with C, but not functional yet

http://codereview.chromium.org/1638008/diff/16001/4004
File media/base/yuv_row_posix.cc (left):

http://codereview.chromium.org/1638008/diff/16001/4004#oldcode1133
media/base/yuv_row_posix.cc:1133: static inline uint32 clip(int32 value) {
On 2010/04/14 18:14:41, sergeyu wrote:
> Isn't clip() used in YuvPixel()?

Yes, in the old one.  But I'll rewrite that to use the same technique as the
MMX.

[Sergey Ulanov, 2010-04-14 19:58:11]

http://codereview.chromium.org/1638008/diff/27002/28006
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/1638008/diff/27002/28006#newcode79
media/base/yuv_convert.cc:79: static_cast<uint16>((source_y_fraction >> 8) ^
255));
I think it is better to use 256 - (source_y_fraction >> 8) here. This way it
will be more consistent with the C version.

http://codereview.chromium.org/1638008/diff/27002/28006#newcode109
media/base/yuv_convert.cc:109: static_cast<int16>((source_y_fraction >> 8) ^
255));
same here

[fbarchard, 2010-04-14 21:23:19]

fixed mac build
FilterRow C same as MMX and 256-frac for better quality.

http://codereview.chromium.org/1638008/diff/27002/28006
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/1638008/diff/27002/28006#newcode79
media/base/yuv_convert.cc:79: static_cast<uint16>((source_y_fraction >> 8) ^
255));
On 2010/04/14 19:58:12, sergeyu wrote:
> I think it is better to use 256 - (source_y_fraction >> 8) here. This way it
> will be more consistent with the C version.

Done.

[fbarchard, 2010-04-14 22:42:54]

put unfiltered scalers back in for windows

http://codereview.chromium.org/1638008/diff/27002/28006
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/1638008/diff/27002/28006#newcode109
media/base/yuv_convert.cc:109: static_cast<int16>((source_y_fraction >> 8) ^
255));
On 2010/04/14 19:58:12, sergeyu wrote:
> same here

Done.

http://codereview.chromium.org/1638008/diff/27002/28006#newcode139
media/base/yuv_convert.cc:139: ybuf[0] = (y0_ptr[0] * (y0_fraction) + y1_ptr[0]
* (y1_fraction)) >> 16;
made this the same as MMX

[Sergey Ulanov, 2010-04-14 23:13:10]

http://codereview.chromium.org/1638008/diff/1011/52006
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/1638008/diff/1011/52006#newcode12
media/base/yuv_convert.cc:12: // YV12 is a full plane of Y and a half
source_height, half source_width chroma planes
80-chars line

http://codereview.chromium.org/1638008/diff/1011/52006#newcode80
media/base/yuv_convert.cc:80: static_cast<uint16>(256-(source_y_fraction >>
8)));
Do we need spaces around operators? E.g. 256-(..) vs. 256 - (..)

http://codereview.chromium.org/1638008/diff/1011/52006#newcode110
media/base/yuv_convert.cc:110: static_cast<int16>(256-(source_y_fraction >>
8)));
Do we need spaces around operators? E.g. 256-(..) vs. 256 - (..)

http://codereview.chromium.org/1638008/diff/1011/52008
File media/base/yuv_convert.h (right):

http://codereview.chromium.org/1638008/diff/1011/52008#newcode15
media/base/yuv_convert.h:15: YV16 = 0,           // YV16 is half source_width
and full source_height chroma channels.
80-chars line

http://codereview.chromium.org/1638008/diff/1011/52007
File media/base/yuv_convert_unittest.cc (right):

http://codereview.chromium.org/1638008/diff/1011/52007#newcode24
media/base/yuv_convert_unittest.cc:24: static const int ksource_width = 640;
Shouldn't it be kSourceWidth?

http://codereview.chromium.org/1638008/diff/1011/52007#newcode26
media/base/yuv_convert_unittest.cc:26: static const int kScaledsource_width =
1024;
kScaledSourceWidth

http://codereview.chromium.org/1638008/diff/1011/52004
File media/base/yuv_row_win.cc (right):

http://codereview.chromium.org/1638008/diff/1011/52004#newcode546
media/base/yuv_row_win.cc:546: void LinearScaleYUVToRGB32Row(const uint8* y_buf,
Can we move C version to a separate file (e.g. yuv_row.cc), so that we don't
have two copies of it?

[fbarchard, 2010-04-14 23:37:22]

fixing up the nits.
build bots are green.

http://codereview.chromium.org/1638008/diff/1011/52006
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/1638008/diff/1011/52006#newcode12
media/base/yuv_convert.cc:12: // YV12 is a full plane of Y and a half
source_height, half source_width chroma planes
On 2010/04/14 23:13:10, sergeyu wrote:
> 80-chars line

Done.

http://codereview.chromium.org/1638008/diff/1011/52006#newcode80
media/base/yuv_convert.cc:80: static_cast<uint16>(256-(source_y_fraction >>
8)));
On 2010/04/14 23:13:10, sergeyu wrote:
> Do we need spaces around operators? E.g. 256-(..) vs. 256 - (..) 

Done.

http://codereview.chromium.org/1638008/diff/1011/52006#newcode110
media/base/yuv_convert.cc:110: static_cast<int16>(256-(source_y_fraction >>
8)));
On 2010/04/14 23:13:10, sergeyu wrote:
> Do we need spaces around operators? E.g. 256-(..) vs. 256 - (..) 

Done.

http://codereview.chromium.org/1638008/diff/1011/52008
File media/base/yuv_convert.h (right):

http://codereview.chromium.org/1638008/diff/1011/52008#newcode15
media/base/yuv_convert.h:15: YV16 = 0,           // YV16 is half source_width
and full source_height chroma channels.
On 2010/04/14 23:13:10, sergeyu wrote:
> 80-chars line

Done.

http://codereview.chromium.org/1638008/diff/1011/52007
File media/base/yuv_convert_unittest.cc (right):

http://codereview.chromium.org/1638008/diff/1011/52007#newcode24
media/base/yuv_convert_unittest.cc:24: static const int ksource_width = 640;
On 2010/04/14 23:13:10, sergeyu wrote:
> Shouldn't it be kSourceWidth?

Done.

http://codereview.chromium.org/1638008/diff/1011/52007#newcode26
media/base/yuv_convert_unittest.cc:26: static const int kScaledsource_width =
1024;
On 2010/04/14 23:13:10, sergeyu wrote:
> kScaledSourceWidth

Done.

http://codereview.chromium.org/1638008/diff/1011/52001
File media/base/yuv_row.h (right):

http://codereview.chromium.org/1638008/diff/1011/52001#newcode78
media/base/yuv_row.h:78: // Method to force C version
add a .

http://codereview.chromium.org/1638008/diff/1011/52002
File media/base/yuv_row_posix.cc (right):

http://codereview.chromium.org/1638008/diff/1011/52002#newcode782
media/base/yuv_row_posix.cc:782: #define paddsw(x, y) (((x) + (y)) < -32768 ?
-32768 : (((x) + (y)) > 32767 ? 32767 : ((x) + (y))))
80 columns

http://codereview.chromium.org/1638008/diff/1011/52003
File media/base/yuv_row_table.cc (right):

http://codereview.chromium.org/1638008/diff/1011/52003#newcode232
media/base/yuv_row_table.cc:232: #undef SIMD_ALIGNED
should not undefine something that was not defined here

http://codereview.chromium.org/1638008/diff/1011/52004
File media/base/yuv_row_win.cc (right):

http://codereview.chromium.org/1638008/diff/1011/52004#newcode546
media/base/yuv_row_win.cc:546: void LinearScaleYUVToRGB32Row(const uint8* y_buf,
On 2010/04/14 23:13:10, sergeyu wrote:
> Can we move C version to a separate file (e.g. yuv_row.cc), so that we don't
> have two copies of it?

Ya I was thinking about that.
But it would be harder to ifdef, especially on linux.

[Sergey Ulanov, 2010-04-15 00:21:46]

LGTM