Clarify GraphicsContext::beginLayer()/endLayer() have unexpected behaviors.

Clarify GraphicsContext::beginLayer()/endLayer() have unexpected behaviors.

beginLayer()/endLayer() behaves like save()/restore() for only CTM and clip states.
SkCanvas::saveLayer() behaves the same as save(), but in addition it allocates an offscreen buffer.
It causes the inconsistency for restoring states managed by between SkCanvas and SkPaint.

For example,
    context.translate(1, 0);
    context.setCompositeOperation(CompositeDestinationIn);

    context.beginLayer(1, CompositeSourceIn);
    context.translate(-1, 0);
    context.setCompositeOperation(CompositeSourceOver);
    context.endLayer();

    // At this moment, CTM is translate(1, 0) while current composite operator is CompositeSourceOver.

It's because CTM is managed by SkCanvas, while current composite operator is managed by GraphicsContext.

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

[dshwang, 2014-10-14 17:21:47]

dongseong.hwang@intel.com changed reviewers:
+ junov@chromium.org

[dshwang, 2014-10-14 17:25:07]

junov@, could you review?

The changes in CanvasRenderingContext2D.cpp are covered by existing canvas
layout tests; fast/canvas/canvas-composite-*

https://codereview.chromium.org/651243002/diff/1/Source/platform/graphics/Gra...
File Source/platform/graphics/GraphicsContextTest.cpp (right):

https://codereview.chromium.org/651243002/diff/1/Source/platform/graphics/Gra...
Source/platform/graphics/GraphicsContextTest.cpp:1295:
EXPECT_EQ(CompositeDestinationIn, context.compositeOperation());
Without this CL, this line and #1298 fail, while BeginLayerPreserveCanvasState
test succeeds.

[Justin Novosad, 2014-10-14 17:50:15]

On 2014/10/14 17:25:07, dshwang wrote:

From an API ease of use standpoint, this change makes sense. But I am concerned
about how it may affect performance. Pushing the entire graphics context state
onto the stack is a heavy operation, which we try to avoid whenever it makes
sense. Fore example, by saving state to local variables. I would find it easier
to support this change if we had a bitmask (or something like that) to select
which part of the state we want to save instead of saving the whole thing all
the time.

Until we do something like that, could you fix this bug by saving just the state
you need, where you need it?

[Justin Novosad, 2014-10-14 17:50:26]

https://codereview.chromium.org/651243002/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left):

https://codereview.chromium.org/651243002/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1585: validateStateStack();
why are you removing this?

[f(malita), 2014-10-14 18:19:22]

fmalita@chromium.org changed reviewers:
+ schenney@chromium.org

[f(malita), 2014-10-14 18:27:41]

fmalita@chromium.org changed reviewers:
+ senorblanco@chromium.org

[f(malita), 2014-10-14 18:28:20]

fmalita@chromium.org changed reviewers:
+ fmalita@chromium.org

[f(malita), 2014-10-14 18:28:20]

I also think this should be a client fix: if state needs to be preserved, it
should be done via explicit save/restore calls.

The current beginLayer/endLayer API doesn't make any promises WRT state
preservation (there's a reason they're not named saveLayer/restoreLayer :)

[dshwang, 2014-10-14 19:00:29]

Thank you for review. Here's my questions.

On 2014/10/14 18:28:20, Florin Malita wrote:
> I also think this should be a client fix: if state needs to be preserved, it
> should be done via explicit save/restore calls.
> 
> The current beginLayer/endLayer API doesn't make any promises WRT state
> preservation (there's a reason they're not named saveLayer/restoreLayer :)

This issue is originated from that GraphicsContext::beginLayer uses
SkCanvas::saveLayer.
When we call SkCanvas::saveLayer, it pushes all canvas states onto the stack
such as CTM, clip regions, etc.
So the current beginLayer/endLayer API makes promises CTM and clip states, but
neither composite operator nor fill style nor etc.

On 2014/10/14 17:50:15, junov wrote:
> On 2014/10/14 17:25:07, dshwang wrote:
> 
> From an API ease of use standpoint, this change makes sense. But I am
concerned
> about how it may affect performance. Pushing the entire graphics context state
> onto the stack is a heavy operation, which we try to avoid whenever it makes
> sense.

We have two level to push stack.
1. CanvasRenderingContext2D::save() defers calling GraphicsContext::save()
2. GraphicsContext::save() defers calling SkCanvas::save()

Which stack do you concern about: GraphicsContext::save() or SkCanvas::save()?

