Maintain SkPaint in GraphicsContextState.

Maintain SkPaint in GraphicsContextState.

The aim is to avoid SkPaint setup time for the common case of filling and stroking shapes. We can't entirely do that, as some of the paint content is not controlled by GraphicsContextState (such as text or temporary paint state set for specific draw operations).

Perf tests on the record_and_rasterize micro-bench indicate a 1-3% average improvement on record times.

BUG=343240
R=adamk@chromium.org, fmalita@chromium.org, senorblanco@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168536

[Stephen Chennney, 2014-02-26 22:19:02]

Note that this is not ready for review yet. I have fixed all the bugs, I think,
but I need a pass to clean up inlining and the like.

[Stephen Chennney, 2014-02-28 00:48:02]

On 2014/02/26 22:19:02, Stephen Chenney wrote:
> Note that this is not ready for review yet. I have fixed all the bugs, I
think,
> but I need a pass to clean up inlining and the like.

Some help reviewing this - it's not really as bad as it seems ...

1) The Gradient changes are to allow GraphicsContext to detect changes to the
Gradient after it has been set in the context but before the paint for it is
requested. Pattern does not ever modify its content after the shader is created.

2) Lots of direct access to GraphicsContextState from GraphicsContext has been
replaced with method calls so that the GraphicsContextState can update the
paint.

3) It is annoying that setPaintDashPathEffect is required but we don't know the
path length until the draw call is made.

4) I switched alpha to be stored 0 - 256 to match the way it is more commonly
used. The 256 part, instead of 255, is because Skia uses it that way.

[Stephen Chennney, 2014-03-03 17:20:27]

Review please. Pretty please.

[f(malita), 2014-03-03 17:50:30]

On 2014/03/03 17:20:27, Stephen Chenney wrote:
> Review please. Pretty please.

Sorry, I missed the switch to "ready for review" on this one :)

Will try to do it today.

[f(malita), 2014-03-03 22:46:21]

On 2014/02/28 00:48:02, Stephen Chenney wrote:

> 1) The Gradient changes are to allow GraphicsContext to detect changes to the
> Gradient after it has been set in the context but before the paint for it is
> requested. Pattern does not ever modify its content after the shader is
created.

This seems weird, are we working around a deeper issue?

