Bug 87747 - [chromium] Stop dropping texture limits when the layer tree host becomes invisible, and initialize with 0 allocation.
Summary: [chromium] Stop dropping texture limits when the layer tree host becomes invi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michal Mocny
URL:
Keywords:
Depends on: 88505
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-29 08:03 PDT by Michal Mocny
Modified: 2013-04-15 07:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.57 KB, patch)
2012-05-29 08:05 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (4.86 KB, patch)
2012-05-29 10:55 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cq-03 (360.60 KB, application/zip)
2012-05-29 12:54 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-02 (358.34 KB, application/zip)
2012-05-29 13:58 PDT, WebKit Review Bot
no flags Details
Patch (4.98 KB, patch)
2012-06-06 08:26 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (4.99 KB, patch)
2012-06-06 13:02 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Mocny 2012-05-29 08:03:02 PDT
[chromium] Stop dropping texture limits in when the layer tree host becomes invisible, and initialize with 0 allocation.
Comment 1 Michal Mocny 2012-05-29 08:05:17 PDT
Created attachment 144562 [details]
Patch
Comment 2 Michal Mocny 2012-05-29 08:09:09 PDT
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 3 Dana Jansens 2012-05-29 09:11:49 PDT
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 4 Michal Mocny 2012-05-29 09:13:08 PDT
Comment on attachment 144562 [details]
Patch

Removing CQ since may need to update some unit tests first.
Comment 5 Adrienne Walker 2012-05-29 10:42:20 PDT
Comment on attachment 144562 [details]
Patch

I'm happy to see this code get removed.  Thanks for following up on this.
Comment 6 Michal Mocny 2012-05-29 10:55:54 PDT
Created attachment 144586 [details]
Patch
Comment 7 Michal Mocny 2012-05-29 10:58:43 PDT
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 8 Adrienne Walker 2012-05-29 12:26:36 PDT
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 9 WebKit Review Bot 2012-05-29 12:53:58 PDT
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
Comment 10 WebKit Review Bot 2012-05-29 12:54:02 PDT
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 11 WebKit Review Bot 2012-05-29 13:58:03 PDT
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
Comment 12 WebKit Review Bot 2012-05-29 13:58:07 PDT
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
Comment 13 Michal Mocny 2012-05-30 10:52:35 PDT
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.
Comment 14 Dana Jansens 2012-05-30 10:53:43 PDT
Can't test shell set an allocation when it sets up the CCLTH?
Comment 15 Michal Mocny 2012-05-30 11:36:19 PDT
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.
Comment 16 Michal Mocny 2012-05-30 11:54:25 PDT
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?
Comment 17 Nat Duca 2012-05-30 12:32:30 PDT
Make webgraphicscontext3dinprocessimpl not say that it supports the manager extension until it actually supports it.
Comment 18 Michal Mocny 2012-05-30 13:00:25 PDT
(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.
Comment 19 Nat Duca 2012-05-30 18:23:38 PDT
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
Comment 20 Michal Mocny 2012-05-31 07:02:13 PDT
(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.
Comment 21 Michal Mocny 2012-06-06 08:26:09 PDT
Created attachment 146039 [details]
Patch
Comment 22 Michal Mocny 2012-06-06 08:32:58 PDT
(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.
Comment 23 Michal Mocny 2012-06-06 13:02:49 PDT
Created attachment 146090 [details]
Patch
Comment 24 WebKit Review Bot 2012-06-06 22:00:17 PDT
Comment on attachment 146090 [details]
Patch

Clearing flags on attachment: 146090

Committed r119683: <http://trac.webkit.org/changeset/119683>
Comment 25 WebKit Review Bot 2012-06-06 22:00:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 WebKit Review Bot 2012-06-06 22:58:31 PDT
Re-opened since this is blocked by 88505
Comment 28 Ryosuke Niwa 2012-06-06 23:56:28 PDT
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
Comment 29 Ryosuke Niwa 2012-06-06 23:57:14 PDT
Also broke the same test on Mac:
http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/11311
Comment 30 Michal Mocny 2012-06-07 06:57:40 PDT
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.
Comment 31 Michal Mocny 2012-06-07 07:02:14 PDT
.. or not.  WebGraphicsContext3DInProcessImpl does not seem to support any extensions by default.
Comment 32 Michal Mocny 2012-06-07 10:33:23 PDT
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.
Comment 33 Stephen Chenney 2013-04-15 07:25:20 PDT
Chromium bug is marked fixed and inactive.