Bug 85108 – [chromium] Contents flash when switching to a tab with all discarded textures

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

[Nat Duca]

[chromium] Contents flash when switching to a tab with all discarded textures

[Nat Duca]

Created attachment 139302 [details]
Patch

[Dana Jansens]

Comment on attachment 139302 [details]
Patch

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

I'm not sure how i feel about counting "tiles" instead of "quads". It does let you only count in the tiled layers.. but I feel a little bit weird that we are excluding other layers from the decision. What if we checkerboard all tiled layers but there's a video updating? Is it correct that we skip frames? Maybe it is.. Enne what do you think?

(If we were to not make it just tiles, I think maybe sticking the stats into CCQuadCuller might be nice. We could have append() and appendPlaceholder/appendCheckerboard() methods, or just pass a checkerboard bool to append.)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:307
> +    // moving due to an impl-animation, or it is fully checkerboarded, we drop

nit: "or the frame is fully"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:349
> +    bool drawFrame = true;
> +    if (hadAnimatingLayerWithCheckerboard)
> +        drawFrame = false;
> +    if (allLayersWereFullyCheckerboarded)
> +        drawFrame = false;

bool drawFrame = !hadAnimatingLayerWithCheckerboard && !allLayersWereFullyCheckerboarded; ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:406
> +        // We're missing textures on an animating layer, or fully whited out.

We're going to have to update this comment every time prepareToDraw changes. Can it say something like "The frame is not able to be drawn, request a commit so we can resume drawing" ?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:74
> +        return m_numTiles > 0;

just return m_numTiles; like in the next method

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:80
> +    bool hadAtLeastOneCheckerboardedTile() const
> +    {
> +        return m_numTilesWithCheckerboards;
> +    }

maybe make each of these functions into a single line since there is only one statement?

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:212
> +            appendStats.didAppendQuad();

only if append() returns true.

[Adrienne Walker]

Comment on attachment 139302 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:343
> +    bool allLayersWereFullyCheckerboarded = numLayersThatAppendedTiles > 0 && numLayersThatAppendedTiles == numLayersThatWereFullyCheckerboarded;

Shouldn't drawFrame also end up being false when numLayersThatAppendedTiles == 0? If so, can you add a test for that case?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:53
> +class CCAppendStats {

I like this a lot.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:57
> +            : m_numTiles(0)
> +            , m_numTilesWithCheckerboards(0)

s/Tiles/Quads/

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:169
> +void CCVideoLayerImpl::appendQuads(CCQuadCuller& quadList, const CCSharedQuadState* sharedQuadState, CCAppendStats&)

Why don't these overrides of appendQuads not set statistics? What if you have a full-screen video layer occluding everything else?

[Nat Duca]

Created attachment 141821 [details]
Patch

[Dana Jansens]

Comment on attachment 141821 [details]
Patch

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

looks good with a few nits

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:309
> +    // moving due to an impl-animation, or it is fully checkerboarded, we drop

nit: "or the frame is fully checkerboarded" (not the layer)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:407
> +        // We're missing textures on an animating layer, or fully whited out.

nit: "or fully checkerboarded"

> Source/WebCore/platform/graphics/chromium/cc/CCQuadList.h:36
> +enum CCQuadCheckerbaordState {

Checkerbaord? :)

> Source/WebCore/platform/graphics/chromium/cc/CCQuadList.h:69
> +    virtual void appendSurface(PassOwnPtr<CCDrawQuad> passDrawQuad)
> +    {
> +        // Surface quads don't count toward the plain quad count since,
> +        // if they were checkerboarded themselves, they would be identified as such
> +        // in a previous draw pass.
> +        m_quads.append(passDrawQuad);
> +    }

is this and the replica one used? seems like only appendQuadInternal does any appending in CCQuadCuller, and just calls the append() function in this class.

[James Robinson]

Comment on attachment 141821 [details]
Patch

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

I'm not sure why you would need to distinguish between "CantCheckerboard" and "normal" quads.  The states seem to be:
1.) No quads at all
2.) Some quads, all checkerboard
3.) Some quads that were checkerboard, some were not

when does it matter how many checkerboardable-vs-non-checkerboardable quads we got that weren't checkerboards?

> Source/WebCore/platform/graphics/chromium/cc/CCQuadList.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

