Bug 68245 – [Chromium] Use SkPicture to defer 2d canvas draw operation and run them in batches

• Component: Canvas
• Status: RESOLVED WONTFIX

[Justin Novosad]

Make canvas drawing asynchronous by using SkPicture.
The purpose of this change is to improve performance by:
- batching rendering work.
- Deferring the graphics context creation, and in many cases reuse the compositor's context, so creating a new context is avoided.
- Automatic switch to the software canvas implementation when the first synchronous operation is a readback.

This improvement will also open the way for other future improvements (to be submitted separately)
- Skip the rendering of undisplayed frames (ï.e. when frame rate > video refresh rate)
- Eliminate the rendering of out of view canvases.

[Justin Novosad]

Created attachment 107658 [details]
Work in progress (Do not submit to EWS).

This is a preliminary patch.  It is not quite functional yet, does not build on all platforms, was not edited for style.
The purpose of this attachment is to get early feedback on the design.

[Vangelis Kokkevis]

(In reply to comment #1)
> Created an attachment (id=107658) [details]
> Work in progress (Do not submit to EWS).
> 
> This is a preliminary patch.  It is not quite functional yet, does not build on all platforms, was not edited for style.
> The purpose of this attachment is to get early feedback on the design.

I think you forgot to add the SkiaAsynchronousCanvas files into the patch!

[Stephen White]

Looks like a good start.  I like the writeOnlyCanvas() vs readableCanvas():  that makes it really clear what a caller intends to do.  Out of curiosity, does reading from a writeOnlyCanvas() generate an error/assertion?  Might be nice, if not.

While we evaluate the perf and correctness, is there a way this could be done behind a runtime switch?  Failing that, a compile-time switch would be better than nothing. Otherwise, you'll have to be maintaining this patch outside of the tree and handling conflicts yourself until we decide which way to go.

Some of the names are a little confusing:  is CanvasContext referring to 2D canvas?  Are we always going to want to use that type strictly for 2D Canvas, or will we use it in the future for other rendering?  If it refers to canvas in the SkCanvas sense, I guess that's ok.  It's just that we already have a plethora of Contexts and Canvases, so it'd be nice to be clear about the naming.  (Similarly, "doRender()" is a pretty meaningless name.)

Come to think of it, since this class is only used by 2D canvas, and not CG-Mac, you can probably safely use SkiaAsynchronousCanvas directly.  Although rather than adding another dependency on a skia-ism, another option would be to leave the PlatformLayer as-is and use a callback to flush the rendering.  Just an idea.  Finally, you should probably set this CanvasContext to NULL in the ImageBuffer destructor, so that it isn't hanging onto a stale ptr (the layer will live on past the destruction of ImageBuffer until the next drawing call).

Also, I noticed you modified LayerTextureUpdaterCanvas.  IIRC, this class is used by --enable-accelerated-rendering in Chrome (correct me if I'm wrong).  Will this change have any impact on that code, or is it transparent?

FYI, makeGrContextCurrent() has been removed by Brian's change in http://trac.webkit.org/changeset/95192, so you may have some conflicts once you update past that.

Finally, it would be great to have a design doc which explains the functionality.  I'm not clever enough to grok the full impact from a patch, I'm afraid.  Why does readableCanvas() sometimes take an acceleration hint, and sometimes not, for example?

[Stephen White]

Oh yes, and please add SkiaAsynchronousCanvas.cpp/.h, that'll aid in understanding too.  :)

[Justin Novosad]

(In reply to comment #3)
> Looks like a good start.  I like the writeOnlyCanvas() vs readableCanvas():  that makes it really clear what a caller intends to do.  Out of curiosity, does reading from a writeOnlyCanvas() generate an error/assertion?  Might be nice, if not.

Yeah, I thought of implementing a security check, but it is kinda tricky and would add too much overhead (like making the interface for SkCanvas virtual)  With the current design, you get this behavior: If you attempt to read from a writeOnlyCanvas, it might work if it just so happens that the SynchronousOnly flag is turned on inside SkiaAsynchronousCanvas.  When that is not the case, you will get a null bitmap back when querying the device bitmap.  Most of the cases I encountered so far asserted when that happened or crashed in SkBitmap::getAddr.

> 
> While we evaluate the perf and correctness, is there a way this could be done behind a runtime switch?  Failing that, a compile-time switch would be better than nothing. Otherwise, you'll have to be maintaining this patch outside of the tree and handling conflicts yourself until we decide which way to go.
>

What we can do is make a runtime switch that that forces SkiaAsynchronousCanvas into SynchronousOnly mode.  When that happens, SkiaAsynchronousCanvas becomes nothing more than a container for SkCanvas.  There would still be a few minor differences with the previous implementation, in particular the canvas resizing and initialization code have small changes, but nothing scarry. The code was mostly copied from ImageBuffer.cpp
 
> Also, I noticed you modified LayerTextureUpdaterCanvas.  IIRC, this class is used by --enable-accelerated-rendering in Chrome (correct me if I'm wrong).  Will this change have any impact on that code, or is it transparent?

It is transparent. The SkiaAsynchronousCanvas object just wraps the SkPicture in this case.

> Finally, it would be great to have a design doc which explains the functionality.  I'm not clever enough to grok the full impact from a patch, I'm afraid.  Why does readableCanvas() sometimes take an acceleration hint, and sometimes not, for example?

Definately, once the design is settled a bit more... Do you think I should document this in the source code?  I like doing that, but apparently it is not the norm in WebKit?

[Vangelis Kokkevis]

> > Finally, it would be great to have a design doc which explains the functionality.  I'm not clever enough to grok the full impact from a patch, I'm afraid.  Why does readableCanvas() sometimes take an acceleration hint, and sometimes not, for example?
> 
> Definately, once the design is settled a bit more... Do you think I should document this in the source code?  I like doing that, but apparently it is not the norm in WebKit?

It would really also help with the review if you could write a high-level description of how the new code works and what's different. The diffs are somewhat spread out so I'm having trouble getting the big picture.  Adding the two missing files will help as well!

[Matthew Delaney]

I haven't looked at the patch or surrounding code enough to fully understand the mechanics here, but if the general intent is to make canvas drawing async (presumably entirely on its own thread) then isn't this something that would be best done cross-platform? People have long suggested in WebKit to put canvas drawing on it own thread, display lists, etc. and we'd certainly be interested in bringing this to all WebKit clients.

[Justin Novosad]

(In reply to comment #7)
> I haven't looked at the patch or surrounding code enough to fully understand the mechanics here, but if the general intent is to make canvas drawing async (presumably entirely on its own thread) then isn't this something that would be best done cross-platform? People have long suggested in WebKit to put canvas drawing on it own thread, display lists, etc. and we'd certainly be interested in bringing this to all WebKit clients.

What we are doing is queuing high-level rendering commands in a sort
of display list so that we can execute them later in batches. There is
no threading involved here. Because of the way skia and the skia
integration are designed, doing this allows us to take advantage of
some platform-specific optimizations.  The display list functionality
we are using is a feature of Skia.  Therefore, changes we intend to
contribute to webkit wouldn't be of much benefit to other platforms.

[Justin Novosad]

(In reply to comment #6)
> It would really also help with the review if you could write a high-level description of how the new code works and what's different. The diffs are somewhat spread out so I'm having trouble getting the big picture.  Adding the two missing files will help as well!

Ok, so here is the basic idea behind SkiaAsynchronousCanvas.  This class encapsulates SkCanvas.  Whenever an SkCanvas object is needed to perform a draw operation to a 2D canvas, SkiaAsynchronousCanvas is responsible for providing the SkCanvas object through SkiaAsynchronousCanvas::canvas().  Internally, SkiaAsynchronousCanvas stores an SkCanvas and an SkPicture.  The SkCanvas can use either a GPU-based rendering device, or CPU-based.  The SkPicture is capable of providing an alternate SkCanvas that records draw commands instead of rendering them.

The canvas method method has an accessMode argument.  There are four access modes:
-Asynchronous : returns a canvas object that will record draw operation without executing them.  This is the SkCanvas provided by the SkPicture object.
-Synchronous : returns a canvas object that executes draw operations immediately, any previously recorded commands are executed immediately. This access mode may trigger the lazy initialization of the SkCanvas with a GPU-based rendering device if the Acceleration flags is set
-ForReadback : returns a canvas object that that has a readable image buffer. This access mode may trigger the lazy initialization of the SkCanvas, in which case the Acceleration flag is ignored and a CPU-base rendering device is used. Returns NULL if the AsynchronousOnly flag is set
-ForGpuReadback : Like 'Synchronous', but returns NULL if the AsynchronousOnly flags is set.

The AsynchronousOnly flag indicates that the SkiaAsynchronousCanvas can only be used for recording, it does not store a proper SkCanvas. Therefore readbacsk are not possible.  This flag gets set when a SkiaAsynchronousCanvas is created with the constructor overload that receives an SkPicture.  When this flag is set the Synchronous accessMode is ignored and returns an asynchronous (SkPicture backed) canvas.

The SynchronousOnly flag indicates that only immediate rendering can be used. This flag is automatically set when the SkCanvas object does not use GPU acceleration, because the benefits of async rendering are lost in that case.  This flag can also be set as a constructor argument.

The Accelerated flag indicates GPU acceleration When this flag is specified at construction time, it indicates that a GPU accelerated canvas is preferred. When this flag is set after the lazy initialization of the SkCanvas, it indicates that the canvas is in fact accelerated. 

There are two wrapper methods for canvas():
-writeOnlyCanvas() is equivalent to canvas(Asynchronous). This may or may not return a canvas that is readable (dending on whether or not the SynchronousOnly flag is set). The semantic is that it returns a canvas that is intended for writing only
-readableCanvas(gpuHint) is equivalent to canvas(gpuHint?ForGpuReadback:ForReadback)

readableCanvas() is to be called when access to the up to date device bitmap is required. The purpose of gpuHint is to guide the lazy initialization of the encapsulated SkCanvas. The proper way to use this argument is to set it to true when the canvas does not need to be readback to CPU memory. If the bitmap needs to be readback to the CPU, then we want seize the opportunity to switch off canvas acceleration before the canvas lazy init, which is achieved with gpuHint=false.  That way we avoid the great performance penalty of a GPU readback.

Another interesting fact about this class is that it does not need a GraphicsContext3D as long as the access mode is asynchronous.  Therefore, the graphics context is lazily initialized. In cases where synchronous (read) access is never requested, rendering can be deferred until it is time to composite the layer, at which time it is possible to use the compositor's context for rendering.  The compositor provides its context when calling the doRender() method.  Therefore, this scheme allows us to often avoid the creation of a secondary graphics context just for canvas rendering, which helps with performance.

[James Robinson]

I think you should move the discussion to a proper design doc and send it out to the team.  It's very difficult to have in-depth discussion and threaded conversations through bugzilla comments.

[Justin Novosad]

Created attachment 107732 [details]
Work in progress v2 (Do not submit to EWS)

Fixed a few bugs, added SkiaAsynchronousCanvas.[hC]

[Justin Novosad]

Created attachment 107734 [details]
Work in progress v3 (Do not submit to EWS)

Really added SkiaAsynchronousCanvas.*

[Justin Novosad]

Created attachment 107737 [details]
Work in progress v4 (Do not submit to EWS)

Correcting svn diff/patch blunder :-(

[Brian Salomon]

Comment on attachment 107737 [details]
Work in progress v4 (Do not submit to EWS)

View in context: https://bugs.webkit.org/attachment.cgi?id=107737&action=review

> Source/WebCore/platform/graphics/skia/SkiaAsynchronousCanvas.h:47
> +    enum AccessMode {

Should this be made private? I don't see any public users. It looks like everybody goes through writableCanvas and readableCanvas which infer an AccessMode. If the outside world doesn't need to understand the fine distinctions perhaps they should be hidden.

> Source/WebCore/platform/graphics/skia/SkiaAsynchronousCanvas.h:80
> +    SkCanvas* readableCanvas(bool accelerationHint = false) {return canvas(accelerationHint ? ForGpuReadback : ForReadback);}

I'm wondering if two-headed-canvas is the right model for the outside world to see. readableCanvas() only seems to be used to access pixels. Would it make more sense to use canvas() for a writable canvas and have a readPixels type function? It seems like some of the design of this class (which might evolve over time or move upstream) might be less visible to all the draw call sites.

[Justin Novosad]

(In reply to comment #14)
> 
> I'm wondering if two-headed-canvas is the right model for the outside world to see. readableCanvas() only seems to be used to access pixels. Would it make more sense to use canvas() for a writable canvas and have a readPixels type function? It seems like some of the design of this class (which might evolve over time or move upstream) might be less visible to all the draw call sites.

I like that idea. I was also thinking that I could fully encaplusate SkCanvas and replicate part of the interface of SkCanvas in SkiaAsynchronousCanvas. That way calling code would not need to worry about figuring out the appropriate access mode or gpuHint.

[Mike Reed]

Can we just add [read,write]Pixels() call to PlatformContextSkia (which already has some wrapper calls that perform magic before calling down to the SkCanvas)? With this, you may not need to modify any other callsites except the few that effectively want to read/write pixels.

[Justin Novosad]

(In reply to comment #16)
> Can we just add [read,write]Pixels() call to PlatformContextSkia (which already has some wrapper calls that perform magic before calling down to the SkCanvas)? With this, you may not need to modify any other callsites except the few that effectively want to read/write pixels.

Yes! And I'd like to go further by fully encapsulating SkCanvas, to prevent these paths from being circumvented through canvas->getDevice->accessBitmap()

[Brian Salomon]

(In reply to comment #17)
> (In reply to comment #16)
> > Can we just add [read,write]Pixels() call to PlatformContextSkia (which already has some wrapper calls that perform magic before calling down to the SkCanvas)? With this, you may not need to modify any other callsites except the few that effectively want to read/write pixels.
> 
> Yes! And I'd like to go further by fully encapsulating SkCanvas, to prevent these paths from being circumvented through canvas->getDevice->accessBitmap()

That's a good point about circumvention. If we don't fully subclass SkCanvas with an async wrapper maybe we should consider a really simple subclass of SkCanvas that would fail or assert in the handful of prohibited methods.

[Brian Salomon]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Can we just add [read,write]Pixels() call to PlatformContextSkia (which already has some wrapper calls that perform magic before calling down to the SkCanvas)? With this, you may not need to modify any other callsites except the few that effectively want to read/write pixels.
> > 
> > Yes! And I'd like to go further by fully encapsulating SkCanvas, to prevent these paths from being circumvented through canvas->getDevice->accessBitmap()
> 
> That's a good point about circumvention. If we don't fully subclass SkCanvas with an async wrapper maybe we should consider a really simple subclass of SkCanvas that would fail or assert in the handful of prohibited methods.

Mike reminded me that this wouldn't be so simple since sometimes the underlying impl would sometimes be a picture and sometimes a base canvas.

[Tom Hudson]

I'd like to second jamesr@ here and suggest that we move the discussion into a design doc. With a big bunch of code here it's easy to focus on the specifics of this implementation, and as came up in standup today, it isn't clear that this is the approach we'll actually want to commit - even as Justin pushes this code forward enough to get some preliminary numbers, we could be evaluating the smell of alternatives?

[Justin Novosad]

This bug is being abandoned, and replaced with this one:
https://bugs.webkit.org/show_bug.cgi?id=76732

I have recoded an alternate solution from scratch, making the discussion and patches found here uninteresting.  So I'm starting over with a clean slate.