[S.P.] Don't draw frames of animated images that are offscreen

[S.P.] Don't draw frames of animated images that are offscreen

To achieve this, introduces the notion of a delayed paint invalidation.
When invalidating paint for an object, the object now has the option
to delay the invalidation until the next frame.

This is a re-land of CL 1008043002, with the test updated to start the
repaint test after 0ms, not 500ms.

BUG=440466
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192643

[chrishtr, 2015-03-20 16:37:00]

chrishtr@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[chrishtr, 2015-03-20 16:58:54]

https://codereview.chromium.org/1026823002/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1026823002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:3127: ASSERT(document().lifecycle().state()
!= DocumentLifecycle::InPaintInvalidation || reason ==
PaintInvalidationDelayedFull);
I also restored the asserts in this method to avoid broken call sites elsewhere.

[chrishtr, 2015-03-20 20:08:39]

Ping? LGTY?

[Stephen Chennney, 2015-03-20 20:14:34]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2015-03-20 20:14:35]

Test still seems flaky based on the try bots.

[chrishtr, 2015-03-25 03:19:51]

chrishtr@chromium.org changed reviewers:
+ pdr@chromium.org

[chrishtr, 2015-03-25 03:19:51]

Ready for re-review. The problem with the tests was that layout tests do not
paint extra times unless you force them to. I added calls to displayAsyncThen
for this purpose.

[esprehn, 2015-03-25 05:06:35]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-03-25 05:06:37]

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutBoxModelObject.cpp:271:
setShouldDoFullPaintInvalidation(reason);
This doesn't seem right from inside the invalidateTreeIfNeeded walk, I think you
want to push these into a separate vector and invalidate after the primary walk.

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutImage.cpp (right):

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:179: bool
LayoutImage::rectIntersectsVisibleViewport(const LayoutRect& localRect)
This generic method doesn't seem neccesary, can we add a bool
intersectsVisibleViewport() that internally reads the visualOverflowRect()
instead?

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:3128: ASSERT(document().lifecycle().state()
!= DocumentLifecycle::InPaintInvalidation || reason ==
PaintInvalidationDelayedFull);
This still can let in bugs with the bits, if you use a separate walk you don't
need to change any of the asserts.

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.h:253: if
(RuntimeEnabledFeatures::slimmingPaintEnabled())
This means bugs can sneak in, if you use a separate vector you can leave the
assert which is good.

[esprehn, 2015-03-25 05:18:31]

Also this seems to make us busy loop pumping frames that don't do anything while
an animation is running that's off screen. So even though we're skipping putting
up pixels we'll end up running BeginFrame at 60fps doing the invalidation phase.
It seems like we need something more sophisticated to not just burn CPU.
Previously we'd paint the images, but gifs don't really animate at 60fps so it'd
be running BeginFrame at a slower rate than this patch makes us.

noel@ also pointed out we should make sure this works if you do something like
add a blur(50) filter to the image. I think that's covered by the
visualOverflowRect(), but we should add a test.

[chrishtr, 2015-03-25 16:48:39]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-03-25 16:48:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1026823002/150001

[commit-bot: I haz the power, 2015-03-25 16:49:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-25 16:49:02]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[chrishtr, 2015-03-25 23:30:36]

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutBoxModelObject.cpp:271:
setShouldDoFullPaintInvalidation(reason);
On 2015/03/25 at 05:06:36, esprehn wrote:
> This doesn't seem right from inside the invalidateTreeIfNeeded walk, I think
you want to push these into a separate vector and invalidate after the primary
walk.

That's a good idea, and as you pointed out elsewhere makes the code safer. Done.

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutImage.cpp (right):

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:179: bool
LayoutImage::rectIntersectsVisibleViewport(const LayoutRect& localRect)
On 2015/03/25 at 05:06:36, esprehn wrote:
> This generic method doesn't seem neccesary, can we add a bool
intersectsVisibleViewport() that internally reads the visualOverflowRect()
instead?

Done.

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1026823002/diff/150001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:3128: ASSERT(document().lifecycle().state()
!= DocumentLifecycle::InPaintInvalidation || reason ==
PaintInvalidationDelayedFull);
On 2015/03/25 at 05:06:36, esprehn wrote:
> This still can let in bugs with the bits, if you use a separate walk you don't
need to change any of the asserts.

Done.

[esprehn, 2015-03-26 02:01:14]

I think this still makes us do BeginFrame at 60fps even though the image isn't
really animating that fast.

[Noel Gordon, 2015-03-26 11:18:04]

What if you call stopAnimation() on the image resource. If it's an animated
image, that'll kill the animation timer.

[Noel Gordon, 2015-03-26 12:42:54]

.. that'd land here

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Not sure I believe the veracity of the comment there though, so something to
check for.

[chrishtr, 2015-03-26 21:03:22]

On 2015/03/26 at 02:01:14, esprehn wrote:
> I think this still makes us do BeginFrame at 60fps even though the image isn't
really animating that fast.

It won't, because the call to scheduleVisualUpdate during
LayoutObject::setShouldDoFullPaintInvalidation has no effect during a standard
call to PageAnimator::updateLayoutAndStyleForPainting, which is the case for
animated GIFs.

Thus we will only generate a new frame if anything else has changed about the
page, such as scroll offset or some other dirtying.

[esprehn, 2015-03-26 21:09:27]

https://codereview.chromium.org/1026823002/diff/330001/Source/core/layout/Lay...
File Source/core/layout/LayoutView.h (right):

https://codereview.chromium.org/1026823002/diff/330001/Source/core/layout/Lay...
Source/core/layout/LayoutView.h:218: Vector<LayoutObject*>
m_pendingDelayedPaintInvalidations;
This is a use-after-free since you don't remove objects from the list when the
RenderImage is destroyed.

[esprehn, 2015-03-26 22:20:29]

On 2015/03/26 at 21:09:27, esprehn wrote:
>
https://codereview.chromium.org/1026823002/diff/330001/Source/core/layout/Lay...
> File Source/core/layout/LayoutView.h (right):
> 
>
https://codereview.chromium.org/1026823002/diff/330001/Source/core/layout/Lay...
> Source/core/layout/LayoutView.h:218: Vector<LayoutObject*>
m_pendingDelayedPaintInvalidations;
> This is a use-after-free since you don't remove objects from the list when the
RenderImage is destroyed.

We talked off line, this Vector only really exists as long as the invalidation
phase is running so there's no UAF here. I suggested passing this Vector as an
argument down the recursion instead and keeping it on the stack so the lifetime
was clear.

lgtm w/ that change.

[chrishtr, 2015-03-26 23:56:50]

Done.

[chrishtr, 2015-03-26 23:56:55]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-03-26 23:56:55]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1026823002/#ps370001
(title: " ")

[commit-bot: I haz the power, 2015-03-26 23:57:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1026823002/370001

[commit-bot: I haz the power, 2015-03-27 01:24:48]

Committed patchset #20 (id:370001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192643