Currently, CanvasRenderingContext2D calls GraphicsContext::beginLayer() when
it's really needed.
The overhead which this CL made is pushing GraphicsContext states into
GraphicsContext stack. SkCanvas states are already pushed when
SkCanvas::saveLayer() is called.
Although copying GraphicsContextState is heavy, SkCanvas::saveLayer() is much
heavier because it allocates new buffer.
So performance regression seems to be hardly noticeable. 

> Fore example, by saving state to local variables. I would find it easier
> to support this change if we had a bitmask (or something like that) to select
> which part of the state we want to save instead of saving the whole thing all
> the time.

Could you explain more? I don't understand yet. Current
GraphicsContext::beginLayer() slightly manages CTM and clip regions.
As I understand, you suggests that we knows that GraphicsContext::beginLayer()
manages CTM and composite operator differently and we will use it carefully,
right?

https://codereview.chromium.org/651243002/diff/1/Source/core/html/canvas/Canv...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/651243002/diff/1/Source/core/html/canvas/Canv...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1567: validateStateStack();
On 2014/10/14 17:50:25, junov wrote:
> why are you removing this?

I move it to here, because drawVideo is used as follows:
c->beginLayer(1, op);
drawVideo();
c->endLayer();

c->beginLayer(1, op) slightly increase state stack while
CanvasRenderingContext2D doesn't know it.

It's a bug. I'll fix it.

[Justin Novosad, 2014-10-14 19:21:44]

On 2014/10/14 19:00:29, dshwang wrote:
> Thank you for review. Here's my questions.
> 
> On 2014/10/14 18:28:20, Florin Malita wrote:
> > I also think this should be a client fix: if state needs to be preserved, it
> > should be done via explicit save/restore calls.
> > 
> > The current beginLayer/endLayer API doesn't make any promises WRT state
> > preservation (there's a reason they're not named saveLayer/restoreLayer :)
> 
> This issue is originated from that GraphicsContext::beginLayer uses
> SkCanvas::saveLayer.
> When we call SkCanvas::saveLayer, it pushes all canvas states onto the stack
> such as CTM, clip regions, etc.
> So the current beginLayer/endLayer API makes promises CTM and clip states, but
> neither composite operator nor fill style nor etc.

To be more accurate, it is nothing else than CTM and clip.
> 
> On 2014/10/14 17:50:15, junov wrote:
> > On 2014/10/14 17:25:07, dshwang wrote:
> > 
> > From an API ease of use standpoint, this change makes sense. But I am
> concerned
> > about how it may affect performance. Pushing the entire graphics context
state
> > onto the stack is a heavy operation, which we try to avoid whenever it makes
> > sense.
> 
> We have two level to push stack.
> 1. CanvasRenderingContext2D::save() defers calling GraphicsContext::save()
> 2. GraphicsContext::save() defers calling SkCanvas::save()
> 
> Which stack do you concern about: GraphicsContext::save() or SkCanvas::save()?

SkCanvas::save() is relatively lightweight. It does not worry me.
GraphicsContext::save() Pushes a rather large state object, and touches a large
number of attributes. That is the one I am concerned about

> 
> Currently, CanvasRenderingContext2D calls GraphicsContext::beginLayer() when
> it's really needed.
> The overhead which this CL made is pushing GraphicsContext states into
> GraphicsContext stack. SkCanvas states are already pushed when
> SkCanvas::saveLayer() is called.

Exactly, and it is the GraphicsContextState I am concerned about.

> Although copying GraphicsContextState is heavy, SkCanvas::saveLayer() is much
> heavier because it allocates new buffer.

I agree.

> So performance regression seems to be hardly noticeable. 
> 
> > Fore example, by saving state to local variables. I would find it easier
> > to support this change if we had a bitmask (or something like that) to
select
> > which part of the state we want to save instead of saving the whole thing
all
> > the time.
> 
> Could you explain more? I don't understand yet. Current
> GraphicsContext::beginLayer() slightly manages CTM and clip regions.
> As I understand, you suggests that we knows that GraphicsContext::beginLayer()
> manages CTM and composite operator differently and we will use it carefully,
> right?

I am suggesting having a bit mask (like in ancient OpenGL with glPushAttrib) so
that we can do partial state saves
Example:

context.save(CompositingState | MatrixState | ClipState);
...
context.restore();

This way, we could reduce a lot of overhead by saving only what we care about.

[f(malita), 2014-10-14 19:35:43]

