Canvas variant for deferred drawing

This CL adds new class SkDeferredCanvas.  This is a subclass of SkCanvas that can be used for deferred drawing.  The main difference between this class and SkPictureRecord is that SkPictureRecord only supports write operations. SkDeferredCanvas encapsulates SkPicture in a way that makes it readable as well as writable, and supports direct access to the device.  It is a full featured subclass of SkCanvas, so it can be dropped-in anywhere an SkCanvas is used. It also encapsulates a concrete canvas and device, used for flushing when necessary. The creation of the concrete device is also deferred by using a factory pattern for lazy initialization.

The motivation behind the change is that it creates important optimization opportunities. One optimization that was implemented in this CL is the purging of pending draw operations when a full-canvas opaque draw operation is performed.  Additional optimizations made possible by this class will be implemented in the skia port of WebKit.

Patch 1 is in an unfinished state, so don't be too nit picky about small details or the lack of comments. I am submitting this to get a preliminary review. Please focus on design.

One comment about the design: I though it preferable to submit this code to skia rather than webkit because I am subclassing SkDevice, which we agreed should no longer be subclassed outside of skia. That being said, it would be pretty straightforward to submit this to WebKit instead.

[junov1, 2011-11-23 22:46:40]

PTAL

[reed1, 2011-11-28 14:24:00]

Can we break this into smaller CLs?

SkCanvas.h seems like it can be stand alone.
- why need restoreToCount be virtual? It calls virtual restore()...

[reed1, 2011-11-28 14:24:44]

SkDevice.cpp
- is this change a necessary part of the larger change?
- What is it fixing?

[junov1, 2011-11-28 15:49:02]

On 2011/11/28 14:24:00, reed1 wrote:
> Can we break this into smaller CLs?
> 
> SkCanvas.h seems like it can be stand alone.
> - why need restoreToCount be virtual? It calls virtual restore()...

We need it to be virtual because it calls fMCStack.count(), which is the wrong
count. In this, we need to bootstrap the fMCStack of the recording canvas.  An
alternative would be to replace the call to fMCStack.count() with a call to
getSaveCount, which is now virtual.  That way, restoreToCount would not need to
be virtual.

[junov1, 2011-11-28 15:55:19]

On 2011/11/28 14:24:44, reed1 wrote:
> SkDevice.cpp
> - is this change a necessary part of the larger change?
> - What is it fixing?

Yes. Flagging the backing store bitmaps of devices as volatile is necessary for
canvas to canvas copies to work properly.  When a bitmap draw occurs, if the
source image is flagged as volatile, it signals to SkDeferredCanvas, that it can
not count on the contents of the bitmap to remain unchanged, which means that
the draw operation can not be deferred without making a copy of the image data.

[reed1, 2011-11-28 16:11:20]

On 2011/11/28 15:55:19, junov1 wrote:
> On 2011/11/28 14:24:44, reed1 wrote:
> > SkDevice.cpp
> > - is this change a necessary part of the larger change?
> > - What is it fixing?
> 
> Yes. Flagging the backing store bitmaps of devices as volatile is necessary
for
> canvas to canvas copies to work properly.  When a bitmap draw occurs, if the
> source image is flagged as volatile, it signals to SkDeferredCanvas, that it
can
> not count on the contents of the bitmap to remain unchanged, which means that
> the draw operation can not be deferred without making a copy of the image
data.

I think this change is too broad, for all raster devices to be marked volatile.
We call notifyPixelsChanged() on each draw to the device, so perhaps you can use
the generationID or some other signal.

[reed1, 2011-11-28 16:11:56]

On 2011/11/28 15:49:02, junov1 wrote:
> On 2011/11/28 14:24:00, reed1 wrote:
> > Can we break this into smaller CLs?
> > 
> > SkCanvas.h seems like it can be stand alone.
> > - why need restoreToCount be virtual? It calls virtual restore()...
> 
> We need it to be virtual because it calls fMCStack.count(), which is the wrong
> count. In this, we need to bootstrap the fMCStack of the recording canvas.  An
> alternative would be to replace the call to fMCStack.count() with a call to
> getSaveCount, which is now virtual.  That way, restoreToCount would not need
to
> be virtual.

Ah, good point.
Just landed SkCanvas.cpp change to call getSaveCount() instead of
fMCStack.count()

