Improve color filter premultiply accuracy

Improve the quality of color matrix filters by using SkPremultiplyARGBInline.  This is closer to (but not exactly the same as) WebKit's implementation.

[reed1, 2012-01-03 21:14:39]

Can we change the filter to just call SkPremultiplyARGB? It uses 
SkMulDiv255Round instead of oSkMulDiv255Ceil. Is there an argument for why Ceil
is better than Round?

[Stephen White, 2012-01-03 21:22:26]

On 2012/01/03 21:14:39, reed1 wrote:
> Can we change the filter to just call SkPremultiplyARGB? It uses 
> SkMulDiv255Round instead of oSkMulDiv255Ceil. Is there an argument for why
Ceil
> is better than Round?

Round is probably ok too.  I was just trying to get it to match WebKit more
closely, and readPixels / config888_copy_to_bitmap() uses Ceil.  Will check.

[bsalomon, 2012-01-03 21:25:59]

On 2012/01/03 21:22:26, Stephen White wrote:
> On 2012/01/03 21:14:39, reed1 wrote:
> > Can we change the filter to just call SkPremultiplyARGB? It uses 
> > SkMulDiv255Round instead of oSkMulDiv255Ceil. Is there an argument for why
> Ceil
> > is better than Round?
> 
> Round is probably ok too.  I was just trying to get it to match WebKit more
> closely, and readPixels / config888_copy_to_bitmap() uses Ceil.  Will check.

config888_copy_to_bitmap only uses it because WK does. It is likely to change.

[Stephen White, 2012-01-03 21:32:39]

On 2012/01/03 21:22:26, Stephen White wrote:
> On 2012/01/03 21:14:39, reed1 wrote:
> > Can we change the filter to just call SkPremultiplyARGB? It uses 
> > SkMulDiv255Round instead of oSkMulDiv255Ceil. Is there an argument for why
> Ceil
> > is better than Round?
> 
> Round is probably ok too.  I was just trying to get it to match WebKit more
> closely, and readPixels / config888_copy_to_bitmap() uses Ceil.  Will check.

Actually, the results are visually quite different with Round than Ceil (e.g.,
on the Greyscale test on svg/W3C-SVG-1.1/filters-color-01-b.svg).  Not as badly
different as with SkAlpha255To256, but still around 50% of the values are off. 
The spec does not seem to specify how premultiplying is done, but I suggest with
stick with Ceil for now, just to avoid confusion until the WebKit code is
switched over to use Skia.

[reed1, 2012-01-03 21:44:01]

I don't follow "avoid confusion until we switch to skia".

I think skia should try to be consistent with skia, so if we're going to change
skia's colormatrix code, using SkPremultiply seems the best move.

Is the issue needing to rebaseline some expected images once we use skia for
colormatrix? If so, lets start to do that now rather than carrying yet more
conditionals in skia to have two different behaviors if possible.

SkTableColorFilter does exactly this pattern as well:


        } else {
            a = SkGetPackedA32(c);
            r = SkGetPackedR32(c);
            g = SkGetPackedG32(c);
            b = SkGetPackedB32(c);

            if (a < 255) {
                SkUnPreMultiply::Scale scale = scaleTable[a];
                r = SkUnPreMultiply::ApplyScale(scale, r);
                g = SkUnPreMultiply::ApplyScale(scale, g);
                b = SkUnPreMultiply::ApplyScale(scale, b);
            }
        }
        dst[i] = SkPremultiplyARGBInline(tableA[a], tableR[r],
                                         tableG[g], tableB[b]);

[Stephen White, 2012-01-03 21:50:59]

On 2012/01/03 21:44:01, reed1 wrote:
> I don't follow "avoid confusion until we switch to skia".

What I meant was, I wanted to avoid visual differences caused by switching from
the WebKit to the skia impl, and finesse the rounding later.  But I'm going to
have to rebaseline anyway (for non-visible differences), so perhaps that's not a
big deal.

> I think skia should try to be consistent with skia, so if we're going to
change
> skia's colormatrix code, using SkPremultiply seems the best move.

OK, fair enough.  Do you want me to use it unconditionally, or do you want it
still hidden behind an #ifdef?

[reed1, 2012-01-03 21:58:13]

I'm fine with it unconditionally moving to SkPremultiply.

[Stephen White, 2012-01-03 22:08:42]

On 2012/01/03 21:58:13, reed1 wrote:
> I'm fine with it unconditionally moving to SkPremultiply.

Done.

[reed1, 2012-01-03 22:18:25]

I think we don't need the alpha!=255 check, since SkPremultplyInline also does
that. Other than that, lgtm.

[Stephen White, 2012-01-03 22:52:30]

On 2012/01/03 22:18:25, reed1 wrote:
> I think we don't need the alpha!=255 check, since SkPremultplyInline also does
> that. Other than that, lgtm.

Done, and landed as r2955.  Closing.