Allowing gargantuan canvases to have accelerated backing stores creates an opportunity for some lame pathologies. <rdar://problem/30119483>
Created attachment 300864 [details] Proposed patch
Please split this into 2 patches.
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?
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
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
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
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
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
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
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
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
Created attachment 300879 [details] Proposed patch
(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.
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?
(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.
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.
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.
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
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
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.
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.
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
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
Created attachment 300939 [details] Proposed patch
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.
Comment on attachment 300939 [details] Proposed patch Clearing flags on attachment: 300939 Committed r211949: <http://trac.webkit.org/changeset/211949>
All reviewed patches have been landed. Closing bug.
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().
(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.
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.
(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().