[junov1, 2011-12-06 14:49:17]

Patch 2 uploaded: using observer pattern to receive notifyPixelsChanged signal
from pixelref objects.

This CL now depends on the following changes:
http://codereview.appspot.com/5449094/ (SkPixelRef)
http://codereview.appspot.com/5451102/ (SkShader)
http://codereview.appspot.com/5450102/ (SkCanvas)

[reed1, 2011-12-06 15:01:29]

I like the attempt at factoring out separable changes into different CLs, and
some of that has paid off. However, I think some are easier to justify/review if
they are combined. Specifically, the new APIs on pixelref and canvas (for
drawiter) only make sense to review in the context of seeing the proposed
caller/subclass.

Of the outstanding separate CLs, I think the shader:isOpaque can stand alone,
but I think the others should be combined into this one.

Looking at all of these, I think we should also insist on using SK_OVERRIDE for
the new methods in subclasses, as it would have caught at least one oversight
(i.e. restoreToCount).

http://codereview.appspot.com/5430058/diff/7001/include/core/SkDeferredCanvas.h
File include/core/SkDeferredCanvas.h (right):

http://codereview.appspot.com/5430058/diff/7001/include/core/SkDeferredCanvas...
include/core/SkDeferredCanvas.h:28: class DeviceFactory;
Skia convention is to use INHERITED rather than ParentClass, and (typically) to
make that private. Does it need to be public here?

http://codereview.appspot.com/5430058/diff/7001/include/core/SkDeferredCanvas...
include/core/SkDeferredCanvas.h:36: explicit SkDeferredCanvas(SkDevice* device);
What is the use and ownership rules for the factory parameter?

http://codereview.appspot.com/5430058/diff/7001/include/core/SkDeferredCanvas...
include/core/SkDeferredCanvas.h:42: virtual SkDevice* setDevice(SkDevice*);
Need dox for setDeviceFactory. What is it used for and when? What are the
ownership rules for the value passed in and the return value?

http://codereview.appspot.com/5430058/diff/7001/include/core/SkDeferredCanvas...
include/core/SkDeferredCanvas.h:44: 
Need dox

http://codereview.appspot.com/5430058/diff/7001/include/core/SkDeferredCanvas...
include/core/SkDeferredCanvas.h:46: 
For all of these overrides (can you add SK_OVERRIDE) do you think we should
propagate the default arguments? Seems better to me to not do it, in case they
ever diff from the base class.

http://codereview.appspot.com/5430058/diff/7001/include/core/SkDeferredCanvas...
include/core/SkDeferredCanvas.h:51: virtual int getSaveCount() const;
restoreToCount is not virtual

[junov1, 2011-12-08 15:53:58]

Done.

[junov1, 2011-12-08 18:49:39]

On 2011/12/08 15:53:58, junov1 wrote:
> Done.

New patch corrects style issues, mostly 80 col and braces.  Also fixed duplicate
concatenated code in new files SkDeferredCanvas.*

[reed1, 2011-12-08 21:03:18]

Question:

Is the addition of notifiers just to make Pictures behave correctly when
drawBitmap is called on them (to get the correct/old bits if the bitmap is later
changed), or are notifiers also intrinsically needed for SkDeferredCanvas?

http://codereview.appspot.com/5430058/diff/12001/src/core/SkDeferredCanvas.cpp
File src/core/SkDeferredCanvas.cpp (right):

