O2D: Fix graphical artifacting with transparent images caused by using non-pr...

O2D: Fix graphical artifacting with transparent images caused by using non-premultiplied alpha ARGB images as if they were premultiplied alpha ARGB. This caused the transparent parts of images to be too bright (and in some cases fully opaque). Cairo only supports premultiplied alpha, but O3D calls SetRect() with non-premultiplied alpha, so we have to convert.

TEST=loaded O2D and verified transparent images look correct now
BUG=none


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

[Tristan Schmelcher 2, 2011-01-21 19:02:42]

Ping?

[fbarchard, 2011-01-24 19:23:08]

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo.cc
File core/cross/cairo/texture_cairo.cc (right):

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:2: * Copyright 2010, Google Inc.
2011

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:38: #include <string.h>
is this needed?

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:174: dst_pixel |= (src_pixel & 0xFF) * alpha /
255;
Should add rounding
dst_pixel |= ((src_pixel & 0xFF) * alpha + 128) / 255;

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:178: dst_pixel |= (((src_pixel >> 16) & 0xFF)
* alpha / 255) << 16;
note that this will be slow overall.
would be more efficient using bytes?
could use table lookup?

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo.h
File core/cross/cairo/texture_cairo.h (right):

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.h:2: * Copyright 2010, Google Inc.
2011

[Tristan Schmelcher 2, 2011-01-24 19:43:37]

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo.cc
File core/cross/cairo/texture_cairo.cc (right):

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:2: * Copyright 2010, Google Inc.
On 2011/01/24 19:23:09, fbarchard wrote:
> 2011

Done.

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:38: #include <string.h>
On 2011/01/24 19:23:09, fbarchard wrote:
> is this needed?

Yes, it's for memcpy.

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:174: dst_pixel |= (src_pixel & 0xFF) * alpha /
255;
On 2011/01/24 19:23:09, fbarchard wrote:
> Should add rounding
> dst_pixel |= ((src_pixel & 0xFF) * alpha + 128) / 255;

Done.

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:178: dst_pixel |= (((src_pixel >> 16) & 0xFF)
* alpha / 255) << 16;
On 2011/01/24 19:23:09, fbarchard wrote:
> note that this will be slow overall.
> would be more efficient using bytes?

Hmm, I will try both and look at the compiled assembly code to see which looks
better.

> could use table lookup?

Probably more work than it's worth, since this won't be on the critical path.

[fbarchard, 2011-01-24 19:56:19]

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo.cc
File core/cross/cairo/texture_cairo.cc (right):

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:178: dst_pixel |= (((src_pixel >> 16) & 0xFF)
* alpha / 255) << 16;
On 2011/01/24 19:43:37, Tristan Schmelcher 2 wrote:
> On 2011/01/24 19:23:09, fbarchard wrote:
> > note that this will be slow overall.
> > would be more efficient using bytes?
> 
> Hmm, I will try both and look at the compiled assembly code to see which looks
> better.

Using int you have 2 shifts, an AND and OR per channel.
Using char, you have a load and store.
Char will certainly take fewer instructions.  But a benchmark is the way to know
performance.
> 
> > could use table lookup?
> 
> Probably more work than it's worth, since this won't be on the critical path.

In a use case that uploads images every frame, this will be a bottleneck.
One simple optimization is skip this for alpha == 0 or alpha == 255.

