LUT-based color filtering prototype in Ganesh.

This CL implements the Ganesh path for the SkTable_ColorFilter color transformation.

A new texture stage dedicated to color transforms has been added, along with the new custom stage implementing the LUT.
Committed: https://code.google.com/p/skia/source/detail?r=4663

[twiz, 2012-07-11 19:49:49]

Hi Guys,

This patch (#2) is working, but there seem to be some texture management issues
with the LUT. 

1 - The LUT is periodically 'dropped' for an empty texture.  I'm not yet certain
of the cause of this, as I am using the LUT texture exactly the same as the
gradient textures.

2 - There are pixel differences between the raster version, and Ganesh.  I
believe that this is due to the sampling of the texture.  I set the mode to
nearest, and ensured (to my best knowledge) that I am sampling the lut at
texture centres.

Please take a look, if you have a moment to review the high level design here.

[TomH, 2012-07-11 19:55:49]

Unverified hunch - can't other things overwrite texture slot 1
(kColorFilterTextureIdx = 1)?

[TomH, 2012-07-11 20:01:56]

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:427: // color once while converting to GrPaint and then
ignored.
I liked the original spelling better.

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:428: inline bool skPaint2GrPaintNoShader(SkGpuDevice*
dev,
Stupid question - why do you only have to support color transforms in the
NoShader() case? Couldn't you have a paint including a color transform that
because of other settings required a shader??

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
File src/gpu/effects/GrColorTableEffect.cpp (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.cpp:42: void
GrGLColorTableEffect::setupVariables(GrGLShaderBuilder* state,
We'll probably merge setupVariables(), emitVS(), and emitFS(), but until then
you could put {} in the class definition above instead of having to recapitulate
all these details here.

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
File src/gpu/effects/GrColorTableEffect.h (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.h:34: void setTexture(GrTexture* texture) {
fTexture = texture; }
So the API I wrote for GrSingleTextureEffect objects supplied the texture to the
constructor. Do you need to be able to change it like this?
...
From examining SkGpuDevice, it seems you do. What an ugly and unfortunate
circularity.

[bsalomon, 2012-07-11 20:07:34]

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:485: colorSampler->setCustomStage(new
GrColorTableEffect)->unref();
SkNEW()

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
File src/gpu/effects/GrColorTableEffect.cpp (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.cpp:21: int stage) SK_OVERRIDE;
Not required at all, but it is legal to use up to 100 cols now.

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.cpp:56: "\t\tvec4 coord = (%s) * 255 / 256 +
1 / 512 * vec4(1, 1, 1, 1);\n",
I think the Mac GLSL parser will complain if you don't put .0 on your floats.

I believe some mobile shader compilers are incredibly un-optimizing. It may be
worthwhile precompute the divisions and not use scalar ops and vector mul.

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.cpp:89: bool 
GrColorTableEffect::isEqual(const GrCustomStage& sBase) const {
uber-super-nit, extra space after bool

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
File src/gpu/effects/GrColorTableEffect.h (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.h:34: void setTexture(GrTexture* texture) {
fTexture = texture; }
On 2012/07/11 20:01:57, TomH wrote:
> So the API I wrote for GrSingleTextureEffect objects supplied the texture to
the
> constructor. Do you need to be able to change it like this?
> ...
> From examining SkGpuDevice, it seems you do. What an ugly and unfortunate
> circularity.


Why do we need texture pointer at all? Did I miss where it is used?

[bsalomon, 2012-07-11 20:11:11]

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:428: inline bool skPaint2GrPaintNoShader(SkGpuDevice*
dev,
On 2012/07/11 20:01:57, TomH wrote:
> Stupid question - why do you only have to support color transforms in the
> NoShader() case? Couldn't you have a paint including a color transform that
> because of other settings required a shader??

skPaint2GrPaint calls this and then installs the shader.

[bsalomon, 2012-07-11 20:14:12]

On 2012/07/11 19:55:49, TomH wrote:
> Unverified hunch - can't other things overwrite texture slot 1
> (kColorFilterTextureIdx = 1)?

No, Jeff added slot 1 to GrPaint by bumping its kMaxTextures from 1 to 2. So
there was no slot 1 before. GrContext puts all its internal stages after the
stages copied from GrPaint by using GrPaint::kTotalStages =
GrPaint::kMaxTextures + GrPaint::kMaxMasks.

[twiz, 2012-07-11 20:39:35]

Thanks for your reviews, guys.  I made the changes suggested.

The glitches indicated still exist, so I don't quite think this change is ready.

There must be something wrong with how the texture is being managed for the
extra stage.

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:427: // color once while converting to GrPaint and then
ignored.
On 2012/07/11 20:01:57, TomH wrote:
> I liked the original spelling better.

I thought the pain was appropriate too. = )

https://codereview.appspot.com/6351081/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:485: colorSampler->setCustomStage(new
GrColorTableEffect)->unref();
On 2012/07/11 20:07:34, bsalomon wrote:
> SkNEW()

Done.

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
File src/gpu/effects/GrColorTableEffect.cpp (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.cpp:21: int stage) SK_OVERRIDE;
On 2012/07/11 20:07:34, bsalomon wrote:
> Not required at all, but it is legal to use up to 100 cols now.

Noted.

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.cpp:42: void
GrGLColorTableEffect::setupVariables(GrGLShaderBuilder* state,
On 2012/07/11 20:01:57, TomH wrote:
> We'll probably merge setupVariables(), emitVS(), and emitFS(), but until then
> you could put {} in the class definition above instead of having to
recapitulate
> all these details here.

Done.

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.cpp:89: bool 
GrColorTableEffect::isEqual(const GrCustomStage& sBase) const {
On 2012/07/11 20:07:34, bsalomon wrote:
> uber-super-nit, extra space after bool

Done.

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
File src/gpu/effects/GrColorTableEffect.h (right):

https://codereview.appspot.com/6351081/diff/2001/src/gpu/effects/GrColorTable...
src/gpu/effects/GrColorTableEffect.h:34: void setTexture(GrTexture* texture) {
fTexture = texture; }
On 2012/07/11 20:07:34, bsalomon wrote:
> On 2012/07/11 20:01:57, TomH wrote:
> > So the API I wrote for GrSingleTextureEffect objects supplied the texture to
> the
> > constructor. Do you need to be able to change it like this?
> > ...
> > From examining SkGpuDevice, it seems you do. What an ugly and unfortunate
> > circularity.
> 
> 
> Why do we need texture pointer at all? Did I miss where it is used?

No, I do not need a texture pointer.  I can remove this function.

This was a piece of dangling code leftover from when I was worried about leaking
the texture due to the auto-unlock issue.

[twiz, 2012-07-13 19:56:21]

Patch set #4 exhibits a glitch:  No content is drawn for the tablecolorfilter gm
slide.

I narrowed down the cause of this, and it is related to the following skia
revision:  https://code.google.com/p/skia/source/detail?r=4555

More investigation forthcoming.

[bsalomon, 2012-07-13 19:58:03]

On 2012/07/13 19:56:21, twiz wrote:
> Patch set #4 exhibits a glitch:  No content is drawn for the tablecolorfilter
gm
> slide.
> 
> I narrowed down the cause of this, and it is related to the following skia
> revision:  https://code.google.com/p/skia/source/detail?r=4555

weird!

> 
> More investigation forthcoming.

[twiz, 2012-07-16 17:25:17]

On 2012/07/13 19:56:21, twiz wrote:
> Patch set #4 exhibits a glitch:  No content is drawn for the tablecolorfilter
gm
> slide.
> 
> I narrowed down the cause of this, and it is related to the following skia
> revision:  https://code.google.com/p/skia/source/detail?r=4555
> 
> More investigation forthcoming.

A small update:  
Running SampleApp with DEFAULT_TO_GPU set to 1 (SampleApp.cpp) still exhibits
the regression.  

I had hoped that the steps of rendering with raster, then picture and then gpu
when running sample app was causing the problem.  This is not the case.

[reed1, 2012-07-16 20:00:06]

https://codereview.appspot.com/6351081/diff/6002/src/effects/SkTableColorFilt...
File src/effects/SkTableColorFilter.cpp (right):

https://codereview.appspot.com/6351081/diff/6002/src/effects/SkTableColorFilt...
src/effects/SkTableColorFilter.cpp:184: 
This is writing into fStorage as if the ARGB rows were always stored in that
order/spacing. See the constructor, where they are stored differently: they are
packed tightly, where we only store the ones that are non-identity. Thus the
code below can overwrite some of that data.

I'm fine if we change the constructor to follow the pattern below, and just
leave gaps for the identity runs, but the two code paths need to agree.

[twiz, 2012-07-17 15:33:33]

Thanks for the review, Mike.

I made the change you suggested, but it was not the root cause of the failure
between 4554 and 4555.

I'm now aggressively tearing apart the change at 4555 to determine which part of
it causes the failure.

https://codereview.appspot.com/6351081/diff/6002/src/effects/SkTableColorFilt...
File src/effects/SkTableColorFilter.cpp (right):

https://codereview.appspot.com/6351081/diff/6002/src/effects/SkTableColorFilt...
src/effects/SkTableColorFilter.cpp:184: 
On 2012/07/16 20:00:06, reed1 wrote:
> This is writing into fStorage as if the ARGB rows were always stored in that
> order/spacing. See the constructor, where they are stored differently: they
are
> packed tightly, where we only store the ones that are non-identity. Thus the
> code below can overwrite some of that data.
> 
> I'm fine if we change the constructor to follow the pattern below, and just
> leave gaps for the identity runs, but the two code paths need to agree.

This is a good catch.  If we make the logic exactly the same, then the flattened
representation of the filter will be larger.

I think the best approach would be to preserve the existing packed structure of
the LUT, and construct the bitmap with the explicitly unpacked, and initialized
identity regions.

[twiz, 2012-07-17 22:09:55]

Corrected the regression.  

I still need to rebaseline to tip of tree, where I believe I will need to
inherit from GrSingleTextureEffect.

https://codereview.appspot.com/6351081/diff/23001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6351081/diff/23001/src/gpu/SkGpuDevice.cpp#new...
src/gpu/SkGpuDevice.cpp:491: grPaint->resetColorFilter();
This was the root cause of the regression between 4554 and 4555.

I narrowed down the exact cause to be the addition of the fAnnotation member to
the SkPaint.  Somehow the addition of this member, coupled with the
uninitialized state of the GrPaint was causing the shader generator pipeline to
fail.

Thanks to Stephen and some good use of valgrind to track this down.

My haunch is that the grPaint was using the same stack space as a SkPaint, and
it just happened to be that adding the extra member variable to SkPaint changed
the memory layout so that the GrPaint was landing on incorrect uninitialized
data.

[twiz, 2012-07-17 22:31:01]

On 2012/07/17 22:09:55, twiz wrote:
> Corrected the regression.  
> 
> I still need to rebaseline to tip of tree, where I believe I will need to
> inherit from GrSingleTextureEffect.

r4608 causes a problem with this patch.  The following assert
(GrContext::setPaint) is triggered:

 	SampleApp.exe!GrDrawState::stagesDisabled()  Line 235	C++
 	SampleApp.exe!GrContext::setPaint(const GrPaint & paint)  Line 1584 + 0xb
bytes	C++
>	SampleApp.exe!GrContext::prepareToDraw(const GrPaint & paint,
GrContext::DrawCategory category)  Line 1647	C++
 	SampleApp.exe!GrContext::drawRect(const GrPaint & paint, const SkRect & rect,
float width, const SkMatrix * matrix)  Line 779 + 0xe bytes	C++
 	SampleApp.exe!GrContext::drawPaint(const GrPaint & paint)  Line 671	C++
 	SampleApp.exe!SkGpuDevice::drawPaint(const SkDraw & draw, const SkPaint &
paint)  Line 669	C++
 	SampleApp.exe!SkCanvas::internalDrawPaint(const SkPaint & paint)  Line
1445	C++
 	SampleApp.exe!SkCanvas::drawPaint(const SkPaint & paint)  Line 1438	C++
 	SampleApp.exe!SkCanvas::drawColor(unsigned int c, SkXfermode::Mode mode)  Line
1903	C++
 	SampleApp.exe!skiagm::GM::onDrawBackground(SkCanvas * canvas)  Line 44	C++



> 
> https://codereview.appspot.com/6351081/diff/23001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.appspot.com/6351081/diff/23001/src/gpu/SkGpuDevice.cpp#new...
> src/gpu/SkGpuDevice.cpp:491: grPaint->resetColorFilter();
> This was the root cause of the regression between 4554 and 4555.
> 
> I narrowed down the exact cause to be the addition of the fAnnotation member
to
> the SkPaint.  Somehow the addition of this member, coupled with the
> uninitialized state of the GrPaint was causing the shader generator pipeline
to
> fail.
> 
> Thanks to Stephen and some good use of valgrind to track this down.
> 
> My haunch is that the grPaint was using the same stack space as a SkPaint, and
> it just happened to be that adding the extra member variable to SkPaint
changed
> the memory layout so that the GrPaint was landing on incorrect uninitialized
> data.

[twiz, 2012-07-17 23:00:12]

On 2012/07/17 22:31:01, twiz wrote:
> On 2012/07/17 22:09:55, twiz wrote:
> > Corrected the regression.  
> > 
> > I still need to rebaseline to tip of tree, where I believe I will need to
> > inherit from GrSingleTextureEffect.

Patch #8 corrects the GrColorTableEffect to inherit from GrSingleTextureEffect,
and makes the appropriate changes in SkGpuDevice.  These changes correct the
assert previously mentioned.

However, the shader pipeline is no longer emits the shader code for the custom
effect.  Debugging further.

> 
> r4608 causes a problem with this patch.  The following assert
> (GrContext::setPaint) is triggered:
> 
>  	SampleApp.exe!GrDrawState::stagesDisabled()  Line 235	C++
>  	SampleApp.exe!GrContext::setPaint(const GrPaint & paint)  Line 1584 + 0xb
> bytes	C++
> >	SampleApp.exe!GrContext::prepareToDraw(const GrPaint & paint,
> GrContext::DrawCategory category)  Line 1647	C++
>  	SampleApp.exe!GrContext::drawRect(const GrPaint & paint, const SkRect &
rect,
> float width, const SkMatrix * matrix)  Line 779 + 0xe bytes	C++
>  	SampleApp.exe!GrContext::drawPaint(const GrPaint & paint)  Line 671	C++
>  	SampleApp.exe!SkGpuDevice::drawPaint(const SkDraw & draw, const SkPaint &
> paint)  Line 669	C++
>  	SampleApp.exe!SkCanvas::internalDrawPaint(const SkPaint & paint)  Line
> 1445	C++
>  	SampleApp.exe!SkCanvas::drawPaint(const SkPaint & paint)  Line 1438	C++
>  	SampleApp.exe!SkCanvas::drawColor(unsigned int c, SkXfermode::Mode mode) 
Line
> 1903	C++
>  	SampleApp.exe!skiagm::GM::onDrawBackground(SkCanvas * canvas)  Line 44	C++
> 
> 
> 
> > 
> > https://codereview.appspot.com/6351081/diff/23001/src/gpu/SkGpuDevice.cpp
> > File src/gpu/SkGpuDevice.cpp (right):
> > 
> >
>
https://codereview.appspot.com/6351081/diff/23001/src/gpu/SkGpuDevice.cpp#new...
> > src/gpu/SkGpuDevice.cpp:491: grPaint->resetColorFilter();
> > This was the root cause of the regression between 4554 and 4555.
> > 
> > I narrowed down the exact cause to be the addition of the fAnnotation member
> to
> > the SkPaint.  Somehow the addition of this member, coupled with the
> > uninitialized state of the GrPaint was causing the shader generator pipeline
> to
> > fail.
> > 
> > Thanks to Stephen and some good use of valgrind to track this down.
> > 
> > My haunch is that the grPaint was using the same stack space as a SkPaint,
and
> > it just happened to be that adding the extra member variable to SkPaint
> changed
> > the memory layout so that the GrPaint was landing on incorrect uninitialized
> > data.

[twiz, 2012-07-18 19:26:12]

One last time, I think this change is ready for prime time.

As discussed earlier, my use of an A8 texture for the LUT should be ok for use
in ES.

PTAL.

[bsalomon, 2012-07-18 19:35:30]

Can you add your new custom stage to GrGLGpu_unittest.cpp?

https://codereview.appspot.com/6351081/diff/31002/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.appspot.com/6351081/diff/31002/src/gpu/GrDrawState.h#newco...
src/gpu/GrDrawState.h:801: if (enabled != s.isStageEnabled(i)) {
This is already checked in.

https://codereview.appspot.com/6351081/diff/31002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6351081/diff/31002/src/gpu/SkGpuDevice.cpp#new...
src/gpu/SkGpuDevice.cpp:533: SkGpuDevice::SkAutoCachedTexture* colorLutTexture,
Could we make this param be:

SkGpuDevice::SkAutoCachedTexture textures[GrPaint::kMaxTextures],

rather than 2 params?

[twiz, 2012-07-18 20:59:10]

Comments addressed, and unittest added.

https://codereview.appspot.com/6351081/diff/31002/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

https://codereview.appspot.com/6351081/diff/31002/src/gpu/GrDrawState.h#newco...
src/gpu/GrDrawState.h:801: if (enabled != s.isStageEnabled(i)) {
On 2012/07/18 19:35:30, bsalomon wrote:
> This is already checked in.

Done.

https://codereview.appspot.com/6351081/diff/31002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6351081/diff/31002/src/gpu/SkGpuDevice.cpp#new...
src/gpu/SkGpuDevice.cpp:533: SkGpuDevice::SkAutoCachedTexture* colorLutTexture,
On 2012/07/18 19:35:30, bsalomon wrote:
> Could we make this param be:
> 
> SkGpuDevice::SkAutoCachedTexture textures[GrPaint::kMaxTextures],
> 
> rather than 2 params?

Done.

[bsalomon, 2012-07-18 21:08:43]

LGTM

[twiz, 2012-07-18 21:47:30]

On 2012/07/18 21:08:43, bsalomon wrote:
> LGTM

Committed:  r4663

Note that this change will require a Chromium skia.gyp change when the DEPS
rolls.