Add AlphaThreshold filter.

Add AlphaThreshold filter.

This is based on the Bitmap Alpha Threshold filter, and will be used by Chromium
to implement the window shape API.

Committed: http://code.google.com/p/skia/source/detail?r=12935

[Zachary Kuznia, 2013-12-14 00:16:30]

Please take a look.

[bsalomon, 2013-12-16 14:20:57]

Just curious, what was wrong with the shader-as-filter approach? I'm wondering
why this will be used as a filter rather than a shader in Chromium. My naive
understanding of how it will be used more closely matches a shader, but I
probably just don't understand it.

Also, if we go this route of having a filter rather than shader we should
probably remove the shader since it will just rot. I'd be sad to see the GM go
away since it draws an image resembling a certain bat-themed comic hero, but
such is life.

https://codereview.chromium.org/115633002/diff/1/include/effects/SkAlphaThres...
File include/effects/SkAlphaThresholdFilter.h (right):

https://codereview.chromium.org/115633002/diff/1/include/effects/SkAlphaThres...
include/effects/SkAlphaThresholdFilter.h:17: SkAlphaThresholdFilter(SkRegion
region, SkScalar innerThreshold, SkScalar outerThreshold);
One pattern we like to use if possible in Skia is to make effects (shader, image
filter, ...) go through factory functions that return the abstract base class
rather than have public constructors on effects. If we decide later that it is
important to optimize the case where the region can be ignored then the factory
can be modified to return a bitmap shader in that case without modifying any
clients.

[reed1, 2013-12-16 14:30:49]

+1 to understand the shader alternative.

https://codereview.chromium.org/115633002/diff/1/src/effects/SkAlphaThreshold...
File src/effects/SkAlphaThresholdFilter.cpp (right):

https://codereview.chromium.org/115633002/diff/1/src/effects/SkAlphaThreshold...
src/effects/SkAlphaThresholdFilter.cpp:285: return true;
Why do we return true, but don't set dst? Can we write a CPU implementation, so
we have something to compare the GPU version with?

[Wez, 2013-12-17 06:39:04]

https://codereview.chromium.org/115633002/diff/1/gm/imagealphathreshold.cpp
File gm/imagealphathreshold.cpp (right):

https://codereview.chromium.org/115633002/diff/1/gm/imagealphathreshold.cpp#n...
gm/imagealphathreshold.cpp:45: region.setRects(rects, 2);
So we need to plumb through the window shape through to here from the setShape()
API?

https://codereview.chromium.org/115633002/diff/1/src/effects/SkAlphaThreshold...
File src/effects/SkAlphaThresholdFilter.cpp (right):

https://codereview.chromium.org/115633002/diff/1/src/effects/SkAlphaThreshold...
src/effects/SkAlphaThresholdFilter.cpp:83: typedef
GrGLUniformManager::UniformHandle UniformHandle;
nit: Why not just using GrGL...::UniformHandle?

[Zachary Kuznia, 2013-12-18 00:06:26]

Restating the bit I explained to bsalomon offline:
Implementing this as a shader instead of a filter would require a much larger
change to Chrome to support the API.  Since the shader keeps the input bitmap
constant, the actual implementation would actually be very similar to filters. 
As such, I decided it would be best to use the existing filter path.

https://codereview.chromium.org/115633002/diff/1/gm/imagealphathreshold.cpp
File gm/imagealphathreshold.cpp (right):

https://codereview.chromium.org/115633002/diff/1/gm/imagealphathreshold.cpp#n...
gm/imagealphathreshold.cpp:45: region.setRects(rects, 2);
On 2013/12/17 06:39:04, Wez wrote:
> So we need to plumb through the window shape through to here from the
setShape()
> API?

Yes.  I have a CL in Chromium that does this; I'll upload it next.

https://codereview.chromium.org/115633002/diff/1/include/effects/SkAlphaThres...
File include/effects/SkAlphaThresholdFilter.h (right):

