Add support for reading the dst pixel value in an effect. Use in a new effect for the kDarken xfer …

Add support for reading the dst pixel value in an effect. Use in a new effect for the kDarken xfer mode.

The current implementation is to always make a copy of the entire dst before the draw.
It will only succeed if the RT is also a texture.
Obviously, there is lots of room for improvement.
Committed: https://code.google.com/p/skia/source/detail?r=8449

[robertphillips, 2013-03-29 18:32:57]

lgtm + nits

https://codereview.chromium.org/13314002/diff/17001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/13314002/diff/17001/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:952: // TODO: Make this work for all non-Coeff
SkXfermode::Mode values
Comment as to behavior?

https://codereview.chromium.org/13314002/diff/17001/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:985: outputColor, dstColorName, inputColor);
two overlength lines

https://codereview.chromium.org/13314002/diff/17001/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:999: virtual bool onIsEqual(const GrEffect& other) const
SK_OVERRIDE { return true; }
INHERITED?

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.cpp#...
src/gpu/GrDrawTarget.cpp:450: }
textureMatrix doesn't appear to be used

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.cpp#...
src/gpu/GrDrawTarget.cpp:458:
this->drawState()->createTextureEffect(kTextureStage, rt->asTexture(), matrix);
I think "drawRect(dstRect, NULL, &dstRect, NULL);" would be okay maybe with
renaming dstRect to copyRect.

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.h#ne...
src/gpu/GrDrawTarget.h:658: // NULL if no copy of the dst is needed for the
draw.
requiresDstCopy? dstCopyNeeded?

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.h#ne...
src/gpu/GrDrawTarget.h:727: 
// Return true on success; false on failure

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
src/gpu/gl/GrGLShaderBuilder.cpp:131: uint32_t configMask =
SkToBool(kUseAlphaConfig_DstReadKeyBit & desc.fDstRead) ?
align with (?

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
src/gpu/gl/GrGLShaderBuilder.cpp:136: fDstCopyTopLeftUniform =
this->addUniform(kFragment_ShaderType,
align with (?

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
src/gpu/gl/GrGLShaderBuilder.h:274: GrGLUniformManager::UniformHandle
getDstCopyTopLeftUniform() const {
} on own line

[jvanverth1, 2013-03-29 18:46:22]

I think the first comment needs to be fixed, otherwise the rest are nits.

https://codereview.chromium.org/13314002/diff/27002/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/13314002/diff/27002/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:984: builder->fsCodeAppendf("\t\t%s.a = 1.0 - (1.0 -
%s.a) * (1.0 - %s.b);\n",
Should the last term be 1.0 - %s.a?

https://codereview.chromium.org/13314002/diff/27002/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:986: builder->fsCodeAppendf("\t\t%s.rgb = min((1.0 -
%s.a) * %s.rgb + %s.rgb, (1.0 - %s.a) * %s.rgb + %s.rgb);\n",
Wrap.

https://codereview.chromium.org/13314002/diff/27002/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/13314002/diff/27002/src/gpu/GrDrawTarget.cpp#...
src/gpu/GrDrawTarget.cpp:490: return;
Do we want to try to draw something here, even if the blend mode is wrong?

https://codereview.chromium.org/13314002/diff/27002/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/13314002/diff/27002/src/gpu/GrGpu.h#newcode485
src/gpu/GrGpu.h:485: // backend-specific flush of the state.s
Typo?

[bsalomon, 2013-03-29 18:59:38]

https://codereview.chromium.org/13314002/diff/17001/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/13314002/diff/17001/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:952: // TODO: Make this work for all non-Coeff
SkXfermode::Mode values
On 2013/03/29 18:32:57, robertphillips wrote:
> Comment as to behavior?

Done.

https://codereview.chromium.org/13314002/diff/17001/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:985: outputColor, dstColorName, inputColor);
On 2013/03/29 18:32:57, robertphillips wrote:
> two overlength lines

Done.

https://codereview.chromium.org/13314002/diff/17001/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:999: virtual bool onIsEqual(const GrEffect& other) const
SK_OVERRIDE { return true; }
On 2013/03/29 18:32:57, robertphillips wrote:
> INHERITED?

