Bug 167968 – Disallow accelerated rendering for ginormous 2D canvases.

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Andreas Kling]

Allowing gargantuan canvases to have accelerated backing stores creates an opportunity for some lame pathologies.

<rdar://problem/30119483>

[Andreas Kling]

Created attachment 300864 [details]
Proposed patch

[Simon Fraser (smfr)]

Please split this into 2 patches.

[Alex Christensen]

Comment on attachment 300864 [details]
Proposed patch

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

> Source/WebCore/page/Settings.in:28
>  minimumAccelerated2dCanvasSize type=int, initial=257*256
>  
> +maximumAccelerated2dCanvasSize type=unsigned, initial=5120*2880

Why are these different types?

[Build Bot]

Comment on attachment 300864 [details]
Proposed patch

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

New failing tests:
inspector/layers/layers-compositing-reasons.html

[Build Bot]

Created attachment 300873 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300864 [details]
Proposed patch

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

New failing tests:
inspector/layers/layers-compositing-reasons.html

[Build Bot]

Created attachment 300874 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300864 [details]
Proposed patch

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

New failing tests:
inspector/layers/layers-compositing-reasons.html

[Build Bot]

Created attachment 300875 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300864 [details]
Proposed patch

Attachment 300864 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3022524

New failing tests:
compositing/canvas/accelerated-canvas-compositing-size-limit.html

[Build Bot]

Created attachment 300876 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Andreas Kling]

Created attachment 300879 [details]
Proposed patch

[Andreas Kling]