https://codereview.chromium.org/115633002/diff/1/include/effects/SkAlphaThres...
include/effects/SkAlphaThresholdFilter.h:17: SkAlphaThresholdFilter(SkRegion
region, SkScalar innerThreshold, SkScalar outerThreshold);
On 2013/12/16 14:20:58, bsalomon wrote:
> One pattern we like to use if possible in Skia is to make effects (shader,
image
> filter, ...) go through factory functions that return the abstract base class
> rather than have public constructors on effects. If we decide later that it is
> important to optimize the case where the region can be ignored then the
factory
> can be modified to return a bitmap shader in that case without modifying any
> clients.

Done.

https://codereview.chromium.org/115633002/diff/1/src/effects/SkAlphaThreshold...
File src/effects/SkAlphaThresholdFilter.cpp (right):

https://codereview.chromium.org/115633002/diff/1/src/effects/SkAlphaThreshold...
src/effects/SkAlphaThresholdFilter.cpp:83: typedef
GrGLUniformManager::UniformHandle UniformHandle;
On 2013/12/17 06:39:04, Wez wrote:
> nit: Why not just using GrGL...::UniformHandle?

Done.

https://codereview.chromium.org/115633002/diff/1/src/effects/SkAlphaThreshold...
src/effects/SkAlphaThresholdFilter.cpp:285: return true;
On 2013/12/16 14:30:49, reed1 wrote:
> Why do we return true, but don't set dst? Can we write a CPU implementation,
so
> we have something to compare the GPU version with? 

Software version added.

[Wez, 2013-12-18 01:21:01]

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
File src/effects/SkAlphaThresholdFilter.cpp (right):

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:178: "\t\t\tfloat scale = inner_thresh /
color.a;\n"
Won't this explode if the source contains fully transparent pixels?

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:179: "\t\t\tcolor.rgb *= scale;\n"
Hmmm, if pixels have pre-multiplied alpha then when we clamp the alpha we're
introducing up-sampling artefacts into the colour. I suppose callers can just
avoid triggering the clamping to avoid that.

[bsalomon, 2013-12-18 14:38:12]

https://codereview.chromium.org/115633002/diff/100001/gm/imagealphathreshold.cpp
File gm/imagealphathreshold.cpp (right):

https://codereview.chromium.org/115633002/diff/100001/gm/imagealphathreshold....
gm/imagealphathreshold.cpp:43: SkPaint paint;
Can we make the GM somehow test that the filter respects the canvas's matrix?

https://codereview.chromium.org/115633002/diff/100001/include/effects/SkAlpha...
File include/effects/SkAlphaThresholdFilter.h (right):

https://codereview.chromium.org/115633002/diff/100001/include/effects/SkAlpha...
include/effects/SkAlphaThresholdFilter.h:6: */
New files use this copyright notice:

/*
 * Copyright 2013 Google Inc.
 *
 * Use of this source code is governed by a BSD-style license that can be
 * found in the LICENSE file.
 */

https://codereview.chromium.org/115633002/diff/100001/include/effects/SkAlpha...
include/effects/SkAlphaThresholdFilter.h:21: * The 0,0 point of the region
corresponds to the upper left corner.
...corresponds to the upper left corner of the src image. (?)

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
File src/effects/SkAlphaThresholdFilter.cpp (right):

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:6: */
new notice

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:13: #include <stdio.h>
Is stdio needed?

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:178: "\t\t\tfloat scale = inner_thresh /
color.a;\n"
On 2013/12/18 01:21:02, Wez wrote:
> Won't this explode if the source contains fully transparent pixels?

Yeah maybe / min(color.a, some-arbitrary-number). AFAIK div by 0 is undefined in
GLSL.

It shouldn't matter what scale is computed (as long as it is finite) when
color.a is zero. Since color is premultiplied color.rgb should also be zero.

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:179: "\t\t\tcolor.rgb *= scale;\n"
On 2013/12/18 01:21:02, Wez wrote:
> Hmmm, if pixels have pre-multiplied alpha then when we clamp the alpha we're
> introducing up-sampling artefacts into the colour. I suppose callers can just
> avoid triggering the clamping to avoid that.

