Bug 76732 – [Chromium] Enable deferred canvas rendering in the skia port

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Justin Novosad]

Use class SkDeferredCanvas rather than SkCanvas in the skia port for ImageBuffer in order to take advantage of deferred rendering.

[Justin Novosad]

Created attachment 123376 [details]
Integration of SkDeferredCanvas

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Justin Novosad]

Patch will fail cr-linux build, which is normal. Must wait for skia to roll past r3066 in chromium trunk in order to build successfully.  I will re-trigger EWS when the time is right.

[WebKit Review Bot]

Comment on attachment 123376 [details]
Integration of SkDeferredCanvas

Attachment 123376 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11297246

[Stephen White]

(In reply to comment #3)
> Patch will fail cr-linux build, which is normal. Must wait for skia to roll past r3066 in chromium trunk in order to build successfully.  I will re-trigger EWS when the time is right.

I think you'll also need a roll of WebKit's chromium DEPS (in Source/WebKit/chromium/DEPS), to pick up the skia roll from Chrome.  You may have to do this yourself.

[Darin Fisher (:fishd, Google)]

Comment on attachment 123376 [details]
Integration of SkDeferredCanvas

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

> Source/WebKit/chromium/public/WebSettings.h:117
> +    virtual void setDeferred2dCanvasEnabled(bool) = 0;

WebKit API change LGTM

[Justin Novosad]

Created attachment 123718 [details]
Same as previous patch.  Reposted to show green on EWS, now that DEPS have rolled.

[James Robinson]

Comment on attachment 123718 [details]
Same as previous patch.  Reposted to show green on EWS, now that DEPS have rolled.

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

> Source/WebCore/platform/graphics/ImageBuffer.h:64
> +        AcceleratedAndDeferred,

why does this need to be a separate mode in cross-platform code? What bits of cross-platform code do you expect will have different behavior based on this flag?

if this is a skia-only setting then we should be able to hide it in skia-only code

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:138
> +    if (m_flushCallback.get())
> +        m_flushCallback->flushCallback();

we do the ganesh flush on the grContext in paintContentsIfDirty(). why is this different? do you need access to the GraphicsContext3D for this call? what thread do you expect this to be called on?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:47
> +    class FlushCallback {

"flush" is pretty ambiguous - what does this mean? when should it be called? at a glance, i'd guess this has something to do with glFlush, but it definitely doesn't. Needs documentation at the very least

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:121
> +AcceleratedDeviceContext AcceleratedDeviceContext::m_instance;

does this require a static initializer? those are no-nos

[Justin Novosad]

(In reply to comment #8)
> > Source/WebCore/platform/graphics/ImageBuffer.h:64
> > +        AcceleratedAndDeferred,
> 
> why does this need to be a separate mode in cross-platform code? What bits of cross-platform code do you expect will have different behavior based on this flag?
> 
> if this is a skia-only setting then we should be able to hide it in skia-only code

I don't disagree, but I can't think of a cleaner path to propagate the setting from WebCore::Settings down to ImageBufferSkia.cpp.  I may be be wrong, but I don't think it's allowed (or desirable) to create a direct dependency on WebCore/page/* from WebCore/platform/graphics/*

WebCore/html seemed like the right place to bridge the two, but that means going through the cross-platform interface.  Any better ideas?

> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:138
> > +    if (m_flushCallback.get())
> > +        m_flushCallback->flushCallback();
> 
> we do the ganesh flush on the grContext in paintContentsIfDirty(). why is this different? 

This is not a simple glFlush. It is for flushing all pending draw operations that are sitting in the queue of the SkDeferredCanvas object.  Perhaps
I should rename it to executePendingDrawCommands, or something like that?

> do you need access to the GraphicsContext3D for this call? 

Yes.

> what thread do you expect this to be called on?

The same thread that is normally used for drawing to the canvas (the main thread).  If that is a problem with the threaded compositor, then I could disable deferred rendering when threaded compositing is enabled, until this problem is figured out.  Speaking of which, would it be legal to access CCProxy from ImageBufferSkia.cpp?

[Stephen White]

Comment on attachment 123718 [details]
Same as previous patch.  Reposted to show green on EWS, now that DEPS have rolled.

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

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:138
>> +        m_flushCallback->flushCallback();
> 
> we do the ganesh flush on the grContext in paintContentsIfDirty(). why is this different? do you need access to the GraphicsContext3D for this call? what thread do you expect this to be called on?

The rest of this function seems to be specific to the double-buffered case (the code below the iftests).  It would be good to move this to paintContentsIfDirty() if possible, partly for symmetry with the existing flushing, and partly because someone working on double-buffered case may not understand why this code is here.

Ideally, the callback could be added to GrContext::flush() in Skia instead, since it seems to be semantically related, and doesn't call into anything WebKit AFAICT (just SkDeferredCanvas stuff), so I think it could be entirely contained in Skia.

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:47
>> +    class FlushCallback {
> 
> "flush" is pretty ambiguous - what does this mean? when should it be called? at a glance, i'd guess this has something to do with glFlush, but it definitely doesn't. Needs documentation at the very least

I agree that the naming could use work.  Since it's not really related to flushing from the Canvas2DLayerChromium's point-of-view, perhaps it should be called an UpdateCallback?  Or PaintCallback?  Or whatever Canvas2DLayerChromium is doing at that point, not what the callee is doing.

If it is possible to move it to GrContext::flush(), then the FlushCallback name makes sense.

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:121
>> +AcceleratedDeviceContext AcceleratedDeviceContext::m_instance;
> 
> does this require a static initializer? those are no-nos

If this really has to be a singleton (which I would avoid if possible), you might consider making it function-static in a static get() function, and just leaking it.  There's a macro for that someplace in WebKit, which I can't find right now.

[Brian Salomon]

Comment on attachment 123718 [details]
Same as previous patch.  Reposted to show green on EWS, now that DEPS have rolled.

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

>>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:47
>>> +    class FlushCallback {
>> 
>> "flush" is pretty ambiguous - what does this mean? when should it be called? at a glance, i'd guess this has something to do with glFlush, but it definitely doesn't. Needs documentation at the very least
> 
> I agree that the naming could use work.  Since it's not really related to flushing from the Canvas2DLayerChromium's point-of-view, perhaps it should be called an UpdateCallback?  Or PaintCallback?  Or whatever Canvas2DLayerChromium is doing at that point, not what the callee is doing.
> 
> If it is possible to move it to GrContext::flush(), then the FlushCallback name makes sense.

I don't think GrContext is quite the right place for this as GrContexts are shared by multiple devices/canvases and are unaware of their users.  We could consider a having a flush on canvas (and device) that would handle deferral as well as GrContext flush.

[Stephen White]

(In reply to comment #11)
> (From update of attachment 123718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123718&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:47
> >>> +    class FlushCallback {
> >> 
> >> "flush" is pretty ambiguous - what does this mean? when should it be called? at a glance, i'd guess this has something to do with glFlush, but it definitely doesn't. Needs documentation at the very least
> > 
> > I agree that the naming could use work.  Since it's not really related to flushing from the Canvas2DLayerChromium's point-of-view, perhaps it should be called an UpdateCallback?  Or PaintCallback?  Or whatever Canvas2DLayerChromium is doing at that point, not what the callee is doing.
> > 
> > If it is possible to move it to GrContext::flush(), then the FlushCallback name makes sense.
> 
> I don't think GrContext is quite the right place for this as GrContexts are shared by multiple devices/canvases and are unaware of their users.  We could consider a having a flush on canvas (and device) that would handle deferral as well as GrContext flush.

Yeah, we were thinking we'd need a vector of callbacks on the GrContext.

I like the idea of a flush function on SkCanvas.  The problem with that is that Canvas2DLayerChromium doesn't currently know about its canvas, only the GraphicsContext3D (from which it gets the GrContext).  That could be changed, of course.  I think the first step is to move the callsite to paintContentsIfDirty, just to see if that timing works for Justin's purposes.  If it doesn't, there's no point in trying to move it to SkCanvas either.

[Justin Novosad]

(In reply to comment #11)
 
> I don't think GrContext is quite the right place for this as GrContexts are shared by multiple devices/canvases and are unaware of their users.  We could consider a having a flush on canvas (and device) that would handle deferral as well as GrContext flush.

But that is not so awesome either because in the multiple canvas case each individual canvas->flush call would issue a new call GrContext::flush, which is unnecessary in most cases and could hurt performance.

[Stephen White]

(In reply to comment #13)
> (In reply to comment #11)
> 
> > I don't think GrContext is quite the right place for this as GrContexts are shared by multiple devices/canvases and are unaware of their users.  We could consider a having a flush on canvas (and device) that would handle deferral as well as GrContext flush.
> 
> But that is not so awesome either because in the multiple canvas case each individual canvas->flush call would issue a new call GrContext::flush, which is unnecessary in most cases and could hurt performance.

I'm not too worried about that, since:

A)  It's only called once per frame, and AFAIK it's a quick early-out if there's nothing to do (Brian can confirm).
B)  That's what happens today anyway (each Canvas2DLayerChromium calls GrContext::flush() on the shared context from paintContentsIfDirty()).

Thinking about it some more, the advantage of Brian's approach over a vector-of-callbacks on GrContext is that 
only the relevant SkDeferredCanvas would have its flush called, vs. all of them, which would be N^2 in the number of canvases.

[Brian Salomon]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > 
> > > I don't think GrContext is quite the right place for this as GrContexts are shared by multiple devices/canvases and are unaware of their users.  We could consider a having a flush on canvas (and device) that would handle deferral as well as GrContext flush.
> > 
> > But that is not so awesome either because in the multiple canvas case each individual canvas->flush call would issue a new call GrContext::flush, which is unnecessary in most cases and could hurt performance.
> 
> I'm not too worried about that, since:
> 
> A)  It's only called once per frame, and AFAIK it's a quick early-out if there's nothing to do (Brian can confirm).

Redundant calls to flush should be pretty cheap. Today GrContext::flush is pretty dumb and flushes everything pending. It gets called internally quite a bit. In the future we could consider being smarter and determining the set of operations that have to be flushed for a particular target (backing a device). We aren't actually queuing much right now so the payoff wouldn't big, though, that's likely to change in the future.

> B)  That's what happens today anyway (each Canvas2DLayerChromium calls GrContext::flush() on the shared context from paintContentsIfDirty()).
> 
> Thinking about it some more, the advantage of Brian's approach over a vector-of-callbacks on GrContext is that 
> only the relevant SkDeferredCanvas would have its flush called, vs. all of them, which would be N^2 in the number of canvases.

[Justin Novosad]

Created attachment 124342 [details]
Patch

[Justin Novosad]

In the latest patch, I got rid of flush callback entirely by using an indirect path through SkCanvas to trigger the execution of deferred draw operations, and I was able to move it to Canvas2DLayerChromium::paintContentsIfDirty.  I have a plan to make that little bit of code clearer in the near future with this: http://code.google.com/p/skia/issues/detail?id=467

Unfortunately, I did not find a clean way to remove traces of the deferred canvas setting from cross-platform code.  I tried accessing the setting through GraphicsContext3DPrivate -> WebViewImpl, but that did not work because the context spawned by SharedGraphicsContext3D does not have a HostWindow, so the reference to WebViewImpl is not set. :-(  Therefore, I left that bit as is.  Still open to suggestions though.  Also: once the feature is stabilized, I intend to remove all traces of the setting, so this is only temporary.

[Stephen White]

Comment on attachment 124342 [details]
Patch

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

Thanks; this is much cleaner than the first rev.  This looks good to me (modulo a comment), but I'll wait for James to take a look before giving an r+.

> Source/WebCore/html/HTMLCanvasElement.cpp:448
> +#if USE(SKIA)

Thinking out loud:  should we use USE(SKIA) or PLATFORM(CHROMIUM) here?  There's precedent for both in this file, and this is a skia-only feature.  I suppose there might be some argument that a non-chromium port which uses Skia might want to use this feature, although they'd probably require a bit of surgery in their platform/graphics/<platform> directory.  OTOH, since Chrome/Mac has switched, I guess the two defines are almost synonymous now, so it's kind of a moot point.

All that to say, I think I've convinced myself that USE(SKIA) is fine.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:122
> +        m_canvas->getDevice()->accessRenderTarget(); // Triggers execution of pending draw operations

Please add a comment indicating that this is temporary, and will go away once we have a real SkCanvas::flush() (it's a little obscure).

[James Robinson]

Comment on attachment 124342 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:484
> +    RenderingMode renderingMode = shouldAccelerate(bufferSize) ? (shouldDefer() ? AcceleratedAndDeferred : Accelerated) : 

still not sold on this - deferred and accelerated are orthogonal bits.  I think it'd be pretty handy to have a deferred-but-not-accelerated canvas, for instance.

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:122
>> +        m_canvas->getDevice()->accessRenderTarget(); // Triggers execution of pending draw operations
> 
> Please add a comment indicating that this is temporary, and will go away once we have a real SkCanvas::flush() (it's a little obscure).

also please end comments with a period

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:114
> +        AcceleratedDeviceContext* deviceContext = new AcceleratedDeviceContext(context3D);
> +        canvas = new SkDeferredCanvas(device, deviceContext);
> +        deviceContext->unref();

do we have any smart pointer types in skia to avoid needing all these raw ptrs and manual unref()s?

[Tom Hudson]

(In reply to comment #19)
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:114
> > +        AcceleratedDeviceContext* deviceContext = new AcceleratedDeviceContext(context3D);
> > +        canvas = new SkDeferredCanvas(device, deviceContext);
> > +        deviceContext->unref();
> 
> do we have any smart pointer types in skia to avoid needing all these raw ptrs and manual unref()s?

No. (There is a RefPtr implementation, but it still requires some manual unref, and consensus on the Skia team is that it's a misfeature; it's only used in one body of code, and we don't want it spreading any farther.)

[Justin Novosad]

(In reply to comment #19)
> (From update of attachment 124342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124342&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:484
> > +    RenderingMode renderingMode = shouldAccelerate(bufferSize) ? (shouldDefer() ? AcceleratedAndDeferred : Accelerated) : 
> 
> still not sold on this - deferred and accelerated are orthogonal bits.  I think it'd be pretty handy to have a deferred-but-not-accelerated canvas, for instance.

a) We could make this parameter a bitfield rather than an enum, but that will impact code for all platform ports.
b) The day someone needs a deferred-but-not-accelerated canvas, they can add that value to the enum.

[Justin Novosad]

(In reply to comment #20)
> (In reply to comment #19)
> > do we have any smart pointer types in skia to avoid needing all these raw ptrs and manual unref()s?
> 
> No. (There is a RefPtr implementation, but it still requires some manual unref, and consensus on the Skia team is that it's a misfeature; it's only used in one body of code, and we don't want it spreading any farther.)

There is also an auto unref helper class, but that feels unnecessary here. For objects newly allocated on the heap, unref after hand off is really the standard M.O. in skia code.

[James Robinson]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > do we have any smart pointer types in skia to avoid needing all these raw ptrs and manual unref()s?
> > 
> > No. (There is a RefPtr implementation, but it still requires some manual unref, and consensus on the Skia team is that it's a misfeature; it's only used in one body of code, and we don't want it spreading any farther.)
> 
> There is also an auto unref helper class, but that feels unnecessary here. For objects newly allocated on the heap, unref after hand off is really the standard M.O. in skia code.

I see.  It looks out of place in WebKit, which strongly prefers using smart pointer types / RAII wherever possible and explicit handoffs.  This makes it harder to understand skia binding code and harder to review.

[Justin Novosad]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > do we have any smart pointer types in skia to avoid needing all these raw ptrs and manual unref()s?
> > > 
> > > No. (There is a RefPtr implementation, but it still requires some manual unref, and consensus on the Skia team is that it's a misfeature; it's only used in one body of code, and we don't want it spreading any farther.)
> > 
> > There is also an auto unref helper class, but that feels unnecessary here. For objects newly allocated on the heap, unref after hand off is really the standard M.O. in skia code.
> 
> I see.  It looks out of place in WebKit, which strongly prefers using smart pointer types / RAII wherever possible and explicit handoffs.  This makes it harder to understand skia binding code and harder to review.

The skia API is designed so that objects of any type can be allocated either on the stack or the heap, or can be non-pointer class members, which is incompatible with RAII and smartpointers.  Basically, if you do not want an object to be auto-destroyed by an unref because it is a class member or it is on the stack, then you do not unref it after handoff, and the ref count should never go down to zero.

[Matthew Delaney]

Comment on attachment 124342 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.h:136
> +    bool shouldDefer() const;

Does this need to be public?

[Brian Salomon]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > do we have any smart pointer types in skia to avoid needing all these raw ptrs and manual unref()s?
> > > 
> > > No. (There is a RefPtr implementation, but it still requires some manual unref, and consensus on the Skia team is that it's a misfeature; it's only used in one body of code, and we don't want it spreading any farther.)
> > 
> > There is also an auto unref helper class, but that feels unnecessary here. For objects newly allocated on the heap, unref after hand off is really the standard M.O. in skia code.
> 
> I see.  It looks out of place in WebKit, which strongly prefers using smart pointer types / RAII wherever possible and explicit handoffs.  This makes it harder to understand skia binding code and harder to review.

In some places in the porting code the SkAutoTUnref class is used when allocating an object just before hand-off. It unrefs in the destructor. (e.g. createAcceleratedCanvas in ImageBufferSkia.cpp)

[Justin Novosad]

(In reply to comment #26)

> In some places in the porting code the SkAutoTUnref class is used

Ok, sold.

[Justin Novosad]

In the latest patch I have addressed the following:

* Made HTMLCanvasElement::shouldDefer private, I also put it inside #if USE(SKIA)
* In ImageBuffer I separated the deferralMode from the renderingMode (separate enum). There is some new #if USE(SKIA) ugliness to avoid unnecessarily adding the new creation parameter to all the other ports, which do not care about it.
* Corrections to comments
* Used Semi-smart pointers to remove explicit unrefs in createAcceleratedCanvas (ImageBufferSkia.cpp)

[Justin Novosad]

Created attachment 124831 [details]
Response to review comments

[Justin Novosad]

Created attachment 124951 [details]
Same as previous patch, resolved merge conflicts

[Stephen White]

Comment on attachment 124951 [details]
Same as previous patch, resolved merge conflicts

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

> Source/WebCore/platform/graphics/ImageBuffer.h:71
> +#if USE(SKIA)

I really don't think peppering the header file with #if USE(SKIA) is a good idea.  We should add this as a default parameter everywhere, and just leave it unused on other ports.  (This is how the renderingMode was introduced, for example.)

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:91
> +static SkCanvas* createAcceleratedCanvas(const IntSize& size, ImageBufferData* data, DeferralMode deferralMode)

It's kind of a side-effect, but I like this a version lot better.  Separating out the deferral mode into its own enum means we only check the renderingMode in one place, which I think is clearer.

[Justin Novosad]

Created attachment 124995 [details]
Patch

[Stephen White]

Comment on attachment 124995 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:485
> +    RenderingMode renderingMode = shouldAccelerate(bufferSize) ? Accelerated : UnacceleratedNonPlatformBuffer;

I think this one should still be wrapped in an #if USE(SKIA) (unless other ports map UnacceleratedNonPlatformBuffer to Unaccelerated).

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:64
> +    , m_canvas(0)

This isn't wrapped in #if USE(SKIA), but the declaration is.  It's your call, but at this point I'd probably just remove #if USE(SKIA) everywhere in this file and in the header.  I don't think this class is much use without Skia anymore.

[Justin Novosad]

Created attachment 124998 [details]
Patch

[Stephen White]

Comment on attachment 124998 [details]
Patch

Looks good!  Thanks for all the fixes.  r=me

[WebKit Review Bot]

Comment on attachment 124998 [details]
Patch

Clearing flags on attachment: 124998

Committed r106500: <http://trac.webkit.org/changeset/106500>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.