Bug 82117 – [chromium] Layers with animating transforms should prepaint even if they are not visible yet

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

[Dana Jansens]

[chromium] Layers with animating transforms should prepaint even if they are not visible yet

[Dana Jansens]

Currently we early-out on these layers in two places.

1) ContentLayerChromium exits the idle paint if visibleLayerRect.isEmpty()
2) TiledLayerChomium exits prepareToUpdateIdle if the set of tiles is empty (true when no tiles have been painted because visibleLayerRect.isEmpty())

[Dana Jansens]

Created attachment 133618 [details]
Patch

[Dana Jansens]

Verified that this hides (not fixes) the poster circle stutter[1] as expected.

[1] http://code.google.com/p/chromium/issues/detail?id=119730

[Adrienne Walker]

Comment on attachment 133618 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:620
> -    if (m_tiler->isEmpty())
> +    if ((m_tiler->isEmpty() || layerRect.isEmpty()) && !drawTransformIsAnimating() && !screenSpaceTransformIsAnimating())
>          return;

I think this conditional is wrong, possibly due to m_tiler->isEmpty() being confusing.  I think if !m_tiler->numTiles() (i.e. the bounds are zero), then we definitely need to bail out even if we're animating, because the layer size is zero.  I'm pretty sure layerRectToTileIndices will assert at you if that's the case.

Can you add a test involving prepareToUpdate/prepareToUpdateIdle where there's a layer with empty bounds?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:672
> +    if ((m_tiler->isEmpty() || layerRect.isEmpty()) && !drawTransformIsAnimating() && !screenSpaceTransformIsAnimating())
> +        return false;

This should be changed to match the above.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:696
> +    if (visibleLayerRect.isEmpty() && (drawTransformIsAnimating() || screenSpaceTransformIsAnimating()))
> +        return IntRect(IntPoint(), contentBounds());

I'm not so sure about this, since returning the whole bounds means that it's going to try to prepaint the entire layer in one go.  Current behavior is that if we OOM during prepainting, we don't do any prepainting, but if it happens to fit and is really large, that could be a huge hitch while you wait for all of that to paint, with no guarantee it will fit.  Additionally, we prepaint in front-to-back order, so prepainting a large animated layer would take precedence over a scrollable layer, which also seems problematic.

This kind of feels like a stopgap solution, where in some ideal world you could predict which tiles will eventually be visible during the animation, prioritizing the ones that become visible first and filtering out the ones that will never be visible.  Maybe I'd prefer that this solution be more conservative, possibly by creating a rect of n tiles (maybe ~3ish?), and letting prepaint rect growth take care of reserving more if there's not more memory contention from other layers.  For small layers, we'll still get them prepainted quickly, but larger layers have less of a chance of causing problematic behavior.

[Dana Jansens]

Comment on attachment 133618 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:696
>> +        return IntRect(IntPoint(), contentBounds());
> 
> I'm not so sure about this, since returning the whole bounds means that it's going to try to prepaint the entire layer in one go.  Current behavior is that if we OOM during prepainting, we don't do any prepainting, but if it happens to fit and is really large, that could be a huge hitch while you wait for all of that to paint, with no guarantee it will fit.  Additionally, we prepaint in front-to-back order, so prepainting a large animated layer would take precedence over a scrollable layer, which also seems problematic.
> 
> This kind of feels like a stopgap solution, where in some ideal world you could predict which tiles will eventually be visible during the animation, prioritizing the ones that become visible first and filtering out the ones that will never be visible.  Maybe I'd prefer that this solution be more conservative, possibly by creating a rect of n tiles (maybe ~3ish?), and letting prepaint rect growth take care of reserving more if there's not more memory contention from other layers.  For small layers, we'll still get them prepainted quickly, but larger layers have less of a chance of causing problematic behavior.

