[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.

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

[chrishtr, 2015-03-14 00:46:07]

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

[chrishtr, 2015-03-14 00:46:26]

Testing is clearly not done for this CL. Sending now for code feedback.

[chrishtr, 2015-03-16 23:07:41]

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

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:200: return PaintInvalidationDelayedFull;
Since this method overrides invalidatePaintIfNeeded, in the case where this line
is reached all the code in LayoutObject::invalidatePaintIfNeeded never runs.
This means that no paint invalidation rects are issued at all, leading to no
paint and no raster.

[Xianzhu, 2015-03-17 23:34:01]

I guess the latest patch set is not a working one yet, because there is no
mechanism to ensure the delayed paint invalidation flag can be reached in the
next paint invalidation (the flags on the container chain may be cleared during
the current paint invalidation).

I still think repaint flags will help this case. I added another comment in the
design doc.

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

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1206: setShouldDoFullPaintInvalidation();
Should we let this be for slimming-paint only?

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.h (left):

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.h:255: 
I think we can keep these for a while for non-SP.

[chrishtr, 2015-03-17 23:53:56]

On 2015/03/17 at 23:34:01, wangxianzhu wrote:
> I guess the latest patch set is not a working one yet, because there is no
mechanism to ensure the delayed paint invalidation flag can be reached in the
next paint invalidation (the flags on the container chain may be cleared during
the current paint invalidation).

There is though. I will mark that in the code so we can discuss.

> 
> I still think repaint flags will help this case. I added another comment in
the design doc.
> 
>
https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
> File Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
> Source/core/layout/LayoutObject.cpp:1206: setShouldDoFullPaintInvalidation();
> Should we let this be for slimming-paint only?
> 
>
https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
> File Source/core/layout/LayoutObject.h (left):
> 
>
https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
> Source/core/layout/LayoutObject.h:255: 
> I think we can keep these for a while for non-SP.

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

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

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:200: return PaintInvalidationDelayedFull;
On 2015/03/16 at 23:07:41, chrishtr wrote:
> Since this method overrides invalidatePaintIfNeeded, in the case where this
line is reached all the code in LayoutObject::invalidatePaintIfNeeded never
runs. This means that no paint invalidation rects are issued at all, leading to
no paint and no raster.

Correct. That is the intention of the CL.

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

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1206: setShouldDoFullPaintInvalidation();
On 2015/03/17 at 23:34:01, Xianzhu wrote:
> Should we let this be for slimming-paint only?

In this CL it only takes effect if PaintInvalidationDelayedFull, which only
happens when Slimming Paint is on.

I suppose I could change the logic to treat "delayed" as PaintInvalidationFull
when Slimmming Paint is off, if you want.

Also, to respond to your global comment: line 1206 is there in order to set the
bits up the tree so that the next time paint invalidation happens, it recurses
down to this LayoutObject. I agree that the mechanism is a little fragile, since
it depends on the bits being cleared top-to-bottom along with the recursion,
suggestions welcome.

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.h (left):

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.h:255: 
On 2015/03/17 at 23:34:01, Xianzhu wrote:
> I think we can keep these for a while for non-SP.

Good point. I can re-add.

[Xianzhu, 2015-03-18 16:37:02]

lgtm (with the assertions for non-SP restored).

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

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1206: setShouldDoFullPaintInvalidation();
On 2015/03/17 23:55:56, chrishtr wrote:
> On 2015/03/17 at 23:34:01, Xianzhu wrote:
> > Should we let this be for slimming-paint only?
> 
> In this CL it only takes effect if PaintInvalidationDelayedFull, which only
> happens when Slimming Paint is on.
> 

Acknowledged.

> I suppose I could change the logic to treat "delayed" as PaintInvalidationFull
> when Slimmming Paint is off, if you want.
> 
> Also, to respond to your global comment: line 1206 is there in order to set
the
> bits up the tree so that the next time paint invalidation happens, it recurses
> down to this LayoutObject. I agree that the mechanism is a little fragile,
since
> it depends on the bits being cleared top-to-bottom along with the recursion,
> suggestions welcome.

My bad. I thought we cleared the paint invalidation state after we walk the
subtrees.

[Xianzhu, 2015-03-18 19:24:53]

Several nits. And please also make the test work.

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

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1280: }
Nit: unnecessary change

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1301: }
Ditto.

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1309: }
Ditto.

[chrishtr, 2015-03-19 22:01:27]

