Bug 64958 – Use software rendering for small canvas

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alok Priyadarshi]

chromium bug: http://code.google.com/p/chromium/issues/detail?id=88063

[Alok Priyadarshi]

Created attachment 101602 [details]
proposed patch

This patch brings tv.adobe.com frame-rate back to 62. The accelerated path runs at 15 due to all the cufon-text canvases.

[Alok Priyadarshi]

Created attachment 101603 [details]
proposed patch

Correct patch. The last patch picked up unintended changes.

[WebKit Review Bot]

Attachment 101602 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:107:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 101603 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:107:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen White]

Comment on attachment 101602 [details]
proposed patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:100
> +static bool shouldAccelerateCanvas(const HTMLCanvasElement* canvas)

This function should probably be protected by #if ENABLE(ACCELERATED_2D_CANVAS), otherwise it'll probably give unused function warnings on other ports.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:108
> +        !settings->legacyAccelerated2dCanvasEnabled())

Not your fault, but the legacy flag doesn't do anything anymore (the legacy implementation has been removed).  So you don't need to check it.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:118
> +    static const int nPixelThreshold = 128 * 128;

This should probably be in a constant at the top of the file (along with the comment).

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:164
> +    if (shouldAccelerateCanvas(canvas)) {

WebKit style is to prefer early-returns over nested if's.  So this should be:

    if (!shouldAccelerateCanvas(canvas)) 
        return;

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:167
> +      if (p && c) {

As above, these should be early-returns.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:169
> +          if (m_context3D) {

Could be an early-return here too, I think.

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:76
> +LayerTextureUpdater::Orientation LayerTextureUpdaterBitmap::orientation()

Is this necessary for this patch, or maybe an accidental inclusion?

[WebKit Review Bot]

Comment on attachment 101602 [details]
proposed patch

Attachment 101602 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9201791

[WebKit Review Bot]

Comment on attachment 101603 [details]
proposed patch

Attachment 101603 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9191730

[Alok Priyadarshi]

Created attachment 101608 [details]
proposed patch

Fixed style issues. Still need to investigate the test failures.

[Matthew Delaney]

I think we need to put heads together here soon to try to make more of this kind of code (that's getting put behind ACCELERATED_2D_CANVAS everywhere) cross-platform.

In particular, I have one comment/suggestion here:

If you have this "shouldAccelerateCanvas" function, and you use it elsewhere down in canvas-land - as in, other than just in context creation - then you might have some problems. Consider what happens if you have the page settings change half way through using a canvas and some of the drawing/recreation code (like when someone does canvas.width=blah) ends up depending on this "shouldAccelerateCanvas" to know if it should be accelerated or not.

It seems simpler and more robust to have the choice to accelerate a canvas or not passed in on creation.

[Alok Priyadarshi]

(In reply to comment #9)
> I think we need to put heads together here soon to try to make more of this kind of code (that's getting put behind ACCELERATED_2D_CANVAS everywhere) cross-platform.

Agree.

> In particular, I have one comment/suggestion here:
> 
> If you have this "shouldAccelerateCanvas" function, and you use it elsewhere down in canvas-land - as in, other than just in context creation - then you might have some problems. Consider what happens if you have the page settings change half way through using a canvas and some of the drawing/recreation code (like when someone does canvas.width=blah) ends up depending on this "shouldAccelerateCanvas" to know if it should be accelerated or not.
> 
> It seems simpler and more robust to have the choice to accelerate a canvas or not passed in on creation.

May be I am missing something. I do not see how passing a flag would make it more robust. The canvas may still fail to accelerate for variety of other reasons - insufficient vram, context lost, etc. If you passed "accelerated" choice or if the page-settings has acceleration enabled, you cannot assume that a canvas is accelerated. You need to call Canvas::isAccelerated() to find out.

[James Robinson]

Comment on attachment 101608 [details]
proposed patch

I think you want to do the size check in reset() as well.  This does complicate the state transitions a bit, since right now we depend on initial m_context3D creation to succeed in order to stay in the accelerated path through reset() calls.  With this patch if a canvas is created with width*height < 128^2 but then resized to be larger, it won't enter the accelerated path.

[Matthew Delaney]

> May be I am missing something. I do not see how passing a flag would make it more robust. The canvas may still fail to accelerate for variety of other reasons - insufficient vram, context lost, etc. If you passed "accelerated" choice or if the page-settings has acceleration enabled, you cannot assume that a canvas is accelerated. You need to call Canvas::isAccelerated() to find out.

Sorry, I wasn't clear there. I meant to say that:
1) Knowing whether or not a live canvas is accelerated or not should be from isAccelerated() for sure. 
2) Since the decision to *want* to accelerate a canvas or not comes from above (i.e. settings and such), ideally it comes down from above. I don't think it's desirable to ask up - explained below.
3) Since canvas internals can be recreated without the canvas itself being recreated, it needs to store the preference passed down on creation and use that for determining if it should try to make accelerated internals.

This really isn't a big issue, but here's the case that I think is wonky here.
- Have accelerated canvas turned on. Open a page. Canvas gets accelerated.
- The setting is turned off.
- Current canvas continues along being accelerated.
- The canvas has its width set, internals get recreated, though this time *not* accelerated.
- Now you have a page that *had* an accelerated canvas and now doesn't. And it's not clear when that happens.
I find it clearer if the settings used on page open persist for the canvas (would be weird if some canvii on the page became non-accel while others still were accel). This is mostly important for debugging, so again, not a big deal. And if the desired behavior of toggling the setting is to get all canvii on the page to immediately become (non-)accel, then I think a better approach is likely to expose the "should" member bool and do a reset (since you need to anyway) and copy over any state - not sure why you'd want all this though for all the hassle it is...

[James Robinson]

(In reply to comment #12)
> > May be I am missing something. I do not see how passing a flag would make it more robust. The canvas may still fail to accelerate for variety of other reasons - insufficient vram, context lost, etc. If you passed "accelerated" choice or if the page-settings has acceleration enabled, you cannot assume that a canvas is accelerated. You need to call Canvas::isAccelerated() to find out.
> 
> Sorry, I wasn't clear there. I meant to say that:
> 1) Knowing whether or not a live canvas is accelerated or not should be from isAccelerated() for sure. 
> 2) Since the decision to *want* to accelerate a canvas or not comes from above (i.e. settings and such), ideally it comes down from above. I don't think it's desirable to ask up - explained below.
> 3) Since canvas internals can be recreated without the canvas itself being recreated, it needs to store the preference passed down on creation and use that for determining if it should try to make accelerated internals.
> 
> This really isn't a big issue, but here's the case that I think is wonky here.
> - Have accelerated canvas turned on. Open a page. Canvas gets accelerated.
> - The setting is turned off.
> - Current canvas continues along being accelerated.
> - The canvas has its width set, internals get recreated, though this time *not* accelerated.
> - Now you have a page that *had* an accelerated canvas and now doesn't. And it's not clear when that happens.
> I find it clearer if the settings used on page open persist for the canvas (would be weird if some canvii on the page became non-accel while others still were accel). This is mostly important for debugging, so again, not a big deal. And if the desired behavior of toggling the setting is to get all canvii on the page to immediately become (non-)accel, then I think a better approach is likely to expose the "should" member bool and do a reset (since you need to anyway) and copy over any state - not sure why you'd want all this though for all the hassle it is...

Settings can't be changed at runtime in chromium.

[Alok Priyadarshi]

Created attachment 101661 [details]
proposed patch

Handled the case where changing canvas size triggers change in acceleration state.

[WebKit Review Bot]

Comment on attachment 101661 [details]
proposed patch

Attachment 101661 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9209830

[James Robinson]

Comment on attachment 101661 [details]
proposed patch

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

I think this looks great.  Not r+'ing because of the compile error on mac, but this is much cleaner and less buggy than the existing code for sure.  I think checking Settings on every canvas creation and reset() is the correct thing to do.

> Source/WebCore/html/HTMLCanvasElement.cpp:250
> +        bool wasAccelerated = context2D->isAccelerated();

you'll have to guard this in the #if guards below, or use some form of UNUSED_???() macro in an #else branch to avoid the unused variable warning.

[Alok Priyadarshi]

Created attachment 101668 [details]
proposed patch

[James Robinson]

Comment on attachment 101668 [details]
proposed patch

Great, I'll let the EWS bots munch on this but I think it's looking fine.

[Alok Priyadarshi]

The bots seem happy. PTAL - this needs to go in M14.

[Stephen White]

Comment on attachment 101668 [details]
proposed patch

Looks good.  r=me

[WebKit Review Bot]

Comment on attachment 101668 [details]
proposed patch

Clearing flags on attachment: 101668

Committed r91599: <http://trac.webkit.org/changeset/91599>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

A number of canvas GPU tests went red on this.  It looks like this path is hit by a lot of the philip suite, and we're now matching the chromium linux skia CPU behavior (which is buggy).  Two thoughts:

1.) Can you fix up the expectations, Alok?
2.) Does this mean that we aren't actually going to be testing the GPU canvas path with the philip suite any more?

