Bug 93028 – [chromium] Paint animated layers immediately to avoid animation hiccups.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Eric Penner]

Paint animated layers immediately to avoid animation hiccups.

[Eric Penner]

Created attachment 156169 [details]
Patch

[Eric Penner]

Created attachment 156212 [details]
tweaks

[Eric Penner]

Created attachment 156214 [details]
tweak

[Eric Penner]

Comment on attachment 156214 [details]
tweak

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-387
> -            // any single time through the function.

Setting the persistent 'updated' flag here seems like a bug to me,
since this loop can exit the function early (making any previously
set 'updated' flags incorrect).

The 'updatedTempFlag' is set unconditionally and only referenced
once below, so it's value doesn't matter before or after this
function.

[Adrienne Walker]

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

Just so I can understand this change, let me say what it looks like this patch is doing.

Previous special case behavior:
* Entirely hidden but small animated layers with <= 9 tiles used to be entirely painted via idle painting
* Entirely hidden but large animated layers had their outer tiles painted

New special case behavior:
* Hidden or unhidden animated layers that smaller than the viewport(ish) get entirely painted
* If the animated layer is partially visible, the "entire painting" pass preempts "normal" painting of the partially visible part of the layer.

I'm fine with tweaking this special case logic to be better about animations in the short term.  However, I'd be happier if I had a better sense of the longer-term plan of how to handle prioritization of animated content, especially if it involved less special case logic in TiledLayerChromium.

> Source/WebCore/ChangeLog:3
> +        Paint animated layers immediately to avoid animation hiccups.

[chromium]

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:81
> +        , updatedTempFlag(false)

This should go in a different patch.  "Tweaks," eh?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:525
> +    IntSize viewportSize = layer->layerTreeHost() ? layer->layerTreeHost()->viewportSize() : IntSize();

deviceViewportSize?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:528
> +    return contentRect.width() <= viewportSize.width() + 64
> +           && contentRect.height() <= viewportSize.height() + 64;

64?

[Eric Penner]

