bitmapshader with A8 bitmap should be colorized by the paint's color

bitmapshader with A8 bitmap should be colorized by the paint's color

BUG=skia:2293

[bsalomon, 2014-03-13 14:59:04]

https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:348: virtual SkBitmap* getBitmap() { return NULL;}
I don't think the Shader should expose a bitmap. If there isn't a way to deduce
this behavior from the Shader already then maybe the Shader subclass should have
explicit control of what part of the SkPaint's color it wants to use.

[yunchao, 2014-03-13 15:05:11]

https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:348: virtual SkBitmap* getBitmap() { return NULL;}
On 2014/03/13 14:59:04, bsalomon wrote:
> I don't think the Shader should expose a bitmap. If there isn't a way to
deduce
> this behavior from the Shader already then maybe the Shader subclass should
have
> explicit control of what part of the SkPaint's color it wants to use.

What about expose an interface like this: bool isA8BitmapShader()?

[yunchao, 2014-03-13 15:32:27]

https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h#...
include/core/SkShader.h:348: virtual SkBitmap* getBitmap() { return NULL;}
On 2014/03/13 14:59:04, bsalomon wrote:
> I don't think the Shader should expose a bitmap. If there isn't a way to
deduce
> this behavior from the Shader already then maybe the Shader subclass should
have
> explicit control of what part of the SkPaint's color it wants to use.

Do you think it is unsafe if the shader expose a bitmap? I think check the
shader's bitmap config in skPaint2GrPaint() is appropriate.

[bsalomon, 2014-03-17 14:01:03]

On 2014/03/13 15:32:27, Richard Ho wrote:
> https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h
> File include/core/SkShader.h (right):
> 
>
https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h#...
> include/core/SkShader.h:348: virtual SkBitmap* getBitmap() { return NULL;}
> On 2014/03/13 14:59:04, bsalomon wrote:
> > I don't think the Shader should expose a bitmap. If there isn't a way to
> deduce
> > this behavior from the Shader already then maybe the Shader subclass should
> have
> > explicit control of what part of the SkPaint's color it wants to use.
> 
> Do you think it is unsafe if the shader expose a bitmap? I think check the
> shader's bitmap config in skPaint2GrPaint() is appropriate.

I don't think it's unsafe. I just think that the shader should not have to
expose that it is a bitmap shader to make this work. SkShader::asNewEffect()
already takes the SkPaint. Perhaps it could return an additional value which is
the input color that should be passed to the returned effect. For a
SkBitmapProcShader with an RGBA bitmap it would return the paint's alpha
(replicated into four channels) and when it is an alpha-only bitmap it would
return the paint's color.

[yunchao, 2014-03-18 10:47:35]

On 2014/03/17 14:01:03, bsalomon wrote:
> On 2014/03/13 15:32:27, Richard Ho wrote:
> > https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h
> > File include/core/SkShader.h (right):
> > 
> >
>
https://codereview.chromium.org/196473010/diff/60001/include/core/SkShader.h#...
> > include/core/SkShader.h:348: virtual SkBitmap* getBitmap() { return NULL;}
> > On 2014/03/13 14:59:04, bsalomon wrote:
> > > I don't think the Shader should expose a bitmap. If there isn't a way to
> > deduce
> > > this behavior from the Shader already then maybe the Shader subclass
should
> > have
> > > explicit control of what part of the SkPaint's color it wants to use.
> > 
> > Do you think it is unsafe if the shader expose a bitmap? I think check the
> > shader's bitmap config in skPaint2GrPaint() is appropriate.
> 
> I don't think it's unsafe. I just think that the shader should not have to
> expose that it is a bitmap shader to make this work. SkShader::asNewEffect()
> already takes the SkPaint. Perhaps it could return an additional value which
is
> the input color that should be passed to the returned effect. For a
> SkBitmapProcShader with an RGBA bitmap it would return the paint's alpha
> (replicated into four channels) and when it is an alpha-only bitmap it would
> return the paint's color.

Brian, I also considered this solution: add a parameter, say bool* onlyAlpha or
SkColor* color, to SkShader::asNewEffect, then set the value in 
SkBitmapProcShader::asNewEffect. But this will spread that parameter into
::asNewEffct to all subclasses of SkShader, right? We can't set the value in
SkShader::asNewEffect for all other shaders. Because SkShader::asNewEffect
returns false directly, it should not be called if the subclasses of SkShader
have their own asNewEffect(). Then we need to set the value in every
Sk**Shader::asNewEffect and Sk**Gradient::asNewEffect. Please correct me if I am
wrong.

So, I add a function to return the bitmap in SkBitmapProcShader for bitmap
shader, and return NULL in SkShader for other shaders.

[reed1, 2014-03-18 14:30:20]

I do not want to add more virtuals to shader if possible. I think we will tackle
this particular bug in-house, as fixing it may involve related considerations as
to how GrEffects interact with the rest of the glsl pipeline.

I have an alternate CL that does incorporate your documentation in SkPaint.h