Bug 55404 – Implement Accelerated 2D <canvas> on Chrome/Mac

• Component: Canvas
• Status: RESOLVED WONTFIX

[Stephen White]

The accelerated 2D canvas implementation currently in use by Chrome/Win and Chrome/Linux should be ported to Chrome/Mac.

[Stephen White]

Created attachment 84130 [details]
First draft (EWS-food)

[WebKit Review Bot]

Attachment 84130 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8074478

[Mark Rowe (bdash)]

Comment on attachment 84130 [details]
First draft (EWS-food)

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

Other random comments about this patch:
1) Is the “canvas” term used throughout this patch referring to the <canvas> element or to the more general idea of a canvas as something that you draw to?  If it’s the former then it would appear to be a big layering violation for code in WebCore/platform to know about it.  If the latter, how does this differ from the idea of a graphics context?
1) HybridCanvas seems like a terrible name.  Hybrid is such a vague term that doesn’t really communicate what’s interesting about the type.
2) ENABLE(ACCELERATED_2D_CANVAS) seems like a confusing and overly generic name for a #define related to this.  This is particularly true given that many of the implementation files appear to be Chromium-speicifc.  It seems that this define is tied to a particular method of accelerating 2D canvas, not to accelerating 2D canvas in general.  I’d suggest that this should be renamed so it’s less confusing.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:36
> +#endif

Don’t jam #if’s in the middle of an otherwise sorted list of #includes.

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:170
> +#if ENABLE(ACCELERATED_2D_CANVAS)
> +static void drawImageGPU(GraphicsContext* ctxt, CGImageRef image, const FloatRect& srcRect, const FloatRect& dstRect, ColorSpace styleColorSpace, CompositeOperator compositeOp)
> +{
> +    ctxt->data()->prepareForHardwareDraw();
> +    GLES2Canvas* gpuCanvas = ctxt->data()->m_hybridCanvas->gpuCanvas();
> +    Texture* texture = gpuCanvas->getTexture(image);
> +    if (!texture) {
> +        texture = gpuCanvas->createTexture(image, Texture::BGRA8, CGImageGetWidth(image), CGImageGetHeight(image));
> +        RetainPtr<CFDataRef> pixelData;
> +        pixelData.adoptCF(CGDataProviderCopyData(CGImageGetDataProvider(image)));
> +        texture->load(CFDataGetBytePtr(pixelData.get()));
> +    }
> +    gpuCanvas->drawTexturedRect(texture, srcRect, dstRect, styleColorSpace, compositeOp);
> +}
> +#endif

It seems like a layering issue for this code to live inside ImageCG.cpp.  It seems like it would be more at home on the GraphicsContext, hiding all of the random GLES / Texture stuff from this code.

[Stephen White]

Thanks for your review.

(In reply to comment #3)
> (From update of attachment 84130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84130&action=review
> 
> Other random comments about this patch:
> 1) Is the “canvas” term used throughout this patch referring to the <canvas> element or to the more general idea of a canvas as something that you draw to?  If it’s the former then it would appear to be a big layering violation for code in WebCore/platform to know about it.  If the latter, how does this differ from the idea of a graphics context?

The latter (a canvas to draw to) -- there's nothing <canvas> specific about this code.  The reason it's a canvas rather than a context is that it doesn't really hold any state.  All of the actual drawing calls and state setting go through to GLES2Canvas or GraphicsContext.  (That said, GLES2Canvas is mis-named:  by the above definition it should be a context, not a canvas, a battle I already fought and lost.  :) )

> 1) HybridCanvas seems like a terrible name.  Hybrid is such a vague term that doesn’t really communicate what’s interesting about the type.

I'm open to suggestions.  The only other names I could come up with were generic things like "RenderManager" or the like which I find even more meaningless.

> 2) ENABLE(ACCELERATED_2D_CANVAS) seems like a confusing and overly generic name for a #define related to this.  This is particularly true given that many of the implementation files appear to be Chromium-speicifc.

Actually, most of the implementation files live in platform/graphics/gpu, which is port-independent.  Only GLES2Canvas still lives in platform/graphics/chromium (for hysterical reasons).  It should be moved to platform/graphics/gpu, since it has no dependencies on other Chromium files.

