Implements RGB to YV12 conversion in YASM.

Implements RGB to YV12 conversions in YASM.
This change implements two conversions (RGB32-to-YV12 and RGB24-to-YV12) in YASM. This change uses 8-bit fixed-point calculation and converts up to four pixels at once.

BUG=none
TEST=YUVConvertTest.SideBySideRGB
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100352

[Hironori Bono, 2011-06-10 09:13:37]

Greetings,

Even though I have implemented this change long time ago, I forgot sending its
review request. (As far as I tested, this YASM function ConvertRGBAtoYUVRow() is
nearly 3x faster than FastConvertRGB32ToYUVRow().)

Regards,

Hironori Bono

[fbarchard, 2011-06-10 17:29:45]

Thanks for doing this!

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row.asm
File media/base/simd/convert_row.asm (right):

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row....
media/base/simd/convert_row.asm:21: ; int16 SIMD_ConvertRGBAToYUV_kTable[8 * 3]
= {
I suggest calling this SIMD_ConvertARGBToYUV which matches the fourcc's in
WebRTC/Talk.  RGBA is also defined and has a deferent definition.

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row.inc
File media/base/simd/convert_row.inc (right):

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row....
media/base/simd/convert_row.inc:38: %define SIZEOF_OWORD            16          
   ; sizeof(OWORD)
MS calls thie DQWORD

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row....
media/base/simd/convert_row.inc:63: %define _EAX_                   rax
Its more conventional to call these _RAX_ etc

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row_...
File media/base/simd/convert_row_yuv.inc (right):

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row_...
media/base/simd/convert_row_yuv.inc:6: ; void
media::simd::ConvertYUVtoRGBARow(const uint8* y,
ConvertYUVtoRGBARow
suggest ConvertYUVtoARGBRow

For Talk/WebRTC we do use ARGB, BGRA, RGBA, RGB and BGR.

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row_...
media/base/simd/convert_row_yuv.inc:315: pmaddwd   xmm2, xmm3
would it make sense to do a pmaddubsw version?  Using doubles requires a lot of
pack/unpack overhead and does half as much per instruction.

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row_...
media/base/simd/convert_row_yuv.inc:591: jmp       .convert_four_pixels
more efficient to put conditional loop at bottom.

[rbultje, 2011-06-10 18:26:33]

Hi,

Frank asked me to look at this since this is the first use (or so he says) of
yasm-style assembly in here. They're global comments, I didn't review the code
closely itself, can do that but Frank is no doubt capable of doing that also.

Ronald

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row.asm
File media/base/simd/convert_row.asm (right):

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row....
media/base/simd/convert_row.asm:49: dw        2048, 0, 2048, 0
You can "simplify" these tables using preprocessor magic:
401 >> (12 - SHIFT_BITS)
You can make that a macro to make it less characters to type.

Also, the times element is nice:
times 2 dw 401, 2064, 1053, 0
times 4 dw 2048, 0

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row_...
File media/base/simd/convert_row_rgb.inc (right):

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row_...
media/base/simd/convert_row_rgb.inc:74: 
So what you're doing here is providing compatibility for the three main calling
conventions on x86: 32, unix64 and win64. you can see how this quickly becomes a
nightmare maintenance burden once you use this 10x, 100x, 1000000x.

Also note how 
My personal opinion is that x264/ffmpeg/libav have the best compatibility
wrapper, and they have released it under a BSD-style license. It basically wraps
_everything_ you do here in a one-line macro. It's called x86inc.asm, have a
look at it and see if that fits Chrome. I highly recommend using that instead of
recreating this yourself.

http://codereview.chromium.org/7003082/diff/2002/media/base/simd/convert_row_...
media/base/simd/convert_row_rgb.inc:85: %define INT_PIXEL       rbx
There's an issue here: width is defined as int, which in x86-64 is actually
still 4bytes. Sometimes this works, sometimes it doesn't. You can't guarantee
that the higher 4 bytes in the stack/register are zero or something else. If an
argument is int, it should be referred to under its 4byte register (eax).

[Alpha Left Google, 2011-08-22 14:16:47]

This is excellent work! and sorry about the long delay for this review.

I'd like to address the style of this code first, then I'll go over the
functional assembly more thoroughly.

Here's some benchmark numbers of this code:

Before

Source image size: 2048x2048
Destination image size: 2048x2048
Number of frames: 500
Number of buffers: 50
Skia: 9.166ms/frame
RGB To YUV: 8.102ms/frame
No filtering: 5.14ms/frame
Bilinear Vertical: 5.744ms/frame
Bilinear Horizontal: 5.138ms/frame
Bilinear: 5.782ms/frame

After

Source image size: 2048x2048
Destination image size: 2048x2048
Number of frames: 500
Number of buffers: 50
Skia: 15.132ms/frame
RGB To YUV: 5.038ms/frame
No filtering: 5.132ms/frame
Bilinear Vertical: 5.798ms/frame
Bilinear Horizontal: 5.16ms/frame
Bilinear: 5.756ms/frame

While RGB to YUV shows a 40% improvement, YUV to RGB shows a 3x slow down. Could
it be because of upsampling performed from yuv to rgb?

I suppose RGB to YUV is subsampled for U and V right?

[Alpha Left Google, 2011-08-22 14:16:58]

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert.cc
File media/base/simd/yuv_convert.cc (right):

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.cc:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
These functions should be defined in yuv_convert_sse2.cc

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.cc:15: void ConvertRGB32toYUV(const uint8* rgbframe,
Call this ConvertRGB32toYUV_SSE2, you can call the old one
ConvertRGB32toYUV_SSE2_Old and put them side by side.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.cc:41: void ConvertRGB24toYUV(const uint8* rgbframe,
Same for this one, call it ConvertRGB24toYUV_SSE2

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.cc:67: void ConvertYUVtoRGB32(const uint8* y_buf,
Call this ConvertYUVtoRGB32_SSE2.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.cc:77: if (yuv_type == YV12) {
Please this use construct:

if (yuv_type != YV12)
  NOTIMPLMENTED();

...
to save an extra scope.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.cc:97: void ConvertYUVtoRGB24(const uint8* y_buf,
Call this ConvertYUVtoRGB24_SSE2.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.cc:107: if (yuv_type == YV12) {

Please this use construct:

if (yuv_type != YV12)
  NOTIMPLMENTED();

...
to save an extra scope.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert.h
File media/base/simd/yuv_convert.h (right):

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.h:12: namespace simd {
I'd put them in yuv_convert_internal.h and name the old function XXX_Old and get
rid of the extra namespace.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.h:14: void ConvertRGB32toYUV(const uint8* rgbframe,
Add a _SSE2 suffix.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.h:24: void ConvertRGB24toYUV(const uint8* rgbframe,
Add a _SSE2 suffix.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.h:34: void ConvertYUVtoRGB32(const uint8* y_buf,
Add a _SSE2 suffix.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_convert...
media/base/simd/yuv_convert.h:45: void ConvertYUVtoRGB24(const uint8* y_buf,
Add a _SSE2 suffix.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.asm
File media/base/simd/yuv_row.asm (right):

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.asm...
media/base/simd/yuv_row.asm:1: ; Copyright (c) 2011 The Chromium Authors. All
rights reserved.
Rename this to media/base/yuv_row_sse2.asm

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.asm...
media/base/simd/yuv_row.asm:40: SIMD_ConvertARGBToYUVRow_kTable:
SSE2_XXX, there may be a different NEON version.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.asm...
media/base/simd/yuv_row.asm:48: SIMD_ConvertYUVtoARGB_kTable:
SSE2_XXX, there may be a different NEON version.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.h
File media/base/simd/yuv_row.h (right):

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.h#n...
media/base/simd/yuv_row.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
Rename this file to media/base/yuv_row_internal.h

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.h#n...
media/base/simd/yuv_row.h:8: namespace media {
These function names are obscure enough that you don't need to have namespace,
makes it easier for your assembly functions.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.h#n...
media/base/simd/yuv_row.h:16: void ConvertRGBtoYUVRow(const uint8* argb,
_SSE2 suffix.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.h#n...
media/base/simd/yuv_row.h:23: void ConvertARGBtoYUVRow(const uint8* argb,
_SSE2 suffix.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.h#n...
media/base/simd/yuv_row.h:30: void ConvertYUVtoRGBRow(const uint8* y,
_SSE2 suffix.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.h#n...
media/base/simd/yuv_row.h:38: void ConvertYUVtoARGBRow(const uint8* y,
_SSE2 suffix.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.inc
File media/base/simd/yuv_row.inc (right):

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.inc...
media/base/simd/yuv_row.inc:1: ; Copyright (c) 2011 The Chromium Authors. All
rights reserved.
Rename this to yuv_row_common_sse2.inc, there might be a neon version.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.inc...
media/base/simd/yuv_row.inc:5: %ifndef MEDIA_BASE_SIMD_COMMON_INC_
This should be MEDIA_BASE_YUV_ROW_COMMON_SSE2_INC_

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.inc...
media/base/simd/yuv_row.inc:83: ; Copies the address to a local symbol to the
specified register.
Do a one liner to explain why we need to do this: because of PIC.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.inc...
media/base/simd/yuv_row.inc:84: %ifdef MACHO
Both Linux and Mac uses PIC.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.inc...
media/base/simd/yuv_row.inc:96: %imacro LOADSYM 2
Do a one liner explanation here.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row.inc...
media/base/simd/yuv_row.inc:101: %endif  ; MEDIA_BASE_SIMD_COMMON_INC_
This one needs to change too.

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row_rgb...
File media/base/simd/yuv_row_rgb.inc (right):

http://codereview.chromium.org/7003082/diff/19001/media/base/simd/yuv_row_rgb...
media/base/simd/yuv_row_rgb.inc:1: ; Copyright (c) 2011 The Chromium Authors.
All rights reserved.
Rename this to media/base/yuv_row_rgb2yuv_sse2.inc

http://codereview.chromium.org/7003082/diff/19001/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/7003082/diff/19001/media/base/yuv_convert.cc#n...
media/base/yuv_convert.cc:38: #define USE_YASM
It's better not to use this #ifdef.

http://codereview.chromium.org/7003082/diff/19001/media/base/yuv_convert.cc#n...
media/base/yuv_convert.cc:58: #ifdef USE_YASM
There seems to be a slow down using this new YUV->RGB32 function, I would like
to verify this.

http://codereview.chromium.org/7003082/diff/19001/media/base/yuv_convert.cc#n...
media/base/yuv_convert.cc:381: if (hasSSE2())
if hasSSE2() compiles on all platform we can get rid of the above ifdef check
for ARM.

http://codereview.chromium.org/7003082/diff/19001/media/base/yuv_convert.cc#n...
media/base/yuv_convert.cc:382: #ifdef USE_YASM
Don't use an ifdef, just apply the new function.

http://codereview.chromium.org/7003082/diff/19001/media/base/yuv_convert.cc#n...
media/base/yuv_convert.cc:385: convert_proc = &ConvertRGB32ToYUV_SSE2;
Remove this line.

http://codereview.chromium.org/7003082/diff/19001/media/base/yuv_convert.cc#n...
media/base/yuv_convert.cc:405: #ifdef USE_YASM
Same here use if without ifdef. Also because with this code, hasSSE2 cannot be
compiled on ARM. I think it's better to have hasSSE2 compiles on all platforms
and return false on ARM.

http://codereview.chromium.org/7003082/diff/19001/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7003082/diff/19001/media/media.gyp#newcode9
media/media.gyp:9: 'use_system_yasm%': 0,
We can drop this and assume we always use our own version of yasm.

http://codereview.chromium.org/7003082/diff/19001/media/media.gyp#newcode355
media/media.gyp:355: 'yasm_path':
'../third_party/yasm/binaries/win/yasm<(EXECUTABLE_SUFFIX)',
We can always depend on yasm#host now on all platforms.

[Alpha Left Google, 2011-08-22 14:26:01]

One more question: it looks like UV are not fully subsampled from a 4x4 block
but just the first row of 2 pixels, is that right?

[Hironori Bono, 2011-09-01 10:25:13]

Greetings,

Sorry for the lack of updates. I have totally re-written this change with
x86inc.asm and SSSE3. (I have decided to use SSSE3 just because atom supports
them.) Unfortunately, this is a work-in-progress change and it causes some test
failures. Also, its subsampling code needs more tests.

Regards,

Hironori Bono

[Alpha Left Google, 2011-09-01 10:29:21]

On 2011/09/01 10:25:13, hbono wrote:
> Greetings,
> 
> Sorry for the lack of updates. I have totally re-written this change with
> x86inc.asm and SSSE3. (I have decided to use SSSE3 just because atom supports
> them.) Unfortunately, this is a work-in-progress change and it causes some
test
> failures. Also, its subsampling code needs more tests.
> 
> Regards,
> 
> Hironori Bono

Wow cool. I'll get to it today. Thanks!

[Alpha Left Google, 2011-09-01 13:24:45]

I haven't looked at YUV -> RGB, but the algorithm for RGB -> YUV looks good to
me.

I have some major concerns:
1. Doesn't run on 64bits system. I tried running this on x86_64 linux and it
crashes, it seems the pointers passing to the assembly functions are incorrect.
It seems that the macro for prologue is the cause but i haven't looked deeper.
2. YUV->RGB is slower than the existing MMX version. I tested the SSSE3 version
to be 60% slower than MMX. I really would like to take a patch that is
consistent for all platforms (since mmx version we have different ones for posix
and win which I'm not very happy about), but I would like to see performance
doesn't suffer.

http://codereview.chromium.org/7003082/diff/34022/media/base/cpu_features_x86.cc
File media/base/cpu_features_x86.cc (right):

http://codereview.chromium.org/7003082/diff/34022/media/base/cpu_features_x86...
media/base/cpu_features_x86.cc:40: asm volatile (
Add a comment here that this part is for x64 i believe?

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
File media/base/simd/convert_rgb_to_yuv.h (right):

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv.h:14: void ConvertRGB32toYUV(const uint8*
rgbframe,
Call this ConvertRGB32ToYUV_SSE3

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv.h:14: void ConvertRGB32toYUV(const uint8*
rgbframe,
Add a comment that this outputs YV12.

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv.h:24: void ConvertRGB24toYUV(const uint8*
rgbframe,
Call this Convert24ToYUV_SSE3

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv.h:24: void ConvertRGB24toYUV(const uint8*
rgbframe,
Add a comment that this outputs YV12.

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
File media/base/simd/convert_rgb_to_yuv_ssse3.asm (right):

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv_ssse3.asm:17: db        25, 2, 66, 0
Please add comments here for what these constants represents, for example which
row is coefficients for Y, U or V.

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv_ssse3.asm:22: 
It looks like this is the UV offset, please add comments here.

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
File media/base/simd/convert_rgb_to_yuv_ssse3.h (right):

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv_ssse3.h:17: void ConvertRGBtoYUVEven(const
uint8* rgb,
Add _SSSE3 suffix to these functions.

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
File media/base/simd/convert_yuv_to_rgb.h (right):

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
media/base/simd/convert_yuv_to_rgb.h:14: void ConvertYUVtoRGB32(const uint8*
y_buf,
Append _SSSE3 suffix.

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
media/base/simd/convert_yuv_to_rgb.h:25: void ConvertYUVtoRGB24(const uint8*
y_buf,
Same here, _SSSE3 suffix.

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
File media/base/simd/convert_yuv_to_rgb_ssse3.h (right):

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
media/base/simd/convert_yuv_to_rgb_ssse3.h:16: void ConvertYUVtoRGBRow(const
uint8* y,
Add _SSSE3 suffix.

http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
media/base/simd/convert_yuv_to_rgb_ssse3.h:23: void ConvertYUVtoARGBRow(const
uint8* y,
Add _SSSE3 suffix.

http://codereview.chromium.org/7003082/diff/34022/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/7003082/diff/34022/media/base/yuv_convert.cc#n...
media/base/yuv_convert.cc:62: 
Add a comment here that we use the MMX version as a fallback.

http://codereview.chromium.org/7003082/diff/34022/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7003082/diff/34022/media/media.gyp#newcode10
media/media.gyp:10: 'use_system_yasm%': 0,
Don't need this, we always build our own yasm.

[Alpha Left Google, 2011-09-01 13:26:41]

http://codereview.chromium.org/7003082/diff/34022/media/base/yuv_convert_unit...
File media/base/yuv_convert_unittest.cc (right):

http://codereview.chromium.org/7003082/diff/34022/media/base/yuv_convert_unit...
media/base/yuv_convert_unittest.cc:360: TEST(YUVConvertTest, SideBySideRGB) {
One more comment on these tests, since they test ConvertRGB32ToYUV which can be
implemented differently under the hood, it's better to test the SSSE3 version
here. I suggest you move this test to be under media/base/simd.

[Hironori Bono, 2011-09-02 09:13:48]

Greetings,

Thank you so much for your review and comments.
My previous change forgot adding a '-DPIC' flag on x64 and it surely causes
crashes on x64. Also, the current YUV->RGB functions are just for my tests and I
forgot removing them. Sorry. Even though I uploaded a new change, I will update
my change again when I finish applying your comments.

Regards,

Hironori Bono

On 2011/09/01 13:24:45, Alpha wrote:
> I haven't looked at YUV -> RGB, but the algorithm for RGB -> YUV looks good to
> me.
> 
> I have some major concerns:
> 1. Doesn't run on 64bits system. I tried running this on x86_64 linux and it
> crashes, it seems the pointers passing to the assembly functions are
incorrect.
> It seems that the macro for prologue is the cause but i haven't looked deeper.
> 2. YUV->RGB is slower than the existing MMX version. I tested the SSSE3
version
> to be 60% slower than MMX. I really would like to take a patch that is
> consistent for all platforms (since mmx version we have different ones for
posix
> and win which I'm not very happy about), but I would like to see performance
> doesn't suffer.
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/cpu_features_x86.cc
> File media/base/cpu_features_x86.cc (right):
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/cpu_features_x86...
> media/base/cpu_features_x86.cc:40: asm volatile (
> Add a comment here that this part is for x64 i believe?
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> File media/base/simd/convert_rgb_to_yuv.h (right):
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> media/base/simd/convert_rgb_to_yuv.h:14: void ConvertRGB32toYUV(const uint8*
> rgbframe,
> Call this ConvertRGB32ToYUV_SSE3
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> media/base/simd/convert_rgb_to_yuv.h:14: void ConvertRGB32toYUV(const uint8*
> rgbframe,
> Add a comment that this outputs YV12.
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> media/base/simd/convert_rgb_to_yuv.h:24: void ConvertRGB24toYUV(const uint8*
> rgbframe,
> Call this Convert24ToYUV_SSE3
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> media/base/simd/convert_rgb_to_yuv.h:24: void ConvertRGB24toYUV(const uint8*
> rgbframe,
> Add a comment that this outputs YV12.
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> File media/base/simd/convert_rgb_to_yuv_ssse3.asm (right):
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> media/base/simd/convert_rgb_to_yuv_ssse3.asm:17: db        25, 2, 66, 0
> Please add comments here for what these constants represents, for example
which
> row is coefficients for Y, U or V.
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> media/base/simd/convert_rgb_to_yuv_ssse3.asm:22: 
> It looks like this is the UV offset, please add comments here.
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> File media/base/simd/convert_rgb_to_yuv_ssse3.h (right):
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_rgb...
> media/base/simd/convert_rgb_to_yuv_ssse3.h:17: void ConvertRGBtoYUVEven(const
> uint8* rgb,
> Add _SSSE3 suffix to these functions.
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
> File media/base/simd/convert_yuv_to_rgb.h (right):
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
> media/base/simd/convert_yuv_to_rgb.h:14: void ConvertYUVtoRGB32(const uint8*
> y_buf,
> Append _SSSE3 suffix.
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
> media/base/simd/convert_yuv_to_rgb.h:25: void ConvertYUVtoRGB24(const uint8*
> y_buf,
> Same here, _SSSE3 suffix.
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
> File media/base/simd/convert_yuv_to_rgb_ssse3.h (right):
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
> media/base/simd/convert_yuv_to_rgb_ssse3.h:16: void ConvertYUVtoRGBRow(const
> uint8* y,
> Add _SSSE3 suffix.
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/simd/convert_yuv...
> media/base/simd/convert_yuv_to_rgb_ssse3.h:23: void ConvertYUVtoARGBRow(const
> uint8* y,
> Add _SSSE3 suffix.
> 
> http://codereview.chromium.org/7003082/diff/34022/media/base/yuv_convert.cc
> File media/base/yuv_convert.cc (right):
> 
>
http://codereview.chromium.org/7003082/diff/34022/media/base/yuv_convert.cc#n...
> media/base/yuv_convert.cc:62: 
> Add a comment here that we use the MMX version as a fallback.
> 
> http://codereview.chromium.org/7003082/diff/34022/media/media.gyp
> File media/media.gyp (right):
> 
> http://codereview.chromium.org/7003082/diff/34022/media/media.gyp#newcode10
> media/media.gyp:10: 'use_system_yasm%': 0,
> Don't need this, we always build our own yasm.

[Alpha Left Google, 2011-09-06 11:27:46]

LGTM. Good work!!

[Alpha Left Google, 2011-09-06 17:08:31]

Sorry about reflecting.. 

There are a couple changes that I'd like to see:
1. use yasm_compile.gypi for compiling asm files
2. assembly files to not have media::simd namespace. This is because we'd like
to refactor the code or package it as a different library.. having mangled
symbols make refactoring harder..
3. move the files to media/base

I have a plan to tidy up the assembly files and color space conversion by moving
them to media/csc, so if you can make the above changes I'd really appreciate
that! Thanks!

http://codereview.chromium.org/7003082/diff/36053/media/base/simd/convert_rgb...
File media/base/simd/convert_rgb_to_yuv.h (right):

http://codereview.chromium.org/7003082/diff/36053/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv.h:12: namespace simd {
Please remove simd to make it consistent with other conversion routines.

http://codereview.chromium.org/7003082/diff/36053/media/base/simd/convert_rgb...
File media/base/simd/convert_rgb_to_yuv_ssse3.asm (right):

http://codereview.chromium.org/7003082/diff/36053/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv_ssse3.asm:249: %define SYMBOL         
__ZN5media4simd25ConvertARGBToYUVRow_SSSE3EPKhPhS3_S3_i
For assembly functions let's not use any namespace, which makes it easier for
refactoring later.

http://codereview.chromium.org/7003082/diff/36053/media/base/simd/convert_rgb...
File media/base/simd/convert_rgb_to_yuv_ssse3.h (right):

http://codereview.chromium.org/7003082/diff/36053/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv_ssse3.h:8: namespace media {
Please remove namespace for assembly functions, it'll make refactoring easier
later.

http://codereview.chromium.org/7003082/diff/36053/media/base/simd/convert_rgb...
media/base/simd/convert_rgb_to_yuv_ssse3.h:15: #ifdef ENABLE_SUBSAMPLING
Actually we don't need to have ifdef, we can always choose not to use them in
the .cc entry point.

http://codereview.chromium.org/7003082/diff/36053/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/7003082/diff/36053/media/media.gyp#newcode389
media/media.gyp:389: [ 'OS=="win"', {
I defined an include gypi file that helps to compile with yasm easier, please
switch to use that: http://codereview.chromium.org/7800039/

[Alpha Left Google, 2011-09-07 10:30:41]

Excellent! LGTM, thanks!

[Steve Block, 2012-03-19 15:37:16]

http://codereview.chromium.org/7003082/diff/51004/media/base/simd/x86inc.asm
File media/base/simd/x86inc.asm (right):

http://codereview.chromium.org/7003082/diff/51004/media/base/simd/x86inc.asm#...
media/base/simd/x86inc.asm:12: ;* copyright notice and this permission notice
appear in all copies.
Hi Hironori, it looks like this file is copied directly from x264ASM? It should
probably be moved to a directory under third_party/, so we can add a
README.chromium and a license file, and include the license in about:credits.

[Hironori Bono, 2012-03-20 13:31:20]

Greetings,

Thanks for your comment.
Yes, but Chrome now uses libyuv and it is better to remove this code to avoid
confusion. I will send a revert change soon.

Regards,

Hironori Bono