Adding opacity test for shaders, used for deferred drawing optimization

Adding a virtual interface for querying whether a shader returns opaque colors.  This is necessary because getFlags (which has a flag for opacity) only returns correct flags after setContext is called, which is not convenient for deferred drawing.

[junov1, 2011-12-06 14:20:00]

PTAL

[reed1, 2011-12-06 14:43:17]

1. Lets have the default return value be false, not true, to be conservative.

2. Can we simplify it by not bother to pass in the paint?

[junov1, 2011-12-06 15:06:24]

On 2011/12/06 14:43:17, reed1 wrote:

> 2. Can we simplify it by not bother to pass in the paint?

We could but with one important caveat: the opacity of the shader may depend on
the paint's alpha value.  So we could test opacity without taking into account
paint alpha, which would be tested separately.

I'm not so sure that is a good idea though. It just so happens that all the
three shader implementations I touched are affected by the paint's alpha in the
same way, but there is no strict guarantee that this will always be the case.
The current architecture leaves it up to the Shader implementation to decide how
the paint's attributes affect the shader output.

[junov1, 2011-12-06 15:06:47]

http://codereview.appspot.com/5451102/diff/2001/include/core/SkShader.h
File include/core/SkShader.h (right):

http://codereview.appspot.com/5451102/diff/2001/include/core/SkShader.h#newco...
include/core/SkShader.h:107: virtual bool isOpaque(const SkPaint& paint) const {
return false; }
default is false, unless you meant some place else?

[reed1, 2011-12-06 15:09:45]

Shaders are defined such that they must modulate their alpha by the paint's
alpha. Thus if the paint is 0xFF, they will be unaffected, and if the paint's
alpha is < 0xFF, they cannot return 0xFF.

Certainly the caller will have to sniff the paint's alpha, but they also need to
sniff other aspects of the paint to understand the total picture. My point is
that from the shader's p.o.v., it need not see the paint in order to determine
its (conservative) behavior.


On 2011/12/06 15:06:24, junov1 wrote:
> On 2011/12/06 14:43:17, reed1 wrote:
> 
> > 2. Can we simplify it by not bother to pass in the paint?
> 
> We could but with one important caveat: the opacity of the shader may depend
on
> the paint's alpha value.  So we could test opacity without taking into account
> paint alpha, which would be tested separately.
> 
> I'm not so sure that is a good idea though. It just so happens that all the
> three shader implementations I touched are affected by the paint's alpha in
the
> same way, but there is no strict guarantee that this will always be the case.
> The current architecture leaves it up to the Shader implementation to decide
how
> the paint's attributes affect the shader output.

[reed1, 2011-12-06 15:09:53]

http://codereview.appspot.com/5451102/diff/2001/include/core/SkShader.h
File include/core/SkShader.h (right):

http://codereview.appspot.com/5451102/diff/2001/include/core/SkShader.h#newco...
include/core/SkShader.h:107: virtual bool isOpaque(const SkPaint& paint) const {
return false; }
On 2011/12/06 15:06:47, junov1 wrote:
> default is false, unless you meant some place else?

Oops, I just misread this, my bad.

[bsalomon, 2011-12-06 15:11:35]

The current tests don't seem conservative since we aren't looking at the
colorfilter, maskfilter, or xfermode which may introduce fractional alpha.

It seems a little bit inside-out to me to ask the shader if it is opaque in the
context of a paint. Each shader subclass implementation of this function will
have to look at all the paint pipeline stages where alpha can be introduced. 

Could we instead ask the paint if it is opaque and it in turn asks the color
filter, the shader, the xfermode, etc if they are opaque? Each of these classes
could have an isOpaque() (perhaps with a param that indicates andy known
guarantees about their incoming colors (e.g. incoming color known to be
opaque))?

[junov1, 2011-12-06 16:13:23]

On 2011/12/06 15:11:35, bsalomon wrote:

Brian, what you are suggesting is exactly what I have done, with the exception
of looking at the paint alpha in SkShader.  I was simply emulating the way
subclasses of SkShader are currently setting kOpaqueAlpha_Flag in setContext()

The global opacity test I have looks like this:

bool SkDeferredCanvas::isOpaquePaint(const SkPaint& paint, const SkBitmap*
bitmap) const {
    if (!(paint.getStyle() == SkPaint::kFill_Style || paint.getStyle() ==
SkPaint::kStrokeAndFill_Style))
        return false;

    if (paint.getMaskFilter() || paint.getLooper())
        return false;

    SkXfermode::Coeff srcCoeff, dstCoeff;
    if (SkXfermode::AsCoeff(paint.getXfermode(), &srcCoeff, &dstCoeff)){
        switch (dstCoeff) {
        case SkXfermode::kZero_Coeff:
            return true;
        case SkXfermode::kISA_Coeff:
            if (paint.getAlpha() != 255)
                break;
            if (bitmap && !bitmap->isOpaque())
                break;
            if (paint.getShader() && !paint.getShader()->isOpaque(paint))
                break;
            if (paint.getColorFilter() && (paint.getColorFilter()->getFlags() &
SkColorFilter::kAlphaUnchanged_Flag == 0))
                break;
            return true;
        case SkXfermode::kSA_Coeff:
            if (paint.getAlpha() != 0)
                break;
            if (bitmap || paint.getShader())
                break;
            if (paint.getColorFilter() && (paint.getColorFilter()->getFlags() &
SkColorFilter::kAlphaUnchanged_Flag == 0))
                break;
            return true;
        case SkXfermode::kSC_Coeff:
            if (paint.getColor() != 0) // all components must be 0
                break;
            if (bitmap || paint.getShader())
                break;
            if (paint.getColorFilter() && (paint.getColorFilter()->getFlags() &
SkColorFilter::kAlphaUnchanged_Flag == 0))
                break;
            return true;
        default:
            break;
        }
    }
    return false;
}


I did not put this in SkPaint because the computation depends on the knowledge
of whether or not the paint will be used for drawing a bitmap.

[junov1, 2011-12-06 16:17:46]

Forgot to mention: I agree with the comment, I will remove the paint argument to
isOpaque.

[bsalomon, 2011-12-06 17:26:48]

On 2011/12/06 16:13:23, junov1 wrote:
> On 2011/12/06 15:11:35, bsalomon wrote:
> 
> Brian, what you are suggesting is exactly what I have done, with the exception
> of looking at the paint alpha in SkShader.  I was simply emulating the way
> subclasses of SkShader are currently setting kOpaqueAlpha_Flag in setContext()
> 
> The global opacity test I have looks like this:
> 
> bool SkDeferredCanvas::isOpaquePaint(const SkPaint& paint, const SkBitmap*
> bitmap) const {
>     if (!(paint.getStyle() == SkPaint::kFill_Style || paint.getStyle() ==
> SkPaint::kStrokeAndFill_Style))
>         return false;
> 
>     if (paint.getMaskFilter() || paint.getLooper())
>         return false;
> 
>     SkXfermode::Coeff srcCoeff, dstCoeff;
>     if (SkXfermode::AsCoeff(paint.getXfermode(), &srcCoeff, &dstCoeff)){
>         switch (dstCoeff) {
>         case SkXfermode::kZero_Coeff:
>             return true;
>         case SkXfermode::kISA_Coeff:
>             if (paint.getAlpha() != 255)
>                 break;
>             if (bitmap && !bitmap->isOpaque())
>                 break;
>             if (paint.getShader() && !paint.getShader()->isOpaque(paint))
>                 break;
>             if (paint.getColorFilter() && (paint.getColorFilter()->getFlags()
&
> SkColorFilter::kAlphaUnchanged_Flag == 0))
>                 break;
>             return true;
>         case SkXfermode::kSA_Coeff:
>             if (paint.getAlpha() != 0)
>                 break;
>             if (bitmap || paint.getShader())
>                 break;
>             if (paint.getColorFilter() && (paint.getColorFilter()->getFlags()
&
> SkColorFilter::kAlphaUnchanged_Flag == 0))
>                 break;
>             return true;
>         case SkXfermode::kSC_Coeff:
>             if (paint.getColor() != 0) // all components must be 0
>                 break;
>             if (bitmap || paint.getShader())
>                 break;
>             if (paint.getColorFilter() && (paint.getColorFilter()->getFlags()
&
> SkColorFilter::kAlphaUnchanged_Flag == 0))
>                 break;
>             return true;
>         default:
>             break;
>         }
>     }
>     return false;
> }
> 
> 
> I did not put this in SkPaint because the computation depends on the knowledge
> of whether or not the paint will be used for drawing a bitmap.

