Bug 64958 - Use software rendering for small canvas
Summary: Use software rendering for small canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alok Priyadarshi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-21 09:44 PDT by Alok Priyadarshi
Modified: 2011-07-25 11:39 PDT (History)
10 users (show)

See Also:


Attachments
proposed patch (11.92 KB, patch)
2011-07-21 10:37 PDT, Alok Priyadarshi
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
proposed patch (3.70 KB, patch)
2011-07-21 10:41 PDT, Alok Priyadarshi
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
proposed patch (3.56 KB, patch)
2011-07-21 11:18 PDT, Alok Priyadarshi
jamesr: review-
Details | Formatted Diff | Diff
proposed patch (8.10 KB, patch)
2011-07-21 16:22 PDT, Alok Priyadarshi
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
proposed patch (8.15 KB, patch)
2011-07-21 16:55 PDT, Alok Priyadarshi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2011-07-21 09:44:23 PDT
chromium bug: http://code.google.com/p/chromium/issues/detail?id=88063
Comment 1 Alok Priyadarshi 2011-07-21 10:37:35 PDT
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.
Comment 2 Alok Priyadarshi 2011-07-21 10:41:02 PDT
Created attachment 101603 [details]
proposed patch

Correct patch. The last patch picked up unintended changes.
Comment 3 WebKit Review Bot 2011-07-21 10:41:48 PDT
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.
Comment 4 WebKit Review Bot 2011-07-21 10:42:45 PDT
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.
Comment 5 Stephen White 2011-07-21 10:46:29 PDT
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?
Comment 6 WebKit Review Bot 2011-07-21 10:54:16 PDT
Comment on attachment 101602 [details]
proposed patch

Attachment 101602 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9201791
Comment 7 WebKit Review Bot 2011-07-21 11:06:30 PDT
Comment on attachment 101603 [details]
proposed patch

Attachment 101603 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9191730
Comment 8 Alok Priyadarshi 2011-07-21 11:18:28 PDT
Created attachment 101608 [details]
proposed patch

Fixed style issues. Still need to investigate the test failures.
Comment 9 Matthew Delaney 2011-07-21 12:13:21 PDT
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.
Comment 10 Alok Priyadarshi 2011-07-21 12:44:04 PDT
(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.
Comment 11 James Robinson 2011-07-21 12:48:42 PDT
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.
Comment 12 Matthew Delaney 2011-07-21 13:11:28 PDT
> 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...
Comment 13 James Robinson 2011-07-21 13:18:43 PDT
(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.
Comment 14 Alok Priyadarshi 2011-07-21 16:22:42 PDT
Created attachment 101661 [details]
proposed patch

Handled the case where changing canvas size triggers change in acceleration state.
Comment 15 WebKit Review Bot 2011-07-21 16:29:30 PDT
Comment on attachment 101661 [details]
proposed patch

Attachment 101661 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9209830
Comment 16 James Robinson 2011-07-21 16:33:21 PDT
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.
Comment 17 Alok Priyadarshi 2011-07-21 16:55:54 PDT
Created attachment 101668 [details]
proposed patch
Comment 18 James Robinson 2011-07-21 16:57:23 PDT
Comment on attachment 101668 [details]
proposed patch

Great, I'll let the EWS bots munch on this but I think it's looking fine.
Comment 19 Alok Priyadarshi 2011-07-22 08:06:12 PDT
The bots seem happy. PTAL - this needs to go in M14.
Comment 20 Stephen White 2011-07-22 08:26:57 PDT
Comment on attachment 101668 [details]
proposed patch

Looks good.  r=me
Comment 21 WebKit Review Bot 2011-07-22 13:05:10 PDT
Comment on attachment 101668 [details]
proposed patch

Clearing flags on attachment: 101668

Committed r91599: <http://trac.webkit.org/changeset/91599>
Comment 22 WebKit Review Bot 2011-07-22 13:05:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 James Robinson 2011-07-22 14:00:24 PDT
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?
Comment 25 Stephen White 2011-07-22 14:22:30 PDT
(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?
Comment 26 James Robinson 2011-07-22 14:27:53 PDT
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 :/
Comment 27 Alok Priyadarshi 2011-07-22 14:32:32 PDT
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.
Comment 28 Brian Salomon 2011-07-22 14:46:03 PDT
(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.
Comment 29 Mike Reed 2011-07-25 05:51:34 PDT
Can we run *all* tests on both GPU and CPU?
Comment 30 Tom Hudson 2011-07-25 07:51:36 PDT
A Chrome user created a custom test case to exercise this at http://code.google.com/p/chromium/issues/list?cursor=86287.
Comment 31 Tom Hudson 2011-07-25 07:52:36 PDT
(Oops, bad link; this should work better: http://code.google.com/p/chromium/issues/detail?id=86287)
Comment 32 Alok Priyadarshi 2011-07-25 08:16:16 PDT
(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.
Comment 33 Mike Reed 2011-07-25 08:31:46 PDT
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.
Comment 34 Alok Priyadarshi 2011-07-25 08:35:05 PDT
(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
Comment 35 Mike Reed 2011-07-25 08:44:38 PDT
Ah, perfect!
Comment 36 Mike Reed 2011-07-25 08:45:36 PDT
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
Comment 37 James Robinson 2011-07-25 11:39:25 PDT
(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.