>  It seems that this define is tied to a particular method of accelerating 2D canvas, not to accelerating 2D canvas in general.  I’d suggest that this should be renamed so it’s less confusing.

I agree this is misleading, since it's not actually <canvas>-specific code at this level.  However, currently its only caller is code which is also protected by ENABLE(ACCELERATED_2D_CANVAS).  On the skia-side, we used to have a USE(GLES2_RENDERING) for this (it was a USE since it used the OpenGL ES2 library), but we removed it once we decided to compile this code in unconditionally in Chrome (it's still protected by a runtime flag).  I could bring it back, but it isn't really appropriate to call it GLES2 anymore, since Chris Marrin asked us to rework our implementation to use the GraphicsContext3D wrapper instead of OpenGL ES2 directly.  ENABLE(GPU_RENDERING)?  ENABLE(GRAPHICS_CONTEXT_GPU)?  ENABLE(GRAPHICS_CONTEXT_2D_GRAPHICS_CONTEXT_3D)?  (ugh)

> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:36
> > +#endif
> 
> Don’t jam #if’s in the middle of an otherwise sorted list of #includes.

Fixed.

> > Source/WebCore/platform/graphics/cg/ImageCG.cpp:170
> > +#if ENABLE(ACCELERATED_2D_CANVAS)
> > +static void drawImageGPU(GraphicsContext* ctxt, CGImageRef image, const FloatRect& srcRect, const FloatRect& dstRect, ColorSpace styleColorSpace, CompositeOperator compositeOp)
> > +{
> > +    ctxt->data()->prepareForHardwareDraw();
> > +    GLES2Canvas* gpuCanvas = ctxt->data()->m_hybridCanvas->gpuCanvas();
> > +    Texture* texture = gpuCanvas->getTexture(image);
> > +    if (!texture) {
> > +        texture = gpuCanvas->createTexture(image, Texture::BGRA8, CGImageGetWidth(image), CGImageGetHeight(image));
> > +        RetainPtr<CFDataRef> pixelData;
> > +        pixelData.adoptCF(CGDataProviderCopyData(CGImageGetDataProvider(image)));
> > +        texture->load(CFDataGetBytePtr(pixelData.get()));
> > +    }
> > +    gpuCanvas->drawTexturedRect(texture, srcRect, dstRect, styleColorSpace, compositeOp);
> > +}
> > +#endif
> 
> It seems like a layering issue for this code to live inside ImageCG.cpp.  It seems like it would be more at home on the GraphicsContext, hiding all of the random GLES / Texture stuff from this code.

I could do that, although I think it would have to a be a public member fn declared in GraphicsContext.h in order for BitmapImage to have access.  Which might tempt external callers to call it.

[Stephen White]

Created attachment 84229 [details]
Second draft -- chrome/mac fix, #include fix

[David Levin]

Comment on attachment 84229 [details]
Second draft -- chrome/mac fix, #include fix

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

Doubtful that I should be the one to r+ this but I notice a few things in passing.

> Source/WebCore/platform/graphics/cg/HybridCanvas.cpp:2
> + * Copyright (c) 2010, Google Inc. All rights reserved.