[Tim H, 2011-01-24 19:57:55]

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo.cc
File core/cross/cairo/texture_cairo.cc (right):

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:165: for (unsigned i = 0; i < src_height; ++i)
{
Are i and j typed correctly?

[Tristan Schmelcher 2, 2011-01-24 20:25:28]

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo.cc
File core/cross/cairo/texture_cairo.cc (right):

http://codereview.chromium.org/6309007/diff/1/core/cross/cairo/texture_cairo....
core/cross/cairo/texture_cairo.cc:165: for (unsigned i = 0; i < src_height; ++i)
{
On 2011/01/24 19:57:55, thaloun wrote:
> Are i and j typed correctly?

Yeah, they are unsigned to prevent compile warnings.

[Tristan Schmelcher 2, 2011-01-25 22:51:28]

I've changed it to use byte logic now. The source code and assembly are both
shorter and simpler. Here's the assembly produced by a Release build for 64-bit
Linux.

 160:   44 0f b6 57 03          movzbl 0x3(%rdi),%r10d
 165:   48 8d 77 03             lea    0x3(%rdi),%rsi
 169:   48 89 f9                mov    %rdi,%rcx
 16c:   4c 89 c3                mov    %r8,%rbx
 16f:   45 0f b6 ca             movzbl %r10b,%r9d
 173:   0f b6 11                movzbl (%rcx),%edx
 176:   48 83 c1 01             add    $0x1,%rcx
 17a:   41 0f af d1             imul   %r9d,%edx
 17e:   83 ea 80                sub    $0xffffffffffffff80,%edx
 181:   89 d0                   mov    %edx,%eax
 183:   41 f7 e3                mul    %r11d
 186:   c1 ea 07                shr    $0x7,%edx
 189:   88 13                   mov    %dl,(%rbx)
 18b:   48 83 c3 01             add    $0x1,%rbx
 18f:   48 39 f1                cmp    %rsi,%rcx
 192:   75 df                   jne    173
<_ZN3o3d3o2d12TextureCairo7SetRectEijjjjPKvi+0x173>
 194:   45 88 50 03             mov    %r10b,0x3(%r8)
 198:   49 83 c0 04             add    $0x4,%r8
 19c:   48 83 c7 04             add    $0x4,%rdi
 1a0:   4d 39 e8                cmp    %r13,%r8
 1a3:   75 bb                   jne    160
<_ZN3o3d3o2d12TextureCairo7SetRectEijjjjPKvi+0x160>

[fbarchard, 2011-01-25 23:19:22]

FYI an efficient implementation would look something like this

movd  xmm0,[esi]
punpcklbw xmm1,xmm0
movdqa xmm2,xmm1
pshufw xmm2,ALPHASPLAT
pmulbw xmm2,xmm1
packuswb xmm2,xmm2
movd  [edi],xmm2

http://codereview.chromium.org/6309007/diff/15001/core/cross/cairo/texture_ca...
File core/cross/cairo/texture_cairo.cc (right):

http://codereview.chromium.org/6309007/diff/15001/core/cross/cairo/texture_ca...
core/cross/cairo/texture_cairo.cc:172: dst_data[i] = (src_data[i] * alpha +
128U) / 255U;
unroll loop
 dst_data[0] = (src_data[0] * alpha + 128U) / 255U;
 dst_data[1] = (src_data[1] * alpha + 128U) / 255U;
 dst_data[2] = (src_data[2] * alpha + 128U) / 255U;
add check for alpha = 0 or 255
if (alpha == 0 || alpha == 255)
 dst_data[0] = src_data[0];
 dst_data[1] = src_data[1];
 dst_data[2] = src_data[2];
else...
FYI for write combining to work, order of writes is important and you should not
mix sizes.
FYI looks like /255 is implemented as multiply.

[Tristan Schmelcher 2, 2011-01-25 23:25:30]

On 2011/01/25 23:19:22, fbarchard wrote:
> FYI an efficient implementation would look something like this
> 
> movd  xmm0,[esi]
> punpcklbw xmm1,xmm0
> movdqa xmm2,xmm1
> pshufw xmm2,ALPHASPLAT
> pmulbw xmm2,xmm1
> packuswb xmm2,xmm2
> movd  [edi],xmm2
> 
>
http://codereview.chromium.org/6309007/diff/15001/core/cross/cairo/texture_ca...
> File core/cross/cairo/texture_cairo.cc (right):
> 
>
http://codereview.chromium.org/6309007/diff/15001/core/cross/cairo/texture_ca...
> core/cross/cairo/texture_cairo.cc:172: dst_data[i] = (src_data[i] * alpha +
> 128U) / 255U;
> unroll loop
>  dst_data[0] = (src_data[0] * alpha + 128U) / 255U;
>  dst_data[1] = (src_data[1] * alpha + 128U) / 255U;
>  dst_data[2] = (src_data[2] * alpha + 128U) / 255U;

I tried unrolling the loop but from the looks of the assembly the compiler
didn't seem to get much out of it.

> add check for alpha = 0 or 255
> if (alpha == 0 || alpha == 255)
>  dst_data[0] = src_data[0];
>  dst_data[1] = src_data[1];
>  dst_data[2] = src_data[2];
> else...

That code actually won't work for alpha == 0, since there is no guarantee that
the colour channels in the source are already zero so it is not safe to copy
them. And I don't think an if statement for alpha == 255 will be likely to make
things much faster, since images with no transparent pixels will already go
through the XRGB path instead of the ARGB path.

> FYI for write combining to work, order of writes is important and you should
not
> mix sizes.
> FYI looks like /255 is implemented as multiply.

Yeah it used a trick where you multiply and then shift.

[fbarchard, 2011-01-26 00:32:48]

LGTM with nit about comment
and some grumbling about efficiency

FYI the formula can be factorized to use 1 multiply instead of 2.

http://codereview.chromium.org/6309007/diff/15001/core/cross/cairo/texture_ca...
File core/cross/cairo/texture_cairo.cc (right):

http://codereview.chromium.org/6309007/diff/15001/core/cross/cairo/texture_ca...
core/cross/cairo/texture_cairo.cc:167: // NOTE: This assumes a little-endian
anchitecture (e.g., x86).
This works for RGBA or BGRA where alpha is in byte 3

[Tristan Schmelcher 2, 2011-01-26 01:12:01]

http://codereview.chromium.org/6309007/diff/15001/core/cross/cairo/texture_ca...
File core/cross/cairo/texture_cairo.cc (right):

http://codereview.chromium.org/6309007/diff/15001/core/cross/cairo/texture_ca...
core/cross/cairo/texture_cairo.cc:167: // NOTE: This assumes a little-endian
anchitecture (e.g., x86).
On 2011/01/26 00:32:48, fbarchard wrote:
> This works for RGBA or BGRA where alpha is in byte 3

Done.