(In reply to comment #5)
> New special case behavior:
> * Hidden or unhidden animated layers that smaller than the viewport(ish) get entirely painted
> * If the animated layer is partially visible, the "entire painting" pass preempts "normal" painting of the partially visible part of the layer.
> 

Yep that's the idea.

> I'm fine with tweaking this special case logic to be better about animations in the short term.  However, I'd be happier if I had a better sense of the longer-term plan of how to handle prioritization of animated content, especially if it involved less special case logic in TiledLayerChromium.

My thinking is that the long term beneficial behavior we want to keep is that animated layers get entirely painted if at all possible. However, it might be possible for that behavior to just fall out naturally with some added heuristics about distance and animation direction. 
- Distance: don't paint all animated layers, but rather 'close' animated layers
- Direction: remove checks for 'small' and 'animated' and replace the animation pass with a pass that tries to paint all tiles that 'will be visible soon', before reverting to visible tiles.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:81
> > +        , updatedTempFlag(false)
> 
> This should go in a different patch.  "Tweaks," eh?
> 

Sorry, I assumed you would only read the later patch since it was quick. I should have put a better description (to insure it doesn't get overlooked). I'll put this loop cleanup as a refactoring patch before this one.

> deviceViewportSize?
> 

I'll change it, but can you describe the difference here? Is this a unit change or is there actually different viewports?

> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:528
> > +    return contentRect.width() <= viewportSize.width() + 64
> > +           && contentRect.height() <= viewportSize.height() + 64;
> 
> 64?

I had it be exact initially, but on Android there was a small delta sometimes (a few pixels) between the animated layer size and the viewport size.

[WebKit Review Bot]

Comment on attachment 156214 [details]
tweak

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

New failing tests:
TiledLayerChromiumTest.pushIdlePaintedOccludedTiles
TiledLayerChromiumTest.tilesNotPaintedWithoutInvalidation

[WebKit Review Bot]

Created attachment 156227 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Eric Penner]

Alas there was a bug in the "tweaks". That is fixed in a follow up patch here:
https://bugs.webkit.org/show_bug.cgi?id=93059.  I'll rebase this on whatever lands there.

[Eric Penner]

Created attachment 156842 [details]
rebase_on_93059

[Dana Jansens]

Comment on attachment 156842 [details]
rebase_on_93059

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591
> +    if (m_tiler->hasEmptyBounds())
> +        return;
> +

Why is this needed now?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:614
> +        for (int j = top; j <= bottom; ++j)
> +            for (int i = left; i <= right; ++i)
> +                if (!tileAt(i, j))
> +                    createTile(i, j);

style nit: use braces for any indentation that is longer than one line.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:660
> +        // FIXME: Make this based on distance once distance can be calculated for offscreen layers.
> +        // For now, prioritize all small animated layers after 512 pixels of pre-painting.

This is encroaching on uncomfortable special-casey-ness I think.. They have priority=distance(512) for texture allocation, but priority=visible for paint ordering. Everything else has paint ordering == texture allocation, right? Can we do something cleaner here?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:662
> +        if (smallAnimatedLayer)
> +            priority = CCPriorityCalculator::maxPriority(priority, priorityCalc.priorityFromDistance(512, drawsToRoot));

Why not use else if, and then you can set it directly instead of needing maxPriority()?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:701
> +    // Animation pre-paint. If the layer is small, try to paint it all
> +    // immediately to avoid paint/upload hiccups while it is animating.

If the priority of the layer is higher than we don't need to special-case it here, right? Can we solve this problem (in this method) with priorities instead?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:711
> +    if (contentRect.isEmpty())
> +        return;

Does this change mean that a layer that is scrolled just outside of view will not prepaint at all? It will have an empty visibleContentRect right? Will this regress that case?

[Eric Penner]

Comment on attachment 156842 [details]
rebase_on_93059

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591
>> +
> 
> Why is this needed now?

Without this we will create a tile below I think. contentRectToTileIndices will return a bunch of 0's which will result in createTile(0,0). This might be a weakness of contentRectToTileIndices/tileXIndexFromSrcCoord since it can't return empty sets of tiles. Although it's probably a good idea to early-out when we can anyway.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:614
>> +                    createTile(i, j);
> 
> style nit: use braces for any indentation that is longer than one line.

Each indentation is indeed only one line? ;)
Seriously though webkit-patch doesn't complain, which style guide is this in?

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:660
>> +        // For now, prioritize all small animated layers after 512 pixels of pre-painting.
> 
> This is encroaching on uncomfortable special-casey-ness I think.. They have priority=distance(512) for texture allocation, but priority=visible for paint ordering. Everything else has paint ordering == texture allocation, right? Can we do something cleaner here?

This will get a lot cleaner iff we have location/direction of offscreen layers (lack of which caused this special case in the first place). I just don't want to wait for that since this has such a big impact. Paint-order and priority-order being different might help for a generic algorithm at which point this case can be removed.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:662
>> +            priority = CCPriorityCalculator::maxPriority(priority, priorityCalc.priorityFromDistance(512, drawsToRoot));
> 
> Why not use else if, and then you can set it directly instead of needing maxPriority()?

It would probably be fine, but becoming visible could actually lower the priority which would be kind of weird.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:701
>> +    // immediately to avoid paint/upload hiccups while it is animating.
> 
> If the priority of the layer is higher than we don't need to special-case it here, right? Can we solve this problem (in this method) with priorities instead?

Yes, I think with both location and direction we probably can, and this special case should just fall-out of some to-be-implemented generic behavior.

However, I'd like to get Android on par to m18 in at least a few cases. It's completely busted right now and this patch should let us determine if there is further work or bugs that need to be addressed.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:711
>> +        return;
> 
> Does this change mean that a layer that is scrolled just outside of view will not prepaint at all? It will have an empty visibleContentRect right? Will this regress that case?