2011 now. Get with the times :) (Also no comma after the year and a capital C "(C)" is the more standard thing to do in WebKit.

> Source/WebCore/platform/graphics/cg/HybridCanvas.cpp:53
> +

extra blank

> Source/WebCore/platform/graphics/cg/HybridCanvas.cpp:111
> +

extra blank

> Source/WebCore/platform/graphics/cg/HybridCanvas.cpp:203
> +    default:

Put the ASSERT_NOT_REACHED outside of the switch (which is equivalent due to the "return;" in the cases.

This will have the nice benefit of getting the compiler to warn you that an enum is not handled in this switch statement if one is added.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1529
> +void GraphicsContext::setSharedGraphicsContext3D(SharedGraphicsContext3D*, DrawingBuffer*, const IntSize&) {}

Put a space inside of the braces.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1530
> +void GraphicsContext::syncSoftwareCanvas() {}

Ditto.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1531
> +void GraphicsContext::markDirtyRect(const IntRect&) {}

Ditto.

> Source/WebCore/platform/graphics/cg/HybridCanvas.h:33
> +

It looks like the contents of this header should be #if ENABLE(ACCELERATED_2D_CANVAS)

Then don't do the #if when you #include it.

> Source/WebCore/platform/graphics/cg/HybridCanvas.h:34
> +#include "GraphicsTypes.h"

Why is this include needed?

> Source/WebCore/platform/graphics/cg/HybridCanvas.h:53
> +    HybridCanvas(GraphicsContext*);

This constructor should be private.

Only expose a static "create" method which returns a PassOwnPtr<HybridCanvas> (like what you did for WillPublishCallbackImp).

> Source/WebCore/platform/graphics/cg/GraphicsContextPlatformPrivateCG.h:29
> +#if ENABLE(ACCELERATED_2D_CANVAS)

This section should be after all other includes.

> Source/WebCore/platform/graphics/cg/GraphicsContextPlatformPrivateCG.h:51
> +        , m_hybridCanvas(0)

Since this is an OwnPtr, you don't need this.

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:34
> +#if ENABLE(ACCELERATED_2D_CANVAS)

if in the middle of includes :(

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:198
> +

extra blank line

> Source/WebCore/WebCore.gypi:2565
> +            'platform/graphics/cg/GraphicsContextPlatformPrivateCG.h',

out of place.

[Stephen White]

Comment on attachment 84229 [details]
Second draft -- chrome/mac fix, #include fix

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

Thanks for the review.

>> Source/WebCore/platform/graphics/cg/HybridCanvas.cpp:2
>> + * Copyright (c) 2010, Google Inc. All rights reserved.
> 
> 2011 now. Get with the times :) (Also no comma after the year and a capital C "(C)" is the more standard thing to do in WebKit.

Fixed.

>> Source/WebCore/platform/graphics/cg/HybridCanvas.cpp:53
>> +
> 
> extra blank

Fixed.

>> Source/WebCore/platform/graphics/cg/HybridCanvas.cpp:111
>> +
> 
> extra blank

Fixed.

>> Source/WebCore/platform/graphics/cg/HybridCanvas.cpp:203
>> +    default:
> 
> Put the ASSERT_NOT_REACHED outside of the switch (which is equivalent due to the "return;" in the cases.
> 
> This will have the nice benefit of getting the compiler to warn you that an enum is not handled in this switch statement if one is added.

Done.  (This was cut-and-paste from PlatformContextSkia, BTW.)

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1529
>> +void GraphicsContext::setSharedGraphicsContext3D(SharedGraphicsContext3D*, DrawingBuffer*, const IntSize&) {}
> 
> Put a space inside of the braces.

Done.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1530
>> +void GraphicsContext::syncSoftwareCanvas() {}
> 
> Ditto.

Done.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1531
>> +void GraphicsContext::markDirtyRect(const IntRect&) {}
> 
> Ditto.

Done.

>> Source/WebCore/platform/graphics/cg/HybridCanvas.h:33
>> +
> 
> It looks like the contents of this header should be #if ENABLE(ACCELERATED_2D_CANVAS)
> 
> Then don't do the #if when you #include it.

Done.

>> Source/WebCore/platform/graphics/cg/HybridCanvas.h:34
>> +#include "GraphicsTypes.h"
> 
> Why is this include needed?

For CompositeOperator.  Can't forward-declare an enum, sadly.

>> Source/WebCore/platform/graphics/cg/HybridCanvas.h:53
>> +    HybridCanvas(GraphicsContext*);
> 
> This constructor should be private.
> 
> Only expose a static "create" method which returns a PassOwnPtr<HybridCanvas> (like what you did for WillPublishCallbackImp).

Done.

>> Source/WebCore/platform/graphics/cg/GraphicsContextPlatformPrivateCG.h:29
>> +#if ENABLE(ACCELERATED_2D_CANVAS)
> 
> This section should be after all other includes.

Done (only includes GLES2Canvas.h now).

>> Source/WebCore/platform/graphics/cg/GraphicsContextPlatformPrivateCG.h:51
>> +        , m_hybridCanvas(0)
> 
> Since this is an OwnPtr, you don't need this.

Removed.

>> Source/WebCore/platform/graphics/cg/ImageCG.cpp:34
>> +#if ENABLE(ACCELERATED_2D_CANVAS)
> 
> if in the middle of includes :(

Fixed.

>> Source/WebCore/platform/graphics/cg/ImageCG.cpp:198
>> +
> 
> extra blank line

Fixed.

>> Source/WebCore/WebCore.gypi:2565
>> +            'platform/graphics/cg/GraphicsContextPlatformPrivateCG.h',
> 
> out of place.

Fixed.

[Stephen White]

Created attachment 84291 [details]
Patch

[David Levin]

Nit: There is no bug link in the changelog (and with that I bow out of this bug).  I suspect jamesr or kbr may jump in here soon.

[Stephen White]

bdash:  ping?

[Simon Fraser (smfr)]

Comment on attachment 84291 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:164
> +    m_data->prepareForSoftwareDraw();

I don't like all these invasive changes. Maybe it's time to make GraphicsContextMethods virtual. Alternatively, we could add a hook like

enum DrawingOperation {
  DrawText,
  DrawLine,
  DrawRect,
 ...
}

void startDrawingOperation(DrawingOperation);

that you could use here.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1518
> +void GraphicsContext::syncSoftwareCanvas()
> +{
> +    if (m_data->m_hybridCanvas)
> +        m_data->m_hybridCanvas->syncSoftwareCanvas();
> +}

It's not clear what "software canvas" means here. This is all software.

> Source/WebCore/platform/graphics/cg/HybridCanvas.h:53
> +class HybridCanvas {

I really don't like the name HybridCanvas, because it a) doesn't tell me what it's a hybrid of, and b) it's unclear if this refers to the canvas element, or some generic drawing surface.

Rather than hybrid, maybe "PartiallyAccelerated". And it seems like this is really some kind of context, rather than a canvas.

[Stephen White]

Comment on attachment 84291 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:164
>> +    m_data->prepareForSoftwareDraw();
> 
> I don't like all these invasive changes. Maybe it's time to make GraphicsContextMethods virtual. Alternatively, we could add a hook like
> 
> enum DrawingOperation {
>   DrawText,
>   DrawLine,
>   DrawRect,
>  ...
> }
> 
> void startDrawingOperation(DrawingOperation);
> 
> that you could use here.

The problem is that it's not just GraphicsContext which would need to be virtualized, but also Font, Path, Image, etc, since callers can (and do) make calls to those functions directly, AFAIK.  Also, all the platform-specific data would have to be moved down into derived classes (although perhaps it could be done in stages).

I like your startDrawingOperation() idea.  I could handle the calls to prepareForSoftwareDraw() there.  However, it wouldn't handle the actual calls to the GPU implementation for the functions which are accelerated, since they need the arguments to the original functions (the path, rect, image, etc), so those would remain in the functions they're in now.  Would that be acceptable?  Alternatively, startDrawingOperation() could take a polyglot of the possible parameters, but that seems pretty ugly.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1518
>> +}
> 
> It's not clear what "software canvas" means here. This is all software.

It's about where the rendering results are:  "hardware" = GPU-rendered; "software" = CPU-rendered.  Alternative names: "CPUCanvas" vs "GPUCanvas" or "RAMCanvas" vs "VRAMCanvas".  I'm open to suggestions.

>> Source/WebCore/platform/graphics/cg/HybridCanvas.h:53
>> +class HybridCanvas {
> 
> I really don't like the name HybridCanvas, because it a) doesn't tell me what it's a hybrid of, and b) it's unclear if this refers to the canvas element, or some generic drawing surface.
> 
> Rather than hybrid, maybe "PartiallyAccelerated". And it seems like this is really some kind of context, rather than a canvas.

OK, PartiallyAcceleratedContext it is.

[Simon Fraser (smfr)]

Comment on attachment 84291 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1518
>>> +}
>> 
>> It's not clear what "software canvas" means here. This is all software.
> 
> It's about where the rendering results are:  "hardware" = GPU-rendered; "software" = CPU-rendered.  Alternative names: "CPUCanvas" vs "GPUCanvas" or "RAMCanvas" vs "VRAMCanvas".  I'm open to suggestions.

We've typically used "accelerated" to mean "done on the GPU".

[Stephen White]

Created attachment 84984 [details]
Patch

[James Robinson]

Comment on attachment 84984 [details]
Patch

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

A few refactors seem in order along with this:

1.) share all the uploaded/backing store state logic in PartiallyAcceleratedContext with PlatformContextSkia somehow
2.) move the upload/readback logic out of PartiallyAcceleratedContext into some helper.  We have a few pieces of code that do uploads/readbacks to/from CG contexts throughout the code and it's kind of unfortunate to write that code again.

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:158
> +static void drawImageGPU(GraphicsContext* ctxt, CGImageRef image, const FloatRect& srcRect, const FloatRect& dstRect, ColorSpace styleColorSpace, CompositeOperator compositeOp)

nit: WebKit style would be technically to not abbreviate 'context' here, although the rest of this file is using ctxt.

> Source/WebCore/platform/graphics/cg/PartiallyAcceleratedContext.cpp:62
> +    return PassOwnPtr<PartiallyAcceleratedContext>(new PartiallyAcceleratedContext(context));

use adoptPtr() here instead of the PassOwnPtr<T> c'tor - it'll pick up the correct type without having to type "PartiallyAcceleratedContext" out twice.

> Source/WebCore/platform/graphics/cg/PartiallyAcceleratedContext.h:72
> +    PartiallyAcceleratedContext(GraphicsContext*);

nit: explicit

[James Robinson]

Not touching the review? flag yet as I'm curious what Simon or other CoreGraphics maintainers think about this.

[Simon Fraser (smfr)]

I'm not big on the intrusive changes to GraphicsContextCG.

[Stephen White]

(In reply to comment #17)
> I'm not big on the intrusive changes to GraphicsContextCG.

I have another idea:  I could put all the drawing methods in PartiallyAcceleratedContext, and make them return a bool if that call is implemented and GPU acceleration is enabled:

e.g., for fillPath:

#if ENABLE(ACCELERATED_2D_CANVAS)
if (m_data->partiallyAcceleratedContext()->fillPath(path)) {
    return;
}
#endif

The state-setting ones wouldn't have the early return:

#if ENABLE(ACCELERATED_2D_CANVAS)
m_data->partiallyAcceleratedContext()->setFillColor(color);
#endif

Then there would be no calls to prepareForXXX() inside GraphicsContextCG, although they would now all be stubs as above.  What do you think?

[Stephen White]

Created attachment 85093 [details]
Patch

[Stephen White]

(In reply to comment #19)
> Created an attachment (id=85093) [details]
> Patch

OK, this is what I had in mind. The changes to GraphicsContextCG are now pretty much as small as possible.  Most of the acceleration-specific stuff is now inside one big #if ENABLE() block in GraphicsContextPlatformPrivateCG.h.  Let me know what you think.

[Build Bot]

Attachment 85093 [details] did not build on win:
Build output: http://queues.webkit.org/results/8107979

[Stephen White]

Created attachment 85170 [details]
Patch

[Eric Seidel (no email)]

I believe there was a time when it was possible to runtime switch GraphicsContexts (at least on the Windows port).  That's probably long dead by now (or I may be mis-remembering).

I thought that CG already did hardware accelerated 2d?  Isn't that what Quartz 2d extreme was supposed to be?  Maybe that never got turned on by default?

[Eric Seidel (no email)]

James: Would you please chime in here?  This has been up for over a month.

[James Robinson]

Do you still want to try to land this, Stephen?  The code seems fine but I don't think the mac port maintainers are interested in taking it (for reasons they seem unwilling or unable to discuss much on this public bug) and I'm not sure if it will be useful in the long term for chromium mac, either.

[Stephen White]

No, it doesn't look like we'll be going down this road.  Marking as WONTFIX.