Bug 87747 – [chromium] Stop dropping texture limits when the layer tree host becomes invisible, and initialize with 0 allocation.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Michal Mocny]

[chromium] Stop dropping texture limits in when the layer tree host becomes invisible, and initialize with 0 allocation.

[Michal Mocny]

Created attachment 144562 [details]
Patch

[Michal Mocny]

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.

[Dana Jansens]

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?

[Michal Mocny]

Comment on attachment 144562 [details]
Patch

Removing CQ since may need to update some unit tests first.

[Adrienne Walker]

Comment on attachment 144562 [details]
Patch

I'm happy to see this code get removed.  Thanks for following up on this.

[Michal Mocny]

Created attachment 144586 [details]
Patch

[Michal Mocny]

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.

[Adrienne Walker]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Michal Mocny]

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.

[Dana Jansens]

Can't test shell set an allocation when it sets up the CCLTH?

[Michal Mocny]

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.

[Michal Mocny]

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?

[Nat Duca]

Make webgraphicscontext3dinprocessimpl not say that it supports the manager extension until it actually supports it.

[Michal Mocny]

(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.

[Nat Duca]

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

[Michal Mocny]

(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.

[Michal Mocny]

Created attachment 146039 [details]
Patch

[Michal Mocny]

(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.

[Michal Mocny]

Created attachment 146090 [details]
Patch

[WebKit Review Bot]

Comment on attachment 146090 [details]
Patch

Clearing flags on attachment: 146090

Committed r119683: <http://trac.webkit.org/changeset/119683>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[WebKit Review Bot]

Re-opened since this is blocked by 88505

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Also broke the same test on Mac:
http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/11311

[Michal Mocny]

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.

[Michal Mocny]

.. or not.  WebGraphicsContext3DInProcessImpl does not seem to support any extensions by default.

[Michal Mocny]

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.

[Stephen Chenney]

Chromium bug is marked fixed and inactive.