Fix partial painting with render pipeline throttling

Fix partial painting with render pipeline throttling

This patch fixes a problem with throttled FrameViews sometimes appearing partially painted.
This was because we generally paint some distance beyond the viewport, i.e., covering the
entire interest rect. FrameViews which are inside the interest rect but outside the viewport
are skipped during painting, so the recorded display list won't include their contents. If
the FrameView is then scrolled on-screen without causing any other paint invalidations, the
display list won't get updated and the FrameView contents will not be shown.

This patch fixes the problem by forcing a repaint of FrameViews when they become
unthrottled, discarding any previous display lists and tile textures for the area they
cover.

BUG=562343
Committed: https://crrev.com/b4cb32a71a3b31c2ad7cbd2c289fa0ecc5d43c4a
Cr-Commit-Position: refs/heads/master@{#371517}

[Sami, 2016-01-19 18:55:06]

Description was changed from

==========
Fix partial painting with render pipeline throttling (WIP)

BUG=562343
==========

to

==========
Fix partial painting with render pipeline throttling (WIP)

BUG=562343
==========

[Sami, 2016-01-19 18:55:06]

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

[Sami, 2016-01-19 18:58:36]

This is more like a request for comments than a review at this point, but does
this make sense to you guys? Can you suggest a better way?

Basically what I think is happening is:

1. On page with an offscreen iframe we first lay everything out and paint the
root GraphicsLayer. Since the iframe is throttled, it ends up painting nothing.

2. We scroll the iframe on screen so it becomes unthrottled, but because the
iframe is layout clean, nothing triggers it to repaint and we just see the
transparent contents from step #1.

[Xianzhu, 2016-01-19 19:08:33]

On 2016/01/19 18:58:36, Sami wrote:
> This is more like a request for comments than a review at this point, but does
> this make sense to you guys? Can you suggest a better way?
> 
> Basically what I think is happening is:
> 
> 1. On page with an offscreen iframe we first lay everything out and paint the
> root GraphicsLayer. Since the iframe is throttled, it ends up painting
nothing.
> 
> 2. We scroll the iframe on screen so it becomes unthrottled, but because the
> iframe is layout clean, nothing triggers it to repaint and we just see the
> transparent contents from step #1.

We can use MayBeClippedByPaintDirtyRect (which perhaps needs a renaming) value
of PaintLayerPainter::PaintResult in the case. The value indicates that a layer
needs to be repainted when interest rect changes. (Just noticed that the comment
to it needs an update. Currently we cache partly painted results and invalidate
the cache when interest rect changes.)

[Sami, 2016-01-20 15:57:34]

Thanks for the tip. Does this look like what you had in mind? I think I still
need to trigger the repaint when unthrottling because it's not guaranteed to
happen otherwise.

[chrishtr, 2016-01-20 16:12:21]

On 2016/01/20 at 15:57:34, skyostil wrote:
> Thanks for the tip. Does this look like what you had in mind? I think I still
need to trigger the repaint when unthrottling because it's not guaranteed to
happen otherwise.

I think the current code should suffice, because scrolling causes the interest
rect code to update, and then paint as necessary due to to the
cache-invalidating flag you're returning in PaintLayePainter. Is there another
case you are testing that doesn't yet work?

[Sami, 2016-01-20 16:54:13]

On 2016/01/20 16:12:21, chrishtr wrote:
> I think the current code should suffice, because scrolling causes the interest
> rect code to update, and then paint as necessary due to to the
> cache-invalidating flag you're returning in PaintLayePainter. Is there another
> case you are testing that doesn't yet work?

See the bug for a pretty reliable test case. If you scroll up and down with
PageUp/Down (with #enable-experimental-web-platform-features), often some of the
red iframes will appear blank.

This is just my reading of the subsequence caching code so I'm probably missing
something, but it looks like the MayBeClippedByPaintDirtyRect result is getting
ignored in this case:

FrameView::paint
=> FrameView::paintContents
   => PaintLayerPainter::paint
      => PaintLayerPainter::paint (ignores return value from paintLayer)
         => PaintLayerPainter::paintLayer
            => PaintLayerPainter::paintLayerContents (early-out with
MaybeClippedByPaintDirtyRect because of throttling)

That is, MaybeClippedByPaintDirtyRect doesn't end up getting recorded anywhere.

The other problem is just scrolling the page doesn't seem to cause another paint
pass to happen (because there hasn't been any style mutations or similar), so we
never get around to re-recording the subsequence.

[chrishtr, 2016-01-20 17:08:11]

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:274: return result;
I think you need to call m_paintLayer.setPreviousPaintResult(result) before
returning here. Does that fix your current bug?

[Sami, 2016-01-20 18:06:43]

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:274: return result;
On 2016/01/20 17:08:11, chrishtr wrote:
> I think you need to call m_paintLayer.setPreviousPaintResult(result) before
> returning here. Does that fix your current bug?

Hmm, isn't that what I'm doing on line 273 or did I miss something?-)

[Sami, 2016-01-20 18:10:10]

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:274: return result;
On 2016/01/20 18:06:42, Sami wrote:
> On 2016/01/20 17:08:11, chrishtr wrote:
> > I think you need to call m_paintLayer.setPreviousPaintResult(result) before
> > returning here. Does that fix your current bug?
> 
> Hmm, isn't that what I'm doing on line 273 or did I miss something?-)

And to be clear the other part of the bugfix is this line here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Xianzhu, 2016-01-20 18:31:04]

