Summary: | [Chromium] Expose skia gpu canvas rendering as a runtime flag | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Novosad <junov> | ||||||||||||
Component: | Layout and Rendering | Assignee: | Justin Novosad <junov> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, fishd, kbr, reed, senorblanco, vangelis | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Justin Novosad
2011-04-15 12:55:23 PDT
Created attachment 90259 [details]
Patch
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. 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.
> 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.
> 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.
(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. 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! ;) Created attachment 90533 [details]
Patch
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? 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. > 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.
(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). Created attachment 90588 [details]
Patch
> 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. :-))
(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 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? (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. (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. Created attachment 91135 [details]
Patch
Created attachment 91171 [details]
Patch
I'll re-review this. Comment on attachment 91171 [details]
Patch
Looks good. Thanks for that final cleanup and bearing with the long review process.
Comment on attachment 91171 [details] Patch Clearing flags on attachment: 91171 Committed r85017: <http://trac.webkit.org/changeset/85017> All reviewed patches have been landed. Closing bug. |