> Source/WebCore/platform/graphics/chromium/cc/CCQuadList.h:36
> +enum CCQuadCheckerbaordState {

CCQuadCheckerbaordState -> CCQuadCheckerboardState (Baord -> Board)

Tools/Scripts/do-webcore-rename might be handy for fixing this in your tree without a bunch of manual edits

> Source/WebCore/platform/graphics/chromium/cc/CCQuadList.h:89
> +    CCDrawQuad* operator[](size_t i) { return m_quads[i].get(); }

who's using the mutating getters?

we have 6 getters on the vector here (7 including size()).  should we just expose a const Vector<OwnPtr<CCDrawQuad> >& getter?

[Nat Duca]

Created attachment 141829 [details]
Patch

[Dana Jansens]

Why the switch to making things like video "CanNotCheckerboard"? Now they don't count as content, and a frame with a non-checkerboarded video layer and a checkerboarded tiled layer would get skipped, no?

It made sense to me that a RenderSurface quad would count as "CanNotCheckerboard" - since it doesn't contribute content to the page, but I don't get the video/etc being this. (Maybe a better name than CanNotCheckerboard? Not that I can think of one right now..)

[Nat Duca]

I think it is still better to seal the vector nature of CCQuadList. I prefer to hide implementation so we can, e.g. serialize it and not deal with random tests that are assuming bits about the vector nature of the quadlist.

Both the const [] and the non consts were needed. There are places that use a const QuadList that want CCQuads, and some places that dont. I could probably remove them if its really giving someone huge heartburn.

The tri-state is my current thinking on how to deal with a screen with 2 quads, 1 that is checkerboarded and 1 that is video. We lost the texture quads but not the video.

The current rule is "if everything is checkerboards." So, having that video quad spoils the party. We dont want to switch back to youtube and see checkerboards and a video, hence the "this cant checkerboard state." The rule is "if everything that can checkerboard IS checkerboarded." That fixes things like debug borders, solid color layers, scrollbar layers, etc... none of whom understand not having textures.

The one open I've got in tis is handling of render passes and replicas that themselves are fully checkerboarded. Right now, we resolve this as "if all the layers we saw were themselves fully checkerboarded." I think thats good enough for m20, but we might need something better in the future.

Thoughts?

[Dana Jansens]

(In reply to comment #9)
> The one open I've got in tis is handling of render passes and replicas that themselves are fully checkerboarded. Right now, we resolve this as "if all the layers we saw were themselves fully checkerboarded." I think thats good enough for m20, but we might need something better in the future.

I guess my last question is about partial swap - and this will be more obvious again with caching render surfaces. If we are partial swapping a single quad out of 50, and it is checkerboarded, then what?

[Nat Duca]

I dont know. What do you think will happen?

[Dana Jansens]

(In reply to comment #11)
> I dont know. What do you think will happen?

I think we'd skip frames so output would freeze due to a small amount of checkerboard.

Since this is supposed to handle the tab switch 0 texture case, what if it's gated on the partialSwapRect == viewportRect?

[Dana Jansens]

Idea: What if we count all quads which are given to CCQuadCuller, before culling is applied. This would avoid not counting quads culled due to partial swap, and count everything in the frame.

Things that are occluded would be counted, but in order to be occluded something else must be visible and not checkerboarded, and we will have counted that. (It might be a video not tiles doing the occluding, but this should catch most cases?)

[Adrienne Walker]

(In reply to comment #9)

> The tri-state is my current thinking on how to deal with a screen with 2 quads, 1 that is checkerboarded and 1 that is video. We lost the texture quads but not the video.

Are you trying to argue that we should or should not draw that state?

> The current rule is "if everything is checkerboards." So, having that video quad spoils the party. We dont want to switch back to youtube and see checkerboards and a video, hence the "this cant checkerboard state." The rule is "if everything that can checkerboard IS checkerboarded." That fixes things like debug borders, solid color layers, scrollbar layers, etc... none of whom understand not having textures.

I don't really agree with this.  For example, a full screen video quad covering the entire root layer.  The root layer will potentially checkerboard because it may have never been painted.  Depending on whether or not you count quads pre-culling or post-culling, you'll either have just a video quad or a video quad and checkerboards.  I think this is a valid case to draw.

> The one open I've got in tis is handling of render passes and replicas that themselves are fully checkerboarded. Right now, we resolve this as "if all the layers we saw were themselves fully checkerboarded." I think thats good enough for m20, but we might need something better in the future.

If a render pass has content in it, then that's probably good enough.  The output of that render pass might be opaque-culled, but if so that means there's other valid content.  So, I think "does any render pass have valid content" is probably a sufficient signal.


To be honest, all of these edge cases give this heuristic a bit of a code smell.  To up-level this a little (*puts on nduca hat*), we're trying to solve the case where we've discarded a ton of textures and shouldn't draw.  In order to get into this state, we've discarded a ton of textures and then committed that information.  This patch is trying to implicitly detect when that has happened.  Why don't we just explicitly have some "please do not draw until you get another commit" state that we set when dropping textures? Don't we already have a similar path with acquireLayerTextures()? If it turns out that we already have all the content we need, the commit will be super quick and it shouldn't matter.  If it turns out we don't, then we can wait to draw until w edo.

[Dana Jansens]

(In reply to comment #14)
> To be honest, all of these edge cases give this heuristic a bit of a code smell.  To up-level this a little (*puts on nduca hat*), we're trying to solve the case where we've discarded a ton of textures and shouldn't draw.  In order to get into this state, we've discarded a ton of textures and then committed that information.  This patch is trying to implicitly detect when that has happened.  Why don't we just explicitly have some "please do not draw until you get another commit" state that we set when dropping textures? Don't we already have a similar path with acquireLayerTextures()? If it turns out that we already have all the content we need, the commit will be super quick and it shouldn't matter.  If it turns out we don't, then we can wait to draw until w edo.

The situation is only slightly different than you stated I think. We do in order:
- setVisible false
- discard all textures
- set texture limit to 0
- setNeedsCommit
- setVisible true
- commit
- set texture limit to normal
- setNeedsCommit
- commit

I think you're suggesting a "needs a commit" flag when we setVisible false? But the initial commit when we become visible is what causes the symptom to show.

It's like we need a "just don't draw until i say so" and the when we get texture memory we say so or something...

[Adrienne Walker]

(In reply to comment #15)
 
> I think you're suggesting a "needs a commit" flag when we setVisible false? But the initial commit when we become visible is what causes the symptom to show.

So, alternatively, don't send that commit when we first become visible, because it's not useful until we change texture limits?

[Dana Jansens]

(In reply to comment #16)
> (In reply to comment #15)
> 
> > I think you're suggesting a "needs a commit" flag when we setVisible false? But the initial commit when we become visible is what causes the symptom to show.
> 
> So, alternatively, don't send that commit when we first become visible, because it's not useful until we change texture limits?

Yeh agree but not sure it's possible. Right now it's implied that we commit after (we setNeedsCommit when becoming non-visible). But I think WebCore could cause us to setNeedsCommit at any time while being non-visible.

We need to sure a commit happens in finite time once its been requested. But what if we still have our normal texture allocation (fast tab switching)? Then there's no texture memory change to setNeedsCommit about.

We could drop commits on the floor while texture limit is 0. But in the future the limit will be reduced to <some value not always 0> to help speed up bringing background tabs to life.

Or we could drop commits on the floor while we have a "background memory allocation" if we flagged them as such?

[Michal Mocny]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > 
> > > I think you're suggesting a "needs a commit" flag when we setVisible false? But the initial commit when we become visible is what causes the symptom to show.
> > 
> > So, alternatively, don't send that commit when we first become visible, because it's not useful until we change texture limits?
> 
> Yeh agree but not sure it's possible. Right now it's implied that we commit after (we setNeedsCommit when becoming non-visible). But I think WebCore could cause us to setNeedsCommit at any time while being non-visible.
I changed it so we do not call commit when becoming non-visible, and we still commit when switching back to visible before getting a memory allocation, so something else is causing a commit (not sure where)

> 
> We need to sure a commit happens in finite time once its been requested. But what if we still have our normal texture allocation (fast tab switching)? Then there's no texture memory change to setNeedsCommit about.
> 
> We could drop commits on the floor while texture limit is 0. But in the future the limit will be reduced to <some value not always 0> to help speed up bringing background tabs to life.
> 
> Or we could drop commits on the floor while we have a "background memory allocation" if we flagged them as such?
I like this idea!  Thoughts?

[Dana Jansens]

(In reply to comment #18)
> I changed it so we do not call commit when becoming non-visible, and we still commit when switching back to visible before getting a memory allocation, so something else is causing a commit (not sure where)

Then the commit when we come back to visible is the offender now instead of the one when we go invisible. The two seem identical to me?

[Dana Jansens]

Created attachment 142585 [details]
Patch

[Dana Jansens]

This patch blocks drawing of frames that have memory allocations which are not meant for display.

Such frames are incomplete even if non-zero. Right now they are always zero. When the GPUMemMgr gives out non-zero allocations, it will be meant to speed up the time to generate a valid frame, the frame with the background allocation will still not be a valid one.

[WebKit Review Bot]

Comment on attachment 142585 [details]
Patch

Attachment 142585 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12723450

[Adrienne Walker]

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

So, I'll accept that maybe the gpu memory limit messages can (in the future) include some intent about whether the limit is intended to be a drawable amount or a reduction because something is invisible.  And, in the present, we can just accept 0 to be "not visible".

However, I'd like to keep that assumption contained to the setContentsMemoryAllocationLimitBytes function.  That way the semantics in the short term are setContentsMemoryAllocationLimitBytes(0) => stop drawing after the next commit and setContentsMemoryAllocationLimitBytes(anything else) => start drawing after the next commit, and there is no interaction with visibility messages or visibility state that may or may not come before memory messages.

So, I'd love if CCLayerTreeHost::m_frameHasMemoryAllocationForVisibleState could only be written to in setContentsMemoryAllocationLimitBytes.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:162
> +    // If we are still using a non-visible memory allocation, but have
> +    // become visible, we shall not draw until we have a frame with a
> +    // proper allocation for display.
> +    if (!m_sourceFrameHasMemoryAllocationForVisibleState)

If you'd like, you could just call this m_canDrawFromSourceFrame on the impl side if it needs this much explanation.

[Dana Jansens]

(In reply to comment #23)
> View in context: https://bugs.webkit.org/attachment.cgi?id=142585&action=review
> 
> So, I'll accept that maybe the gpu memory limit messages can (in the future) include some intent about whether the limit is intended to be a drawable amount or a reduction because something is invisible.  And, in the present, we can just accept 0 to be "not visible".
> 
> However, I'd like to keep that assumption contained to the setContentsMemoryAllocationLimitBytes function.  That way the semantics in the short term are setContentsMemoryAllocationLimitBytes(0) => stop drawing after the next commit and setContentsMemoryAllocationLimitBytes(anything else) => start drawing after the next commit, and there is no interaction with visibility messages or visibility state that may or may not come before memory messages.

This would be nice, I tried this at first, but is a race. That's the "frameHas" part of the variable. We care about what the allocation was when we actually allocated the textures (updateLayers) for the frame. The allocation can change mid-way through a commit, but the value is passed to hostImpl at the end of the commit. So we save the value at the beginning of the commit, to go along with the allocation value we actually used for that frame.

> So, I'd love if CCLayerTreeHost::m_frameHasMemoryAllocationForVisibleState could only be written to in setContentsMemoryAllocationLimitBytes.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:162
> > +    // If we are still using a non-visible memory allocation, but have
> > +    // become visible, we shall not draw until we have a frame with a
> > +    // proper allocation for display.
> > +    if (!m_sourceFrameHasMemoryAllocationForVisibleState)
> 
> If you'd like, you could just call this m_canDrawFromSourceFrame on the impl side if it needs this much explanation.

Hm, yeh, I think the variable is already descriptive enough. I made it better since I wrote that comment :) Thanks.

[Dana Jansens]

Created attachment 142601 [details]
Patch

[Michal Mocny]

Comment on attachment 142585 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:459
> +    m_frameHasMemoryAllocationForVisibleState = false;

Does this need to reset here?

[Michal Mocny]

(In reply to comment #24)
> (In reply to comment #23)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=142585&action=review
> > 
> > So, I'll accept that maybe the gpu memory limit messages can (in the future) include some intent about whether the limit is intended to be a drawable amount or a reduction because something is invisible.  And, in the present, we can just accept 0 to be "not visible".
> > 
> > However, I'd like to keep that assumption contained to the setContentsMemoryAllocationLimitBytes function.  That way the semantics in the short term are setContentsMemoryAllocationLimitBytes(0) => stop drawing after the next commit and setContentsMemoryAllocationLimitBytes(anything else) => start drawing after the next commit, and there is no interaction with visibility messages or visibility state that may or may not come before memory messages.
> 
> This would be nice, I tried this at first, but is a race. That's the "frameHas" part of the variable. We care about what the allocation was when we actually allocated the textures (updateLayers) for the frame. The allocation can change mid-way through a commit, but the value is passed to hostImpl at the end of the commit. So we save the value at the beginning of the commit, to go along with the allocation value we actually used for that frame.
> 

Wouldn't it suffice to set m_frameHasMemoryAllocationForVisibleState inside setContentsMemoryAllocationLimitBytes, and not update it in updateLayers?

This way, as @enne suggests, the only test for zero/non-zero is constrained to there.

Also, could we rename frameHasMemoryAllocationForVisibleState to something less tied to visibility, like frameHasDrawableMemoryAllocation?
It will be possible one day for a non-visible tab to still retain a "foreground" allocation, and the reverse may also be necessary.

[Dana Jansens]

Had a fun plan in half-sleep!

setContentsMemoryAllocationLimitBytes can change what the "current allocation" is for. Then when we create a frame, we save that as the "frame allocation", and we can give that to impl.

@mocny, We have to save the allocation at the time we make the frame, because it can change during the frame creation, and the value from the time updateLayers is what matters. It's probably a bug that the allocation can change during a frame though. If we drop the allocation value between the time we updateLayers and do the commit of the tree, we'll have deleted texture ids on impl. This won't bite us right now, since we drop the lower (0) allocations when we are visible, but it's something to consider beyond just the thread issues.

The reason it doesn't bite us but we do have to save the value in updateLayers is:
nonVisAllocation -> updateLayers -> visAllocation -> commit visAllocation  ==  we draw a bad frame to the screen.  we avoid this by commiting the value saved in updateLayers
visAllocation -> upateLayers -> nonVisAllocation (dropped if visible) -> commit visAllocation == if we're visible, we dropped it. if not, we don't draw a good frame which is not a visible error.

[Dana Jansens]

Created attachment 142748 [details]
Patch

[Adrienne Walker]

Comment on attachment 142748 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:428
> +    if (m_visible && !isMemoryAllocationForDisplay)
> +        return;

The visible check here seems a little dodgy and dependent on gpu memory limit calls racing visibility calls.  What are you trying to do?

[Michal Mocny]

(In reply to comment #30)
> (From update of attachment 142748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142748&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:428
> > +    if (m_visible && !isMemoryAllocationForDisplay)
> > +        return;
> 
> The visible check here seems a little dodgy and dependent on gpu memory limit calls racing visibility calls.  What are you trying to do?

Thats exactly right.

GpuMemoryManager sends allocations based on visibility state which may have changed.
As you mentioned previously we should change to tag memory allocations with an expected visibility state and then test "if (m_visible != expected_visibility_state)".  For now, we use allocation == 0 as a proxy.

Not sure if there is a more elegant solution, aside from tagging every visibility/allocation ipc with an incrementing "state of the world" id and testing that they all agree.

[Dana Jansens]

Created attachment 142779 [details]
Patch

rebased

[Adrienne Walker]

I understand the visibility check, but the fact that setVisible and setContentsMemoryAllocationLimitBytes both result in touching memory limits seems really wrong to me.  That visibility check in setContentsMemoryAllocationLimitBytes only builds on top of this mess, which is why it feels so dodgy to me.

I'd like to clean this up now and not punt.  (I'm fine with splitting these changes up however makes sense, but I consider them blocking to fixing this properly, if that makes sense.)

(1) setVisible should primarily affect draw scheduling, preventing draws when the impl thread knows that it is not visible.  It should probably also prevent additional commits (insert hand waving here about getting in another commit to delete textures when !visible).  setVisible should never adjust memory limits.

(2) CCLayerTreeHost::didBecomeInvisibleOnImplThread feels like a hack.  It changes memory limits and is like a mini-commit.  It should not change memory limits and ideally information like memory changes should be transferred in a real commit so that this function doesn't accrete more things in the same way that it has accreted animation updates.  My hope is that this function can go away entirely.

(3) setContentsMemoryAllocationLimitBytes should be the only thing that changes memory limits.  It should never need to check visibility state.  For now, I'm fine with implicitly assuming that setContentsMemoryAllocationLimitBytes(0) is the only non-drawable state and all other memory limits are drawable.  Texture memory limits changes should result in a commit even when !visible (insert hand-waving here).

This gets us into a state where we no longer have to care about raciness between setVisible and setContentsMemoryAllocationLimitBytes because the concerns and responsibilities have been separated.

[Nat Duca]

Agree with Enne on all fronts. We missed RBB, so lets do this right and not do anything based on setVisible.

[Dana Jansens]

All well said. I don't see that as a blocker for this change though.. do you?

[WebKit Review Bot]

Comment on attachment 142779 [details]
Patch

Attachment 142779 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12717959

New failing tests:
compositing/checkerboard.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
compositing/direct-image-compositing.html
compositing/video-page-visibility.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/scrollbar-painting.html
compositing/backface-visibility.html
compositing/clip-change.html
compositing/text-on-large-layer.html
compositing/layers-inside-overflow-scroll.html
compositing/sibling-positioning.html
compositing/generated-content.html
compositing/fixed-position-changed-in-composited-layer.html
compositing/self-painting-layers.html
compositing/culling/filter-occlusion-alpha-large.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-hierarchical-transform.html
compositing/culling/filter-occlusion-alpha.html
compositing/culling/clear-fixed-iframe.html
compositing/compositing-visible-descendant.html
compositing/fixed-position-changed-within-composited-parent-layer.html
compositing/color-matching/image-color-matching.html
compositing/color-matching/pdf-image-match.html

[WebKit Review Bot]

Created attachment 142785 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dana Jansens]

Created attachment 142789 [details]
Patch

Er, the default memory allocation is meant for display.

--- a/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp
+++ b/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp
@@ -148,6 +148,7 @@ void CCLayerTreeHost::initializeLayerRenderer()
     m_contentsTextureManager = TextureManager::create(TextureManager::highLimitBytes(deviceViewportSize()),
                                                       TextureManager::reclaimLimitBytes(deviceViewportSize()),
                                                       m_proxy->layerRendererCapabilities().maxTextureSize);
+    m_currentMemoryAllocationIsForDisplay = true;
 
     m_layerRendererInitialized = true;

[Adrienne Walker]

(In reply to comment #35)
> All well said. I don't see that as a blocker for this change though.. do you?

I agree that they're somewhat orthogonal, but this patch is harder to understand (especially if we need to do any followups to it) because it has to work around that mess or consider raciness between the visibility and memory messages.

That m_visibile conditional in setContentsMemoryAllocationLimitBytes is just too fragile for me.

[Dana Jansens]

(In reply to comment #39)
> (In reply to comment #35)
> > All well said. I don't see that as a blocker for this change though.. do you?
> 
> I agree that they're somewhat orthogonal, but this patch is harder to understand (especially if we need to do any followups to it) because it has to work around that mess or consider raciness between the visibility and memory messages.
> 
> That m_visibile conditional in setContentsMemoryAllocationLimitBytes is just too fragile for me.

I guess the other option is to have the GpuMemoryManager block the compositor.

GpuMemMgr <- Browser   ::   setVisible(false)

GpuMemMgr -> Compositor   ::   blockCompositor()
GpuMemMgr -> Compositor   ::   isVisible() == false?
GpuMemMgr -> Compositor   ::   setMemoryAllocation(0)
GpuMemMgr -> Compositor   ::   unblockCompositor()

I'm not sure how else to make the compositor only receive a visible allocation when it is visible, since the Browser can change it's state at any time.

[Dana Jansens]

Created attachment 142842 [details]
Patch

[Dana Jansens]

Created attachment 142844 [details]
Patch

[Dana Jansens]

Here we make visibility part of the commit instead of a special case, and we only set the impl thread visible when we are (visible && haveAValidMemoryAllocation) and we make sure we only update the memory allocation during a commit.

[Dana Jansens]

Created attachment 142877 [details]
Patch

- Making impl "visible" only when it has a frame to draw is a problem, because it has to become "visible" to get a visible memory allocation. so adding a second state to the impl side "sourceFrameCanBeDrawn" that is true when the frame being committed is one that is allowed to be drawn (has a foreground memory allocation).
- canDraw returns false when !sourceFrameCanBeDrawn
- CCSchedulerStateMachine should know that !visible means we are not going to draw until we commit. since commits are what make us visible now.
- Only force a commit when becoming visible
- Set memory limits in setVisible(false) using the same setContentsMemoryLimitBytes() method as any other code that wants to set a memory limit.
- CCSingleThreadProxy does not animate when !visible. Animation is handled by CCLTHImpl by posting a timer task when it is not visible, this makes behaviour consistent with CCThreadProxy and is verified by CCLayerTreeHostTestTickAnimationWhileBackgrounded.runSingleThread. Without this change, the timer was not used in single-thread as each animation tick would cause a composite which would immediately animate again, and background animations were unthrottled. We now use willAnimateLayers() in this test instead so that we can setVisible(false) before the animation happens and posts a commit in single-thread, ensuring that we only commit once while visible.
- Improve the unit test for sourceFrameCanBeDrawn business.


I removed an assert from CCSchedulerStateMachine. It said that if you were WAITING_FOR_FIRST_DRAW and you drew, then you expected to be (needsCommit || !visible). This is because if you changed to become visible, then it expected that you would need a commit as a result. However, now we change visiblity inside a commit, which occurs after ACTION_COMMIT. So we may draw now, while WAITING_FOR_FIRST_DRAW, because the commit made us visible, and that's not illegal.

[Adrienne Walker]

Comment on attachment 142877 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:411
> +    if (!m_visible && m_layerRendererInitialized) {
> +        // FIXME: When can we know that the GPU Memory Manager will handle this?
> +        if (m_proxy->layerRendererCapabilities().contextHasCachedFrontBuffer)
> +            setContentsMemoryAllocationLimitBytes(0);
> +        else
> +            setContentsMemoryAllocationLimitBytes(m_contentsTextureManager->preferredMemoryLimitBytes());

Not to be a broken record, but setVisible shouldn't change memory limits.

[Dana Jansens]

Comment on attachment 142877 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:411
>> +            setContentsMemoryAllocationLimitBytes(m_contentsTextureManager->preferredMemoryLimitBytes());
> 
> Not to be a broken record, but setVisible shouldn't change memory limits.

Yeh, hence FIXME. I'd like to punt on this little piece for now and let mocny address this if possible?

[Michal Mocny]

*** Bug 86539 has been marked as a duplicate of this bug. ***

[Dana Jansens]

Created attachment 143307 [details]
Patch

- Always force commit visibility changes, so they take priority over drawing frames, so we can go non-visible as fast as before.
- Only force commit memory limit changes when we're not visible.
- Piman and I seem to have added the scheduleComposite stuff to CCLTHTest at the same time :)

[Adrienne Walker]

Comment on attachment 143307 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:149
> -    m_contentsTextureManager = TextureManager::create(TextureManager::highLimitBytes(deviceViewportSize()),
> -                                                      TextureManager::reclaimLimitBytes(deviceViewportSize()),
> -                                                      m_proxy->layerRendererCapabilities().maxTextureSize);
> +    m_contentsTextureManager = TextureManager::create(0, 0, m_proxy->layerRendererCapabilities().maxTextureSize);

Where do you set the preferred limit, since you removed it here?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:405
> +    // FIXME: Remove this stuff, it is here just for the m20 merge.

Can you file a bug to remove this?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:428
> +    if (!m_visible)
> +        setNeedsForcedCommit();
> +    else
> +        setNeedsCommit();

This needs a comment.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:476
> +    bool memoryAllocationStateMatchesVisibility = m_visible == m_memoryAllocationIsForDisplay;
> +    if (memoryAllocationStateMatchesVisibility) {
> +        m_contentsTextureManager->setMemoryAllocationLimitBytes(m_memoryAllocationBytes);
> +        m_frameIsForDisplay = m_memoryAllocationIsForDisplay;
> +    }

This interaction of visibility and memory limits makes me sad.  What's going on here?  Why can't you just set the texture manager limits directly in setContentsMemoryAllocationLimitBytes and always set m_frameIsForDisplay here?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:339
> +    TRACE_EVENT("CCThreadProxy::setNeedsForcdCommitOnImplThread", this, 0);

s/Forcd/Forced/

[Dana Jansens]

Comment on attachment 143307 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:149
>> +    m_contentsTextureManager = TextureManager::create(0, 0, m_proxy->layerRendererCapabilities().maxTextureSize);
> 
> Where do you set the preferred limit, since you removed it here?

m_contentsTextureManager->setMemoryAllocationLimitBytes(m_memoryAllocationBytes);

This method, called from updateLayers, sets max and preferred.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:428
>> +        setNeedsCommit();
> 
> This needs a comment.

k

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:476
>> +    }
> 
> This interaction of visibility and memory limits makes me sad.  What's going on here?  Why can't you just set the texture manager limits directly in setContentsMemoryAllocationLimitBytes and always set m_frameIsForDisplay here?

This is taking care of the fact that memory allocations and visibility are completely asynchronous from each other.

If we are visible:
  If the current requested allocation is for being visible, update the contents texture manager.
  If the current requested allocation is for being invisible, don't use it right now. (We would use it if we went non-visible.)

If we are not visible:
  If the current requested allocation is for being invisible, update the contents texture manager.
  If the current requested allocation is for being visible, don't use it right now. (We would use it if we become visible.)

m_frameIsForDisplay matches the current allocation in the texture manager.

[Adrienne Walker]

Comment on attachment 143307 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:149
>>> +    m_contentsTextureManager = TextureManager::create(0, 0, m_proxy->layerRendererCapabilities().maxTextureSize);
>> 
>> Where do you set the preferred limit, since you removed it here?
> 
> m_contentsTextureManager->setMemoryAllocationLimitBytes(m_memoryAllocationBytes);
> 
> This method, called from updateLayers, sets max and preferred.

Wow, that's non-obvious from the method name and the rest of the API.  Sorry for the misunderstanding.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:476
>>> +    }
>> 
>> This interaction of visibility and memory limits makes me sad.  What's going on here?  Why can't you just set the texture manager limits directly in setContentsMemoryAllocationLimitBytes and always set m_frameIsForDisplay here?
> 
> This is taking care of the fact that memory allocations and visibility are completely asynchronous from each other.
> 
> If we are visible:
>   If the current requested allocation is for being visible, update the contents texture manager.
>   If the current requested allocation is for being invisible, don't use it right now. (We would use it if we went non-visible.)
> 
> If we are not visible:
>   If the current requested allocation is for being invisible, update the contents texture manager.
>   If the current requested allocation is for being visible, don't use it right now. (We would use it if we become visible.)
> 
> m_frameIsForDisplay matches the current allocation in the texture manager.

What would go wrong if you removed that conditional and always executed the code inside the block?  I follow what the code does, but I don't understand the reasoning.

[Dana Jansens]

Comment on attachment 143307 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:476
>>>> +    }
>>> 
>>> This interaction of visibility and memory limits makes me sad.  What's going on here?  Why can't you just set the texture manager limits directly in setContentsMemoryAllocationLimitBytes and always set m_frameIsForDisplay here?
>> 
>> This is taking care of the fact that memory allocations and visibility are completely asynchronous from each other.
>> 
>> If we are visible:
>>   If the current requested allocation is for being visible, update the contents texture manager.
>>   If the current requested allocation is for being invisible, don't use it right now. (We would use it if we went non-visible.)
>> 
>> If we are not visible:
>>   If the current requested allocation is for being invisible, update the contents texture manager.
>>   If the current requested allocation is for being visible, don't use it right now. (We would use it if we become visible.)
>> 
>> m_frameIsForDisplay matches the current allocation in the texture manager.
> 
> What would go wrong if you removed that conditional and always executed the code inside the block?  I follow what the code does, but I don't understand the reasoning.

We can get memory allocations of 0 while we are visible, due to fast tab switching. We'd commit and stop drawing until we got a non-0 allocation again.

[Michal Mocny]

Comment on attachment 143307 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:149
>>>> +    m_contentsTextureManager = TextureManager::create(0, 0, m_proxy->layerRendererCapabilities().maxTextureSize);
>>> 
>>> Where do you set the preferred limit, since you removed it here?
>> 
>> m_contentsTextureManager->setMemoryAllocationLimitBytes(m_memoryAllocationBytes);
>> 
>> This method, called from updateLayers, sets max and preferred.
> 
> Wow, that's non-obvious from the method name and the rest of the API.  Sorry for the misunderstanding.

Yes, sorry about this.  We initially wanted to remove the concepts of preferred/max and have a single limit.  This was not an option for Android, so instead we decided to call a single setter and have the texture manager divide it up internally.
This is all going to change with TextureManagerV2 anyway, so I am not sure how much time we should spend cleaning up.

[Adrienne Walker]

> We can get memory allocations of 0 while we are visible, due to fast tab switching. We'd commit and stop drawing until we got a non-0 allocation again.

...then the memory manager needs to have some hysteresis built into it and not do that.  The compositor should not have to be smart about this.

[Dana Jansens]

(In reply to comment #54)
> > We can get memory allocations of 0 while we are visible, due to fast tab switching. We'd commit and stop drawing until we got a non-0 allocation again.
> 
> ...then the memory manager needs to have some hysteresis built into it and not do that.  The compositor should not have to be smart about this.

Yeh, totally agree. This is not something that will ever be occurring in M20 though. Would you prefer we drop frames in 20/until GpuMemMgr is smarter? Or add a FIXME and file a bug?

[Michal Mocny]

(In reply to comment #54)
> > We can get memory allocations of 0 while we are visible, due to fast tab switching. We'd commit and stop drawing until we got a non-0 allocation again.
> 
> ...then the memory manager needs to have some hysteresis built into it and not do that.  The compositor should not have to be smart about this.

Sadly, the reason this happens is because of the delay it takes for the messages to propagate, not because the memory manager is sending the wrong allocation.

However, we could mitigate the issue inside the memory manager by limiting the amount of spam sent during rapid tab switch etc instead of trying to handle it here.

Dana, if we remove this check, is the temporary halt in frame draw actually visible?

[Adrienne Walker]

(In reply to comment #55)
> (In reply to comment #54)
> > > We can get memory allocations of 0 while we are visible, due to fast tab switching. We'd commit and stop drawing until we got a non-0 allocation again.
> > 
> > ...then the memory manager needs to have some hysteresis built into it and not do that.  The compositor should not have to be smart about this.
> 
> Yeh, totally agree. This is not something that will ever be occurring in M20 though. Would you prefer we drop frames in 20/until GpuMemMgr is smarter? Or add a FIXME and file a bug?

FIXME + bug sounds good to me.  If we're going to add this logic, I want it to be clear that it's working around something and isn't desirable.  Thanks.

[Antoine Labour]

I beg you, please do not "fix" races by adding delays, smoothing etc. It will only mask the problem, but not fix it, so the problem will still happen, but in a way that is more difficult to debug.


With the current design, the memory manager sits in another process, and communicates asynchronously. In particular, from the renderer point of view, there is a round trip between the time it's told it's visible, and the time it receives an allocation that lets it draw. No matter how much smarts you put into the memory manager, there will be a delay there.

So the compositor *has* to be smart about it:
1- either delay the draw until it receives an allocation that is compatible with that
2- or it has to ignore the /suggested/ allocation and pick one that is compatible with drawing


Other designs are possible, in particular where the renderer receives together the visibility flag and the corresponding allocation. But that is not what we have currently, and from my discussions with Nat/Michal it sounded like we prefer an asynchronous+loose allocator than a synchronous+strict one. We need to acknowledge that and implement code that is compatible with it.

[Dana Jansens]

Comment on attachment 143307 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:405
>> +    // FIXME: Remove this stuff, it is here just for the m20 merge.
> 
> Can you file a bug to remove this?

https://code.google.com/p/chromium/issues/detail?id=129266

>>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:476
>>>>> +    }
>>>> 
>>>> This interaction of visibility and memory limits makes me sad.  What's going on here?  Why can't you just set the texture manager limits directly in setContentsMemoryAllocationLimitBytes and always set m_frameIsForDisplay here?
>>> 
>>> This is taking care of the fact that memory allocations and visibility are completely asynchronous from each other.
>>> 
>>> If we are visible:
>>>   If the current requested allocation is for being visible, update the contents texture manager.
>>>   If the current requested allocation is for being invisible, don't use it right now. (We would use it if we went non-visible.)
>>> 
>>> If we are not visible:
>>>   If the current requested allocation is for being invisible, update the contents texture manager.
>>>   If the current requested allocation is for being visible, don't use it right now. (We would use it if we become visible.)
>>> 
>>> m_frameIsForDisplay matches the current allocation in the texture manager.
>> 
>> What would go wrong if you removed that conditional and always executed the code inside the block?  I follow what the code does, but I don't understand the reasoning.
> 
> We can get memory allocations of 0 while we are visible, due to fast tab switching. We'd commit and stop drawing until we got a non-0 allocation again.

https://code.google.com/p/chromium/issues/detail?id=129269

[Adrienne Walker]

(In reply to comment #58)
> I beg you, please do not "fix" races by adding delays, smoothing etc. It will only mask the problem, but not fix it, so the problem will still happen, but in a way that is more difficult to debug.

Thanks for chiming in.  I think we're on the same page here.  When I suggested hysteresis, it was to specifically address the "fast tab switching back and forth" use case and not the general ordering problem.

> So the compositor *has* to be smart about it:
> 1- either delay the draw until it receives an allocation that is compatible with that

This is what I would like to do.  Visibility and memory limits should ideally be considered independently and we should only draw when we both are visible and have a valid memory limit.

> 2- or it has to ignore the /suggested/ allocation and pick one that is compatible with drawing

This is what I would like to avoid.  Attempting to sort out the intended state of the world from conflicting information delivered asynchronously seems like the wrong kind of smart.

> Other designs are possible, in particular where the renderer receives together the visibility flag and the corresponding allocation. But that is not what we have currently, and from my discussions with Nat/Michal it sounded like we prefer an asynchronous+loose allocator than a synchronous+strict one. We need to acknowledge that and implement code that is compatible with it.

This needs more discussion after this patch.  I was under the (possibly incorrect) impression that the renderer receiving the visibility flag along with the allocation was a likely direction we were going, and wanted this patch to start heading there.

[Antoine Labour]

Ok, I think we're on the same page. I'm fine with delaying draws until we receive good allocation.
We need to keep in mind that during fast switches, we may receive multiple allocations that reduce to 0 and back to full while we stay visible, and we need to handle that too (which this patch does, I think) - we can reduce the likelihood of it happening by adding smoothing on the GPU process side, but we can't rely on it not happening at all.

As to whether or not we're moving towards a synchronous visibility+allocation scheme, I was under the impression that not but I don't know (and I'm not writing the code). I would be happier too if we did.

[Dana Jansens]

Created attachment 143437 [details]
Patch

[Dana Jansens]

WebViewImpl has been protecting all visible textures on the NonCompositedContentHost when a tab becomes invisible. It was introduced in https://bugs.webkit.org/show_bug.cgi?id=67883 . This code seems to be outdated, as we've been unprotecting all textures explicitly when we become non-visible for some time, and this violates the assumption that textures are unprotected in commitComplete() and left unprotected until updateLayers().

I've removed this code from WebViewImpl and NCCH, along with protectVisibleTileTextures() from LayerChromium/TiledLayerChromium. It was causing asserts in the TextureManager when we set the memory limit in updateLayers for a tab that had become invisible but now had protected textures.

[Adrienne Walker]

Comment on attachment 143437 [details]
Patch

R=me.  Let's land this puppy.

[WebKit Review Bot]

Comment on attachment 143437 [details]
Patch

Rejecting attachment 143437 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 lines).
Hunk #5 succeeded at 620 (offset 2 lines).
Hunk #6 succeeded at 639 (offset 2 lines).
Hunk #7 succeeded at 1220 (offset -2 lines).
Hunk #8 succeeded at 1577 (offset 2 lines).
Hunk #9 succeeded at 1887 (offset 1 line).
1 out of 9 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adrienne W..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12779310

[Dana Jansens]

Created attachment 143689 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 143689 [details]
Patch for landing

Rejecting attachment 143689 [details] from commit-queue.

New failing tests:
CCLayerTreeHostTestCanDrawBlocksDrawing.runSingleThread
Full output: http://queues.webkit.org/results/12800042

[WebKit Review Bot]

Created attachment 143712 [details]
Archive of layout-test-results from ec2-cq-02

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dana Jansens]

Heh.. my beautiful new unit test bites back.


diff --git a/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp b/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
index 90178fb..e149497 100644
--- a/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
+++ b/Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
@@ -941,6 +941,7 @@ public:
 
     virtual void beginTest()
     {
+        postSetNeedsCommitToMainThread();
     }

[Dana Jansens]

Created attachment 143833 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 143833 [details]
Patch for landing

Clearing flags on attachment: 143833

Committed r118383: <http://trac.webkit.org/changeset/118383>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.