On 2016/01/20 16:54:13, Sami wrote:
> On 2016/01/20 16:12:21, chrishtr wrote:
> > I think the current code should suffice, because scrolling causes the
interest
> > rect code to update, and then paint as necessary due to to the
> > cache-invalidating flag you're returning in PaintLayePainter. Is there
another
> > case you are testing that doesn't yet work?
> 
> See the bug for a pretty reliable test case. If you scroll up and down with
> PageUp/Down (with #enable-experimental-web-platform-features), often some of
the
> red iframes will appear blank.
> 
> This is just my reading of the subsequence caching code so I'm probably
missing
> something, but it looks like the MayBeClippedByPaintDirtyRect result is
getting
> ignored in this case:
> 
> FrameView::paint
> => FrameView::paintContents
>    => PaintLayerPainter::paint
>       => PaintLayerPainter::paint (ignores return value from paintLayer)
>          => PaintLayerPainter::paintLayer
>             => PaintLayerPainter::paintLayerContents (early-out with
> MaybeClippedByPaintDirtyRect because of throttling)
> 
> That is, MaybeClippedByPaintDirtyRect doesn't end up getting recorded
anywhere.
>

It a bug that MaybeClippedByPaintDirtyRect is not propagated to ancestor layers
of the FrameView's layer, with the current subsequence caching design. However,
propagating MaybeClippedByPaintDirtyRect will degrade performance because we'll
repaint all ancestor layers when interest rect changes if there is a throttled
frame, so we should avoid the propagation. Perhaps in
FrameView::synchronizedPaint() we should iterate the previously throttled frames
to force repaint of the frames becoming non-throttled.
 
> The other problem is just scrolling the page doesn't seem to cause another
paint
> pass to happen (because there hasn't been any style mutations or similar), so
we
> never get around to re-recording the subsequence.

Is FrameView::updateAllLifecyclePhases() called in the case?

[Xianzhu, 2016-01-20 18:31:12]

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:274: return result;
I think we need to clear m_paintLayer's other previousXXX states to avoid them
happening to be the same as the new ones once the FrameView becomes
non-throttled.

[Sami, 2016-01-20 18:44:11]

On 2016/01/20 18:31:04, Xianzhu wrote:
> It a bug that MaybeClippedByPaintDirtyRect is not propagated to ancestor
layers
> of the FrameView's layer, with the current subsequence caching design.
However,
> propagating MaybeClippedByPaintDirtyRect will degrade performance because
we'll
> repaint all ancestor layers when interest rect changes if there is a throttled
> frame, so we should avoid the propagation.

I see, having a throttled frame would basically mean everything gets repainted
every time we scroll for instance. I guess we'd still be using cached
subsequences in that case but the overhead is still pretty big.

> Perhaps in
> FrameView::synchronizedPaint() we should iterate the previously throttled
frames
> to force repaint of the frames becoming non-throttled.

Would it be equivalent to call PaintLayer::setNeedsPaint() when unthrottling
(which is outside of the lifecycle update)?
  
> > The other problem is just scrolling the page doesn't seem to cause another
> paint
> > pass to happen (because there hasn't been any style mutations or similar),
so
> we
> > never get around to re-recording the subsequence.
> 
> Is FrameView::updateAllLifecyclePhases() called in the case?

Yes, it is. That's how we notice the frame is now unthrottled.

[Sami, 2016-01-20 18:49:31]

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:274: return result;
On 2016/01/20 18:31:12, Xianzhu wrote:
> I think we need to clear m_paintLayer's other previousXXX states to avoid them
> happening to be the same as the new ones once the FrameView becomes
> non-throttled.

Good point. Looks like it should be sufficient to reset previousPaintDirtyRect?
(or instead ensure needsRepaint is set if we decide to do that instead.)

[Xianzhu, 2016-01-20 18:51:03]

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/1603983002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:274: return result;
On 2016/01/20 18:49:31, Sami wrote:
> On 2016/01/20 18:31:12, Xianzhu wrote:
> > I think we need to clear m_paintLayer's other previousXXX states to avoid
them
> > happening to be the same as the new ones once the FrameView becomes
> > non-throttled.
> 
> Good point. Looks like it should be sufficient to reset
previousPaintDirtyRect?
> (or instead ensure needsRepaint is set if we decide to do that instead.)

needsRepaint should be suffice.

[Sami, 2016-01-25 11:34:02]

I dug a little more into this and found that PaintLayer::setNeedsRepaint()
wasn't quite enough because it didn't set the dirty rect on the GraphicsLayer.
This causes DisplayListRecordingSource (and other subsequent pipeline eparts) to
early-out since there's no damage to the layer.

To work around this I'm now calling
LayoutView::setShouldDoFullPaintInvalidation(PaintInvalidationBecameVisible) for
FrameViews that become unthrottled. This makes sure the FrameView's paint is
invalidated and the corresponding area in the GraphicsLayer is marked as needing
display. I think this also causes us to also refresh any subsequence cache so
there's no risk of stale content. PTAL.

[Sami, 2016-01-25 11:41:24]

Description was changed from

==========
Fix partial painting with render pipeline throttling (WIP)

BUG=562343
==========

to

==========
Fix partial painting with render pipeline throttling

This patch fixes a problem with throttled FrameViews sometimes appearing
partially painted. This was because we generally paint some distance beyond the
viewport, i.e., covering the 
entire interest rect. FrameViews which are inside the interest rect but outside
the viewport 
are skipped during painting, so the recorded display list won't include their
contents. If 
the FrameView is then scrolled on-screen without causing any other paint
invalidations, the 
display list won't get updated and the FrameView contents will not be shown.

This patch fixes the problem by forcing a repaint of FrameViews when they become

unthrottled, discarding any previous display lists and tile textures for the
area they
cover.

BUG=562343
==========

[Sami, 2016-01-25 11:41:37]

Description was changed from

==========
Fix partial painting with render pipeline throttling

This patch fixes a problem with throttled FrameViews sometimes appearing
partially painted. This was because we generally paint some distance beyond the
viewport, i.e., covering the 
entire interest rect. FrameViews which are inside the interest rect but outside
the viewport 
are skipped during painting, so the recorded display list won't include their
contents. If 
the FrameView is then scrolled on-screen without causing any other paint
invalidations, the 
display list won't get updated and the FrameView contents will not be shown.

This patch fixes the problem by forcing a repaint of FrameViews when they become

unthrottled, discarding any previous display lists and tile textures for the
area they
cover.

BUG=562343
==========

to

==========
Fix partial painting with render pipeline throttling

This patch fixes a problem with throttled FrameViews sometimes appearing
partially painted.
This was because we generally paint some distance beyond the viewport, i.e.,
covering the 
entire interest rect. FrameViews which are inside the interest rect but outside
the viewport 
are skipped during painting, so the recorded display list won't include their
contents. If 
the FrameView is then scrolled on-screen without causing any other paint
invalidations, the 
display list won't get updated and the FrameView contents will not be shown.

This patch fixes the problem by forcing a repaint of FrameViews when they become

unthrottled, discarding any previous display lists and tile textures for the
area they
cover.

BUG=562343
==========

[Xianzhu, 2016-01-25 18:33:17]

lgtm

https://codereview.chromium.org/1603983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/1603983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:281: 
Can you add a test for the following scenario (or please ignore this if there is
already one):

- A frame is not throttled;
- the frame becomes throttled;
- some object changes style in the frame;
- the frame becomes not throttled;
ensure the object changed style is painted with the new style.

Based on the code this seems to work because we paint nothing (so cache nothing)
for throttled frames, but perhaps in the future we could cache the previous
non-throttled painted results.

[Sami, 2016-01-25 18:44:23]

https://codereview.chromium.org/1603983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp (right):

https://codereview.chromium.org/1603983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/FrameThrottlingTest.cpp:281: 
On 2016/01/25 18:33:17, Xianzhu wrote:
> Can you add a test for the following scenario (or please ignore this if there
is
> already one):
> 
> - A frame is not throttled;
> - the frame becomes throttled;
> - some object changes style in the frame;
> - the frame becomes not throttled;
> ensure the object changed style is painted with the new style.
> 
> Based on the code this seems to work because we paint nothing (so cache
nothing)
> for throttled frames, but perhaps in the future we could cache the previous
> non-throttled painted results.

Great idea, done.

[Sami, 2016-01-26 10:59:38]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-01-26 10:59:39]

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/1603983002/#ps100001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-01-26 10:59:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1603983002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1603983002/100001

[commit-bot: I haz the power, 2016-01-26 12:07:38]

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

[commit-bot: I haz the power, 2016-01-26 12:07:38]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sami, 2016-01-26 12:08:26]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2016-01-26 12:08:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1603983002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1603983002/100001

[commit-bot: I haz the power, 2016-01-26 13:04:25]

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

[commit-bot: I haz the power, 2016-01-26 13:04:26]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2016-01-26 13:31:14]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-01-26 13:31:14]

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/1603983002/#ps120001
(title: "Undo changes to LayerTreeAsText.")