It seems like this should be somewhere reusable. What about something like
bool SkPaint::isOpaque(SkBitmap* bmpReplacesShader = NULL) const;

[junov1, 2011-12-07 18:34:18]

> It seems like this should be somewhere reusable. What about something like
> bool SkPaint::isOpaque(SkBitmap* bmpReplacesShader = NULL) const;

Good idea. See new patch.

[reed1, 2011-12-07 18:49:54]

The xfermode switch statement needs more { } around ifs I think

http://codereview.appspot.com/5451102/diff/5003/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (left):

http://codereview.appspot.com/5451102/diff/5003/src/core/SkPaint.cpp#oldcode1975
src/core/SkPaint.cpp:1975: 
Why the check for maskfilter?

http://codereview.appspot.com/5451102/diff/5003/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

http://codereview.appspot.com/5451102/diff/5003/src/core/SkPaint.cpp#newcode1975
src/core/SkPaint.cpp:1975: bool SkPaint::isOpaque(const SkBitmap*
bmpReplacesShader) const {
Why the check for fill-style?

http://codereview.appspot.com/5451102/diff/5003/src/core/SkPaint.cpp#newcode1982
src/core/SkPaint.cpp:1982: SkXfermode::Coeff srcCoeff, dstCoeff;
Lets at least add a TODO to put a virtual on xfermode for isOpaque, since other
modes can be opaque but not have coefficients.

http://codereview.appspot.com/5451102/diff/5003/src/core/SkShader.cpp
File src/core/SkShader.cpp (left):

http://codereview.appspot.com/5451102/diff/5003/src/core/SkShader.cpp#oldcode229
src/core/SkShader.cpp:229: return;
Why this style change? If this is clearer to you, please change flatten() to
match it, or change neither. I just want it to be trivial to look at both and
see that they are mirrors of each other.

http://codereview.appspot.com/5451102/diff/5003/src/effects/SkGradientShader.cpp
File src/effects/SkGradientShader.cpp (left):

http://codereview.appspot.com/5451102/diff/5003/src/effects/SkGradientShader....
src/effects/SkGradientShader.cpp:164: SkColor*    fOrigColors;
Nice addition. Can we name the field something a little clearer (for me), like
fColorsAreOpaque or something like that.

[bsalomon, 2011-12-07 19:25:25]

It seems like there are two questions that need to be asked to determine if a
draw covers the entire device:

1) Does the geometry fully cover the entire device (after accounting for fill
rule, AA, mask filter [blur radius])?

2) Can I see through the geometry (given the paint alpha, color filter,
xfermode)?

It seems like SkPaint should answer question 2 but not question 1. Question 1
requires knowledge of what geometry is being drawn (rectangle, path,...) as well
as some info from the paint (if AA inset by 1 pixel, if blur inset by some
radius, if stroke then transform the geometry [or punt]).

[junov1, 2011-12-07 19:25:48]

On 2011/12/07 18:49:54, reed1 wrote:
> The xfermode switch statement needs more { } around ifs I think
Right, skia style guide says to always put braces, unlike in WebKit...

> Why the check for maskfilter?
It can result in semi-transparent pixels around the edges of the primitive.