[James Robinson]

Dashboard link:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=canvas%2Fphilip%2Ftests%2F2d.gradient.radial.cone.cylinder.html%2Ccanvas%2Fphilip%2Ftests%2F2d.line.width.basic.html%2Ccanvas%2Fphilip%2Ftests%2F2d.line.width.transformed.html%2Ccanvas%2Fphilip%2Ftests%2F2d.path.arcTo.shape.curve1.html%2Ccanvas%2Fphilip%2Ftests%2F2d.path.arcTo.shape.curve2.html%2Ccanvas%2Fphilip%2Ftests%2F2d.shadow.enable.blur.html%2Ccanvas%2Fphilip%2Ftests%2F2d.transformation.setTransform.skewed.html%2Ccanvas%2Fphilip%2Ftests%2F2d.transformation.transform.skewed.html%2Ccompositing%2Flayer-creation%2FspanOverlapsCanvas.html%2Cfast%2Fcanvas%2Fcanvas-currentColor.html%2Cfast%2Fcanvas%2Fcanvas-composite.html%2Cfast%2Fcanvas%2Fcanvas-transform-skewed.html%2Cfast%2Fcanvas%2Fcanvas-transforms-during-path.html%2Cfast%2Fcanvas%2Ffillrect_gradient.html%2Cfast%2Fcanvas%2Fimage-pattern-rotate.html%2Cfast%2Fcanvas%2FsetWidthResetAfterForcedRender.html