[commit-bot: I haz the power, 2016-01-26 13:32:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1603983002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1603983002/120001

[commit-bot: I haz the power, 2016-01-26 15:41:31]

Description was changed from

==========
Fix partial painting with render pipeline throttling

This patch fixes a problem with throttled FrameViews sometimes appearing
partially painted.
This was because we generally paint some distance beyond the viewport, i.e.,
covering the 
entire interest rect. FrameViews which are inside the interest rect but outside
the viewport 
are skipped during painting, so the recorded display list won't include their
contents. If 
the FrameView is then scrolled on-screen without causing any other paint
invalidations, the 
display list won't get updated and the FrameView contents will not be shown.

This patch fixes the problem by forcing a repaint of FrameViews when they become

unthrottled, discarding any previous display lists and tile textures for the
area they
cover.

BUG=562343
==========

to

==========
Fix partial painting with render pipeline throttling

This patch fixes a problem with throttled FrameViews sometimes appearing
partially painted.
This was because we generally paint some distance beyond the viewport, i.e.,
covering the 
entire interest rect. FrameViews which are inside the interest rect but outside
the viewport 
are skipped during painting, so the recorded display list won't include their
contents. If 
the FrameView is then scrolled on-screen without causing any other paint
invalidations, the 
display list won't get updated and the FrameView contents will not be shown.

This patch fixes the problem by forcing a repaint of FrameViews when they become

unthrottled, discarding any previous display lists and tile textures for the
area they
cover.

BUG=562343
==========

[commit-bot: I haz the power, 2016-01-26 15:41:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-26 15:41:51]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-01-26 15:42:49]