Test is now working. Added two tests, one for a visible animated gif, and one
for a not-visible one. The tests run in slimming paint mode and non-slimming
paint mode, with different expectation files.

As a side effect, I've also enabled slimming paint for the paint/ directory, and
added one rebaseline as a result.

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

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1280: }
On 2015/03/18 at 19:24:53, Xianzhu wrote:
> Nit: unnecessary change

Done.

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1301: }
On 2015/03/18 at 19:24:53, Xianzhu wrote:
> Ditto.

Done.

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:1309: }
On 2015/03/18 at 19:24:53, Xianzhu wrote:
> Ditto.

Done.

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.h (left):

https://codereview.chromium.org/1008043002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.h:255: 
On 2015/03/17 at 23:55:56, chrishtr wrote:
> On 2015/03/17 at 23:34:01, Xianzhu wrote:
> > I think we can keep these for a while for non-SP.
> 
> Good point. I can re-add.

Done.

[Xianzhu, 2015-03-19 22:14:22]

lgtm.

https://codereview.chromium.org/1008043002/diff/140001/Source/core/paint/Imag...
File Source/core/paint/ImagePainter.cpp (right):

https://codereview.chromium.org/1008043002/diff/140001/Source/core/paint/Imag...
Source/core/paint/ImagePainter.cpp:130: fprintf(stderr, "painting image\n");
Remove.

[chrishtr, 2015-03-19 22:15:16]

https://codereview.chromium.org/1008043002/diff/140001/Source/core/paint/Imag...
File Source/core/paint/ImagePainter.cpp (right):

https://codereview.chromium.org/1008043002/diff/140001/Source/core/paint/Imag...
Source/core/paint/ImagePainter.cpp:130: fprintf(stderr, "painting image\n");
On 2015/03/19 at 22:14:22, Xianzhu wrote:
> Remove.

Good catch. Fixed.

[chrishtr, 2015-03-19 22:16:35]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-03-19 22:16:36]

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

[commit-bot: I haz the power, 2015-03-19 22:17:04]

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

[commit-bot: I haz the power, 2015-03-19 22:20:06]

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

[commit-bot: I haz the power, 2015-03-19 22:20:07]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/48299)

[chrishtr, 2015-03-19 22:36:27]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-03-19 22:36:27]

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

[commit-bot: I haz the power, 2015-03-19 22:36:41]

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

[commit-bot: I haz the power, 2015-03-20 01:56:51]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192219

[esprehn, 2015-03-20 06:52:31]

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

[esprehn, 2015-03-20 06:52:32]

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

https://codereview.chromium.org/1008043002/diff/180001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:196: return
LayoutReplaced::invalidatePaintIfNeeded(paintInvalidationState,
paintInvalidationContainer);
It might be nice to paint the first frame of an animated image, otherwise if you
scroll fast you might see the content behind the image in the prepaint area.

https://codereview.chromium.org/1008043002/diff/180001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/1008043002/diff/180001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:3120: ASSERT(document().lifecycle().state()
!= DocumentLifecycle::InPaintInvalidation);
Are you sure this is safe? Calling markContainerChainForPaintInvalidation inside
paint invalidation seems like it would leave dirty bits in the tree.

[yurys, 2015-03-20 08:54:04]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1025603002/ by yurys@chromium.org.

The reason for reverting is: Several animation tests started failing
intermittently after this change

Dashboard link:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=pai....

[chrishtr, 2015-03-20 16:44:45]

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

https://codereview.chromium.org/1008043002/diff/180001/Source/core/layout/Lay...
Source/core/layout/LayoutImage.cpp:196: return
LayoutReplaced::invalidatePaintIfNeeded(paintInvalidationState,
paintInvalidationContainer);
On 2015/03/20 at 06:52:32, esprehn wrote:
> It might be nice to paint the first frame of an animated image, otherwise if
you scroll fast you might see the content behind the image in the prepaint area.

That is indeed a bug in this version, thanks for pointing it out. Will fix.

https://codereview.chromium.org/1008043002/diff/180001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/1008043002/diff/180001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.cpp:3120: ASSERT(document().lifecycle().state()
!= DocumentLifecycle::InPaintInvalidation);
On 2015/03/20 at 06:52:32, esprehn wrote:
> Are you sure this is safe? Calling markContainerChainForPaintInvalidation
inside paint invalidation seems like it would leave dirty bits in the tree.

Good point, this assert is still valuable for preventing other bad use. I'll
adjust it.