Bug 59929 – [Chromium] Make accelerated 2d canvas enabled by default with skia

• Component: Canvas
• Status: RESOLVED FIXED

[Justin Novosad]

The setting acceleratedCanvas2DEnabled will soon be turned on by default in Chromium.  The effect of this run-time flag must be changed so that it enables the Skia-based GPU acceleration of the 2D canvas.
Furthermore, a new runtime flag must be added in order to enable the legacy accelerated 2D canvas.  The legacy flag will have priority when both flags are enabled simultaneously.

[Justin Novosad]

Created attachment 91924 [details]
Patch

[Stephen White]

Comment on attachment 91924 [details]
Patch

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

> Source/WebCore/page/Page.cpp:752
> +      m_sharedGraphicsContext3D = SharedGraphicsContext3D::create(chrome(), settings()->legacyAccelerated2dCanvasEnabled() ? 0 : SharedGraphicsContext3D::UseSkiaGPU);

Since this now makes skia-gpu the default path for accelerated canvas, won't we also need to add all failing tests to test_expectations along with this change?

[Vangelis Kokkevis]

Comment on attachment 91924 [details]
Patch

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

> Source/WebCore/page/Settings.cpp:722
> +void Settings::setLegacyAccelerated2dCanvasEnabled(bool enabled)

This is a chromium-only flag so I think it should only exist in the chromium websettings files much like it was done with the compositeToTexture flag in :

https://bugs.webkit.org/show_bug.cgi?id=52311

It will actually simplify things and you won't have to touch quite that many files.

[Justin Novosad]

Delivery plan proposed by Stephen White:

1:  Add --enable-legacy-accelerated-2d-canvas flag to both Chrome and DRT (won't do anything yet, since old path is still the default)
2:  Change the --accelerated-2d-canvas flag to mean Ganesh, not the old path, and remove the double-meaning of --enable-accelerated-drawing (have it mean only Alok's compositor -> skia stuff, not canvas2D).  This should be a 1-liner in the code.  Along with this change, submit suppressions to test_expectations for the new failures.
3:  Make canvas2D GPU (skia path) on by default in Chrome.  Replace the --enable-accelerated-2d-canvas flag with --disable-accelerated-2d-canvas.  Pass --disable-accelerated-2d-canvas to the CPU step on the canary and trybots, and remove --enable-accelerated-2d-canvas from the GPU step.
4.  Rebaseline the fast/canvas tests that need new pixel results, and remove the suppressions from test_expectations.

[Vangelis Kokkevis]

(In reply to comment #4)
> Delivery plan proposed by Stephen White:
> 
> 1:  Add --enable-legacy-accelerated-2d-canvas flag to both Chrome and DRT (won't do anything yet, since old path is still the default)
> 2:  Change the --accelerated-2d-canvas flag to mean Ganesh, not the old path, and remove the double-meaning of --enable-accelerated-drawing (have it mean only Alok's compositor -> skia stuff, not canvas2D).  This should be a 1-liner in the code.  Along with this change, submit suppressions to test_expectations for the new failures.
> 3:  Make canvas2D GPU (skia path) on by default in Chrome.  Replace the --enable-accelerated-2d-canvas flag with --disable-accelerated-2d-canvas.  Pass --disable-accelerated-2d-canvas to the CPU step on the canary and trybots, and remove --enable-accelerated-2d-canvas from the GPU step.
> 4.  Rebaseline the fast/canvas tests that need new pixel results, and remove the suppressions from test_expectations.

Sounds good.  We should probably flip #3 and #4 so that we have the tests producing the correct results before we turn this on by default.

[Justin Novosad]

(In reply to comment #3)
> (From update of attachment 91924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91924&action=review
> 
> > Source/WebCore/page/Settings.cpp:722
> > +void Settings::setLegacyAccelerated2dCanvasEnabled(bool enabled)
> 
> This is a chromium-only flag so I think it should only exist in the chromium websettings files much like it was done with the compositeToTexture flag in :
> 
> https://bugs.webkit.org/show_bug.cgi?id=52311
> 
> It will actually simplify things and you won't have to touch quite that many files.

I tried to do what you suggest, but the problem is that I can't figure out a clean way to access WebSettings from outside of WebKit/chromium/src.  I think your suggestion might not work for what we are trying to do here, unless I am missing something?

[Kenneth Russell]

Comment on attachment 91924 [details]
Patch

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

>>> Source/WebCore/page/Settings.cpp:722
>>> +void Settings::setLegacyAccelerated2dCanvasEnabled(bool enabled)
>> 
>> This is a chromium-only flag so I think it should only exist in the chromium websettings files much like it was done with the compositeToTexture flag in :
>> 
>> https://bugs.webkit.org/show_bug.cgi?id=52311
>> 
>> It will actually simplify things and you won't have to touch quite that many files.
> 
> I tried to do what you suggest, but the problem is that I can't figure out a clean way to access WebSettings from outside of WebKit/chromium/src.  I think your suggestion might not work for what we are trying to do here, unless I am missing something?

No, you are correct. You can't access the Chromium WebKit API from WebCore. You can access it from Chrome sources and from within WebKit/chromium/src. The way you've done things looks fine.

> Source/WebCore/page/Settings.h:492
> +        bool m_legacyAcceleratedCanvas2dEnabled : 1;

You need to initialize this new bit to false in the Settings constructor.

> Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h:100
> +#define WebKitLegacyAccelerated2dCanvasEnabledPreferenceKey @"WebKitLegacyAccelerated2dCanvasEnabled"

You don't need to touch any of these Mac-specific files. The Chromium port is the only one using this legacy accelerated canvas 2D code at this point, so it's better not to add the new flag to any of the other ports' WebPreferences-like classes.

[Justin Novosad]

Created attachment 92099 [details]
Patch

[Kenneth Russell]

Comment on attachment 92099 [details]
Patch

Looks fine.

[WebKit Commit Bot]

Comment on attachment 92099 [details]
Patch

Clearing flags on attachment: 92099

Committed r85720: <http://trac.webkit.org/changeset/85720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Justin Novosad]

(In reply to comment #11)
> All reviewed patches have been landed.  Closing bug.
The patch that was landed is only phase 1.
Further work on this issue continues here:
https://bugs.webkit.org/show_bug.cgi?id=60173

[Simon Fraser (smfr)]

Add legacyAccelerated2dCanvasEnabled to Settings with no explanatory comment is a bad thing to do. I have no idea what the difference is between these two prefs.

[Justin Novosad]

(In reply to comment #13)
> Add legacyAccelerated2dCanvasEnabled to Settings with no explanatory comment is a bad thing to do. I have no idea what the difference is between these two prefs.

Thanks for spotting this. This code is now vestigial, and should be cleaned out.  Just created: https://bugs.webkit.org/show_bug.cgi?id=75108