Add correct support for videos with YUVJ420P color format, in the software conversion path.

Add correct support for videos with YUVJ420P color format, in the software conversion path.

BUG=172898
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268061

[rileya (GONE FROM CHROMIUM), 2014-04-22 17:57:50]

This depends on: https://codereview.chromium.org/245103003/

This should also land around the same time as
https://codereview.chromium.org/92703003/ so that the software and hardware
paths are consistent with one another.

https://codereview.chromium.org/242643011/diff/1/media/base/yuv_convert.h
File media/base/yuv_convert.h (right):

https://codereview.chromium.org/242643011/diff/1/media/base/yuv_convert.h#new...
media/base/yuv_convert.h:34: #define GET_LOOKUP_TABLE(type) \
Ugly macro, since we can't return a sized/multidimensional array from a function
in C++. Making all the conversion functions take a plain pointer would let us
use a function instead, but I sort of like making the table size explicit in the
function signatures ("table[foo][bar]" also looks nicer than "table[foo + bar *
1024]"). Thoughts?

[scherkus (not reviewing), 2014-04-22 18:53:33]

https://codereview.chromium.org/242643011/diff/1/media/base/simd/scale_yuv_to...
File media/base/simd/scale_yuv_to_rgb_sse2_x64.asm (right):

https://codereview.chromium.org/242643011/diff/1/media/base/simd/scale_yuv_to...
media/base/simd/scale_yuv_to_rgb_sse2_x64.asm:42: %define     COMPq R1q
nit: it seems like a lot of these .asm files whitespace-align the defines

https://codereview.chromium.org/242643011/diff/1/media/base/simd/yuv_to_rgb_t...
File media/base/simd/yuv_to_rgb_table.cc (right):

https://codereview.chromium.org/242643011/diff/1/media/base/simd/yuv_to_rgb_t...
media/base/simd/yuv_to_rgb_table.cc:669: 
nit: remove extra line

https://codereview.chromium.org/242643011/diff/1/media/base/yuv_convert.h
File media/base/yuv_convert.h (right):

https://codereview.chromium.org/242643011/diff/1/media/base/yuv_convert.h#new...
media/base/yuv_convert.h:34: #define GET_LOOKUP_TABLE(type) \
On 2014/04/22 17:57:50, rileya wrote:
> Ugly macro, since we can't return a sized/multidimensional array from a
function
> in C++. Making all the conversion functions take a plain pointer would let us
> use a function instead, but I sort of like making the table size explicit in
the
> function signatures ("table[foo][bar]" also looks nicer than "table[foo + bar
*
> 1024]"). Thoughts?

so there is some heinous syntax you can use to pass back a const-reference to a
sized array

const int (&GetLookupTable(YUVType type))[1024][4] {
  ...
}

since the arrays are global statics it's safe to pass back const-ref

downside is all the parameters to functions get a bit ugly too:

void MyFunc(const int (&table)[1024][4]) {
  ...
}

[rileya (GONE FROM CHROMIUM), 2014-04-23 21:23:50]

https://codereview.chromium.org/242643011/diff/1/media/base/simd/scale_yuv_to...
File media/base/simd/scale_yuv_to_rgb_sse2_x64.asm (right):

https://codereview.chromium.org/242643011/diff/1/media/base/simd/scale_yuv_to...
media/base/simd/scale_yuv_to_rgb_sse2_x64.asm:42: %define     COMPq R1q
On 2014/04/22 18:53:33, scherkus wrote:
> nit: it seems like a lot of these .asm files whitespace-align the defines

Fixed.

https://codereview.chromium.org/242643011/diff/1/media/base/simd/yuv_to_rgb_t...
File media/base/simd/yuv_to_rgb_table.cc (right):

https://codereview.chromium.org/242643011/diff/1/media/base/simd/yuv_to_rgb_t...
media/base/simd/yuv_to_rgb_table.cc:669: 
On 2014/04/22 18:53:33, scherkus wrote:
> nit: remove extra line

Removed.

https://codereview.chromium.org/242643011/diff/1/media/base/yuv_convert.h
File media/base/yuv_convert.h (right):