[Stephen White]

(In reply to comment #23)
> A number of canvas GPU tests went red on this.  It looks like this path is hit by a lot of the philip suite, and we're now matching the chromium linux skia CPU behavior (which is buggy).  Two thoughts:
> 
> 1.) Can you fix up the expectations, Alok?
> 2.) Does this mean that we aren't actually going to be testing the GPU canvas path with the philip suite any more?

We really shouldn't do that.  This will cut down our code coverage for GPU canvas by a lot.  Perhaps we should add a runtime flag to disable (or change the threshold for) the force-software behaviour, and add that to the bot runs?

[James Robinson]

Looking more closely, most of the philip tests use a canvas with the default dimensions (150x300) which won't trigger this path, but a few do use smaller canvases explicitly.  Additionally some of our layout tests are written assuming that 100x50 canvases are composited.  This test situation is kind of dodgy :/

[Alok Priyadarshi]

I do not think we should suppress philip tests. Adjusting expectations is cumbersome too because we might tweak the threshold in future. It seems plumbing a threshold via settings would be the right thing to do. For gpu tests this value could be zero. Do you guys see any problem with that.

[Brian Salomon]

(In reply to comment #27)
> I do not think we should suppress philip tests. Adjusting expectations is cumbersome too because we might tweak the threshold in future. It seems plumbing a threshold via settings would be the right thing to do. For gpu tests this value could be zero. Do you guys see any problem with that.

The flag could also just cause us to ignore the size constraint. I'd definitely prefer something like that to not running those tests on the gpu.

[Mike Reed]

Can we run *all* tests on both GPU and CPU?

[Tom Hudson]

A Chrome user created a custom test case to exercise this at http://code.google.com/p/chromium/issues/list?cursor=86287.

[Tom Hudson]

(Oops, bad link; this should work better: http://code.google.com/p/chromium/issues/detail?id=86287)

[Alok Priyadarshi]

(In reply to comment #29)
> Can we run *all* tests on both GPU and CPU?

AFAIK we already run all canvas tests on both CPU and GPU.

The compositing tests cannot be run on CPU unless the compositor is written using skia. OTOH we could run *all* tests on GPU with --force-compositing-mode and --enable-accelerated-drawing.

[Mike Reed]

I would just like to run all tests with SW canvas, as if the heuristic always said SW, rather than only for small ones. Likewise, run all tests with HW canvas, even if the heuristic *would have* chosen SW.

[Alok Priyadarshi]

(In reply to comment #33)
> I would just like to run all tests with SW canvas, as if the heuristic always said SW, rather than only for small ones. Likewise, run all tests with HW canvas, even if the heuristic *would have* chosen SW.

Which effectively means - switch off the heuristic? This patch does that - https://bugs.webkit.org/show_bug.cgi?id=65053

[Mike Reed]

Ah, perfect!

[Mike Reed]

Now we just need to run out bots 3 times!

- normal setting (the one users will see)
- set minimum to 9999999 so we always use SW canvas
- set minimum to 0 so we always use HW canvas

[James Robinson]

(In reply to comment #36)
> Now we just need to run out bots 3 times!
> 
> - normal setting (the one users will see)
> - set minimum to 9999999 so we always use SW canvas
> - set minimum to 0 so we always use HW canvas

We run two of those already, in that we run all the canvas tests in a pure non-GPU mode, which always uses the SW canvas backend, and in a GPU mode with the minimum set to 0 which always uses the HW canvas backend.  What we are lacking is a way to tests the normal settings, that users actually see.  I think we need at least some test coverage for switching a canvas back and forth between HW and SW modes to make sure that all of the setup and teardown works.  We probably don't need to run the full test suite in this mode.

One way to do this would be to expose this setting through layoutTestController and write a few tests that switch back and forth between HW and SW a few times.  There isn't a whole ton of code involved with the switch but it's important to test the path that we ship to users.