Is this true? I thought it starts with the layerRect (which would be 0 or 1 tiles.. not sure.. and then adds rows/columns until it fills the prepaint rect which is the whole layer here, so it would still do a bit at a time?

[Dana Jansens]

Comment on attachment 133618 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:696
>>> +        return IntRect(IntPoint(), contentBounds());
>> 
>> I'm not so sure about this, since returning the whole bounds means that it's going to try to prepaint the entire layer in one go.  Current behavior is that if we OOM during prepainting, we don't do any prepainting, but if it happens to fit and is really large, that could be a huge hitch while you wait for all of that to paint, with no guarantee it will fit.  Additionally, we prepaint in front-to-back order, so prepainting a large animated layer would take precedence over a scrollable layer, which also seems problematic.
>> 
>> This kind of feels like a stopgap solution, where in some ideal world you could predict which tiles will eventually be visible during the animation, prioritizing the ones that become visible first and filtering out the ones that will never be visible.  Maybe I'd prefer that this solution be more conservative, possibly by creating a rect of n tiles (maybe ~3ish?), and letting prepaint rect growth take care of reserving more if there's not more memory contention from other layers.  For small layers, we'll still get them prepainted quickly, but larger layers have less of a chance of causing problematic behavior.
> 
> Is this true? I thought it starts with the layerRect (which would be 0 or 1 tiles.. not sure.. and then adds rows/columns until it fills the prepaint rect which is the whole layer here, so it would still do a bit at a time?

It does protect the whole layer though, maybe it should just protect the tiles it is going to prepaint in this iteration?

[Adrienne Walker]

(In reply to comment #6)
> (From update of attachment 133618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133618&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:696
> >>> +        return IntRect(IntPoint(), contentBounds());
> >> 
> >> I'm not so sure about this, since returning the whole bounds means that it's going to try to prepaint the entire layer in one go.  Current behavior is that if we OOM during prepainting, we don't do any prepainting, but if it happens to fit and is really large, that could be a huge hitch while you wait for all of that to paint, with no guarantee it will fit.  Additionally, we prepaint in front-to-back order, so prepainting a large animated layer would take precedence over a scrollable layer, which also seems problematic.
> >> 
> >> This kind of feels like a stopgap solution, where in some ideal world you could predict which tiles will eventually be visible during the animation, prioritizing the ones that become visible first and filtering out the ones that will never be visible.  Maybe I'd prefer that this solution be more conservative, possibly by creating a rect of n tiles (maybe ~3ish?), and letting prepaint rect growth take care of reserving more if there's not more memory contention from other layers.  For small layers, we'll still get them prepainted quickly, but larger layers have less of a chance of causing problematic behavior.
> > 
> > Is this true? I thought it starts with the layerRect (which would be 0 or 1 tiles.. not sure.. and then adds rows/columns until it fills the prepaint rect which is the whole layer here, so it would still do a bit at a time?
> 
> It does protect the whole layer though, maybe it should just protect the tiles it is going to prepaint in this iteration?

You're quite right.  It uses idlePaintRect for protection, but layerRect for the initial number of tiles to draw.  So, my worries above are unfounded.  Sorry for the panic.  :)

As for handling the protection end of things, the protection there I think is not necessary anymore.  As it prepaints, it will reserve textures as it goes.
Maybe to avoid reserving the entire layer, this logic above should only affect needsIdlePaint but not idlePaintRect?

[Dana Jansens]

Created attachment 133671 [details]
Patch

The needsIdlePaint() and prepareToUpdateIdle() must match in their expectations of what we would prepaint if we had infinite memory, to prevent from committing indefinitely trying to prepaint more (needsIdlePaint() says there is more to do!) while prepareToUpdateIdle() thinks it is done. So it is important that idlePaintRect() give them both the same answer, and that they early out under the same conditions.

I discussed that protect of the whole prepaintRect with @epenner, and understand its purpose now. Layers toward the front would steal tiles from layers behind them over time until all layers finished prepainting anyways (each time through they would reserve more, and possibly evict tiles from below). And the benefit is that if a tile was already painted, but we didn't get to it yet in the prepaint order, we don't end up causing it to get evicted by some layer below, whose tile will get evicted by us again in the future anyways!

Great catch on the layers with no tiles (empty bounds()) point. Fixed the early outs to leave in this case, and have unit tests for empty bounded layers with or without animations.

So attached is new proposal for prepainting animating layers that won't trash low memory environments (android).
1. We limit prepainting non-visible animating layers to layers that are at most 9 tiles.
2. For such layers, we prepaint the outer set of tiles on the layer (the outer set here is pretty much all the tiles for the layer sizes concerned).

[Adrienne Walker]

Comment on attachment 133671 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:106
> +    size_t numPaintedTiles() { return m_tiler->tiles().size(); }

Can you group this with the other testing functions above?

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:837
> +            bool hasTile = i < have || j < have || i >= width - have || j >= height - have;

Cute.

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:867
> +            bool shouldPrepaint = contentBounds.width() / 100 * contentBounds.height() / 100 <= 9;

I don't like having this magic constant baked in here and in the implementation. Can you use idlePaintRect as the condition instead so that the test is more independent of the exact implementation?

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:883
> +                printf("fail\n");

Debugging code you forgot to remove?

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:888
> +            testHaveOuterTiles(layerImpl.get(), width[i] / 100, height[j] / 100, shouldPrepaint ? 1 : 0);

Use tile size here instead of a magic number.

[Dana Jansens]

Created attachment 133714 [details]
Patch

[Dana Jansens]

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

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:106
>> +    size_t numPaintedTiles() { return m_tiler->tiles().size(); }
> 
> Can you group this with the other testing functions above?

done.

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:837
>> +            bool hasTile = i < have || j < have || i >= width - have || j >= height - have;
> 
> Cute.

:)

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:867
>> +            bool shouldPrepaint = contentBounds.width() / 100 * contentBounds.height() / 100 <= 9;
> 
> I don't like having this magic constant baked in here and in the implementation. Can you use idlePaintRect as the condition instead so that the test is more independent of the exact implementation?

ok. i wanted to test that small layers were getting prepainted, but large ones were not. maybe that is not something that belongs in a unit test tho..

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:883
>> +                printf("fail\n");
> 
> Debugging code you forgot to remove?

wooops

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:888
>> +            testHaveOuterTiles(layerImpl.get(), width[i] / 100, height[j] / 100, shouldPrepaint ? 1 : 0);
> 
> Use tile size here instead of a magic number.

done, and made the arrays hold number of tiles instead of number of pixels.

[Dana Jansens]

Created attachment 133715 [details]
Patch

comment fix

[Dana Jansens]

Created attachment 133718 [details]
Patch

comment fix

[Adrienne Walker]

Comment on attachment 133718 [details]
Patch

Thanks for all the changes. R=me.

[WebKit Review Bot]

Comment on attachment 133718 [details]
Patch

Clearing flags on attachment: 133718

Committed r112049: <http://trac.webkit.org/changeset/112049>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.