https://codereview.chromium.org/242643011/diff/1/media/base/yuv_convert.h#new...
media/base/yuv_convert.h:34: #define GET_LOOKUP_TABLE(type) \
On 2014/04/22 18:53:33, scherkus wrote:
> On 2014/04/22 17:57:50, rileya wrote:
> > Ugly macro, since we can't return a sized/multidimensional array from a
> function
> > in C++. Making all the conversion functions take a plain pointer would let
us
> > use a function instead, but I sort of like making the table size explicit in
> the
> > function signatures ("table[foo][bar]" also looks nicer than "table[foo +
bar
> *
> > 1024]"). Thoughts?
> 
> so there is some heinous syntax you can use to pass back a const-reference to
a
> sized array
> 
> const int (&GetLookupTable(YUVType type))[1024][4] {
>   ...
> }
> 
> since the arrays are global statics it's safe to pass back const-ref
> 
> downside is all the parameters to functions get a bit ugly too:
> 
> void MyFunc(const int (&table)[1024][4]) {
>   ...
> }

Looks like that worked without needing to modify the parameters (though I won't
be too surprised if visual studio chokes on it...).

[scherkus (not reviewing), 2014-04-24 19:48:27]

lgtm

re: passing back const-ref ... I'm not 100% sure but it might be worth sanity
checking that we're not accidentally doing a pass-by-value copy of the table
when going from const-ref to non-const-ref types

(sorry I had this typed up two days ago but didn't realize I never hit publish!)

[rileya (GONE FROM CHROMIUM), 2014-05-02 21:39:23]

I'm pretty certain an array parameter with square brackets will always decompose
into a pointer. The C++ spec says: "A consistent rule is followed for
multidimensional arrays. If E is an n-dimensional array of rank i×j×. . .×k,
then E appearing in an expression that is subject to the array-to-pointer
conversion (4.2) is converted to a pointer to an (n−1)-dimensional array with
rank j ×. . .×k. If the * operator, either explicitly or implicitly as a result
of subscripting, is applied to this pointer, the result is the pointed-to (n −
1)-dimensional array, which itself is immediately converted into a pointer."

I also made a little one-off program to test and it doesn't seem to be making a
copy, even for non-const arrays. Also the perf tests numbers are unaffected on
my machine, and I imagine copying an 8kb array onto the stack would be bad for
performance...

[rileya (GONE FROM CHROMIUM), 2014-05-02 21:39:40]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-05-02 21:39:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/242643011/50001

[commit-bot: I haz the power, 2014-05-02 22:08:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 22:08:13]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium

[rileya (GONE FROM CHROMIUM), 2014-05-02 22:11:53]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-05-02 22:12:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/242643011/70001

[commit-bot: I haz the power, 2014-05-02 22:48:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 22:48:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium

[rileya (GONE FROM CHROMIUM), 2014-05-02 23:19:58]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-05-02 23:20:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/242643011/90001

[commit-bot: I haz the power, 2014-05-02 23:48:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 23:48:57]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium

[rileya (GONE FROM CHROMIUM), 2014-05-02 23:52:13]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-05-02 23:52:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/242643011/110001

[commit-bot: I haz the power, 2014-05-03 00:52:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-03 00:52:48]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[rileya (GONE FROM CHROMIUM), 2014-05-03 01:05:38]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-05-03 01:06:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/242643011/130001

[commit-bot: I haz the power, 2014-05-03 03:28:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-03 03:28:33]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[rileya (GONE FROM CHROMIUM), 2014-05-03 04:22:28]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-05-03 04:22:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/242643011/130001

[commit-bot: I haz the power, 2014-05-03 10:17:31]

Change committed as 268061

[mlamouri (slow - plz ping), 2014-05-03 10:36:08]

A revert of this CL has been created in
https://codereview.chromium.org/263723004/ by mlamouri@chromium.org.

The reason for reverting is: Broke Linux Clang (dbg) bot.

http://build.chromium.org/p/chromium.linux/builders/Linux%20Clang%20%28dbg%29...

http://build.chromium.org/p/chromium.linux/builders/Linux%20Clang%20%28dbg%29....