On 2014/10/14 19:00:29, dshwang wrote:
> Thank you for review. Here's my questions.
> 
> On 2014/10/14 18:28:20, Florin Malita wrote:
> > I also think this should be a client fix: if state needs to be preserved, it
> > should be done via explicit save/restore calls.
> > 
> > The current beginLayer/endLayer API doesn't make any promises WRT state
> > preservation (there's a reason they're not named saveLayer/restoreLayer :)
> 
> This issue is originated from that GraphicsContext::beginLayer uses
> SkCanvas::saveLayer.
> When we call SkCanvas::saveLayer, it pushes all canvas states onto the stack
> such as CTM, clip regions, etc.
> So the current beginLayer/endLayer API makes promises CTM and clip states, but
> neither composite operator nor fill style nor etc.

The fact that endLayer() can mutate canvas state is an (unfortunate)
implementation side effect. The current GC API contract requires all clients
interested in preserving state to use explicit save/restore guards (or manage
state explicitly via setters/etc). Take a look at
beginFilterEffect/endFilterEffect for examples.

We could change the contract (and deal with the fallout), or you could update
the client to correctly guard state just like all other beginLayer/endLayer
users. I think the latter is easier to land.

[f(malita), 2014-10-14 19:48:58]

On 2014/10/14 19:21:44, junov wrote:
> I am suggesting having a bit mask (like in ancient OpenGL with glPushAttrib)
so
> that we can do partial state saves
> Example:
> 
> context.save(CompositingState | MatrixState | ClipState);

(note that partial matrix/clip saves have been removed from Skia, so those bits
are not independent.)

