Bug 58683 – [Chromium] Expose skia gpu canvas rendering as a runtime flag

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Justin Novosad]

Instead of the SKIA_GPU build time flag, we should use the acceleratedDrawinEnabled setting to control whether the 2D canvas is rendered using the skia GPU accelerated path.

[Justin Novosad]

Created attachment 90259 [details]
Patch

[Justin Novosad]

To the reviewer: this patch will break the chromium canary build on mac if it is submitted before skia rev 1151 is rolled-in to Chromium.  So do not put this in the commit queue right away.

[Kenneth Russell]

Comment on attachment 90259 [details]
Patch

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

It's unfortunate that the early warning system bots can't test the Chromium Mac build, but you need to test your patch there manually. I can guarantee that it won't currently compile.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:542
> +    if (isAccelerated() && op != CompositeSourceOver && !(m_context3D.get() && m_context3D->grContext())) {

The test of SharedGraphicsContext3D::grContext() doesn't belong in the platform-independent code; or, if it is here, it needs to be guarded with a platform guard like USE(SKIA).

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:45
> +#include "GrContext.h"

You can't uniformly include this header on all platforms. Any port using this class but not Skia won't compile any more.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:65
> +PassRefPtr<SharedGraphicsContext3D> SharedGraphicsContext3D::create(Page* page)

I am pretty sure that this change introduces an abstraction violation that is not allowed. The rule in WebKit is that the DOM and HTML layers can refer to classes in platform/, but not vice versa. I am 99% sure that page/Page, though it isn't strictly a DOM element, is considered part of the higher level layers. I think you'll need to figure out a different way to pass down the acceleratedDrawingEnabled() setting on the Page.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:124
> -#if ENABLE(SKIA_GPU)
> +#if ENABLE(ACCELERATED_2D_CANVAS)

This code needs to still be guarded with e.g. #if USE(SKIA).

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:471
> -#if ENABLE(SKIA_GPU)
> +#if ENABLE(ACCELERATED_2D_CANVAS)

Same here.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:46
> -#if ENABLE(SKIA_GPU)
> +#if ENABLE(ACCELERATED_2D_CANVAS)

I think this should be guarded with #if USE(SKIA).

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:143
> -#if ENABLE(SKIA_GPU)
> +#if ENABLE(ACCELERATED_2D_CANVAS)

Same here.

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:247
> +    if (platformContext()->accelerationMode() == PlatformContextSkia::GPU)

This boilerplate copy/paste seems fragile to me. I think adding a helper method would improve maintainability.

[Justin Novosad]

> It's unfortunate that the early warning system bots can't test the Chromium Mac build, but you need to test your patch there manually. I can guarantee that it won't currently compile.
> 

I'm not sure I understand.  Test what manually? I ran the chromium mac and mac_layout_rel try bots with this change and they came back clean. Is there something that would break a manual build that would not break a tryserver build?I used the sub_rep flag to also pick-up the skia change that is required for this to link on mac.

[Justin Novosad]

> The test of SharedGraphicsContext3D::grContext() doesn't belong in the platform-independent code; or, if it is here, it needs to be guarded with a platform guard like USE(SKIA).

My intent was to guard the Ganesh code with ENABLE(ACCELERATED_2D_CANVAS) as a platform guard, based on the assumption that Ganesh is available on all platforms that support the accelerated canvas.  Even though the mac builds of chromium do not use skia for rendering, they still link against skia (not sure why, but they do), so Ganesh is available.

[Stephen White]

(In reply to comment #5)
> > The test of SharedGraphicsContext3D::grContext() doesn't belong in the platform-independent code; or, if it is here, it needs to be guarded with a platform guard like USE(SKIA).
> 
> My intent was to guard the Ganesh code with ENABLE(ACCELERATED_2D_CANVAS) as a platform guard, based on the assumption that Ganesh is available on all platforms that support the accelerated canvas.  Even though the mac builds of chromium do not use skia for rendering, they still link against skia (not sure why, but they do), so Ganesh is available.

I think Ken's right:  if you're making ganesh-specific calls, USE(SKIA) is the correct guard.  This means we're compiling and linking against Skia, which now implies Ganesh.  There's nothing specific to accelerated canvas about this code, so it would be misleading to use that as a guard.

OTOH, if you want to avoid making Ganesh-specific call here, perhaps we could have a "supportsCompositingOperation(op)" check to SharedGraphicsContext3D, and return true always for Ganesh.

[Stephen White]

Comment on attachment 90259 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:39
> +#if ENABLE(ACCELERATED_2D_CANVAS)

These should also probably be USE(SKIA).  There's nothing accelerated-canvas-specific about this code, but it is skia-specific.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:48
> +#include "Page.h"

This is definitely a layering violation.  We'll have to find some other way of doing this.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:60
> +#include "GrContext.h"

This, and SkGpu* below, should probably be broken out into their own #if USE(SKIA) clause.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:187
> +    bool accelerationMode() { return m_accelerationMode; }

Should this still be a bool?  Looks like it should be an AccelerationMode.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:230
> +    unsigned m_accelerationMode;

Is it a bool, is it an unsigned?  No, it's a SuperAccelerationMode!  ;)

[Justin Novosad]

Created attachment 90533 [details]
Patch

[Kenneth Russell]

Comment on attachment 90533 [details]
Patch

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

Stephen, could you do the unofficial review of this patch? One comment.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:65
> +PassRefPtr<SharedGraphicsContext3D> SharedGraphicsContext3D::create(HostWindow* hostWindow, bool useSkiaGpu)

It's generally frowned upon in WebKit to add various bool arguments to methods and constructors, because it makes the call sites less descriptive. Also, conditionalizing the signature of this method is really gross. Would it be possible to reuse the AccelerationMode enum from below, and make the decision at the call site which value to pass?

[Stephen White]

Comment on attachment 90533 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:544
> +        && !(m_context3D.get() && m_context3D->grContext())

Sorry to be a pain, but I think I would prefer the other option I suggested here:  make a supportsCompositeOperation() on SharedGraphicsContext3D, and return true always for Ganesh, and op == CompositeSourceOver for the current path.  Then we don't have to have anything Ganesh-specific in this file.

[Justin Novosad]

>  Would it be possible to reuse the AccelerationMode enum from below, and make the decision at the call site which value to pass?

That would mean adding this to Page.cpp:
#if USE(SKIA)
#include "PlatformContextSkia.h"
#endif

I think the cleaner option is to a have a separate, dedicated enum in SGC3D, for creation flags.

[Kenneth Russell]

(In reply to comment #11)
> >  Would it be possible to reuse the AccelerationMode enum from below, and make the decision at the call site which value to pass?
> 
> That would mean adding this to Page.cpp:
> #if USE(SKIA)
> #include "PlatformContextSkia.h"
> #endif
> 
> I think the cleaner option is to a have a separate, dedicated enum in SGC3D, for creation flags.

My thought would be to move the AccelerationMode enum into SGC3D.h and reference it from wherever necessary, reusing it in PlatformContextSkia.h (which already seems to have a dependency on SGC3D.h).

[Justin Novosad]

Created attachment 90588 [details]
Patch

[Justin Novosad]

> My thought would be to move the AccelerationMode enum into SGC3D.h and reference it from wherever necessary, reusing it in PlatformContextSkia.h (which already seems to have a dependency on SGC3D.h).

D'Oh! I just read that right after uploading the latest patch. I promise I am not being stubborn on purpose. :-))

[Stephen White]

(In reply to comment #12)
> (In reply to comment #11)
> > >  Would it be possible to reuse the AccelerationMode enum from below, and make the decision at the call site which value to pass?
> > 
> > That would mean adding this to Page.cpp:
> > #if USE(SKIA)
> > #include "PlatformContextSkia.h"
> > #endif
> > 
> > I think the cleaner option is to a have a separate, dedicated enum in SGC3D, for creation flags.
> 
> My thought would be to move the AccelerationMode enum into SGC3D.h and reference it from wherever necessary, reusing it in PlatformContextSkia.h (which already seems to have a dependency on SGC3D.h).

We discussed this, and my thought was that:

1)  we only really need a boolean, since the "None" state doesn't really mean anything here (it's always accelerated)
2)  we might want to add other SGC3D creation flags later, so we need or-able flags, not a 3-way

[Kenneth Russell]

Comment on attachment 90588 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:72
> +#endif

Is it possible to name this enum, have a Skia-specific bit as its only value, and reference that type instead of "unsigned" in the constructor?

[Justin Novosad]

(In reply to comment #16)
> (From update of attachment 90588 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90588&action=review
> 
> > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:72
> > +#endif
> 
> Is it possible to name this enum, have a Skia-specific bit as its only value, and reference that type instead of "unsigned" in the constructor?

That is technically possible, but it is not clean IMHO.  Two bit flags OR-ed together gives an integer that does not necessarily map to a member of the flag enum. You can still force the flags into the enum type, but you have to violate type safety by using a C-style cast or a static_cast.  Is that how we do it in WebKit?  Nothing in the coding guidelines.

[Kenneth Russell]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 90588 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=90588&action=review
> > 
> > > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:72
> > > +#endif
> > 
> > Is it possible to name this enum, have a Skia-specific bit as its only value, and reference that type instead of "unsigned" in the constructor?
> 
> That is technically possible, but it is not clean IMHO.  Two bit flags OR-ed together gives an integer that does not necessarily map to a member of the flag enum. You can still force the flags into the enum type, but you have to violate type safety by using a C-style cast or a static_cast.  Is that how we do it in WebKit?  Nothing in the coding guidelines.

I wasn't sure, but a search through the code base turned up GraphicsContext::TextDrawingMode and the typedef of TextDrawingModeFlags as one way to handle it. You're right, it needs to bottom out as an integer type, but we can at least add a typedef to distinguish it from a random int.

[Justin Novosad]

Created attachment 91135 [details]
Patch

[Justin Novosad]

Created attachment 91171 [details]
Patch

[Kenneth Russell]

I'll re-review this.

[Kenneth Russell]

Comment on attachment 91171 [details]
Patch

Looks good. Thanks for that final cleanup and bearing with the long review process.

[WebKit Commit Bot]

Comment on attachment 91171 [details]
Patch

Clearing flags on attachment: 91171

Committed r85017: <http://trac.webkit.org/changeset/85017>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.