Provide a GPU implementation of SkArithmeticMode

Provide a GPU implementation of SkArithmeticMode, using a custom GrEffect exposed via asNewEffectOrCoeff().

Doing it this way required modifying the arithmode GM to use saveLayer()/restore() rather than creating an offscreen SkBitmap, since otherwise the compositing is always done in raster mode. Fixing that in turn exposed that SkArithmeticMode did not work in Picture mode, since it wasn't flattenable. Made it so.

Note: this will require rebaselining the arithmode GM (again).

R=bsalomon@google.com, reed@google.com

Originally committed: https://code.google.com/p/skia/source/detail?r=9324
Reverted: https://code.google.com/p/skia/source/detail?r=9325

Committed: https://code.google.com/p/skia/source/detail?r=9330

[Stephen White, 2013-05-28 13:58:15]

PTAL. Thanks!

[bsalomon, 2013-05-28 14:21:20]

The GPU code mostly looks good to me. A couple of questions inline.

https://codereview.chromium.org/16064002/diff/2001/src/effects/SkArithmeticIm...
File src/effects/SkArithmeticImageFilter.cpp (right):

https://codereview.chromium.org/16064002/diff/2001/src/effects/SkArithmeticIm...
src/effects/SkArithmeticImageFilter.cpp:238: fK1Uni =
builder->addUniform(GrGLShaderBuilder::kFragment_ShaderType,
Just wondering if it'd make sense to use a vec4 for the k values, just to reduce
the amount of C code and gl calls to upload.

https://codereview.chromium.org/16064002/diff/2001/src/effects/SkArithmeticIm...
src/effects/SkArithmeticImageFilter.cpp:267:
builder->fsCodeAppend("\t\tfgColor.rgb = clamp(fgColor.rgb / fgColor.a, 0.0,
1.0);\n");
What does clamp do if the first param is NaN?

[Stephen White, 2013-05-28 15:08:33]

https://codereview.chromium.org/16064002/diff/2001/src/effects/SkArithmeticIm...
File src/effects/SkArithmeticImageFilter.cpp (right):

https://codereview.chromium.org/16064002/diff/2001/src/effects/SkArithmeticIm...
src/effects/SkArithmeticImageFilter.cpp:238: fK1Uni =
builder->addUniform(GrGLShaderBuilder::kFragment_ShaderType,
On 2013/05/28 14:21:20, bsalomon wrote:
> Just wondering if it'd make sense to use a vec4 for the k values, just to
reduce
> the amount of C code and gl calls to upload.

Yeah, I thought of that. Just thought it might make the shader look less
readable/obvious. Would help with uniform allocation on dumb drivers, though.
Will try it out.

https://codereview.chromium.org/16064002/diff/2001/src/effects/SkArithmeticIm...
src/effects/SkArithmeticImageFilter.cpp:267:
builder->fsCodeAppend("\t\tfgColor.rgb = clamp(fgColor.rgb / fgColor.a, 0.0,
1.0);\n");
On 2013/05/28 14:21:20, bsalomon wrote:
> What does clamp do if the first param is NaN?

It returns zero, as far as I can tell (empirically and according to the spec):

clamp is defined to return min(max(x, minVal), maxVal).

max(x, minVal) is defined to return x if x > minVal, minVal otherwise.  Since x
is NaN, the comparison always fails, so it returns minVal, or 0.

Then min(0, 1) => 0. So, 0.

[Stephen White, 2013-05-28 15:35:27]

Changed the shader as requested. PTAnotherL. :)

[bsalomon, 2013-05-28 15:40:54]

On 2013/05/28 15:35:27, Stephen White wrote:
> Changed the shader as requested. PTAnotherL. :)

gpu lgtm

[reed1, 2013-05-28 16:22:02]

SkArithmeticMode.cpp -- might want to land this on its own.

https://codereview.chromium.org/16064002/diff/7001/include/effects/SkArithmet...
File include/effects/SkArithmeticImageFilter.h (right):

https://codereview.chromium.org/16064002/diff/7001/include/effects/SkArithmet...
include/effects/SkArithmeticImageFilter.h:16: SkArithmeticImageFilter(SkScalar
k1, SkScalar k2, SkScalar k3, SkScalar k4, SkImageFilter* background,
SkImageFilter* foreground = NULL);
Add some dox, both for the k values, and for the two additional filter objects.

[reed1, 2013-05-28 16:25:37]

Do we already have an imagefilter that just applies an arbitrary xfermode
between two other filter results? Wouldn't that subsume this subclass?

[Stephen White, 2013-05-28 16:40:06]

On 2013/05/28 16:25:37, reed1 wrote:
> Do we already have an imagefilter that just applies an arbitrary xfermode
> between two other filter results? Wouldn't that subsume this subclass?