I agree with your last statement. It seems like the use case for the threshold
is when the app has produced invalid window content, but I'm not too familiar
with the details.

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:244: : INHERITED(0), fRegion(region),
fInnerThreshold(innerThreshold), fOuterThreshold(outerThreshold) {
style nit:

    : INHERITED(0)
    , fRegion(region)
    //...

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:263: GrAutoScratchTexture ast(context,
maskDesc, GrContext::kApprox_ScratchTexMatch);
I imagine we will want to cache this texture (in a later CL) but that will
involve adding unique IDs to regions. I assume that the same region is typically
used many times.

[Zachary Kuznia, 2013-12-19 01:28:40]

https://codereview.chromium.org/115633002/diff/100001/gm/imagealphathreshold.cpp
File gm/imagealphathreshold.cpp (right):

https://codereview.chromium.org/115633002/diff/100001/gm/imagealphathreshold....
gm/imagealphathreshold.cpp:43: SkPaint paint;
On 2013/12/18 14:38:12, bsalomon wrote:
> Can we make the GM somehow test that the filter respects the canvas's matrix?

Done.

https://codereview.chromium.org/115633002/diff/100001/include/effects/SkAlpha...
File include/effects/SkAlphaThresholdFilter.h (right):

https://codereview.chromium.org/115633002/diff/100001/include/effects/SkAlpha...
include/effects/SkAlphaThresholdFilter.h:6: */
On 2013/12/18 14:38:12, bsalomon wrote:
> New files use this copyright notice:
> 
> /*
>  * Copyright 2013 Google Inc.
>  *
>  * Use of this source code is governed by a BSD-style license that can be
>  * found in the LICENSE file.
>  */

Done.

https://codereview.chromium.org/115633002/diff/100001/include/effects/SkAlpha...
include/effects/SkAlphaThresholdFilter.h:21: * The 0,0 point of the region
corresponds to the upper left corner.
On 2013/12/18 14:38:12, bsalomon wrote:
> ...corresponds to the upper left corner of the src image. (?)

Done.

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
File src/effects/SkAlphaThresholdFilter.cpp (right):

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:6: */
On 2013/12/18 14:38:12, bsalomon wrote:
> new notice

Done.

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:13: #include <stdio.h>
On 2013/12/18 14:38:12, bsalomon wrote:
> Is stdio needed?

Done.

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:178: "\t\t\tfloat scale = inner_thresh /
color.a;\n"
On 2013/12/18 14:38:12, bsalomon wrote:
> On 2013/12/18 01:21:02, Wez wrote:
> > Won't this explode if the source contains fully transparent pixels?
> 
> Yeah maybe / min(color.a, some-arbitrary-number). AFAIK div by 0 is undefined
in
> GLSL.
> 
> It shouldn't matter what scale is computed (as long as it is finite) when
> color.a is zero. Since color is premultiplied color.rgb should also be zero.
> 

Done.

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:179: "\t\t\tcolor.rgb *= scale;\n"
On 2013/12/18 14:38:12, bsalomon wrote:
> On 2013/12/18 01:21:02, Wez wrote:
> > Hmmm, if pixels have pre-multiplied alpha then when we clamp the alpha we're
> > introducing up-sampling artefacts into the colour. I suppose callers can
just
> > avoid triggering the clamping to avoid that.
> 
> I agree with your last statement. It seems like the use case for the threshold
> is when the app has produced invalid window content, but I'm not too familiar
> with the details.

What equation would you prefer here?

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:244: : INHERITED(0), fRegion(region),
fInnerThreshold(innerThreshold), fOuterThreshold(outerThreshold) {
On 2013/12/18 14:38:12, bsalomon wrote:
> style nit:
> 
>     : INHERITED(0)
>     , fRegion(region)
>     //...

Done.

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:263: GrAutoScratchTexture ast(context,
maskDesc, GrContext::kApprox_ScratchTexMatch);
On 2013/12/18 14:38:12, bsalomon wrote:
> I imagine we will want to cache this texture (in a later CL) but that will
> involve adding unique IDs to regions. I assume that the same region is
typically
> used many times.

I'll investigate this once we have the entire pipeline in place.

[bsalomon, 2013-12-19 14:50:01]

If the sw and gpu impls produce similar results then lgtm.

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
File src/effects/SkAlphaThresholdFilter.cpp (right):

https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:179: "\t\t\tcolor.rgb *= scale;\n"
On 2013/12/19 01:28:40, Zachary Kuznia wrote:
> On 2013/12/18 14:38:12, bsalomon wrote:
> > On 2013/12/18 01:21:02, Wez wrote:
> > > Hmmm, if pixels have pre-multiplied alpha then when we clamp the alpha
we're
> > > introducing up-sampling artefacts into the colour. I suppose callers can
> just
> > > avoid triggering the clamping to avoid that.
> > 
> > I agree with your last statement. It seems like the use case for the
threshold
> > is when the app has produced invalid window content, but I'm not too
familiar
> > with the details.
> 
> What equation would you prefer here?

I think it is fine as is, but more a question for you guys than me :)

[Zachary Kuznia, 2013-12-19 20:55:01]

On 2013/12/19 14:50:01, bsalomon wrote:
> If the sw and gpu impls produce similar results then lgtm.
> 
>
https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
> File src/effects/SkAlphaThresholdFilter.cpp (right):
> 
>
https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
> src/effects/SkAlphaThresholdFilter.cpp:179: "\t\t\tcolor.rgb *= scale;\n"
> On 2013/12/19 01:28:40, Zachary Kuznia wrote:
> > On 2013/12/18 14:38:12, bsalomon wrote:
> > > On 2013/12/18 01:21:02, Wez wrote:
> > > > Hmmm, if pixels have pre-multiplied alpha then when we clamp the alpha
> we're
> > > > introducing up-sampling artefacts into the colour. I suppose callers can
> > just
> > > > avoid triggering the clamping to avoid that.
> > > 
> > > I agree with your last statement. It seems like the use case for the
> threshold
> > > is when the app has produced invalid window content, but I'm not too
> familiar
> > > with the details.
> > 
> > What equation would you prefer here?
> 
> I think it is fine as is, but more a question for you guys than me :)

