Augmenting SkDeferredCanvas notification interface to signal flushes and recorded commands

Augmenting SkDeferredCanvas notification interface to signal flushes and memory allocations
Renamed SkDeferredCanvas::DeviceContext to SkDeferredCanvas::NotificationClient

BUG=http://code.google.com/p/chromium/issues/detail?id=136828
TEST=Added coverage for new API to DeferredCanvas unit test. Added DeferredCanvas bench test to track deferred canvas overhead cost.
Committed: https://code.google.com/p/skia/source/detail?r=5078

[reed1, 2012-08-13 15:09:41]

I like the general formulation of a notification client better than the previous
models. thanks.

https://codereview.appspot.com/6442108/diff/1001/include/utils/SkDeferredCanv...
File include/utils/SkDeferredCanvas.h (right):

https://codereview.appspot.com/6442108/diff/1001/include/utils/SkDeferredCanv...
include/utils/SkDeferredCanvas.h:40: *  @param client Interface for dispatching
notifications
Does the (deferred)canvas take a ref on the client? Same (documentation)
question for setNotificationClient().

Can we only have setNotificationClient()? We are trying to hide the notion of
devices from the public SkCanvas API (i.e. surfaces, etc.) so it would be a tiny
bit easier if we didn't encourage this form of the constructor.

https://codereview.appspot.com/6442108/diff/1001/include/utils/SkDeferredCanv...
include/utils/SkDeferredCanvas.h:183: 
Wow. What is the performance impact for calling 2 virtuals on every draw
command? It may be required, but I'm curious what the cost is.

https://codereview.appspot.com/6442108/diff/1001/include/utils/SkDeferredCanv...
include/utils/SkDeferredCanvas.h:186: */
Is this called before every draw, or just before the flushPending?

[junov1, 2012-08-13 15:39:15]

On 2012/08/13 15:09:41, reed1 wrote:
> 
> Can we only have setNotificationClient()? We are trying to hide the notion of
> devices from the public SkCanvas API (i.e. surfaces, etc.) so it would be a
tiny
> bit easier if we didn't encourage this form of the constructor.

Sounds good, but can I do that in a follow-up patch? I need to make changes on
the webkit side so that it can swallow that.  It would be easier to remove this
at the same time as cleaning up the DeviceContext bootstraps.

> 
>
https://codereview.appspot.com/6442108/diff/1001/include/utils/SkDeferredCanv...
> include/utils/SkDeferredCanvas.h:183: 
> Wow. What is the performance impact for calling 2 virtuals on every draw
> command? It may be required, but I'm curious what the cost is.

I'll check it out.  If the impact is measurable, I was thinking I could reduce
the frequency of the calls by adding rule.  Something like:

if (0 == fRecordedCommandCount++ % X || bitmapWasDeepCopied)

>
https://codereview.appspot.com/6442108/diff/1001/include/utils/SkDeferredCanv...
> include/utils/SkDeferredCanvas.h:186: */
> Is this called before every draw, or just before the flushPending?

Just before the flushPending

[reed1, 2012-08-13 18:18:02]

On 2012/08/13 15:39:15, junov1 wrote:
> On 2012/08/13 15:09:41, reed1 wrote:
> > 
> > Can we only have setNotificationClient()? We are trying to hide the notion
of
> > devices from the public SkCanvas API (i.e. surfaces, etc.) so it would be a
> tiny
> > bit easier if we didn't encourage this form of the constructor.
> 
> Sounds good, but can I do that in a follow-up patch? I need to make changes on
> the webkit side so that it can swallow that.  It would be easier to remove
this
> at the same time as cleaning up the DeviceContext bootstraps.

Sure.

> 
> > 
> >
>
https://codereview.appspot.com/6442108/diff/1001/include/utils/SkDeferredCanv...
> > include/utils/SkDeferredCanvas.h:183: 
> > Wow. What is the performance impact for calling 2 virtuals on every draw
> > command? It may be required, but I'm curious what the cost is.
> 
> I'll check it out.  If the impact is measurable, I was thinking I could reduce
> the frequency of the calls by adding rule.  Something like:
> 
> if (0 == fRecordedCommandCount++ % X || bitmapWasDeepCopied)
> 

Makes sense. Can you add such a measurement in this or a follow-on CL?

> >
>
https://codereview.appspot.com/6442108/diff/1001/include/utils/SkDeferredCanv...
> > include/utils/SkDeferredCanvas.h:186: */
> > Is this called before every draw, or just before the flushPending?
> 
> Just before the flushPending

My question sprang from your comment in the header. Can you update that comment?

[junov1, 2012-08-13 21:30:04]

New Patch. Addressed all comments.

Regarding the added virtual call per draw op:
-I added a bench test that captures basic SkDeferredCanvas recording overhead.
-I found there the performance impact of the added virtual call was barely
measurable (~0.3%).  Still nice to avoid if possible.
-I tried a few things, and finally opted to reduce the frequency of the virtual
call by changing its semantics: drawCommandRecorded became
storageAllocatedForRecordingChanged, which is what we really need to track in
webkit's notification client.  This way, the notification does not happen that
frequently. It happens when a new mutable bitmap snapshot is taken, and when the
pipe controller allocates a new 16k chunk.  A lot of draw commands can fit in a
16k chunk!

[reed1, 2012-08-14 13:17:33]

nice refinement.
lgtm