We have never pre-painted layers that are not visible :(  (with the exception of this special case for animated layers).  We don't know where the off-screen layers are relative to being visible, so we don't know how to prioritize them.

[Adrienne Walker]

Comment on attachment 156842 [details]
rebase_on_93059

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

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591
>>> +
>> 
>> Why is this needed now?
> 
> Without this we will create a tile below I think. contentRectToTileIndices will return a bunch of 0's which will result in createTile(0,0). This might be a weakness of contentRectToTileIndices/tileXIndexFromSrcCoord since it can't return empty sets of tiles. Although it's probably a good idea to early-out when we can anyway.

With as many loops as we have at this point, we should probably just have an iterator that takes a rect.

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:614
>>> +                    createTile(i, j);
>> 
>> style nit: use braces for any indentation that is longer than one line.
> 
> Each indentation is indeed only one line? ;)
> Seriously though webkit-patch doesn't complain, which style guide is this in?

http://www.webkit.org/coding/coding-style.html#braces-one-line

[Eric Penner]

Comment on attachment 156842 [details]
rebase_on_93059

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

>>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591
>>>> +
>>> 
>>> Why is this needed now?
>> 
>> Without this we will create a tile below I think. contentRectToTileIndices will return a bunch of 0's which will result in createTile(0,0). This might be a weakness of contentRectToTileIndices/tileXIndexFromSrcCoord since it can't return empty sets of tiles. Although it's probably a good idea to early-out when we can anyway.
> 
> With as many loops as we have at this point, we should probably just have an iterator that takes a rect.

I was thinking along the same lines. Or maybe it could fill a vector that you pass in with the tiles, which we could then pass around to the various functions instead of doing all the double for-loops.

>>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:614
>>>> +                    createTile(i, j);
>>> 
>>> style nit: use braces for any indentation that is longer than one line.
>> 
>> Each indentation is indeed only one line? ;)
>> Seriously though webkit-patch doesn't complain, which style guide is this in?
> 
> http://www.webkit.org/coding/coding-style.html#braces-one-line

My take on that is that only this be wrong:
for (int j = top; j <= bottom; ++j)
    for (int i = left; i <= right; ++i)
        if (!tileAt(i, j))
            createTile(i,
                j);
But I'm surely wrong so I've changed it. Thanks for the link :)



To see the effect of this patch, go here on a debug build, and use 'flow':
http://jquerymobile.com/demos/1.1.0/docs/pages/page-transitions.html

I see unpainted tiles during the 'flow' transition without this patch.
Do we not have the logic to block frames anymore? In any event blocking
frames wasn't sufficient on Android as it would just jump to the end.

[Eric Penner]

Created attachment 157346 [details]
rebase_on_refactor

[Eric Penner]

Created attachment 157369 [details]
rebase_to_master

[Eric Penner]

Comment on attachment 157369 [details]
rebase_to_master

Addressed previous comments, unless there is remaining issues, should be gtg.

[Eric Penner]

Comment on attachment 156842 [details]
rebase_on_93059

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

>>>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591
>>>>> +
>>>> 
>>>> Why is this needed now?
>>> 
>>> Without this we will create a tile below I think. contentRectToTileIndices will return a bunch of 0's which will result in createTile(0,0). This might be a weakness of contentRectToTileIndices/tileXIndexFromSrcCoord since it can't return empty sets of tiles. Although it's probably a good idea to early-out when we can anyway.
>> 
>> With as many loops as we have at this point, we should probably just have an iterator that takes a rect.
> 
> I was thinking along the same lines. Or maybe it could fill a vector that you pass in with the tiles, which we could then pass around to the various functions instead of doing all the double for-loops.

Btw, I added an ASSERT in contentRectToTileIndices to catch this for now. It didn't get hit in unit tests or chrome so that's good.

[Adrienne Walker]