I fixed the alpha issue, and now both versions output the same.

[bsalomon, 2013-12-19 20:58:01]

On 2013/12/19 20:55:01, Zachary Kuznia wrote:
> On 2013/12/19 14:50:01, bsalomon wrote:
> > If the sw and gpu impls produce similar results then lgtm.
> > 
> >
>
https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
> > File src/effects/SkAlphaThresholdFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
> > src/effects/SkAlphaThresholdFilter.cpp:179: "\t\t\tcolor.rgb *= scale;\n"
> > On 2013/12/19 01:28:40, Zachary Kuznia wrote:
> > > On 2013/12/18 14:38:12, bsalomon wrote:
> > > > On 2013/12/18 01:21:02, Wez wrote:
> > > > > Hmmm, if pixels have pre-multiplied alpha then when we clamp the alpha
> > we're
> > > > > introducing up-sampling artefacts into the colour. I suppose callers
can
> > > just
> > > > > avoid triggering the clamping to avoid that.
> > > > 
> > > > I agree with your last statement. It seems like the use case for the
> > threshold
> > > > is when the app has produced invalid window content, but I'm not too
> > familiar
> > > > with the details.
> > > 
> > > What equation would you prefer here?
> > 
> > I think it is fine as is, but more a question for you guys than me :)
> 
> I fixed the alpha issue, and now both versions output the same.

lgtm. If you care about the perf of the sw backend ask reed@ to look at it. I
think we have some functions that could help avoid so much conversion back forth
between chars and floats.

[reed1, 2013-12-19 21:09:14]

https://codereview.chromium.org/115633002/diff/140001/src/effects/SkAlphaThre...
File src/effects/SkAlphaThresholdFilter.cpp (right):

https://codereview.chromium.org/115633002/diff/140001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:344: float scale = (float)innerThreshold
/ SkColorGetA(source);
What if src alpha is zero?

[bsalomon, 2013-12-19 21:09:46]

Actually I think you're #if SK_SUPPORT_GPU usage. There are two of these blocks
nested in each other. It might make things easier to move all the #include stuff
for Gr headers to just before the effect code and asNewEffect impl.

[Zachary Kuznia, 2013-12-19 21:09:57]