We do (at least I wrote one over in Blink-land), but it's lower performing on
the GPU-side, since we already have both inputs as textures in this case. This
way, we can do the draw as a single pass.

[reed1, 2013-05-28 16:41:08]

Hmmm, sounds like we want a way for the xfermode to emit some effect snippet so
we can handle this optimally... brian?

[robertphillips, 2013-05-28 16:49:56]

Why can't we just add a asNewEffectOrCoeff method to SkArithmeticMode and use
that?

[bsalomon, 2013-05-28 16:59:14]

On 2013/05/28 16:49:56, robertphillips wrote:
> Why can't we just add a asNewEffectOrCoeff method to SkArithmeticMode and use
> that?

The image filter version takes two inputs and produces a third output rather
than replacing one of the inputs. We would need some additional info to
communicate that the "dst" isn't really the dst surface.

Is this not a problem for the raster implementation as well?

[Stephen White, 2013-05-28 17:11:00]

On 2013/05/28 16:49:56, robertphillips wrote:
> Why can't we just add a asNewEffectOrCoeff method to SkArithmeticMode and use
> that?

We could, but it would require a two-pass draw on the GPU side, where in this
case we already have both inputs as textures.

[Stephen White, 2013-05-28 17:14:37]

On 2013/05/28 16:59:14, bsalomon wrote:
> On 2013/05/28 16:49:56, robertphillips wrote:
> > Why can't we just add a asNewEffectOrCoeff method to SkArithmeticMode and
use
> > that?
> 
> The image filter version takes two inputs and produces a third output rather
> than replacing one of the inputs. We would need some additional info to
> communicate that the "dst" isn't really the dst surface.
> 
> Is this not a problem for the raster implementation as well?

The raster implementation does a 2-pass draw (as does SkBlendImageFilter). Since
we're not yet using the raster implementation in Blink (and the Blink raster
implementation is also 2-pass), I'm not too concerned about it yet.

[Stephen White, 2013-05-28 17:21:00]

On 2013/05/28 17:11:00, Stephen White wrote:
> On 2013/05/28 16:49:56, robertphillips wrote:
> > Why can't we just add a asNewEffectOrCoeff method to SkArithmeticMode and
use
> > that?
> 
> We could, but it would require a two-pass draw on the GPU side, where in this
> case we already have both inputs as textures.

Actually, hmm. I haven't looked into the new effect-based compositing modes, but
maybe we could do both: move the effect creation to
SkArithmeticMode::asNewEffectOrCoeff, and call that from a generic
SkXferModeImageFilter which applies the effect to the two given inputs. Then it
would be 2-pass for the generic compositing path, but still 1-pass for image
filters. Brian, would that work?

[bsalomon, 2013-05-28 17:40:39]

On 2013/05/28 17:21:00, Stephen White wrote:
> On 2013/05/28 17:11:00, Stephen White wrote:
> > On 2013/05/28 16:49:56, robertphillips wrote:
> > > Why can't we just add a asNewEffectOrCoeff method to SkArithmeticMode and
> use
> > > that?
> > 
> > We could, but it would require a two-pass draw on the GPU side, where in
this
> > case we already have both inputs as textures.
> 
> Actually, hmm. I haven't looked into the new effect-based compositing modes,
but
> maybe we could do both: move the effect creation to
> SkArithmeticMode::asNewEffectOrCoeff, and call that from a generic
> SkXferModeImageFilter which applies the effect to the two given inputs. Then
it
> would be 2-pass for the generic compositing path, but still 1-pass for image
> filters. Brian, would that work?

We'd to somehow communicate the second input texture to
SkArithmeticMode::asNewEffectOrCoeff. Otherwise, yes, that should work.

[reed1, 2013-05-28 18:14:52]

I'm fine for me to change the raster virtual for xfermodes to take the dst as a
3rd buffer.

[bsalomon, 2013-05-28 18:38:44]

On 2013/05/28 18:14:52, reed1 wrote:
> I'm fine for me to change the raster virtual for xfermodes to take the dst as
a
> 3rd buffer.

For the GPU we can add a texture param to asNewEffectOrCoeff that takes the
place of the dst if non-null. I think we'd also have document that only a
successful "answer" to the request can only be an effect (not coeffs) in that
case. That function is pretty new so there should only be a few implementations
to update.

[Stephen White, 2013-05-28 18:47:13]

On 2013/05/28 18:38:44, bsalomon wrote:
> On 2013/05/28 18:14:52, reed1 wrote:
> > I'm fine for me to change the raster virtual for xfermodes to take the dst
as
> a
> > 3rd buffer.
> 
> For the GPU we can add a texture param to asNewEffectOrCoeff that takes the
> place of the dst if non-null. I think we'd also have document that only a
> successful "answer" to the request can only be an effect (not coeffs) in that
> case. That function is pretty new so there should only be a few
implementations
> to update.

