mmx for linux yuv convert function....

mmx for linux yuv convert function.
BUG=18449, 20718
TEST=play a video on linux without scaling and it should go 4 times faster than with scaling.


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

[fbarchard, 2009-08-25 20:41:07]

The code is not tested, but lets get the ball rolling...

[fbarchard, 2009-08-25 21:06:11]

enabled MMX for linux and updated unittest

[scherkus (not reviewing), 2009-08-25 21:27:00]

we need to clean up how we use defines because right now it's inconsistent with
the rest of the code base

also what's the status with mac?  if they're both gcc can we have a single
implementation?

http://codereview.chromium.org/174442/diff/1/2
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/174442/diff/1/2#newcode16
Line 16: #if USE_MMX && !defined(ARCH_CPU_X86_64)
should be:
#if defined(USE_MMX)

http://codereview.chromium.org/174442/diff/1/2#newcode17
Line 17: #define MMX_CONVERT 1
likewise, we would just define MMX_CONVERT without setting a value

http://codereview.chromium.org/174442/diff/1/2#newcode19
Line 19: #define MMX_CONVERT 0
...and remove this line as a result

http://codereview.chromium.org/174442/diff/1/2#newcode21
Line 21: namespace media {
add blank line before namespace media

http://codereview.chromium.org/174442/diff/1/2#newcode23
Line 23: 
remove extra blank line

http://codereview.chromium.org/174442/diff/1/2#newcode24
Line 24: #if MMX_CONVERT
#if defined(MMX_CONVERT)

http://codereview.chromium.org/174442/diff/1/2#newcode42
Line 42: static_cast<int16>(-0.813 * 64 * (i - 128) + 0.5), \
is this completely duplicated from media/base/yuv_row_win.cc?

if so can we create a media/base/yuv_row_common.cc that contains a single
definition of the tables?

http://codereview.chromium.org/174442/diff/1/2#newcode50
Line 50: MMX_ALIGNED(int16 coefficients_RGB_Y[256][4]) = {
static? const?

also coefficients_RGB_Y -> g_coefficients_RGB_Y

if const, then technically should be:
kCoefficientsRGBY

http://codereview.chromium.org/174442/diff/1/2#newcode117
Line 117: MMX_ALIGNED(int16 coefficients_RGB_U[256][4]) = {
same thing here

http://codereview.chromium.org/174442/diff/1/2#newcode184
Line 184: MMX_ALIGNED(int16 coefficients_RGB_V[256][4]) = {
same thing here

http://codereview.chromium.org/174442/diff/1/2#newcode263
Line 263: "ConvertYV12ToRGB32Row:\n"
should this be FastConvertYUVToRGB32Row?  it looks like if MMX_CONVERT was
defined, FastConvertYUVToRGB32Row would be unresolved

http://codereview.chromium.org/174442/diff/1/2#newcode271
Line 271: "1:\n"
could we have a more descriptive label than "1" or is it a gcc asm limitation
(for example your windows code uses "wloop")

http://codereview.chromium.org/174442/diff/1/2#newcode294
Line 294: );
I'm wondering if you may need to let gcc know which registers you're
clobbering.. It may not matter since this is declared globally

see http://www.ibiblio.org/gferg/ldp/GCC-Inline-Assembly-HOWTO.html#ss5.3

http://codereview.chromium.org/174442/diff/1/2#newcode297
Line 297: #endif
#endif  // defined(MMX_CONVERT)

http://codereview.chromium.org/174442/diff/1/2#newcode443
Line 443: #if !MMX_CONVERT
#if !defined(MMX_CONVERT)

it might also make more sense if this was specified as an #else clause to the
original defined(MMX_CONVERT) above

http://codereview.chromium.org/174442/diff/1/2#newcode461
Line 461: #endif
#endif  // !defined(MMX_CONVERT)

http://codereview.chromium.org/174442/diff/1005/1008
File media/base/yuv_convert_unittest.cc (right):

http://codereview.chromium.org/174442/diff/1005/1008#newcode34
Line 34: #if USE_MMX && (!defined(__linux) || !defined(ARCH_CPU_X86_64))
you should use the flags we have declared in /build/build_config.h:

__linux --> OS_LINUX

again USE_MMX should really be either defined or not so we can use
defined(USE_MMX) syntax

http://codereview.chromium.org/174442/diff/1005/1008#newcode89
Line 89: #if MMX_CONVERT
#if defined(MMX_CONVERT)

http://codereview.chromium.org/174442/diff/1005/1008#newcode131
Line 131: #if MMX_CONVERT
#if defined(MMX_CONVERT)

http://codereview.chromium.org/174442/diff/1005/1008#newcode242
Line 242: #if MMX_CONVERT
#if defined(MMX_CONVERT)

http://codereview.chromium.org/174442/diff/1005/1007
File media/base/yuv_row.h (right):

http://codereview.chromium.org/174442/diff/1005/1007#newcode63
Line 63: // MMX for Windows; C++ for other platforms.
comment is out of date

http://codereview.chromium.org/174442/diff/1005/1007#newcode64
Line 64: #ifndef USE_MMX
#if !defined(USE_MMX)

http://codereview.chromium.org/174442/diff/1005/1007#newcode65
Line 65: #if defined(_MSC_VER)
use defines declared in build/build_config.h:
#if defined(COMPILER_MSVC)

http://codereview.chromium.org/174442/diff/1005/1007#newcode67
Line 67: #elif defined(__linux)
use defines declared in build/build_config.h:
#if defined(OS_LINUX)

http://codereview.chromium.org/174442/diff/1005/1007#newcode70
Line 70: #define USE_MMX 0
avoid defining things as 0 and instead use:
#if defined(USE_MMX)

http://codereview.chromium.org/174442/diff/1005/1007#newcode74
Line 74: #if USE_MMX
#if defined(USE_MMX)

http://codereview.chromium.org/174442/diff/1005/1007#newcode75
Line 75: #if defined(_MSC_VER)
use defines declared in build/build_config.h:
#if defined(COMPILER_MSVC)

http://codereview.chromium.org/174442/diff/1005/1007#newcode77
Line 77: #else
should be:
#elif defined(COMPILER_GCC)
#define EMMS() asm("emms")
#else
#error Unsupported compiler
#endif

[Alpha Left Google, 2009-08-25 21:27:48]

http://codereview.chromium.org/174442/diff/1005/1009
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/174442/diff/1005/1009#newcode134
Line 134: #if USE_MMX && defined(_MSC_VER)
We define USE_MMX when _MSC_VER is defined, why do we need to add it again here?

http://codereview.chromium.org/174442/diff/1005/1008
File media/base/yuv_convert_unittest.cc (right):

http://codereview.chromium.org/174442/diff/1005/1008#newcode35
Line 35: #define MMX_CONVERT 1
We already have USE_MMX defined, why do we need to define MMX_CONVERT here?

http://codereview.chromium.org/174442/diff/1005/1007
File media/base/yuv_row.h (right):

http://codereview.chromium.org/174442/diff/1005/1007#newcode67
Line 67: #elif defined(__linux)
Shouldn't "ARCH_CPU_X86_64" goes here?

[fbarchard, 2009-08-26 04:23:18]

scale function implemented

http://codereview.chromium.org/174442/diff/1/2
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/174442/diff/1/2#newcode16
Line 16: #if USE_MMX && !defined(ARCH_CPU_X86_64)
USE_MMX is always defined... as either 0 or 1
That way it can be undefined and the logic will decide, or the command line can
force USE_MMX to either value.
The logic for this will change in the next update anyway

http://codereview.chromium.org/174442/diff/1/2#newcode17
Line 17: #define MMX_CONVERT 1
I'll remove this in the next update anyway

http://codereview.chromium.org/174442/diff/1/2#newcode19
Line 19: #define MMX_CONVERT 0
I'll remove this in the next update anyway

http://codereview.chromium.org/174442/diff/1/2#newcode21
Line 21: namespace media {
On 2009/08/25 21:27:00, scherkus wrote:
> add blank line before namespace media

Done.

http://codereview.chromium.org/174442/diff/1/2#newcode23
Line 23: 
On 2009/08/25 21:27:00, scherkus wrote:
> remove extra blank line

Done.

http://codereview.chromium.org/174442/diff/1/2#newcode24
Line 24: #if MMX_CONVERT
replacing with USE_MMX in next one.

http://codereview.chromium.org/174442/diff/1005/1009
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/174442/diff/1005/1009#newcode134
Line 134: #if USE_MMX && defined(_MSC_VER)
Those functions are not ported to linux MMX

http://codereview.chromium.org/174442/diff/1005/1008
File media/base/yuv_convert_unittest.cc (right):

http://codereview.chromium.org/174442/diff/1005/1008#newcode34
Line 34: #if USE_MMX && (!defined(__linux) || !defined(ARCH_CPU_X86_64))
okay... note that I test this code outside of chrome, but I've already had to
duplicate that header for base stuff.
Done.

[scherkus (not reviewing), 2009-08-26 08:38:25]

http://codereview.chromium.org/174442/diff/1048/61
File media/base/yuv_row.h (right):

http://codereview.chromium.org/174442/diff/1048/61#newcode65
Line 65: #ifndef USE_MMX
#if !defined(USE_MMX)

http://codereview.chromium.org/174442/diff/1048/60
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/174442/diff/1048/60#newcode41
Line 41: MMX_ALIGNED(int16 kCoefficients_RGB_Y[256][4]) = {
kCoefficients_RGB_Y -> kCoefficientsRGBY

http://codereview.chromium.org/174442/diff/1048/60#newcode41
Line 41: MMX_ALIGNED(int16 kCoefficients_RGB_Y[256][4]) = {
int16 -> const int16 (can't remember if extern "C" will complain about const
keyword)

what about static?

http://codereview.chromium.org/174442/diff/1048/60#newcode108
Line 108: MMX_ALIGNED(int16 kCoefficients_RGB_U[256][4]) = {
kCoefficients_RGB_U -> kCoefficientsRGBU

http://codereview.chromium.org/174442/diff/1048/60#newcode108
Line 108: MMX_ALIGNED(int16 kCoefficients_RGB_U[256][4]) = {
int16 -> const int16 (can't remember if extern "C" will complain about const
keyword)

what about static?

http://codereview.chromium.org/174442/diff/1048/60#newcode175
Line 175: MMX_ALIGNED(int16 kCoefficients_RGB_V[256][4]) = {
kCoefficients_RGB_V -> kCoefficientsRGBV

http://codereview.chromium.org/174442/diff/1048/60#newcode175
Line 175: MMX_ALIGNED(int16 kCoefficients_RGB_V[256][4]) = {
int16 -> const int16 (can't remember if extern "C" will complain about const
keyword)

what about static?

http://codereview.chromium.org/174442/diff/1048/60#newcode412
Line 412: #else  /* USE_MMX */
#else  // USE_MMX

http://codereview.chromium.org/174442/diff/1048/60#newcode595
Line 595: #endif /* USE_MMX */
#endif  // USE_MMX

[fbarchard, 2009-08-27 00:09:54]

this one may actually work!  its got the basic 2 functions for scaling and 1:1.
But not the specialized versions for scaling down, rotation and double size.

http://codereview.chromium.org/174442/diff/1048/61
File media/base/yuv_row.h (right):

http://codereview.chromium.org/174442/diff/1048/61#newcode65
Line 65: #ifndef USE_MMX
same thing.  is that a coding convention?

http://codereview.chromium.org/174442/diff/1048/61#newcode65
Line 65: #ifndef USE_MMX
On 2009/08/26 08:38:25, scherkus wrote:
> #if !defined(USE_MMX)

Done.

http://codereview.chromium.org/174442/diff/1048/60
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/174442/diff/1048/60#newcode41
Line 41: MMX_ALIGNED(int16 kCoefficients_RGB_Y[256][4]) = {
On 2009/08/26 08:38:25, scherkus wrote:
> kCoefficients_RGB_Y -> kCoefficientsRGBY

The Y being seperate is important.
maybe kCoefficientsRgbY?

http://codereview.chromium.org/174442/diff/1048/60#newcode41
Line 41: MMX_ALIGNED(int16 kCoefficients_RGB_Y[256][4]) = {
On 2009/08/26 08:38:25, scherkus wrote:
> int16 -> const int16 (can't remember if extern "C" will complain about const
> keyword)
> 
const causes build issues, we had to revert that.
Done.
But note that I may want to change them based on jpeg vs mpeg coefficients in
the future.  And const doesnt have a lot of meaning in assembler :)
> what about static?

Done.

http://codereview.chromium.org/174442/diff/1048/60#newcode41
Line 41: MMX_ALIGNED(int16 kCoefficients_RGB_Y[256][4]) = {
On 2009/08/26 08:38:25, scherkus wrote:
> kCoefficients_RGB_Y -> kCoefficientsRGBY

Done.

http://codereview.chromium.org/174442/diff/1048/60#newcode108
Line 108: MMX_ALIGNED(int16 kCoefficients_RGB_U[256][4]) = {
On 2009/08/26 08:38:25, scherkus wrote:
> int16 -> const int16 (can't remember if extern "C" will complain about const
> keyword)
> 
> what about static?

Done.

http://codereview.chromium.org/174442/diff/1048/60#newcode108
Line 108: MMX_ALIGNED(int16 kCoefficients_RGB_U[256][4]) = {
On 2009/08/26 08:38:25, scherkus wrote:
> kCoefficients_RGB_U -> kCoefficientsRGBU

Done.

http://codereview.chromium.org/174442/diff/1048/60#newcode175
Line 175: MMX_ALIGNED(int16 kCoefficients_RGB_V[256][4]) = {
On 2009/08/26 08:38:25, scherkus wrote:
> int16 -> const int16 (can't remember if extern "C" will complain about const
> keyword)
> 
> what about static?

Done.

http://codereview.chromium.org/174442/diff/1048/60#newcode175
Line 175: MMX_ALIGNED(int16 kCoefficients_RGB_V[256][4]) = {
On 2009/08/26 08:38:25, scherkus wrote:
> kCoefficients_RGB_V -> kCoefficientsRGBV

Done.

http://codereview.chromium.org/174442/diff/1048/60#newcode412
Line 412: #else  /* USE_MMX */
I do this on purpose when doing comments on preprocessor lines.  If you do //
the comment can affect code usage.

is this a coding guide violation?

http://codereview.chromium.org/174442/diff/1048/60#newcode595
Line 595: #endif /* USE_MMX */
On 2009/08/26 08:38:25, scherkus wrote:
> #endif  // USE_MMX

ditto

[fbarchard, 2009-08-27 00:56:11]

removing static

http://codereview.chromium.org/174442/diff/94/95
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/174442/diff/94/95#newcode42
Line 42: MMX_ALIGNED(static int16 kkCoefficientsRgbY[256][4]) = {
remove static - produces build error that symbol is unused, because C is not
aware of assembly

[scherkus (not reviewing), 2009-08-27 01:23:48]

few tiny nits but we're looking good!

http://codereview.chromium.org/174442/diff/104/1082
File media/base/yuv_row.h (right):

http://codereview.chromium.org/174442/diff/104/1082#newcode15
Line 15: extern "C" {
as a sanity check... no one outside of yuv_convert.cc calls these functions,
correct?

clients outside the media namespace may experience a build break if they were
previously calling the functions with a media:: prefix

I did some quick searching and I think we're ok -- if the try servers don't
complain we should be good

http://codereview.chromium.org/174442/diff/104/1081
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/174442/diff/104/1081#newcode249
Line 249: void __attribute__((optimize("O2", "no-gcse")))
can we comment or add a TODO on what our plans are for this function?

http://codereview.chromium.org/174442/diff/104/1081#newcode419
Line 419: #else  /* USE_MMX */
#else  // USE_MMX

If you're curious:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Comment_Style

Doing some grepping (not including third_party code):
There are 3165 instances of using // with # directives
There are 4 instances of using /* */ with # directives

In general // is preferred, but as long as it's consistent

http://codereview.chromium.org/174442/diff/104/1081#newcode602
Line 602: #endif /* USE_MMX */
#endif  // USE_MMX

http://codereview.chromium.org/174442/diff/104/1084
File media/base/yuv_row_win.cc (right):

http://codereview.chromium.org/174442/diff/104/1084#newcode34
Line 34: MMX_ALIGNED(static int16 kCoefficientsRgbY[256][4]) = {
static is not an issue for windows?

[fbarchard, 2009-08-28 16:59:26]

status - linux builds and doesnt crash, but scaler fails unit test.
mac doesnt link - the .global directive fails.
If we want is asap, I'd do visual inspection of the scaler and update the
unittest checksum if it looks okay.  And then remove mac support.
Otherwise I'll plug away on Mac and deliver both at the same time.

http://codereview.chromium.org/174442/diff/104/1082
File media/base/yuv_row.h (right):

http://codereview.chromium.org/174442/diff/104/1082#newcode15
Line 15: extern "C" {
correct.
The row functions are only in their own modules to separate out the platform
dependent code.
In the future, I could reattempt C++ with name mangling... it wont hurt the
performance.  I may end up using some really ugly mangled names to refer to the
tables.  I'd rather leave that til I do 64 bit assembly, or maybe the mac port.

http://codereview.chromium.org/174442/diff/104/1081
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/174442/diff/104/1081#newcode249
Line 249: void __attribute__((optimize("O2", "no-gcse")))
On 2009/08/27 01:23:48, scherkus wrote:
> can we comment or add a TODO on what our plans are for this function?

Done.

http://codereview.chromium.org/174442/diff/104/1081#newcode419
Line 419: #else  /* USE_MMX */
Okay, conventions are important.  Its just caused me bugs in the past, and once
bitten...
Done.

http://codereview.chromium.org/174442/diff/104/1081#newcode602
Line 602: #endif /* USE_MMX */
On 2009/08/27 01:23:48, scherkus wrote:
> #endif  // USE_MMX

Done.

http://codereview.chromium.org/174442/diff/104/1084
File media/base/yuv_row_win.cc (right):

http://codereview.chromium.org/174442/diff/104/1084#newcode34
Line 34: MMX_ALIGNED(static int16 kCoefficientsRgbY[256][4]) = {
oops... didn't update this file.
but it works for visual c because the assembly knows about labels, whereas gcc's
assembly is a big hack, where a string is passed thru the compiler and ends up
in the assembler, but the compiler didnt know about the content of the string.

[fbarchard, 2009-09-01 00:12:48]

This version contains working linux code.
Mac is partially working - it passes unittests, but fails to link into DLL. 
Disabled in this version.

[Alpha Left Google, 2009-09-01 00:47:14]

LGTM