Comment on attachment 157369 [details]
rebase_to_master

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:600
> +        m_tiler->contentRectToTileIndices(createTilesRect, left, top, right, bottom);
> +        for (int j = top; j <= bottom; ++j) {
> +            for (int i = left; i <= right; ++i) {

Can you explain why this loop operates on createTilesRect but the partial update / buffered update pass directly below this operates on visibleContentRect? Shouldn't this special case also go down that latter path too?

[Eric Penner]

Comment on attachment 157369 [details]
rebase_to_master

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:600
>> +            for (int i = left; i <= right; ++i) {
> 
> Can you explain why this loop operates on createTilesRect but the partial update / buffered update pass directly below this operates on visibleContentRect? Shouldn't this special case also go down that latter path too?

Hmm, that sounds like an improvement yes! This would only help in the case of a layer that changes it's content while becoming visible, but since we now have a bound on the size we can do better than normal pre-painting here. I'll see if it's easy to change that. 

Possibly this could be mirrored in the generic pre-painting case, but that seems a lot harder since the size is unbounded and something still has to give when painting is too slow. So maybe a special case will always be warranted even in the more general case, which enables double-buffering when the pre-paint size is bounded and reasonable.

[Eric Penner]

Created attachment 157561 [details]
Patch

[Adrienne Walker]

Comment on attachment 157561 [details]
Patch

R=me.

[Eric Penner]

Created attachment 157584 [details]
fix_assert

[Eric Penner]

Comment on attachment 157584 [details]
fix_assert

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:146
> +    if (size.isEmpty()) {

Without this we were hitting the ASSERT. Looks like a layer that is resized to zero could keep a tile without this.

[Eric Penner]

Comment on attachment 157584 [details]
fix_assert

If you are okay with the ASSERT fix, gtg.

[Adrienne Walker]

Comment on attachment 157584 [details]
fix_assert

R=me.  Yeah, that totally looks like a minor bug in setBounds(), thanks.  :)

[WebKit Review Bot]

Comment on attachment 157584 [details]
fix_assert

Clearing flags on attachment: 157584

Committed r125230: <http://trac.webkit.org/changeset/125230>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

This change caused a compile error on Chromium-mac bots, and I landed a fix:
http://trac.webkit.org/changeset/125238

Please take a look and ping me if my fix was not okay.

[Yuta Kitamura]

Turned out that this change has caused some chromiumos browser_tests to time out:

http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20ChromiumOS%20Tests%20%282%29&number=5902
BrowserNonClientFrameViewAshTest.UseShortHeader
LauncherAppBrowserTest.LaunchMaximized

These tests failed consistently on Chromium's main waterfall, and, after some
local testing, I confirmed this change (r125230) is the culprit.

I'm going to ROLL OUT this change to make WebKit roll possible. Please fix
the above issue and land the patch again.

I appreciate your understanding.

[WebKit Review Bot]

Re-opened since this is blocked by 93698

[Eric Penner]

Created attachment 157769 [details]
fix_typo

[Eric Penner]

Fixed the typo mentioned. That was had a pretty minor effect so there must be an issue on ChromeOS that I'm not seeing. Gonna look into that now.

[Eric Penner]

Comment on attachment 157769 [details]
fix_typo

Removing r? until I can sort out what went wrong on ChromeOS

[Eric Penner]

Created attachment 157836 [details]
fix_idlePaintRect

[Eric Penner]

Comment on attachment 157836 [details]
fix_idlePaintRect

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:802
> +    // Don't inflate an empty rect.

This was the only change. I removed the special old case but didn't add back the previous code.

[Eric Penner]

Comment on attachment 157836 [details]
fix_idlePaintRect

Sorry 'bout that. Should be gtg.

[Adrienne Walker]

(In reply to comment #37)
> (From update of attachment 157836 [details])
> Sorry 'bout that. Should be gtg.

Can you explain more what went wrong?

[Eric Penner]

(In reply to comment #38)
> (In reply to comment #37)
> > (From update of attachment 157836 [details] [details])
> > Sorry 'bout that. Should be gtg.
> 
> Can you explain more what went wrong?

IntRect::inflate() in idlePaintRect will happily inflate an empty visible rect into a non-empty rect (without the special check I just added back). Then we would think there is more painting to do in needsIdlePaint, but we would never paint anything because we correctly exit early when actually painting.

[Adrienne Walker]

(In reply to comment #39)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > (From update of attachment 157836 [details] [details] [details])
> > > Sorry 'bout that. Should be gtg.
> > 
> > Can you explain more what went wrong?
> 
> IntRect::inflate() in idlePaintRect will happily inflate an empty visible rect into a non-empty rect (without the special check I just added back). Then we would think there is more painting to do in needsIdlePaint, but we would never paint anything because we correctly exit early when actually painting.

I see that the inflate in the previous patch is wrong, but I don't understand how this fixed the test failures.  Were you able to repro those timeouts locally, or did you run a try job to check if this fixed them?

[Eric Penner]

(In reply to comment #40)
> (In reply to comment #39)
> > (In reply to comment #38)
> > > (In reply to comment #37)
> > > > (From update of attachment 157836 [details] [details] [details] [details])
> > > > Sorry 'bout that. Should be gtg.
> > > 
> > > Can you explain more what went wrong?
> > 
> > IntRect::inflate() in idlePaintRect will happily inflate an empty visible rect into a non-empty rect (without the special check I just added back). Then we would think there is more painting to do in needsIdlePaint, but we would never paint anything because we correctly exit early when actually painting.
> 
> I see that the inflate in the previous patch is wrong, but I don't understand how this fixed the test failures.  Were you able to repro those timeouts locally, or did you run a try job to check if this fixed them?

I was able to build the failing test using chromeos=1 locally and see that the fixed worked. This just barely escapes the unit tests so I was looking at adding one that would catch this as well.

[Eric Penner]

Created attachment 158127 [details]
fix_unit_tests_and_rebase

[Eric Penner]

Comment on attachment 158127 [details]
fix_unit_tests_and_rebase

There was two different bugs in idlePaintNonVisibleLayers (there wasn't enough memory to allocate the tile, and we didn't call updateTextures(), both of which make it impossible for the test to fail). So I fixed that up, watched it fail/pass along with the chromeos test, and added some extra checks that we are keeping memory after layers become invisible.

[Adrienne Walker]

I'm still missing a piece of this.  Why were the ChromeOS tests failing?

[Eric Penner]

(In reply to comment #44)
> I'm still missing a piece of this.  Why were the ChromeOS tests failing?

It was in an endless painting loop in webkit. I'm not sure what part of the test checks that webkit is done working (on desktop this would just be a battery drain but otherwise fine), but I could see it spinning away thinking it needed to paint and off-screen layer but then never doing so.

[Eric Penner]

(In reply to comment #45)
> (In reply to comment #44)
> > I'm still missing a piece of this.  Why were the ChromeOS tests failing?
> 
> It was in an endless painting loop in webkit. I'm not sure what part of the test checks that webkit is done working (on desktop this would just be a battery drain but otherwise fine), but I could see it spinning away thinking it needed to paint and off-screen layer but then never doing so.

Rather, it was in an endless sequence of setNeedsCommit()'s.  This sums it up:
- PaintSomething -> Does nothing
- NeedIdlePaint? -> Yes -> setNeedsCommit
- repeat.

[Adrienne Walker]

Comment on attachment 158127 [details]
fix_unit_tests_and_rebase

R=me.  The bugfix looks correct to me.  I'm still not sure I understand why endless committing would cause the test to timeout, but it sounds plausible.

[Eric Penner]

(In reply to comment #47)
> (From update of attachment 158127 [details])
> R=me.  The bugfix looks correct to me.  I'm still not sure I understand why endless committing would cause the test to timeout, but it sounds plausible.

I kind of jumped to the bug introduced by this CL, but didn't investigate why endless commits would cause it to time-out or if that is expected.

[WebKit Review Bot]

Comment on attachment 158127 [details]
fix_unit_tests_and_rebase

Clearing flags on attachment: 158127

Committed r125498: <http://trac.webkit.org/changeset/125498>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.