Why is the gradient changing after it's been set on GC? Can we enforce a sane
mutation sequence at call sites?

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/Gradient.h (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/Gradient.h:147: bool m_shaderChanged;
Looks like we can consolidate these three bools + the one below in a bitfield.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:1352:
paint.setColorFilter(immutableState()->colorFilter());
Why does this paint need special handling? (why not just use fillPaint?)

[Stephen Chennney, 2014-03-04 13:20:35]

Thanks.

The gradient change is due to the canvas spec. You create a gradient and hold it
in JS. You then add it to the context, which invokes
GraphicsContext.setGradient. Then you can make changes to the gradient JS object
you're holding, and that modifies the Gradient in Blink. We actually have a test
for this.

We had no problem before because we only asked for the gradient shader at time
of use. But now that we are caching it we need an invalidation scheme.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/Gradient.h (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/Gradient.h:147: bool m_shaderChanged;
On 2014/03/03 22:46:21, Florin Malita wrote:
> Looks like we can consolidate these three bools + the one below in a bitfield.

I missed the odd one at the bottom. So yes.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:1352:
paint.setColorFilter(immutableState()->colorFilter());
On 2014/03/03 22:46:21, Florin Malita wrote:
> Why does this paint need special handling? (why not just use fillPaint?)

It doesn't (apart from the color at the end). I must have overlooked it.

[f(malita), 2014-03-04 14:23:28]

On 2014/03/04 13:20:35, Stephen Chenney wrote:
> The gradient change is due to the canvas spec. You create a gradient and hold
it
> in JS. You then add it to the context, which invokes
> GraphicsContext.setGradient. Then you can make changes to the gradient JS
object
> you're holding, and that modifies the Gradient in Blink. We actually have a
test
> for this.

Argh. I wish that was encapsulated within canvas, and not exposed to GC.

Still, it looks like Gradient does its own shader caching/invalidation. I think
we might as well use that and unconditionally setShader(gradient->shader()) in
fill/strokePaint() - it's probably just as cheap as the dirty shader check. Or
at least use the current shader inval logic in Gradient and avoid adding an
extra flag.

[f(malita), 2014-03-04 14:23:46]

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/Gradient.h (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/Gradient.h:75: bool shaderChanged() const { return
m_shaderChanged; }
How about

  return !m_shader?

Gradient already takes care of tracking/invalidating its shader, I think we can
use that.

[Stephen Chennney, 2014-03-04 14:47:10]

Done. Just need to build and test.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/Gradient.h (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/Gradient.h:75: bool shaderChanged() const { return
m_shaderChanged; }
On 2014/03/04 14:23:47, Florin Malita wrote:
> How about
> 
>   return !m_shader?
> 
> Gradient already takes care of tracking/invalidating its shader, I think we
can
> use that.

Done.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/Gradient.h:147: bool m_shaderChanged;
On 2014/03/04 13:20:36, Stephen Chenney wrote:
> On 2014/03/03 22:46:21, Florin Malita wrote:
> > Looks like we can consolidate these three bools + the one below in a
bitfield.
> 
> I missed the odd one at the bottom. So yes.

Done.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:1352:
paint.setColorFilter(immutableState()->colorFilter());
On 2014/03/04 13:20:36, Stephen Chenney wrote:
> On 2014/03/03 22:46:21, Florin Malita wrote:
> > Why does this paint need special handling? (why not just use fillPaint?)
> 
> It doesn't (apart from the color at the end). I must have overlooked it.

Done.

[Stephen White, 2014-03-04 15:24:13]

Some parts of the API now have small inconsistencies, and surprising corner
cases. It would be nice if we could hide that inside GraphicsContextState, and
give it a more consistent API. Other than that, I'm ok with this.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:592:
immutableState()->strokeData().setupPaintDashPathEffect(paint, length);
Seems a little surprising not to handle this automatically (see below).

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:977: setupPaintForFilling(&paint);
IIRC, Florin's patch should get rid of this one; maybe we can get rid of
setupPaintForFilling altogether after that? (Or at least the calls from
GraphicsContext.)

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContextState.cpp (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.cpp:74:
m_fillPaint.setShader(m_fillGradient->shader());
I'm surprised not to see patterns here. How are those handled now?

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContextState.h (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:48: class GraphicsContextState
final {
Unless I missed something, we can't use C++11 in Blink yet; should be FINAL for
now.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:69: // Does not set up the
necessary path effect for dotted or dashed lines.
Why the exception? Performance?

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:124: void setAlpha(const float);
This is confusing (int 0->256 vs float 0->1, I assume). Could we rename one or
the other to make it clear? Maybe setFloatAlpha()?

(Also surprised to see 256 alpha here, but apparently that's not new to this
patch.)

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:157: // These are mutbale to
enable gradient updates when the paints are fetched for use.
Hmm.. if they're mutable, than won't that prevent us from benefiting from the
const SkPaint plans coming up in Skia? (Maybe I'm off-base here.)

[Stephen Chennney, 2014-03-04 17:41:29]

Thanks. I've hopefully explained why things are as they are, and I'll fix the
things that can be fixed.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContextState.cpp (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.cpp:74:
m_fillPaint.setShader(m_fillGradient->shader());
On 2014/03/04 15:24:13, Stephen White wrote:
> I'm surprised not to see patterns here. How are those handled now?

Patterns have no methods that change the shader pointer after creation. All you
can do is modify the transform, which modifies the shader without recreating it.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContextState.h (right):

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:69: // Does not set up the
necessary path effect for dotted or dashed lines.
On 2014/03/04 15:24:13, Stephen White wrote:
> Why the exception? Performance?

We need the length of the path to correctly set up the dash length. Any line
should always start and end with a dash, so that, for example, lines joining at
a corner will actually meet at the corner.

I agree that it's bad, so I think I'll change it to take the length and target
paint into strokePaint(), so that the operation is all-in-one for callers.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:124: void setAlpha(const float);
On 2014/03/04 15:24:13, Stephen White wrote:
> This is confusing (int 0->256 vs float 0->1, I assume). Could we rename one or
> the other to make it clear? Maybe setFloatAlpha()?
> 
> (Also surprised to see 256 alpha here, but apparently that's not new to this
> patch.)

Fair call. The 256 is a Skia thing, due to the way they multiply alpha.

https://codereview.chromium.org/169283008/diff/100001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:157: // These are mutbale to
enable gradient updates when the paints are fetched for use.
On 2014/03/04 15:24:13, Stephen White wrote:
> Hmm.. if they're mutable, than won't that prevent us from benefiting from the
> const SkPaint plans coming up in Skia? (Maybe I'm off-base here.)

Yes. The fact that canvas gradients are mutable after they are set on the
graphics context led to this change. I would leave it as is for now because
we'll have to address the gradient problem as part of addressing the SkPaint
immutability.

[Stephen Chennney, 2014-03-05 01:51:07]

Need a review for a method rename in Source/web/tests/GraphicsContextTest.cpp

[jbroman, 2014-03-05 02:00:47]

Don't mean to butt in here, but I saw one thing which looked unusual (possibly
the result of an error when running a substitution).

https://codereview.chromium.org/169283008/diff/140001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContextState.h (right):

https://codereview.chromium.org/169283008/diff/140001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:69: void setStrokeStyle(const
StrokeStyle);
I haven't seen us pass const enums, const floats, const bools, etc. to functions
(except const references/pointers). Is this intentional?

[jamesr, 2014-03-05 02:02:12]

https://codereview.chromium.org/169283008/diff/140001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContextState.h (right):

https://codereview.chromium.org/169283008/diff/140001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:69: void setStrokeStyle(const
StrokeStyle);
On 2014/03/05 02:00:47, jbroman wrote:
> I haven't seen us pass const enums, const floats, const bools, etc. to
functions
> (except const references/pointers). Is this intentional?

the 'const' here doesn't actually mean anything - it doesn't change the
signature of the function, change how the function can be called or change how
the function can be defined. i'd nuke it.

[Stephen White, 2014-03-05 13:39:01]

LGTM

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:609: SkPaint
paint(immutableState()->fillPaint());
Nit: is it necessary to copy-construct here? Could we just pass the const-ref
directly to drawOval() instead?

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:1320: SkPaint paint =
immutableState()->fillPaint();
Won't this potentially include a gradient, where it didn't before? (OTOH, it
doesn't really make sense to me that it was calling setupPaintCommon before, and
not setupPaintForFilling, so maybe this was a bug.)

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.h:153: void setAlphaAsFloat(float
alpha) { mutableState()->setAlphaAsFloat(alpha);}
Hmm. I didn't intend for this to propagate to GraphicsContext; I meant that only
GraphicsContextState should change to setAlphaAsFloat(float). But I suppose if I
picked the colour, I shouldn't complain that you've painted *all* the sheds.

[f(malita), 2014-03-05 14:09:25]

lgtm

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContextState.h (right):

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
Source/platform/graphics/GraphicsContextState.h:148: return (c & 0x00FFFFFF) |
(a << 24);
Unrelated to this CL, but we should use Skia macros and not make packing
assumptions here.

[Stephen Chennney, 2014-03-05 16:17:10]

Done done. And one more patch that avoids some computation.

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:609: SkPaint
paint(immutableState()->fillPaint());
On 2014/03/05 13:39:02, Stephen White wrote:
> Nit: is it necessary to copy-construct here? Could we just pass the const-ref
> directly to drawOval() instead?

Done.

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:1320: SkPaint paint =
immutableState()->fillPaint();
On 2014/03/05 13:39:02, Stephen White wrote:
> Won't this potentially include a gradient, where it didn't before? (OTOH, it
> doesn't really make sense to me that it was calling setupPaintCommon before,
and
> not setupPaintForFilling, so maybe this was a bug.)

Good point. I believe the change is OK because this method is always used with a
state to which a gradient could not have been applied. Put another way, anything
that sets up a gradient pushes a save/restore and things that use this method to
draw rects (like underlines) do not ever draw with that state. Given how often
we invoke this, I would expect any problems to show up in testing, and I'm happy
for future changes to have to account for the gradient.

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/169283008/diff/180001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.h:153: void setAlphaAsFloat(float
alpha) { mutableState()->setAlphaAsFloat(alpha);}
On 2014/03/05 13:39:02, Stephen White wrote:
> Hmm. I didn't intend for this to propagate to GraphicsContext; I meant that
only
> GraphicsContextState should change to setAlphaAsFloat(float). But I suppose if
I
> picked the colour, I shouldn't complain that you've painted *all* the sheds.

Your point was a valid one everywhere I think. The set takes a float while the
get returns an int. I think this will avoid confusion down the road.

[adamk, 2014-03-05 19:33:36]

lgtm for Source/web/

[Stephen Chennney, 2014-03-05 19:35:20]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-03-05 19:35:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/schenney@chromium.org/169283008/200001

[commit-bot: I haz the power, 2014-03-05 21:00:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/schenney@chromium.org/169283008/200001

[Stephen Chennney, 2014-03-05 22:12:19]

Committed patchset #6 manually as r168536 (presubmit successful).