(In reply to comment #2)
> Please split this into 2 patches.

k!

(In reply to comment #3)
> Comment on attachment 300864 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300864&action=review
> 
> > Source/WebCore/page/Settings.in:28
> >  minimumAccelerated2dCanvasSize type=int, initial=257*256
> >  
> > +maximumAccelerated2dCanvasSize type=unsigned, initial=5120*2880
> 
> Why are these different types?

They should both be unsigned. I'm trying to keep this change minimal, so didn't touch it now.

[Simon Fraser (smfr)]

Comment on attachment 300879 [details]
Proposed patch

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

> LayoutTests/compositing/canvas/accelerated-canvas-compositing-size-limit.html:27
> +<p>Verifies that 2D canvases with higher than 5120x2880 resolution don't create accelerated backing stores.</p>

So this test is actually detecting whether we create compositing layers for big canvases, not whether we're accelerating the canvas backing store. This are correlated (but you may break this correlation in your next patch).

> LayoutTests/platform/ios-simulator/TestExpectations:2855
> +compositing/canvas/accelerated-canvas-compositing-size-limit.html [ Failure ]

I think this is the wrong way to handle this. Why not just land iOS-specific results, so we can tell if we broke the limits in future?

[Andreas Kling]

(In reply to comment #14)
> Comment on attachment 300879 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300879&action=review
> 
> > LayoutTests/compositing/canvas/accelerated-canvas-compositing-size-limit.html:27
> > +<p>Verifies that 2D canvases with higher than 5120x2880 resolution don't create accelerated backing stores.</p>
> 
> So this test is actually detecting whether we create compositing layers for
> big canvases, not whether we're accelerating the canvas backing store. This
> are correlated (but you may break this correlation in your next patch).

Duh. Let me add a way to include the accelerates drawing flag in layer tree dumps..

> > LayoutTests/platform/ios-simulator/TestExpectations:2855
> > +compositing/canvas/accelerated-canvas-compositing-size-limit.html [ Failure ]
> 
> I think this is the wrong way to handle this. Why not just land iOS-specific
> results, so we can tell if we broke the limits in future?

That would be better, let's do that.

[Andreas Kling]

Created attachment 300895 [details]
Proposed patch

With "acceleratesDrawing" in layer tree dumps (optional). I don't have a test baseline for iOS at the moment, but I'm sure EWS is gonna give me one.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/testing/Internals.h:263:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/testing/Internals.h:264:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 13 files


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

[Build Bot]

Comment on attachment 300895 [details]
Proposed patch

Attachment 300895 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3025466

New failing tests:
compositing/canvas/accelerated-canvas-compositing-size-limit.html

[Build Bot]

Created attachment 300904 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Andreas Kling]

Created attachment 300926 [details]
Proposed patch

My machine doesn't seem to care about the maxActivePixelMemory change I wanted to make anymore.
This one is still very relevant though.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/testing/Internals.h:263:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/testing/Internals.h:264:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 14 files


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

[Build Bot]

Comment on attachment 300926 [details]
Proposed patch

Attachment 300926 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3026595

New failing tests:
compositing/canvas/accelerated-canvas-compositing-size-limit.html

[Build Bot]

Created attachment 300936 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Andreas Kling]

Created attachment 300939 [details]
Proposed patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/testing/Internals.h:263:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/testing/Internals.h:264:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 14 files


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

[WebKit Commit Bot]

Comment on attachment 300939 [details]
Proposed patch

Clearing flags on attachment: 300939

Committed r211949: <http://trac.webkit.org/changeset/211949>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 300939 [details]
Proposed patch

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

> Source/WebCore/platform/graphics/GraphicsLayer.cpp:789
> +    if (behavior & LayerTreeAsTextIncludeAcceleratesDrawing && m_acceleratesDrawing) {
> +        writeIndent(ts, indent + 1);
> +        ts << "(acceleratesDrawing " << m_acceleratesDrawing << ")\n";
> +    }

This has no relation to whether the canvas itself is using accelerated drawing. This is just about the compositing layers, and will always be 1 on Mac, and 0 in the iOS simulator. So I don't think this is giving you accurate information about the behavior of HTMLCanvasElement::shouldAccelerate().

[Andreas Kling]

(In reply to comment #28)
> Comment on attachment 300939 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300939&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.cpp:789
> > +    if (behavior & LayerTreeAsTextIncludeAcceleratesDrawing && m_acceleratesDrawing) {
> > +        writeIndent(ts, indent + 1);
> > +        ts << "(acceleratesDrawing " << m_acceleratesDrawing << ")\n";
> > +    }
> 
> This has no relation to whether the canvas itself is using accelerated
> drawing. This is just about the compositing layers, and will always be 1 on
> Mac, and 0 in the iOS simulator. So I don't think this is giving you
> accurate information about the behavior of
> HTMLCanvasElement::shouldAccelerate().

Are you sure? The only thing we do with the return value from shouldAccelerate() is call GraphicsLayer::setAcceleratesDrawing() if it's true.

[Andreas Kling]

iOS simulator doesn't enable USE_IOSURFACE_CANVAS_BACKING_STORE so I think shouldAccelerate() will always return false there, unless we're building with ENABLE(ACCELERATED_2D_CANVAS)?

If you look at the expected results from my new test, you'll see that acceleratesDrawing is not present for the over-5K canvas's layer.

[Simon Fraser (smfr)]

(In reply to comment #29)
> (In reply to comment #28)
> > Comment on attachment 300939 [details]
> > Proposed patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=300939&action=review
> > 
> > > Source/WebCore/platform/graphics/GraphicsLayer.cpp:789
> > > +    if (behavior & LayerTreeAsTextIncludeAcceleratesDrawing && m_acceleratesDrawing) {
> > > +        writeIndent(ts, indent + 1);
> > > +        ts << "(acceleratesDrawing " << m_acceleratesDrawing << ")\n";
> > > +    }
> > 
> > This has no relation to whether the canvas itself is using accelerated
> > drawing. This is just about the compositing layers, and will always be 1 on
> > Mac, and 0 in the iOS simulator. So I don't think this is giving you
> > accurate information about the behavior of
> > HTMLCanvasElement::shouldAccelerate().
> 
> Are you sure? The only thing we do with the return value from
> shouldAccelerate() is call GraphicsLayer::setAcceleratesDrawing() if it's
> true.

GraphicsLayer::acceleratesDrawing() is true by default on Mac and iOS device, so this line is mostly a no-op. I'm actually not sure why it was added (maybe before we turned on accel. drawing by default?).

What really matters for canvas backing store is the code in HTMLCanvasElement::createImageBuffer().