On 2013/12/19 20:58:01, bsalomon wrote:
> On 2013/12/19 20:55:01, Zachary Kuznia wrote:
> > On 2013/12/19 14:50:01, bsalomon wrote:
> > > If the sw and gpu impls produce similar results then lgtm.
> > > 
> > >
> >
>
https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
> > > File src/effects/SkAlphaThresholdFilter.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/115633002/diff/100001/src/effects/SkAlphaThre...
> > > src/effects/SkAlphaThresholdFilter.cpp:179: "\t\t\tcolor.rgb *= scale;\n"
> > > On 2013/12/19 01:28:40, Zachary Kuznia wrote:
> > > > On 2013/12/18 14:38:12, bsalomon wrote:
> > > > > On 2013/12/18 01:21:02, Wez wrote:
> > > > > > Hmmm, if pixels have pre-multiplied alpha then when we clamp the
alpha
> > > we're
> > > > > > introducing up-sampling artefacts into the colour. I suppose callers
> can
> > > > just
> > > > > > avoid triggering the clamping to avoid that.
> > > > > 
> > > > > I agree with your last statement. It seems like the use case for the
> > > threshold
> > > > > is when the app has produced invalid window content, but I'm not too
> > > familiar
> > > > > with the details.
> > > > 
> > > > What equation would you prefer here?
> > > 
> > > I think it is fine as is, but more a question for you guys than me :)
> > 
> > I fixed the alpha issue, and now both versions output the same.
> 
> lgtm. If you care about the perf of the sw backend ask reed@ to look at it. I
> think we have some functions that could help avoid so much conversion back
forth
> between chars and floats.

I'll see how it performs for now, and revisit it if needed.  Thanks!

[commit-bot: I haz the power, 2013-12-19 21:10:12]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zork@chromium.org/115633002/140001

[bsalomon, 2013-12-19 21:10:39]

On 2013/12/19 21:09:46, bsalomon wrote:
> Actually I think you're #if SK_SUPPORT_GPU usage. There are two of these
blocks
> nested in each other. It might make things easier to move all the #include
stuff
> for Gr headers to just before the effect code and asNewEffect impl.

Sorry.. meant "Actually I think you're #if SK_SUPPORT_GPU usage is incorrect."

[bsalomon, 2013-12-19 21:11:20]

https://codereview.chromium.org/115633002/diff/140001/src/effects/SkAlphaThre...
File src/effects/SkAlphaThresholdFilter.cpp (right):

https://codereview.chromium.org/115633002/diff/140001/src/effects/SkAlphaThre...
src/effects/SkAlphaThresholdFilter.cpp:177: "\t\t\tfloat scale = inner_thresh /
color.a;\n"
need max() here too?

[Zachary Kuznia, 2013-12-19 21:39:24]

On 2013/12/19 21:11:20, bsalomon wrote:
>
https://codereview.chromium.org/115633002/diff/140001/src/effects/SkAlphaThre...
> File src/effects/SkAlphaThresholdFilter.cpp (right):
> 
>
https://codereview.chromium.org/115633002/diff/140001/src/effects/SkAlphaThre...
> src/effects/SkAlphaThresholdFilter.cpp:177: "\t\t\tfloat scale = inner_thresh
/
> color.a;\n"
> need max() here too?

Put the max() there and in the software implementation.  I removed it from the
other location, because that should be protected by if (color.a > thresh).

The #ifdef should be in the proper location now, too.

[Zachary Kuznia, 2014-01-06 22:13:10]

On 2013/12/19 21:39:24, Zachary Kuznia wrote:
> On 2013/12/19 21:11:20, bsalomon wrote:
> >
>
https://codereview.chromium.org/115633002/diff/140001/src/effects/SkAlphaThre...
> > File src/effects/SkAlphaThresholdFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/115633002/diff/140001/src/effects/SkAlphaThre...
> > src/effects/SkAlphaThresholdFilter.cpp:177: "\t\t\tfloat scale =
inner_thresh
> /
> > color.a;\n"
> > need max() here too?
> 
> Put the max() there and in the software implementation.  I removed it from the
> other location, because that should be protected by if (color.a > thresh).
> 
> The #ifdef should be in the proper location now, too.

bsalomon: Any other issues I should address on this?

[bsalomon, 2014-01-06 22:17:37]

lgtm

[commit-bot: I haz the power, 2014-01-06 22:31:43]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zork@chromium.org/115633002/310001

[commit-bot: I haz the power, 2014-01-06 22:45:54]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zork@chromium.org/115633002/380001

[commit-bot: I haz the power, 2014-01-06 23:41:18]

Change committed as 12935