http://codereview.appspot.com/5430058/diff/12001/src/core/SkDeferredCanvas.cp...
src/core/SkDeferredCanvas.cpp:466: {
raster device calls notifyPixelsChanged before *every* draw. Do you need to do
all of these for each of these, or can you set some sort of dirty flag to know
when it is required?

[reed1, 2011-12-08 21:06:20]

Maintaining the list of notifiers in SkPixelRef is not thread-safe. SkPixelRefs
love to live in more than one thread. This means the call into the notifier
object can happen in different threads, so the notifier itself needs to be
thread-safe, and it means that when the notifier installs or removes itself, the
pixelref will need to be thread-safe.

[junov1, 2011-12-09 15:14:03]

On 2011/12/08 21:03:18, reed1 wrote:
> Question:
> 
> Is the addition of notifiers just to make Pictures behave correctly when
> drawBitmap is called on them (to get the correct/old bits if the bitmap is
later
> changed), or are notifiers also intrinsically needed for SkDeferredCanvas?
> 
It is for drawBitmap. The most frequent use case is a canvas to canvas copy. 
For example, if you record a deferred draw from canvas1 to canvas2, then we need
to either flush the deferred draw command or save a copy of canvas1 before the
drawing to canvas1 again.  The current implementation in this patch triggers a
flush.  

> http://codereview.appspot.com/5430058/diff/12001/src/core/SkDeferredCanvas.cpp
> File src/core/SkDeferredCanvas.cpp (right):
> 
>
http://codereview.appspot.com/5430058/diff/12001/src/core/SkDeferredCanvas.cp...
> src/core/SkDeferredCanvas.cpp:466: {
> raster device calls notifyPixelsChanged before *every* draw. Do you need to do
> all of these for each of these, or can you set some sort of dirty flag to know
> when it is required?

With the current implementation in this patch, SkDeferredCanvas will deregister
as an observer when it performs a flush by calling clearPixelRefDependencies. 
SkDeferredSanvas never receives notifications that are unnecessary.

There is one important exception though, which is causing unnecessary
notifications: setting LCD text filtering.  I will log a separate bug for this. 
The issue is in WebKit. Function isCanvasMultiLayered in SkiaFontWin.cpp creates
a layer iterator for the sole purpose of verifying the existence of multiple
layers. The iterator assumes that it is used for drawing, and therefore makes a
call to SkDevice::accessBitmap, which triggers a notification.  This is
currently causing unnecessary texture cache invalidations.

[junov1, 2012-01-12 19:23:14]

As discussed by e-mail, I have reduced the scope of the change. The problem of
handling the deferral of draw operations involving writable bitmaps can be
resolved later in a separate change.  The latest patch still delivers extremely
useful performance improvements for Chrome, with very few changes to existing
API.

[reed1, 2012-01-12 19:48:05]

What would it take to not make SkDevice.h aware of this new subclass (friend)?
As the core never instantiates this subclass, it would be nice to be able to
move it into utils/ or effects/ or something like that.

http://codereview.appspot.com/5430058/diff/23001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/5430058/diff/23001/include/core/SkCanvas.h#newc...
include/core/SkCanvas.h:926: // need to overload this method.
// The impl must keep track of this, as it is not released or deleted by the
caller.

http://codereview.appspot.com/5430058/diff/23001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (left):

http://codereview.appspot.com/5430058/diff/23001/src/core/SkCanvas.cpp#oldcod...
src/core/SkCanvas.cpp:216: SkDrawIter(SkCanvas* canvas, bool skipEmptyClips =
true) {
Is this change equivalent to just adding

    canvas = canvas->canvasForDrawIter();

before the rest of the (old) code?

[junov1, 2012-01-12 21:33:49]

On 2012/01/12 19:48:05, reed1 wrote:
> What would it take to not make SkDevice.h aware of this new subclass (friend)?

Just checked... it is actually not required. I think this is a remnant of the
old observer pattern implementation which we backtracked on.

> 
> http://codereview.appspot.com/5430058/diff/23001/include/core/SkCanvas.h
> File include/core/SkCanvas.h (right):
> 
>
http://codereview.appspot.com/5430058/diff/23001/include/core/SkCanvas.h#newc...
> include/core/SkCanvas.h:926: // need to overload this method.
> // The impl must keep track of this, as it is not released or deleted by the
> caller.

I think this change does not make the code any less safe than it was before in
terms of lifetime management.  I'm not sure I totally understand your angle
here. Do you feel SkDrawIter should ref/unref the canvas?

> Is this change equivalent to just adding
> 
>     canvas = canvas->canvasForDrawIter();
> 
> before the rest of the (old) code?

Yes.

[reed1, 2012-01-12 21:46:21]

I would like the comment, just to make it clear this method is not a factory,
that's all.

I'd like to move this subclass into utils. To do that I think we should also
unfriend it from SkDevice.h

[reed1, 2012-01-12 21:47:49]

http://codereview.appspot.com/5430058/diff/23001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (left):

http://codereview.appspot.com/5430058/diff/23001/src/core/SkCanvas.cpp#oldcod...
src/core/SkCanvas.cpp:216: SkDrawIter(SkCanvas* canvas, bool skipEmptyClips =
true) {
Lets recode this as just a single new line. Makes the diff much easier to read,
and can possibly make the code faster, since the compiler won't consider
reloading fCanvas' value after each call.

On 2012/01/12 19:48:05, reed1 wrote:
> Is this change equivalent to just adding
> 
>     canvas = canvas->canvasForDrawIter();
> 
> before the rest of the (old) code?

[junov1, 2012-01-16 16:30:45]

Done.

[reed1, 2012-01-17 14:29:24]

http://codereview.appspot.com/5430058/diff/29001/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

http://codereview.appspot.com/5430058/diff/29001/src/utils/SkDeferredCanvas.c...
src/utils/SkDeferredCanvas.cpp:626: flushPending();
OMG -- this looks way too tricky. Can we avoid this sort of thing by changing
the top API on canvas to access the bitmap, so it can be redirected to the
proper device?

Should we change the API to return a copy of the device's bitmap (so there can
be no question of retaining counts?)

Are we unable to return the correct device in the first place? Does something
else need to be virtual?

[junov1, 2012-01-17 15:11:46]

This trickery, along with the whole factory pattern is all so that we can defer
the construction of the SkGpuDevice along with the allocation of the backing
store.  Let me rip that all out, it is not where the main performance benefit
lies anyways.  Later, I will analyze the performance benefit of that improvement
in isolation.

On 2012/01/17 14:29:24, reed1 wrote:
>
http://codereview.appspot.com/5430058/diff/29001/src/utils/SkDeferredCanvas.cpp
> File src/utils/SkDeferredCanvas.cpp (right):
> 
>
http://codereview.appspot.com/5430058/diff/29001/src/utils/SkDeferredCanvas.c...
> src/utils/SkDeferredCanvas.cpp:626: flushPending();
> OMG -- this looks way too tricky. Can we avoid this sort of thing by changing
> the top API on canvas to access the bitmap, so it can be redirected to the
> proper device?
> 
> Should we change the API to return a copy of the device's bitmap (so there can
> be no question of retaining counts?)
> 
> Are we unable to return the correct device in the first place? Does something
> else need to be virtual?

[junov1, 2012-01-17 21:12:23]

Latest patch gets rid of DeviceFactory in favor of a much simpler DeviceContext
class.

[reed1, 2012-01-17 21:38:22]

Wow, much smallerer. LGTM on the SkCanvas changes for sure.

Just a small question on the DeferredCanvas, about shadowing a
base-virtual-method.

http://codereview.appspot.com/5430058/diff/23015/include/utils/SkDeferredCanv...
File include/utils/SkDeferredCanvas.h (right):

http://codereview.appspot.com/5430058/diff/23015/include/utils/SkDeferredCanv...
include/utils/SkDeferredCanvas.h:66: */
May not be clean to overload the name setDevice w/ one version being virtual and
the other not.

Does this method need to be public? (just curious)

[junov1, 2012-01-17 21:52:18]

On 2012/01/17 21:38:22, reed1 wrote:
> May not be clean to overload the name setDevice w/ one version being virtual
and
> the other not.
> 
> Does this method need to be public? (just curious)

The method does need to be public. But I think I know what to do: I can keep
just the virtual single arg version of the method, and add separate
setDeviceContext method.

[reed1, 2012-01-17 21:57:09]

On 2012/01/17 21:52:18, junov1 wrote:
> On 2012/01/17 21:38:22, reed1 wrote:
> > May not be clean to overload the name setDevice w/ one version being virtual
> and
> > the other not.
> > 
> > Does this method need to be public? (just curious)
> 
> The method does need to be public. But I think I know what to do: I can keep
> just the virtual single arg version of the method, and add separate
> setDeviceContext method.

SGTM

[junov1, 2012-01-17 22:36:35]

New adds setDeviceContext, and remove the 2 argument version of setDevice.

[junov1, 2012-01-17 22:41:59]

Additional minor correction in patch 11

[reed1, 2012-01-18 13:15:25]

Will be valuable to test for memory leaks just in case, given all of the extra
objects holding objects.

lgtm

[junov1, 2012-01-18 16:11:53]

On 2012/01/18 13:15:25, reed1 wrote:
> Will be valuable to test for memory leaks just in case, given all of the extra
> objects holding objects.
> 
> lgtm

Valgrind on chromium says we're good.