> src/core/SkPaint.cpp:1975: bool SkPaint::isOpaque(const SkBitmap*
> bmpReplacesShader) const {
> Why the check for fill-style?

Good question.  For the use case I have for this method, it is important to
determine whether a rectangle draw occludes everything inside the area of the
rect.  But I guess that issue is semantically orthogonal to the question of
whether the colors are opaque.  I will move this check out of SkPaint::isOpaque,
and put it directly in SkDeferredCanvas.

> http://codereview.appspot.com/5451102/diff/5003/src/core/SkShader.cpp
> File src/core/SkShader.cpp (left):
> 
>
http://codereview.appspot.com/5451102/diff/5003/src/core/SkShader.cpp#oldcode229
> src/core/SkShader.cpp:229: return;
> Why this style change? If this is clearer to you, please change flatten() to
> match it, or change neither. I just want it to be trivial to look at both and
> see that they are mirrors of each other.
> 
>
http://codereview.appspot.com/5451102/diff/5003/src/effects/SkGradientShader.cpp
> File src/effects/SkGradientShader.cpp (left):
> 
>
http://codereview.appspot.com/5451102/diff/5003/src/effects/SkGradientShader....
> src/effects/SkGradientShader.cpp:164: SkColor*    fOrigColors;
> Nice addition. Can we name the field something a little clearer (for me), like
> fColorsAreOpaque or something like that.

[junov1, 2011-12-07 19:32:22]

On 2011/12/07 19:25:25, bsalomon wrote:

> It seems like SkPaint should answer question 2 but not question 1. 

Agreed. Thanks for mentioning AA, I totally overlooked it.
For now, I think I will be conservative about blur and just consider the draw as
non opaque when a blur is detected.

[junov1, 2011-12-08 15:56:06]

Done.

[bsalomon, 2011-12-08 16:42:02]

Overall looks good, minor inline comments

http://codereview.appspot.com/5451102/diff/8001/src/effects/SkGradientShader.cpp
File src/effects/SkGradientShader.cpp (right):

http://codereview.appspot.com/5451102/diff/8001/src/effects/SkGradientShader....
src/effects/SkGradientShader.cpp:308: initCommon();
this->initCommond

http://codereview.appspot.com/5451102/diff/8001/src/effects/SkGradientShader....
src/effects/SkGradientShader.cpp:342: initCommon();
ditto

http://codereview.appspot.com/5451102/diff/8001/src/effects/SkGradientShader....
src/effects/SkGradientShader.cpp:403: bool opaque = isOpaque();
is this used?

[reed1, 2011-12-08 20:28:47]

I'm good with all of the changes related to shaders. Find if we land those as
is.

The changes to SkPaint.[h,cpp] are still very specialized. isOpaque() doesn't
capture (for me) the notion that this method is just checking
shader/colorfilter/xfermode. Here's a suggestion to get this in faster: Can we
write it as a utility function that takes a paint, and just make it private to
your caller for now? We can migrate it outwards later if we choose.

e.g. put this in SkDeferredCanvas or where ever the caller is.

static bool isOpaqueWithCaveats(const SkPaint&) { ...

http://codereview.appspot.com/5451102/diff/12001/src/effects/SkGradientShader...
File src/effects/SkGradientShader.cpp (left):

http://codereview.appspot.com/5451102/diff/12001/src/effects/SkGradientShader...
src/effects/SkGradientShader.cpp:164: SkColor*    fOrigColors;
Maybe add a comment: // This is for the original colors, not after they have
been modulated by the paint in setContext.

[reed1, 2011-12-08 20:32:46]

Should be quick to add a unittest for the new API...

[junov1, 2011-12-08 22:53:20]

On 2011/12/08 20:32:46, reed1 wrote:
> Should be quick to add a unittest for the new API...

Tests, were a good idea, found a bug in SkBitmapProcShader::isOpaque()

[reed1, 2011-12-09 13:04:38]

LGTM w/ comment fix

http://codereview.appspot.com/5451102/diff/15001/include/core/SkShader.h
File include/core/SkShader.h (right):

http://codereview.appspot.com/5451102/diff/15001/include/core/SkShader.h#newc...
include/core/SkShader.h:104: *  which only works properly when the context is
set.
remove @param comment, since there's no paint parameter

[bsalomon, 2011-12-09 13:43:21]

On 2011/12/09 13:04:38, reed1 wrote:
> LGTM w/ comment fix
> 
> http://codereview.appspot.com/5451102/diff/15001/include/core/SkShader.h
> File include/core/SkShader.h (right):
> 
>
http://codereview.appspot.com/5451102/diff/15001/include/core/SkShader.h#newc...
> include/core/SkShader.h:104: *  which only works properly when the context is
> set.
> remove @param comment, since there's no paint parameter

LGTM