Medium/long term I would actually prefer to solve this by getting rid of (most
of) the GC paint state. The vast majority of those flags really don't belong in
GC: clients push *local* state to GC just to draw something and then immediately
pop. Ideally, all the local flags (stroke/fill/text params) should be taken out
and only properties which are truly global/inherited should be managed on GC
(global alpha, ?). Let clients pass local state via draw call params (akin to
Skia's paint):

  gc->drawRect(rect, paint)

instead of

  gc->save()
  (push paint properties to gc)
  gc->drawRect(rect)
  gc->restore()

which we do today all over the place.

[dshwang, 2014-10-14 20:47:23]

Thank you for fast response!

On 2014/10/14 19:21:44, junov wrote:
> To be more accurate, it is nothing else than CTM and clip.
> 
> SkCanvas::save() is relatively lightweight. It does not worry me.
> GraphicsContext::save() Pushes a rather large state object, and touches a
large
> number of attributes. That is the one I am concerned about

That's good information.
I made this CL under the assumption that SkCanvas::save() is much heavier than
GraphicsContext::save() itself.
However, true is opposite.

Now I think it's better to preserve the intended contract of
beginLayer/endLayer, which don't changes any states.
It's easily achieved in that GraphicsContext::endLayer() changes CTM and clip
after restoring. Let me try.

> > 
> > Could you explain more? I don't understand yet. Current
> > GraphicsContext::beginLayer() slightly manages CTM and clip regions.
> > As I understand, you suggests that we knows that
GraphicsContext::beginLayer()
> > manages CTM and composite operator differently and we will use it carefully,
> > right?
> 
> I am suggesting having a bit mask (like in ancient OpenGL with glPushAttrib)
so
> that we can do partial state saves
> Example:
> 
> context.save(CompositingState | MatrixState | ClipState);
> ...
> context.restore();
> 
> This way, we could reduce a lot of overhead by saving only what we care about.

It's good suggestion to optimize existing GraphicsContext::save() also.


On 2014/10/14 19:35:43, Florin Malita wrote:
> On 2014/10/14 19:00:29, dshwang wrote:
> > Thank you for review. Here's my questions.
> > 
> > On 2014/10/14 18:28:20, Florin Malita wrote:
> > > I also think this should be a client fix: if state needs to be preserved,
it
> > > should be done via explicit save/restore calls.
> > > 
> > > The current beginLayer/endLayer API doesn't make any promises WRT state
> > > preservation (there's a reason they're not named saveLayer/restoreLayer :)
> > 
> > This issue is originated from that GraphicsContext::beginLayer uses
> > SkCanvas::saveLayer.
> > When we call SkCanvas::saveLayer, it pushes all canvas states onto the stack
> > such as CTM, clip regions, etc.
> > So the current beginLayer/endLayer API makes promises CTM and clip states,
but
> > neither composite operator nor fill style nor etc.
> 
> The fact that endLayer() can mutate canvas state is an (unfortunate)
> implementation side effect. The current GC API contract requires all clients
> interested in preserving state to use explicit save/restore guards (or manage
> state explicitly via setters/etc). Take a look at
> beginFilterEffect/endFilterEffect for examples.
> 
> We could change the contract (and deal with the fallout), or you could update
> the client to correctly guard state just like all other beginLayer/endLayer
> users. I think the latter is easier to land.

I agree. I'll update this CL as you mentioned :)


On 2014/10/14 19:48:58, Florin Malita wrote:
> On 2014/10/14 19:21:44, junov wrote:
> > I am suggesting having a bit mask (like in ancient OpenGL with glPushAttrib)
> so
> > that we can do partial state saves
> > Example:
> > 
> > context.save(CompositingState | MatrixState | ClipState);
> 
> (note that partial matrix/clip saves have been removed from Skia, so those
bits
> are not independent.)
> 
> Medium/long term I would actually prefer to solve this by getting rid of (most
> of) the GC paint state. The vast majority of those flags really don't belong
in
> GC: clients push *local* state to GC just to draw something and then
immediately
> pop. Ideally, all the local flags (stroke/fill/text params) should be taken
out
> and only properties which are truly global/inherited should be managed on GC
> (global alpha, ?). Let clients pass local state via draw call params (akin to
> Skia's paint):
> 
>   gc->drawRect(rect, paint)
> 
> instead of
> 
>   gc->save()
>   (push paint properties to gc)
>   gc->drawRect(rect)
>   gc->restore()
> 
> which we do today all over the place.

I agree. Moreover, I don't understand why we don't remove GC after forking
blink. Why don't we use skia directly as WebGL uses WebGraphicsContext3D after
removing GraphicsContext3D?

[f(malita), 2014-10-14 21:34:24]

On 2014/10/14 20:47:23, dshwang wrote:
> 
> I agree. Moreover, I don't understand why we don't remove GC after forking
> blink. Why don't we use skia directly as WebGL uses WebGraphicsContext3D after
> removing GraphicsContext3D?

I think everyone agrees that it's a good idea to remove this friction, and we've
been gradually pushing GC closer to SkCanvas. But there is a significant
architectural baggage to deal with. Separating transient paint state is a big
piece of it, and the API surface overall is much larger than it needs to be IMO
(for the new paint system redesign in particular we could use a slimmer GC for
sure).

So yeah, I'm in violent agreement :) We need to keep pushing and eventually
we'll get there.

[dshwang, 2014-10-16 15:53:51]

Hi Florin and Justin, could you review again?
Now beginLayer()/endLayer() don't behave like save()/restore() as other
developers expect.

endLayer() preserves CTM and clip states which is everything GrahpicsContext
changes. In impl detail, endLayer() keeps current CTM and clip before calling
SkCanvas::restore() and sets the states to SkCanvas after calling
SkCanvas::restore().

[dshwang, 2014-10-16 15:58:20]

https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:257: }
Feel like using implementation detail of skia, but I cannot find a better way.
I hope new unittests in GraphicsContextTest is to catch it if clip
implementation of SkCanvas is updated incompatible to this CL.

https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:439: void restoreLayer();
go to private:

[Justin Novosad, 2014-10-16 17:51:58]

https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:220: SkClipStack
currentSkClipStack(*m_canvas->getClipStack());
Seems unfortunate to duplicate the entire clip stack when all you care about is
elements from m_clipStackPointersOfLayerStack.last() to the end.

https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.cpp:257: }
On 2014/10/16 15:58:20, dshwang wrote:
> Feel like using implementation detail of skia, but I cannot find a better way.
> I hope new unittests in GraphicsContextTest is to catch it if clip
> implementation of SkCanvas is updated incompatible to this CL.

I agree. reverse engineering the state seems unfortunate. Have you considered
making a change to skia? Perhaps adding a flavor of saveLayer that does not save
the matrix and clip?

https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
Source/platform/graphics/GraphicsContext.h:439: void restoreLayer();
On 2014/10/16 15:58:20, dshwang wrote:
> go to private:

Good idea

[dshwang, 2014-10-16 18:37:35]

dongseong.hwang@intel.com changed reviewers:
+ brian@chromium.org

[dshwang, 2014-10-16 18:38:00]

dongseong.hwang@intel.com changed reviewers:
- brian@chromium.org

[dshwang, 2014-10-16 18:40:01]

dongseong.hwang@intel.com changed reviewers:
+ bsalomon@google.com

[dshwang, 2014-10-16 18:44:59]

On 2014/10/16 17:51:58, junov wrote:
>
https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
> File Source/platform/graphics/GraphicsContext.cpp (right):
> 
>
https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
> Source/platform/graphics/GraphicsContext.cpp:220: SkClipStack
> currentSkClipStack(*m_canvas->getClipStack());
> Seems unfortunate to duplicate the entire clip stack when all you care about
is
> elements from m_clipStackPointersOfLayerStack.last() to the end.

