Add ARGBExtractAlpha function

Add ARGBExtractAlpha function

BUG=libyuv:572
R=fbarchard@google.com

Committed: https://chromium.googlesource.com/libyuv/libyuv/+/942db3016a1653e66eb7935966449e06bdceb7b3

[magjed_chromium, 2016-05-20 10:57:10]

Patchset #3 (id:40001) has been deleted

[magjed_chromium, 2016-05-20 10:57:18]

Patchset #2 (id:20001) has been deleted

[magjed_chromium, 2016-05-20 10:57:24]

Patchset #1 (id:1) has been deleted

[magjed_chromium, 2016-05-20 12:10:15]

Patchset #2 (id:80001) has been deleted

[magjed_chromium, 2016-05-20 12:10:20]

Patchset #1 (id:60001) has been deleted

[magjed_chromium, 2016-05-20 12:11:03]

Description was changed from

==========
Add ARGBExtractAlpha function

BUG=libyuv:571
==========

to

==========
Add ARGBExtractAlpha function

BUG=libyuv:571
==========

[magjed_chromium, 2016-05-20 12:11:03]

magjed@chromium.org changed reviewers:
+ fbarchard@chromium.org

[magjed_chromium, 2016-05-20 12:11:41]

Frank - Please take a look.

[magjed_chromium, 2016-05-23 08:43:57]

Description was changed from

==========
Add ARGBExtractAlpha function

BUG=libyuv:571
==========

to

==========
Add ARGBExtractAlpha function

BUG=libyuv:572
==========

[fbarchard1, 2016-05-25 01:45:10]

fbarchard@google.com changed reviewers:
+ fbarchard@google.com

[fbarchard1, 2016-05-25 01:45:11]

https://codereview.chromium.org/1995293002/diff/100001/source/row_any.cc
File source/row_any.cc (right):

https://codereview.chromium.org/1995293002/diff/100001/source/row_any.cc#newc...
source/row_any.cc:519: #ifdef HAS_ARGBCOPYALPHAROW_SSE2
good catch!

https://codereview.chromium.org/1995293002/diff/100001/source/row_any.cc#newc...
source/row_any.cc:523: ANY11B(ARGBExtractAlphaRow_Any_SSE2,
ARGBExtractAlphaRow_SSE2, 0, 4, 1, 7)
I think ANY11B is the wrong function to use.  These functions read the
destination, modify it, and write it back.

ANY11 will simply copy from source to destination.

[magjed_chromium, 2016-05-25 08:19:46]

https://codereview.chromium.org/1995293002/diff/100001/source/row_any.cc
File source/row_any.cc (right):

https://codereview.chromium.org/1995293002/diff/100001/source/row_any.cc#newc...
source/row_any.cc:523: ANY11B(ARGBExtractAlphaRow_Any_SSE2,
ARGBExtractAlphaRow_SSE2, 0, 4, 1, 7)
On 2016/05/25 01:45:11, fbarchard1 wrote:
> I think ANY11B is the wrong function to use.  These functions read the
> destination, modify it, and write it back.
> 
> ANY11 will simply copy from source to destination.

I see. Changed to ANY11 instead.

https://codereview.chromium.org/1995293002/diff/120001/source/row_any.cc
File source/row_any.cc (right):

https://codereview.chromium.org/1995293002/diff/120001/source/row_any.cc#newc...
source/row_any.cc:300: SIMD_ALIGNED(uint8 temp[128 * 2]);                       
               \
What's the reason for using 128 instead of (MASK + 1) in these macros?

[fbarchard1, 2016-05-25 18:44:12]

https://codereview.chromium.org/1995293002/diff/120001/source/row_any.cc
File source/row_any.cc (right):

https://codereview.chromium.org/1995293002/diff/120001/source/row_any.cc#newc...
source/row_any.cc:300: SIMD_ALIGNED(uint8 temp[128 * 2]);                       
               \
On 2016/05/25 08:19:46, magjed_chromium wrote:
> What's the reason for using 128 instead of (MASK + 1) in these macros?

simplicity.  they used to be the width of a single SIMD register, but some code
is unrolled to more than that and some use different BPP, so it was error prone.
If you feel inclined to change it, it could be a macro for max simd alignment,
or use 'mask', which is width - 1 of the SIMD loop alignment, it would be easier
to use the alignment value so you dont have to add + 1.  Then multiply by BPP
and consider subsampling if applicable.

[fbarchard1, 2016-05-25 21:34:36]

lgtm

https://codereview.chromium.org/1995293002/diff/120001/source/row_gcc.cc
File source/row_gcc.cc (right):

https://codereview.chromium.org/1995293002/diff/120001/source/row_gcc.cc#newc...
source/row_gcc.cc:2951: "packuswb  %%xmm1, %%xmm0                  \n"
ditto bit... could use xmm0
packuswb  %%xmm0, %%xmm0

https://codereview.chromium.org/1995293002/diff/120001/source/row_win.cc
File source/row_win.cc (right):

https://codereview.chromium.org/1995293002/diff/120001/source/row_win.cc#newc...
source/row_win.cc:3551: packuswb   xmm0, xmm1
nit
should this be packuswb   xmm0, xmm0?

packssdw   xmm0, xmm1
took 32 bytes (8 ints) in xmm0 and xmm1 and made 16 bytes (8 shorts) in xmm0.
I guess the end result isnt different but packuswb   xmm0, xmm0 would take 16
bytes (8 shorts) and produce 8 bytes, using just the 1 register.
ditto for gcc version.

https://codereview.chromium.org/1995293002/diff/120001/source/row_win.cc#newc...
source/row_win.cc:3552: movq       qword ptr [edx], xmm0
fyi could be a little faster if it writes full 16 bytes, so making loop do 64
bytes of source.  Probably best to keep the loop smaller though

[magjed_chromium, 2016-05-26 08:28:33]

https://codereview.chromium.org/1995293002/diff/120001/source/row_gcc.cc
File source/row_gcc.cc (right):

https://codereview.chromium.org/1995293002/diff/120001/source/row_gcc.cc#newc...
source/row_gcc.cc:2951: "packuswb  %%xmm1, %%xmm0                  \n"
On 2016/05/25 21:34:35, fbarchard1 wrote:
> ditto bit... could use xmm0
> packuswb  %%xmm0, %%xmm0 

Done.

https://codereview.chromium.org/1995293002/diff/120001/source/row_win.cc
File source/row_win.cc (right):

https://codereview.chromium.org/1995293002/diff/120001/source/row_win.cc#newc...
source/row_win.cc:3551: packuswb   xmm0, xmm1
On 2016/05/25 21:34:35, fbarchard1 wrote:
> nit
> should this be packuswb   xmm0, xmm0?
> 
> packssdw   xmm0, xmm1
> took 32 bytes (8 ints) in xmm0 and xmm1 and made 16 bytes (8 shorts) in xmm0.
> I guess the end result isnt different but packuswb   xmm0, xmm0 would take 16
> bytes (8 shorts) and produce 8 bytes, using just the 1 register.
> ditto for gcc version.

Done.

[magjed_chromium, 2016-05-26 08:31:02]

Description was changed from

==========
Add ARGBExtractAlpha function

BUG=libyuv:572
==========

to

==========
Add ARGBExtractAlpha function

BUG=libyuv:572
R=fbarchard@google.com

Committed:
https://chromium.googlesource.com/libyuv/libyuv/+/942db3016a1653e66eb79359664...
==========

[magjed_chromium, 2016-05-26 08:31:04]

Committed patchset #3 (id:140001) manually as
942db3016a1653e66eb7935966449e06bdceb7b3 (presubmit successful).