I've logged this as https://code.google.com/p/skia/issues/detail?id=1320

Since it would be a moderately deep change, can be proceed with this filter for
now (with the raster side extracted to
https://codereview.chromium.org/15917010/) and refactor it once the
infrastructure changes are done?

[reed1, 2013-05-28 18:52:26]

Brian thought he could update the gpu signature to allow for
SkXfermodeImageFilter to take its place. Can we wait for that to happen? I'm not
worried about the raster optimization, since there is no regression from today's
perf.

[Stephen White, 2013-05-28 18:55:21]

On 2013/05/28 18:52:26, reed1 wrote:
> Brian thought he could update the gpu signature to allow for
> SkXfermodeImageFilter to take its place. Can we wait for that to happen? I'm
not
> worried about the raster optimization, since there is no regression from
today's
> perf.

Sure, that sounds fine.

[bsalomon, 2013-05-28 18:58:47]

On 2013/05/28 18:55:21, Stephen White wrote:
> On 2013/05/28 18:52:26, reed1 wrote:
> > Brian thought he could update the gpu signature to allow for
> > SkXfermodeImageFilter to take its place. Can we wait for that to happen? I'm
> not
> > worried about the raster optimization, since there is no regression from
> today's
> > perf.
> 
> Sure, that sounds fine.

Updating the signature is pretty trivial. It probably makes sense to just do it
in the change that adds SkXfermodeImageFilter since that would be the only
caller that would use the new param.

[Stephen White, 2013-05-29 18:07:24]

This patch moves the arithmetic effect to SkArithmeticMode's GPU path, using a
custom GrEffect exposed via asNewEffectOrCoeff(). This will be reused later in a
future generic SkXfermodeImageFilter, but is simply implemented as an xfermode
for now.

Doing it this way required modifying the GM to use saveLayer()/restore() rather
than creating an offscreen SkBitmap, since otherwise the compositing is always
done in raster mode. Fixing that in turn exposed that SkArithmeticMode did not
work in Picture mode, since it wasn't flattenable. Made it so.

Note: this will require rebaselining the arithmode GM (again).

[reed1, 2013-05-29 18:15:24]

Thanks for the flattening catch/fix. Will need to add this mode to
SkGlobalInitialization_default.cpp.

raster change lgtm, deferring to brian for the gpu code.

[bsalomon, 2013-05-29 18:23:17]

On 2013/05/29 18:15:24, reed1 wrote:
> Thanks for the flattening catch/fix. Will need to add this mode to
> SkGlobalInitialization_default.cpp.
> 
> raster change lgtm, deferring to brian for the gpu code.

gpu lgtm

[Stephen White, 2013-05-29 18:45:23]

On 2013/05/29 18:15:24, reed1 wrote:
> Thanks for the flattening catch/fix. Will need to add this mode to
> SkGlobalInitialization_default.cpp.

Good point!  Done. PTAL?

> raster change lgtm, deferring to brian for the gpu code.

[reed1, 2013-05-29 18:49:25]

lgtm

[Stephen White, 2013-05-29 18:51:01]

Committed patchset #7 manually as r9324 (presubmit successful).

[Stephen White, 2013-05-29 19:33:12]

Brian: PTAL. I was assuming I could write to the inputColor variable, which
isn't always true.

(BTW, Is it safe to assume I can always write to dstColor, or should I put that
one in a temporary too?)

[bsalomon, 2013-05-29 20:48:48]

On 2013/05/29 19:33:12, Stephen White wrote:
> Brian: PTAL. I was assuming I could write to the inputColor variable, which
> isn't always true.
> 
> (BTW, Is it safe to assume I can always write to dstColor, or should I put
that
> one in a temporary too?)

Oh right, I should have caught that. It is always safe to write to the output
color. The input color might be varying or uniform but the output color is
always just a non-const var scoped by the main funciton.

[bsalomon, 2013-05-29 20:48:55]

On 2013/05/29 20:48:48, bsalomon wrote:
> On 2013/05/29 19:33:12, Stephen White wrote:
> > Brian: PTAL. I was assuming I could write to the inputColor variable, which
> > isn't always true.
> > 
> > (BTW, Is it safe to assume I can always write to dstColor, or should I put
> that
> > one in a temporary too?)
> 
> Oh right, I should have caught that. It is always safe to write to the output
> color. The input color might be varying or uniform but the output color is
> always just a non-const var scoped by the main funciton.

lgtm

[Stephen White, 2013-05-29 20:55:16]

Committed patchset #8 manually as r9330 (presubmit successful).