That's good point. Done in new patch set.

>
https://codereview.chromium.org/651243002/diff/20001/Source/platform/graphics...
> Source/platform/graphics/GraphicsContext.cpp:257: }
> On 2014/10/16 15:58:20, dshwang wrote:
> > Feel like using implementation detail of skia, but I cannot find a better
way.
> > I hope new unittests in GraphicsContextTest is to catch it if clip
> > implementation of SkCanvas is updated incompatible to this CL.
> 
> I agree. reverse engineering the state seems unfortunate. Have you considered
> making a change to skia? Perhaps adding a flavor of saveLayer that does not
save
> the matrix and clip?

I didn't hardly consider of it, because SkCanvas::saveLayer() is enhance version
of save(), and SkCanvas uses restore() to escape the layer. There is not
SkCanvas::restoreLayer().
+bsalomon, what do you think about adding SkCanvas::beginLayer()/endLayer() not
to impact state stack?

FYI, both options has its own cons;
- this CL: feels reverse engineering
- adding new api on SkCanvas: feels bloating api

[Stephen White, 2014-10-16 18:55:27]

I'm not fond of the current patch, and I agree with Florin that this should be
fixed at the call site.

For now, could we simply document the current behaviour of beginLayer/endLayer()
(ie., that they
will save/restore the CTM and clip, but nothing else?) If we eventually move to
eliminating 
GraphicsContext in favour of using Skia directly, those are the semantics we'll
be using 
(that saveLayer()/restore() provides), so we'd have to fix it at the callsite
anyway, no?

[Justin Novosad, 2014-10-16 19:16:05]

And if you do want to inspect the content of a clip stack, you should not do it
directly. SkClipStack::Element and Iter are not exported. That is why the
component builds are failing on the bots. You could use SkCanvas::replayClips.
But this whole approach seems backwards.  Can't we just work with the knowledge
that beginLayer/endLayer() affect the matrix and clip, and make the call sites
deal with that correctly?

[dshwang, 2014-10-16 19:31:10]

On 2014/10/16 18:55:27, Stephen White wrote:
> I'm not fond of the current patch, and I agree with Florin that this should be
> fixed at the call site.
> 
> For now, could we simply document the current behaviour of
beginLayer/endLayer()
> (ie., that they
> will save/restore the CTM and clip, but nothing else?) If we eventually move
to
> eliminating 
> GraphicsContext in favour of using Skia directly, those are the semantics
we'll
> be using 
> (that saveLayer()/restore() provides), so we'd have to fix it at the callsite
> anyway, no?

Ok, I agree. It would be the best solution to comment this issue until removing
GraphicsContext.

On 2014/10/16 19:16:05, junov wrote:
> And if you do want to inspect the content of a clip stack, you should not do
it
> directly. SkClipStack::Element and Iter are not exported.

Interesting. linux component build succeeds while win fails.

> That is why the
> component builds are failing on the bots. You could use SkCanvas::replayClips.

We cannot use replayClips() also because SkCanvasClipVisitor is not exposed.

> But this whole approach seems backwards.  Can't we just work with the
knowledge
> that beginLayer/endLayer() affect the matrix and clip, and make the call sites
> deal with that correctly?

The conclusion would be that call sites uses it carefully. Currently we don't
have a bug about it. I just submitted this CL because code looks wrong.

[dshwang, 2014-10-16 19:40:36]

Now this CL just adds comments explaining CTM and clip.
Could you review again?

[Justin Novosad, 2014-10-17 16:10:49]

On 2014/10/16 19:40:36, dshwang wrote:
> Now this CL just adds comments explaining CTM and clip.
> Could you review again?

lgtm

Sorry about all the back and forth we did in this code review.

[f(malita), 2014-10-17 16:55:34]

lgtm++

[f(malita), 2014-10-17 16:55:44]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-10-17 16:56:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651243002/60001

[commit-bot: I haz the power, 2014-10-17 17:00:50]

Committed patchset #4 (id:60001) as 183902

[dshwang, 2014-10-17 20:31:12]

On 2014/10/17 16:10:49, junov wrote:
> On 2014/10/16 19:40:36, dshwang wrote:
> > Now this CL just adds comments explaining CTM and clip.
> > Could you review again?
> 
> lgtm
> 
> Sorry about all the back and forth we did in this code review.

On 2014/10/17 16:55:34, Florin Malita wrote:
> lgtm++

Thank you for lgtm! Justin, no problem. Now I know how to manipulate clip stack
:)