Making clipping accessor methods virtual in SkCanvas to fix deferred canvas

[junov1, 2012-02-24 15:46:10]

PTAL

[reed1, 2012-02-24 16:07:45]

I think this is a bad trend for performance -- making query methods on SkCanvas
become virtual. This makes them a little slower to call, and it means we cannot
inline them in the future.

For example: were are in the midst of measuring the win of making quickRejectY
inline for android. If all of the clip query methods need to become virtual,
then this one will as well, eliminating our ability to make it inline.

As another approach, consider what SkPictureRecording does. For all matrix and
clip functions (the mutating methods which are already virtual), it sends those
to its inherited (i.e. SkCanvas) and then also sends those to its recorder. In
this way the base class sees the current clip/region/matrix state, so that all
of the non-virtual query methods still work. Can we do that fir DeferredCanvas
as well?

[junov1, 2012-02-24 18:08:52]

Ok, I can do that.  In the case of SkDeferredCanvas, that will mean the
state will be entirely duplicated, which seems wasteful to me, but I guess
it is worth it to avoid imposing an unnecessary burden on all the other
canvas use cases that do not care about deferred canvas. I will try that
out.

On Fri, Feb 24, 2012 at 11:07 AM, <reed@google.com> wrote:

> I think this is a bad trend for performance -- making query methods on
> SkCanvas become virtual. This makes them a little slower to call, and it
> means we cannot inline them in the future.
>
> For example: were are in the midst of measuring the win of making
> quickRejectY inline for android. If all of the clip query methods need
> to become virtual, then this one will as well, eliminating our ability
> to make it inline.
>
> As another approach, consider what SkPictureRecording does. For all
> matrix and clip functions (the mutating methods which are already
> virtual), it sends those to its inherited (i.e. SkCanvas) and then also
> sends those to its recorder. In this way the base class sees the current
> clip/region/matrix state, so that all of the non-virtual query methods
> still work. Can we do that fir DeferredCanvas as well?
>
>
http://codereview.appspot.com/**5697052/<http://codereview.appspot.com/5697052/>
>

[reed1, 2012-02-24 18:46:33]

The 'duplicated' state (which is only a matrix/clip per save) also means
DeferredCanvas will be faster when anyone calls those query methods...

[junov1, 2012-02-24 19:06:52]

Here we go. The latest patch does the trick.

[reed1, 2012-02-24 19:49:36]

lgtm with possible name change: clipLayerBounds actually isn't really about
layers per-se.

clipRectBounds(...)

Indicating that its like clipRect, but it takes the conservative bounds of the
transformed rect (e.g. a rotated rect becomes another rect, not a polygon).

[junov1, 2012-02-24 21:30:13]

Uploaded new patch. 
Note: There is some diff because I rebased my working copy.  The only changes
since patch 2 are:
* the rename to clipRectBounds
* SkCanvas.cpp, line 1060

That second change was necessary to prevent a re-entry in SkPicture that was
corrupting the recording.  The problem had to do with the call to accessBitmap
causing a deferred canvas flush while recording a canvas operation.  Fix was
very simple since all we needed from the bitmap was its size, so no need for
pixel access.

[reed1, 2012-02-24 22:10:46]

definitely think getDevice is lighter-weight than accessBitmap!

lgtm