ScaleYV12 optimization....

YUV with clipping.
All functions do 2 pixels at a time.
90 and 270 rotations implemented.
YV16 refactored. YV12 code accepts a YuvType that allows the same code to support YV16 as well.
Special case for half size removed.
Special case for doubling added.  3.62 ms versus 8.62 for general purpose code.


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

[fbarchard, 2009-05-14 19:00:57]

Bit faster MMX code for scaling.

[fbarchard, 2009-05-15 19:58:32]

I've removed YV16 entry point for convert... see what you think

[fbarchard, 2009-05-15 21:42:45]

Take a look at the Chrome change
http://codereview.chromium.org/113407/diff/1010/1015
I think the new function looks pretty, scales up to more formats, and saved me a
lot of implementation and testing.

[scherkus (not reviewing), 2009-05-16 00:48:59]

Mostly style nits.

I'm wondering if we should switch to using a struct to pass in the parameters so
we don't need to change the function signature every time.

http://codereview.chromium.org/113407/diff/1010/1015
File chrome/renderer/media/video_renderer_impl.cc (right):

http://codereview.chromium.org/113407/diff/1010/1015#newcode87
Line 87: frame_in.data[media::VideoSurface::kUPlane],
fix alignment here

http://codereview.chromium.org/113407/diff/1010/1018
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/113407/diff/1010/1018#newcode95
Line 95: for (int y = 0; y < static_cast<int>(height); ++y) {
if you do end up changing width/height to int, don't forget to remove the
static_casts

http://codereview.chromium.org/113407/diff/1010/1018#newcode102
Line 102: u_ptr,
fix alignment here

http://codereview.chromium.org/113407/diff/1010/1018#newcode112
Line 112: const uint8* u_buf,
fix alignment here

http://codereview.chromium.org/113407/diff/1010/1019
File media/base/yuv_convert.h (right):

http://codereview.chromium.org/113407/diff/1010/1019#newcode12
Line 12: // Type of YUV surface.
mention that the values of the constant are rather important and should not be
changed

http://codereview.chromium.org/113407/diff/1010/1019#newcode13
Line 13: enum YuvType {
nit: YuvType -> YUVType

http://codereview.chromium.org/113407/diff/1010/1019#newcode32
Line 32: // YV16 (aka YUV422) supported by passing 0 for y_shift
update this comment to say something like "Convert YUV to 32 bit ARGB.  Pass in
YV16/YV12 depending on source format"

or something like that

http://codereview.chromium.org/113407/diff/1010/1019#newcode37
Line 37: size_t frame_width,
should frame_width/frame_height be int as well to match ScaleYUVtoRGB32?

http://codereview.chromium.org/113407/diff/1010/1019#newcode44
Line 44: // Scale a frame of YV12 (aka YUV420) to 32 bit ARGB.
same with this comment

http://codereview.chromium.org/113407/diff/1010/1016
File media/base/yuv_row.h (right):

http://codereview.chromium.org/113407/diff/1010/1016#newcode15
Line 15: // Can only do 1x
period at end of comment

http://codereview.chromium.org/113407/diff/1010/1016#newcode16
Line 16: // Odd width handled efficiently.  Odd rgb alignment works, but 20%
slower.
rgb -> RGB

http://codereview.chromium.org/113407/diff/1010/1016#newcode19
Line 19: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1016#newcode24
Line 24: // Can do 1x, half size or any scale down by an integer amount
period at end of comment

http://codereview.chromium.org/113407/diff/1010/1016#newcode26
Line 26: // Odd width handled efficiently.  Odd rgb alignment works, but 20%
slower.
rgb -> RGB

http://codereview.chromium.org/113407/diff/1010/1016#newcode29
Line 29: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1016#newcode40
Line 40: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1016#newcode51
Line 51: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1016#newcode61
Line 61: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1016#newcode67
Line 67: // Deprecated.
can you say why or perhaps which other functions to call instead?

http://codereview.chromium.org/113407/diff/1010/1016#newcode70
Line 70: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1012
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/113407/diff/1010/1012#newcode154
Line 154: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1012#newcode187
Line 187: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1012#newcode218
Line 218: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1013
File media/base/yuv_row_mac.cc (right):

http://codereview.chromium.org/113407/diff/1010/1013#newcode155
Line 155: const uint8* v_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1013#newcode187
Line 187: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1013#newcode218
Line 218: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1020
File media/base/yuv_row_win.cc (right):

http://codereview.chromium.org/113407/diff/1010/1020#newcode250
Line 250: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1020#newcode294
Line 294: const uint8* v_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1020#newcode355
Line 355: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1020#newcode401
Line 401: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1020#newcode448
Line 448: const uint8* u_buf,
fix alignment

http://codereview.chromium.org/113407/diff/1010/1023
File media/player/view.h (right):

http://codereview.chromium.org/113407/diff/1010/1023#newcode12
Line 12: #define TESTING 1
did you mean to leave this uncommented?

http://codereview.chromium.org/113407/diff/1010/1023#newcode207
Line 207: double yuv_time_start = GetTime();             // Start timer.
two spaces between ; and //

http://codereview.chromium.org/113407/diff/1010/1023#newcode224
Line 224: if (!sws_context_) {
do we still need sws_scale code here?

if so, replace the /* */ with #if 0 / #endif

http://codereview.chromium.org/113407/diff/1010/1023#newcode235
Line 235: height_, data_out, stride_out);
if we're keeping this, indent this line to align these parameters

http://codereview.chromium.org/113407/diff/1010/1023#newcode402
Line 402: media::ConvertYUVToRGB32(frame_in.data[0],
this is over-indented by two spaces

http://codereview.chromium.org/113407/diff/1010/1023#newcode403
Line 403: frame_in.data[1],
fix alignment

http://codereview.chromium.org/113407/diff/1010/1023#newcode412
Line 412: media::ScaleYUVToRGB32(frame_in.data[0],
this is over-indented by two spaces

http://codereview.chromium.org/113407/diff/1010/1023#newcode413
Line 413: frame_in.data[1],
fix alignment

[fbarchard, 2009-05-16 04:44:22]

Done.
The Windows version now implements all assembler functions with 2 pixels at a
time for efficiency, but handles odd number  of pixels.
The Mac and Linux still use reference C code and do not support rotation.

http://codereview.chromium.org/113407/diff/1010/1015
File chrome/renderer/media/video_renderer_impl.cc (right):

http://codereview.chromium.org/113407/diff/1010/1015#newcode87
Line 87: frame_in.data[media::VideoSurface::kUPlane],
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment here

Done.

http://codereview.chromium.org/113407/diff/1010/1018
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/113407/diff/1010/1018#newcode95
Line 95: for (int y = 0; y < static_cast<int>(height); ++y) {
On 2009/05/16 00:48:59, scherkus wrote:
> if you do end up changing width/height to int, don't forget to remove the
> static_casts

Done.

http://codereview.chromium.org/113407/diff/1010/1018#newcode112
Line 112: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment here

Done.

http://codereview.chromium.org/113407/diff/1010/1019
File media/base/yuv_convert.h (right):

http://codereview.chromium.org/113407/diff/1010/1019#newcode12
Line 12: // Type of YUV surface.
On 2009/05/16 00:48:59, scherkus wrote:
> mention that the values of the constant are rather important and should not be
> changed

Done.

http://codereview.chromium.org/113407/diff/1010/1019#newcode13
Line 13: enum YuvType {
On 2009/05/16 00:48:59, scherkus wrote:
> nit: YuvType -> YUVType

Done.

http://codereview.chromium.org/113407/diff/1010/1019#newcode32
Line 32: // YV16 (aka YUV422) supported by passing 0 for y_shift
On 2009/05/16 00:48:59, scherkus wrote:
> update this comment to say something like "Convert YUV to 32 bit ARGB.  Pass
in
> YV16/YV12 depending on source format"
> 
> or something like that

Done.

http://codereview.chromium.org/113407/diff/1010/1019#newcode37
Line 37: size_t frame_width,
On 2009/05/16 00:48:59, scherkus wrote:
> should frame_width/frame_height be int as well to match ScaleYUVtoRGB32?

Done.

http://codereview.chromium.org/113407/diff/1010/1019#newcode44
Line 44: // Scale a frame of YV12 (aka YUV420) to 32 bit ARGB.
On 2009/05/16 00:48:59, scherkus wrote:
> same with this comment

Done.

http://codereview.chromium.org/113407/diff/1010/1016
File media/base/yuv_row.h (right):

http://codereview.chromium.org/113407/diff/1010/1016#newcode15
Line 15: // Can only do 1x
On 2009/05/16 00:48:59, scherkus wrote:
> period at end of comment

Done.

http://codereview.chromium.org/113407/diff/1010/1016#newcode16
Line 16: // Odd width handled efficiently.  Odd rgb alignment works, but 20%
slower.
On 2009/05/16 00:48:59, scherkus wrote:
> rgb -> RGB

Done.

http://codereview.chromium.org/113407/diff/1010/1016#newcode19
Line 19: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1016#newcode24
Line 24: // Can do 1x, half size or any scale down by an integer amount
On 2009/05/16 00:48:59, scherkus wrote:
> period at end of comment

Done.

http://codereview.chromium.org/113407/diff/1010/1016#newcode26
Line 26: // Odd width handled efficiently.  Odd rgb alignment works, but 20%
slower.
On 2009/05/16 00:48:59, scherkus wrote:
> rgb -> RGB

Done.

http://codereview.chromium.org/113407/diff/1010/1016#newcode29
Line 29: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1016#newcode40
Line 40: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1016#newcode51
Line 51: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1016#newcode61
Line 61: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1016#newcode67
Line 67: // Deprecated.
removed them

http://codereview.chromium.org/113407/diff/1010/1016#newcode70
Line 70: const uint8* u_buf,
removed

http://codereview.chromium.org/113407/diff/1010/1012
File media/base/yuv_row_linux.cc (right):

http://codereview.chromium.org/113407/diff/1010/1012#newcode154
Line 154: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1012#newcode187
Line 187: const uint8* u_buf,
removed this function

http://codereview.chromium.org/113407/diff/1010/1012#newcode218
Line 218: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1013
File media/base/yuv_row_mac.cc (right):

http://codereview.chromium.org/113407/diff/1010/1013#newcode155
Line 155: const uint8* v_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1013#newcode187
Line 187: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1013#newcode218
Line 218: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1020
File media/base/yuv_row_win.cc (right):

http://codereview.chromium.org/113407/diff/1010/1020#newcode250
Line 250: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1020#newcode294
Line 294: const uint8* v_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1020#newcode355
Line 355: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1020#newcode401
Line 401: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1020#newcode448
Line 448: const uint8* u_buf,
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1023
File media/player/view.h (right):

http://codereview.chromium.org/113407/diff/1010/1023#newcode12
Line 12: #define TESTING 1
On 2009/05/16 00:48:59, scherkus wrote:
> did you mean to leave this uncommented?

Done.

http://codereview.chromium.org/113407/diff/1010/1023#newcode207
Line 207: double yuv_time_start = GetTime();             // Start timer.
On 2009/05/16 00:48:59, scherkus wrote:
> two spaces between ; and //

Done.

http://codereview.chromium.org/113407/diff/1010/1023#newcode224
Line 224: if (!sws_context_) {
Yes, I occassionally want to test performance against swscaler.
I'll make it use a macro
#ifdef TEST_SWSCALER

http://codereview.chromium.org/113407/diff/1010/1023#newcode235
Line 235: height_, data_out, stride_out);
On 2009/05/16 00:48:59, scherkus wrote:
> if we're keeping this, indent this line to align these parameters

Done.

http://codereview.chromium.org/113407/diff/1010/1023#newcode402
Line 402: media::ConvertYUVToRGB32(frame_in.data[0],
On 2009/05/16 00:48:59, scherkus wrote:
> this is over-indented by two spaces

Done.

http://codereview.chromium.org/113407/diff/1010/1023#newcode403
Line 403: frame_in.data[1],
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

http://codereview.chromium.org/113407/diff/1010/1023#newcode412
Line 412: media::ScaleYUVToRGB32(frame_in.data[0],
On 2009/05/16 00:48:59, scherkus wrote:
> this is over-indented by two spaces

Done.

http://codereview.chromium.org/113407/diff/1010/1023#newcode413
Line 413: frame_in.data[1],
On 2009/05/16 00:48:59, scherkus wrote:
> fix alignment

Done.

[Alpha Left Google, 2009-05-18 20:50:28]

a few nits, will be checked in soon! :)

http://codereview.chromium.org/113407/diff/1259/1267
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/113407/diff/1259/1267#newcode10
Line 10: //   These formulae are based on the NTSC standard;
semicolon -> colon?

http://codereview.chromium.org/113407/diff/1259/1267#newcode76
Line 76: // Header for low level assembler functions
low level row functions?

http://codereview.chromium.org/113407/diff/1259/1267#newcode91
Line 91: int y_shift = yuv_type;
can you change this to unsigned so the right shift below is always good?

http://codereview.chromium.org/113407/diff/1259/1267#newcode124
Line 124: int y_shift = yuv_type;
unsigned?

http://codereview.chromium.org/113407/diff/1259/1267#newcode153
Line 153: int scaled_dy = height * 16 / scaled_height;
beware of divided by zero.

http://codereview.chromium.org/113407/diff/1259/1268
File media/base/yuv_convert.h (right):

http://codereview.chromium.org/113407/diff/1259/1268#newcode16
Line 16: YV12 = 1,           // YV12 is half width and half height chroma
channels.
can just say YV12, the compiler assigns the values incrementally.

http://codereview.chromium.org/113407/diff/1259/1268#newcode48
Line 48: const uint8* uplane,
indentation.

[fbarchard, 2009-05-18 21:50:41]

done

http://codereview.chromium.org/113407/diff/1259/1267
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/113407/diff/1259/1267#newcode76
Line 76: // Header for low level assembler functions
On 2009/05/18 20:50:28, Alpha wrote:
> low level row functions?

Done.

http://codereview.chromium.org/113407/diff/1259/1267#newcode91
Line 91: int y_shift = yuv_type;
On 2009/05/18 20:50:28, Alpha wrote:
> can you change this to unsigned so the right shift below is always good?

Done.

http://codereview.chromium.org/113407/diff/1259/1267#newcode124
Line 124: int y_shift = yuv_type;
On 2009/05/18 20:50:28, Alpha wrote:
> unsigned?

Done.

http://codereview.chromium.org/113407/diff/1259/1267#newcode153
Line 153: int scaled_dy = height * 16 / scaled_height;
On 2009/05/18 20:50:28, Alpha wrote:
> beware of divided by zero.

Done.

http://codereview.chromium.org/113407/diff/1259/1268
File media/base/yuv_convert.h (right):

http://codereview.chromium.org/113407/diff/1259/1268#newcode16
Line 16: YV12 = 1,           // YV12 is half width and half height chroma
channels.
Yes, but the value really matters, so I feel safer assigning it the value is
needs to be.

http://codereview.chromium.org/113407/diff/1259/1268#newcode48
Line 48: const uint8* uplane,
On 2009/05/18 20:50:28, Alpha wrote:
> indentation.

Done.

[Alpha Left Google, 2009-05-18 21:51:56]

LGTM.