planar blend use signed images

planar blend use signed images

R=dhrosa@google.com, harryjin@google.com, jzern@chromium.org
BUG=libyuv:527
Committed: https://chromium.googlesource.com/libyuv/libyuv/+/8af0ebf8166a141e0d8798cfee1ac6b3b9365511

[fbarchard, 2015-12-01 19:36:35]

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

[fbarchard, 2015-12-01 19:36:35]

unittests are passing.  ptal

[harryjin, 2015-12-01 21:29:46]

lgtm

[jzern, 2015-12-02 00:31:32]

jzern@chromium.org changed reviewers:
+ jzern@chromium.org

[jzern, 2015-12-02 00:31:33]

lgtm.
as mentioned offline with more implementations it might be worth it to test the
0/255 cases and compare against the source for correctness.

https://codereview.chromium.org/1491533002/diff/60001/source/row_win.cc
File source/row_win.cc (right):

https://codereview.chromium.org/1491533002/diff/60001/source/row_win.cc#newco...
source/row_win.cc:4068: // =((G2*C2)+(H2*(D2))+32768+127)/256
(D2) could be D2 to match the other side of the addition

[Diony Rosa, 2015-12-02 18:26:23]

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

[Diony Rosa, 2015-12-02 18:26:24]

https://codereview.chromium.org/1491533002/diff/60001/source/row_common.cc
File source/row_common.cc (right):

https://codereview.chromium.org/1491533002/diff/60001/source/row_common.cc#ne...
source/row_common.cc:2027: *dst++ = (((a) * f) + ((255 - a) * b) + 255) >> 8;
Why not use the BLEND macro here?

[Diony Rosa, 2015-12-02 19:10:26]

lgtm

https://codereview.chromium.org/1491533002/diff/60001/source/row_common.cc
File source/row_common.cc (right):

https://codereview.chromium.org/1491533002/diff/60001/source/row_common.cc#ne...
source/row_common.cc:2027: *dst++ = (((a) * f) + ((255 - a) * b) + 255) >> 8;
On 2015/12/02 18:26:24, Diony Rosa wrote:
> Why not use the BLEND macro here?

Done.

Oops, BLEND is for premultiplied alpha blends.

[fbarchard, 2015-12-02 22:13:43]

bumped version and added test for alpha = 255 producing exactly the source
image.

https://codereview.chromium.org/1491533002/diff/60001/source/row_common.cc
File source/row_common.cc (right):

https://codereview.chromium.org/1491533002/diff/60001/source/row_common.cc#ne...
source/row_common.cc:2027: *dst++ = (((a) * f) + ((255 - a) * b) + 255) >> 8;
On 2015/12/02 18:26:24, Diony Rosa wrote:
> Why not use the BLEND macro here?

Acknowledged.
3 reasons:
1. ARGB Blending uses RGB premultiplied by alpha, so the formula is f+b*(1-a),
while YUV uses unmultiplied YUV so its f*a+ b * (1-a)

2. BLEND doesnt round.  Its better to round, to preserve the full range of 0 to
255, but takes an additional add instruction.  For YUV I used a bias that
required the add anyway.

3. BLEND and the current C code, use unsigned math that the assembly should
match closely, if not exact.  But the SSSE3 code uses signed math that is fairly
different, and yet expected to match.  In this first version, to prove the
concept that the signed math matches the unsigned reference formula, I did them
differently.  I plan to followup with redoing this C version using signed math
that exactly matches the SSSE3, to better document how the assembly works.
Although the NEON version will probably use unsigned math, since it dosent have
pmaddubsw saturation constraint, so it could stay as is.
I'll also follow up with optimizing both the C and SSSE3.  The C should be
unrolled and it would be more clear at that point to create a BLEND macro.

Arm can do rounding (shift) for free, so time permitting, I'd like to revisit
the C and Intel versions and add rounding.  And perhaps redo ARGB alpha blending
to support unmultiplied RGB, with an implementation similar to YUV (using
pmaddubsw)

[fbarchard1, 2015-12-02 22:20:22]

Description was changed from

==========
planar blend use signed images

R=harryjin@google.com
BUG=libyuv:527
==========

to

==========
planar blend use signed images

R=dhrosa@google.com, harryjin@google.com, jzern@chromium.org
BUG=libyuv:527

Committed:
https://chromium.googlesource.com/libyuv/libyuv/+/8af0ebf8166a141e0d8798cfee1...
==========

[fbarchard1, 2015-12-02 22:20:24]

Committed patchset #4 (id:60001) manually as
8af0ebf8166a141e0d8798cfee1ac6b3b9365511 (presubmit successful).