[chromium] Stop dropping texture limits in when the layer tree host becomes invisible, and initialize with 0 allocation.
Created attachment 144562 [details] Patch
Fix for crbug.com/129266 As well as not lowering memory limit when going invisible, we should not increase our allocation on initialization. We would like to get rid of all places where memory allocation numbers are assumed by the renderer.
Comment on attachment 144562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144562&action=review > Source/WebCore/ChangeLog:3 > + [chromium] Stop dropping texture limits in when the layer tree host becomes invisible, and initialize with 0 allocation. nit: "in when"? > Source/WebCore/ChangeLog:8 > + GpuMemoryManager manager texture memory allocation limits, and will send a 0 allocation when a renderer becomes nit: GpuMemoryManager manages?
Comment on attachment 144562 [details] Patch Removing CQ since may need to update some unit tests first.
Comment on attachment 144562 [details] Patch I'm happy to see this code get removed. Thanks for following up on this.
Created attachment 144586 [details] Patch
Comment on attachment 144586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144586&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-153 > - m_memoryAllocationBytes = TextureManager::highLimitBytes(deviceViewportSize()); Original patch set these to (0, false), but that is the default value given in the constructor, and it means we overwrite any potential allocations received before initialization. At the moment, we won't receive any allocations before this point (except in tests), but its unnecessary to blindly overwrite here anyways.
Comment on attachment 144586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144586&action=review R=me. Thanks! >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-153 >> - m_memoryAllocationBytes = TextureManager::highLimitBytes(deviceViewportSize()); > > Original patch set these to (0, false), but that is the default value given in the constructor, and it means we overwrite any potential allocations received before initialization. > At the moment, we won't receive any allocations before this point (except in tests), but its unnecessary to blindly overwrite here anyways. Ah, that makes a lot of sense.
Comment on attachment 144586 [details] Patch Rejecting attachment 144586 [details] from commit-queue. New failing tests: compositing/checkerboard.html compositing/backface-visibility/backface-visibility-hierarchical-transform.html animations/3d/matrix-transform-type-animation.html animations/3d/state-at-end-event-transform.html compositing/direct-image-compositing.html compositing/backface-visibility/backface-visibility-non3d.html compositing/video-page-visibility.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html compositing/scrollbar-painting.html compositing/clip-change.html compositing/text-on-large-layer.html compositing/layers-inside-overflow-scroll.html compositing/animation/state-at-end-event-transform-layer.html compositing/sibling-positioning.html compositing/generated-content.html compositing/fixed-position-changed-in-composited-layer.html compositing/self-painting-layers.html animations/3d/change-transform-in-end-event.html compositing/absolute-position-changed-with-composited-parent-layer.html compositing/absolute-position-changed-in-composited-layer.html compositing/flat-with-transformed-child.html compositing/backface-visibility/backface-visibility-3d.html compositing/compositing-visible-descendant.html compositing/fixed-position-changed-within-composited-parent-layer.html compositing/color-matching/image-color-matching.html compositing/backface-visibility/backface-visibility-simple.html compositing/color-matching/pdf-image-match.html Full output: http://queues.webkit.org/results/12844602
Created attachment 144603 [details] Archive of layout-test-results from ec2-cq-03 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 144586 [details] Patch Attachment 144586 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12853347 New failing tests: compositing/checkerboard.html compositing/backface-visibility/backface-visibility-hierarchical-transform.html animations/3d/matrix-transform-type-animation.html animations/3d/state-at-end-event-transform.html compositing/direct-image-compositing.html compositing/backface-visibility/backface-visibility-non3d.html compositing/video-page-visibility.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html compositing/scrollbar-painting.html compositing/clip-change.html compositing/text-on-large-layer.html compositing/layers-inside-overflow-scroll.html compositing/animation/state-at-end-event-transform-layer.html compositing/sibling-positioning.html compositing/generated-content.html compositing/fixed-position-changed-in-composited-layer.html compositing/self-painting-layers.html animations/3d/change-transform-in-end-event.html compositing/absolute-position-changed-with-composited-parent-layer.html compositing/absolute-position-changed-in-composited-layer.html compositing/flat-with-transformed-child.html compositing/backface-visibility/backface-visibility-3d.html compositing/compositing-visible-descendant.html compositing/fixed-position-changed-within-composited-parent-layer.html compositing/color-matching/image-color-matching.html compositing/backface-visibility/backface-visibility-simple.html compositing/color-matching/pdf-image-match.html
Created attachment 144614 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Layout tests are failing because DRT uses the in-process web graphics context which isn't written to use memory allocations yet. Working on that now, but it will require a chromium patch, so this will have to wait.
Can't test shell set an allocation when it sets up the CCLTH?
Test shell doesn't seem to have the hooks I would need. I've been digging for a while but it only has access to WebViewHost, etc.
A reasonable option seems to be to add a "default allocation value" to WebSettings, and set a non-zero value in TestShell via its WebPreferences. @enne, can you advice if this is a good approach?
Make webgraphicscontext3dinprocessimpl not say that it supports the manager extension until it actually supports it.
(In reply to comment #17) > Make webgraphicscontext3dinprocessimpl not say that it supports the manager extension until it actually supports it. For that matter, I need to give CCLTH some default allocation when using a context that doesn't support the extension.
That seems clean. Saying back to make sure I'm following, I'm hearing: 0. make wgc3dipcbi not advertise gpumm extension 1. Make the compositor behave correctly when the extension isn't supported 2. this patch
(In reply to comment #19) > That seems clean. Saying back to make sure I'm following, I'm hearing: > 0. make wgc3dipcbi not advertise gpumm extension Landed yesterday: http://src.chromium.org/viewvc/chrome?view=rev&revision=139680 > 1. Make the compositor behave correctly when the extension isn't supported I am planning to have LRC set a default value when it tests the extension support during initialization. Patch up soon. > 2. this patch Yep.
Created attachment 146039 [details] Patch
(In reply to comment #20) > (In reply to comment #19) > > That seems clean. Saying back to make sure I'm following, I'm hearing: > > 0. make wgc3dipcbi not advertise gpumm extension > Landed yesterday: http://src.chromium.org/viewvc/chrome?view=rev&revision=139680 > > > 1. Make the compositor behave correctly when the extension isn't supported > I am planning to have LRC set a default value when it tests the extension support during initialization. Patch up soon. This landed yesterday (http://trac.webkit.org/changeset/119554). > > > 2. this patch > Yep. Rebased this patch and running layout tests locally now, but its already clear that this went from all pixel tests failing to most passing with this patch.
Created attachment 146090 [details] Patch
Comment on attachment 146090 [details] Patch Clearing flags on attachment: 146090 Committed r119683: <http://trac.webkit.org/changeset/119683>
All reviewed patches have been landed. Closing bug.
It appears that this patch caused WebLayerTreeViewThreadedTest.InstrumentationCallbacks to fail: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/17516/steps/webkit_unit_tests/logs/InstrumentationCallbacks http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.7/builds/6972/steps/webkit_unit_tests/logs/InstrumentationCallbacks blame list: http://trac.webkit.org/log/?verbose=on&rev=119683&stop_rev=119678
Re-opened since this is blocked by 88505
It appears that this patch also broke Canvas2DAllowed browser test: http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/21762 (view as text) GpuFeatureTest.Canvas2DAllowed: [22863:22863:0606/231709:1910369731:WARNING:zygote_host_impl_linux.cc(165)] Running without the SUID sandbox! See http://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment for more information on developing with the sandbox on. chrome/test/gpu/gpu_feature_browsertest.cc:147: Failure Expected: (analyzer_->FindEvents(Query::EventName() == Query::String("SwapBuffers"), &events)) > (size_t(0)), actual: 0 vs 0
Also broke the same test on Mac: http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/11311
These seem to have failed because they are tests running outside the sandbox where WebGraphicsContext3DInProcessImpl is used. I am going to land a patch to fix allocations in that context as well (I was not aware that it was used). This seems to be the last context, so hopefully these will be the last test to fail.
.. or not. WebGraphicsContext3DInProcessImpl does not seem to support any extensions by default.
GpuFeatureTest.Canvas2DAllowed is actually using the standard non-in-process WebGraphicsContext3DCommandBufferImpl which does support the gpu_memory_manager extension. However, the test itself seems to be using request animation frame and counting swaps, and perhaps RAF is firing even when we aren't drawing frames due to having no allocation. Investigating.
Chromium bug is marked fixed and inactive.