Description was changed from

==========
Fix partial painting with render pipeline throttling

This patch fixes a problem with throttled FrameViews sometimes appearing
partially painted.
This was because we generally paint some distance beyond the viewport, i.e.,
covering the 
entire interest rect. FrameViews which are inside the interest rect but outside
the viewport 
are skipped during painting, so the recorded display list won't include their
contents. If 
the FrameView is then scrolled on-screen without causing any other paint
invalidations, the 
display list won't get updated and the FrameView contents will not be shown.

This patch fixes the problem by forcing a repaint of FrameViews when they become

unthrottled, discarding any previous display lists and tile textures for the
area they
cover.

BUG=562343
==========

to

==========
Fix partial painting with render pipeline throttling

This patch fixes a problem with throttled FrameViews sometimes appearing
partially painted.
This was because we generally paint some distance beyond the viewport, i.e.,
covering the
entire interest rect. FrameViews which are inside the interest rect but outside
the viewport
are skipped during painting, so the recorded display list won't include their
contents. If
the FrameView is then scrolled on-screen without causing any other paint
invalidations, the
display list won't get updated and the FrameView contents will not be shown.

This patch fixes the problem by forcing a repaint of FrameViews when they become
unthrottled, discarding any previous display lists and tile textures for the
area they
cover.

BUG=562343

Committed: https://crrev.com/b4cb32a71a3b31c2ad7cbd2c289fa0ecc5d43c4a
Cr-Commit-Position: refs/heads/master@{#371517}
==========

[commit-bot: I haz the power, 2016-01-26 15:42:50]

Patchset 7 (id:??) landed as
https://crrev.com/b4cb32a71a3b31c2ad7cbd2c289fa0ecc5d43c4a
Cr-Commit-Position: refs/heads/master@{#371517}