Done.

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.cpp#...
src/gpu/GrDrawTarget.cpp:450: }
On 2013/03/29 18:32:57, robertphillips wrote:
> textureMatrix doesn't appear to be used

Done.

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.cpp#...
src/gpu/GrDrawTarget.cpp:458:
this->drawState()->createTextureEffect(kTextureStage, rt->asTexture(), matrix);
On 2013/03/29 18:32:57, robertphillips wrote:
> I think "drawRect(dstRect, NULL, &dstRect, NULL);" would be okay maybe with
> renaming dstRect to copyRect.

Done.

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.h#ne...
src/gpu/GrDrawTarget.h:658: // NULL if no copy of the dst is needed for the
draw.
On 2013/03/29 18:32:57, robertphillips wrote:
> requiresDstCopy? dstCopyNeeded?

but it actually returns the copy, not a bool. Changed to getDstCopy()

https://codereview.chromium.org/13314002/diff/17001/src/gpu/GrDrawTarget.h#ne...
src/gpu/GrDrawTarget.h:727: 
On 2013/03/29 18:32:57, robertphillips wrote:
> // Return true on success; false on failure

Done.

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
src/gpu/gl/GrGLShaderBuilder.cpp:131: uint32_t configMask =
SkToBool(kUseAlphaConfig_DstReadKeyBit & desc.fDstRead) ?
On 2013/03/29 18:32:57, robertphillips wrote:
> align with (?

Done. (made an if/else)

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
src/gpu/gl/GrGLShaderBuilder.cpp:136: fDstCopyTopLeftUniform =
this->addUniform(kFragment_ShaderType,
On 2013/03/29 18:32:57, robertphillips wrote:
> align with (?

Done.

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/13314002/diff/17001/src/gpu/gl/GrGLShaderBuil...
src/gpu/gl/GrGLShaderBuilder.h:274: GrGLUniformManager::UniformHandle
getDstCopyTopLeftUniform() const {
On 2013/03/29 18:32:57, robertphillips wrote:
> } on own line

Done.

https://codereview.chromium.org/13314002/diff/27002/src/core/SkXfermode.cpp
File src/core/SkXfermode.cpp (right):

https://codereview.chromium.org/13314002/diff/27002/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:984: builder->fsCodeAppendf("\t\t%s.a = 1.0 - (1.0 -
%s.a) * (1.0 - %s.b);\n",
On 2013/03/29 18:46:22, JimVV wrote:
> Should the last term be 1.0 - %s.a?

yeah.. nice catch!

https://codereview.chromium.org/13314002/diff/27002/src/core/SkXfermode.cpp#n...
src/core/SkXfermode.cpp:986: builder->fsCodeAppendf("\t\t%s.rgb = min((1.0 -
%s.a) * %s.rgb + %s.rgb, (1.0 - %s.a) * %s.rgb + %s.rgb);\n",
On 2013/03/29 18:46:22, JimVV wrote:
> Wrap.

Done.

https://codereview.chromium.org/13314002/diff/27002/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/13314002/diff/27002/src/gpu/GrDrawTarget.cpp#...
src/gpu/GrDrawTarget.cpp:490: return;
On 2013/03/29 18:46:22, JimVV wrote:
> Do we want to try to draw something here, even if the blend mode is wrong?

We could... and probably should. I'll put a comment. Since this is likely to
ship soon it felt safer to bail.

https://codereview.chromium.org/13314002/diff/27002/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/13314002/diff/27002/src/gpu/GrGpu.h#newcode485
src/gpu/GrGpu.h:485: // backend-specific flush of the state.s
On 2013/03/29 18:46:22, JimVV wrote:
> Typo?

Done.

[bsalomon, 2013-03-29 19:11:23]

Add support for reading the dst pixel value in an effect. Use in a new effect
for the kDarken xfer mode.

The current implementation is to always make a copy of the entire dst before the
draw.
It will only succeed if the RT is also a texture.
Obviously, there is lots of room for improvement.

[jvanverth1, 2013-03-29 19:19:09]

lgtm

[bsalomon, 2013-03-29 19:22:45]

Committed patchset #9 manually as r8449 (presubmit successful).