Add convolveAlpha flag to matrix convolution

This patch adds support for optional processing of the alpha channel.  Test cases are added to the GM and the bench.

[Stephen White, 2012-09-20 23:35:12]

PTAL

[reed1, 2012-09-21 12:25:45]

My read of the spec is that if we are *not* convolving alpha, then we need to 

1. unpremul
2. convolve rgb
3. re-premul

The "convolve-alpha" step is the easy one, since everything can just stay premul
as is.

Is that different from your read?

[Stephen White, 2012-09-21 12:39:54]

On 2012/09/21 12:25:45, reed1 wrote:
> My read of the spec is that if we are *not* convolving alpha, then we need to 
> 
> 1. unpremul
> 2. convolve rgb
> 3. re-premul
> 
> The "convolve-alpha" step is the easy one, since everything can just stay
premul
> as is.
> 
> Is that different from your read?

I think you're correct (and the WebKit implementation is wrong, or I read it
wrong).

[schenney, 2012-09-21 14:28:19]

The spec is clearly as Mike says. If existing WK is wrong, file a WK bug
and leave it unassigned; I don't think it's worth us taking the time to fix
it now that we are replacing it.

Cheers,
Stephen.

On Fri, Sep 21, 2012 at 8:39 AM, <senorblanco@chromium.org> wrote:

> On 2012/09/21 12:25:45, reed1 wrote:
>
>> My read of the spec is that if we are *not* convolving alpha, then we
>>
> need to
>
>  1. unpremul
>> 2. convolve rgb
>> 3. re-premul
>>
>
>  The "convolve-alpha" step is the easy one, since everything can just
>>
> stay premul
>
>> as is.
>>
>
>  Is that different from your read?
>>
>
> I think you're correct (and the WebKit implementation is wrong, or I
> read it wrong).
>
>
http://codereview.appspot.com/**6547049/<http://codereview.appspot.com/6547049/>
>



-- 
Stephen Chenney | Software Engineer | schenney@google.com | 404-314-1809

[Stephen White, 2012-09-24 16:44:40]

On 2012/09/21 14:28:19, schenney wrote:
> The spec is clearly as Mike says. If existing WK is wrong, file a WK bug
> and leave it unassigned; I don't think it's worth us taking the time to fix
> it now that we are replacing it.

Now that I look at it again, it seems that the WebKit implementation is doing it
correctly:  if preserveAlpha is true, the filter simply requests both its input
and result as an unpremultiplied buffers, otherwise it requests them as
premultiplied.  So it puts the onus on the previous filter to do the unpremult
(if necessary), and the next filter to do the mult (if necessary).  This is a
good optimization, IMHO.

We don't currently have the infrastructure for it in Skia, since SkBitmaps are
always assumed to be premultiplied (correct me if I'm wrong) and filters make no
exception to this rule.  For now, I'll just bake in the unpremult/mult into this
filter, and we can look at doing a smarter avoid-mult-unpremult later.  This
might be a pretty deep change, though, since for example colorfilters always
unpremult/remult, AFAIK.

> Cheers,
> Stephen.
> 
> On Fri, Sep 21, 2012 at 8:39 AM, <mailto:senorblanco@chromium.org> wrote:
> 
> > On 2012/09/21 12:25:45, reed1 wrote:
> >
> >> My read of the spec is that if we are *not* convolving alpha, then we
> >>
> > need to
> >
> >  1. unpremul
> >> 2. convolve rgb
> >> 3. re-premul
> >>
> >
> >  The "convolve-alpha" step is the easy one, since everything can just
> >>
> > stay premul
> >
> >> as is.
> >>
> >
> >  Is that different from your read?
> >>
> >
> > I think you're correct (and the WebKit implementation is wrong, or I
> > read it wrong).
> >
> >
>
http://codereview.appspot.com/**6547049/%3Chttp://codereview.appspot.com/6547...>
> >
> 
> 
> 
> -- 
> Stephen Chenney | Software Engineer | mailto:schenney@google.com |
404-314-1809

[Stephen White, 2012-09-24 19:12:33]

PTAL:  for convolveAlpha = false, the patch allocates an offscreen, does
unpremul to it, then convolves w/remul when writing the dest pixel.  (In case it
isn't obvious, the unpremul is done to an offscreen since if we did it inline,
it'd be done NxM times for each pixel for an NxM kernel).

[reed1, 2012-09-24 19:23:54]

we can skip the unpremul if the src is marked as opaque...

lgtm

https://codereview.appspot.com/6547049/diff/9001/src/effects/SkMatrixConvolut...
File src/effects/SkMatrixConvolutionImageFilter.cpp (right):

https://codereview.appspot.com/6547049/diff/9001/src/effects/SkMatrixConvolut...
src/effects/SkMatrixConvolutionImageFilter.cpp:199: if (!fConvolveAlpha) {
&& !src.isOpaque()

[Stephen White, 2012-09-24 19:27:08]

On 2012/09/24 19:23:54, reed1 wrote:
> we can skip the unpremul if the src is marked as opaque...
> 
> lgtm
> 
>
https://codereview.appspot.com/6547049/diff/9001/src/effects/SkMatrixConvolut...
> File src/effects/SkMatrixConvolutionImageFilter.cpp (right):
> 
>
https://codereview.appspot.com/6547049/diff/9001/src/effects/SkMatrixConvolut...
> src/effects/SkMatrixConvolutionImageFilter.cpp:199: if (!fConvolveAlpha) {
> && !src.isOpaque()

Thanks, will fix on landing.

[Stephen White, 2012-09-25 15:12:30]

Landed as https://code.google.com/p/skia/source/detail?r=5661.  Closing.