1233585 - [e10s] Youtube triggers content paints because of the display port

Status: NEW

(Core :: Layout, defect, P3)

Description

[Matt Woodrow (:mattwoodrow)]

I'm seeing the YouTube comments section contain only the words 'Loading' and an animated gif that causes us to trigger repaints all the time.

This isn't actually visible, but it's within the displayport, so trigger invalidations/paints seems correct.

When you scroll down to bring it actually into view, then it quickly loads the actual comments and we stop painting.

I haven't looked into exactly how it works, but I suspect it's similar to others in bug 1232491.

Comment 1

[Matt Woodrow (:mattwoodrow)]

YouTube may not want to use the proposal from bug 1232491, since I'm guessing they're intentionally trying to avoid loading comments if the user hasn't scrolled down to see them. Displayport pre-rendering is unlikely to matter much for this use case.

Should we consider suppressing invalidations for animated images that are outside of the visible area, but within the displayport? Will it matter that you can APZ scroll to an out-of-date animated image?

The other option is layerizing the images, and animating them off the main thread and then have the compositors invalidation skip them since they're offscreen. That's a lot more work, but likely something we want eventually.

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Should we consider suppressing invalidations for animated images that are
> outside of the visible area, but within the displayport? Will it matter that
> you can APZ scroll to an out-of-date animated image?

Skipping the invalidation should be easy, but how do we make sure to repaint when the image moves into the visible area?

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> Should we consider suppressing invalidations for animated images that are
> outside of the visible area, but within the displayport? Will it matter that
> you can APZ scroll to an out-of-date animated image?

Can we pause animations that are outside the displayport?

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Do we know how much of an issue this is for power use or performance? It might be good to have a fix for this before e10s is rolled out to the release channel.

Comment 5

[Matt Woodrow (:mattwoodrow)]

It's mainly a power usage problem, very unlikely to cause performance issues. I think it depends on screen resolution as to whether it occurs or not.

The power difference should be fairly easy to measure if someone wants to.

It would be nice to fix this, but we don't have a great plan for doing so right now.

The simplest option is to move animated gifs into their own layers, and make sure we hit the 'empty transactions' path for those updates, but that's likely to have other unwanted effects.

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Timothy Nikkel (:tnikkel) from comment #2)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> > Should we consider suppressing invalidations for animated images that are
> > outside of the visible area, but within the displayport? Will it matter that
> > you can APZ scroll to an out-of-date animated image?
> 
> Skipping the invalidation should be easy, but how do we make sure to repaint
> when the image moves into the visible area?

I don't have a good answer for this.

If the async scroll triggers a display port change on the content side, then we'll rebuild the display list, and DLBI should be able to determine that an image that was previously in 'suppressed' state is now visible within the scrollport.

For smaller scrolls that aren't enough to moved the snapped display port, then we currently skip display list building, so that won't work.

We could track when we're currently suppressing invalidations for images, and always build display lists in that case. We could improve that to only track when a suppressed image is outside of the scrollport, but entirely within a snapped tile size, so that we only do extra display list building for images that wouldn't trigger it already.

That's all sounding pretty complex though, maybe just layerizing animated images is better (and what we do for video).

Any thoughts Tim?

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> We could track when we're currently suppressing invalidations for images,
> and always build display lists in that case. We could improve that to only
> track when a suppressed image is outside of the scrollport, but entirely
> within a snapped tile size, so that we only do extra display list building
> for images that wouldn't trigger it already.

The only improvement I have to what you said is to add the image frame to a list of suppressed images on the scrollframe, then we can iterate that list when we scroll instead of displaylist building to see if they need to be redraw (or if they should still be suppressed).

Layerizing would be nice and get us other benefits.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

FWIW, I think I'd prefer not layerizing animated gifs unless we need to. The layers system doesn't currently deal very well with large numbers of layers (especially creating them). Where is the cpu time being spent during this use case?

Comment 9

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> FWIW, I think I'd prefer not layerizing animated gifs unless we need to. The
> layers system doesn't currently deal very well with large numbers of layers
> (especially creating them). Where is the cpu time being spent during this
> use case?

It's not a huge amount of CPU really, but most of it is spent in display list building.

We're doing a full paint/composite cycle at 60fps though, so it'll add up to a fair amount of battery over the course of a long video.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> > FWIW, I think I'd prefer not layerizing animated gifs unless we need to. The
> > layers system doesn't currently deal very well with large numbers of layers
> > (especially creating them). Where is the cpu time being spent during this
> > use case?
> 
> It's not a huge amount of CPU really, but most of it is spent in display
> list building.
> 
> We're doing a full paint/composite cycle at 60fps though, so it'll add up to
> a fair amount of battery over the course of a long video.

Shouldn't we be able to drop the frames that don't change anything in the current viewport in the compositor?

Comment 12

[Matt Woodrow (:mattwoodrow)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> > > FWIW, I think I'd prefer not layerizing animated gifs unless we need to. The
> > > layers system doesn't currently deal very well with large numbers of layers
> > > (especially creating them). Where is the cpu time being spent during this
> > > use case?
> > 
> > It's not a huge amount of CPU really, but most of it is spent in display
> > list building.
> > 
> > We're doing a full paint/composite cycle at 60fps though, so it'll add up to
> > a fair amount of battery over the course of a long video.
> 
> Shouldn't we be able to drop the frames that don't change anything in the
> current viewport in the compositor?

Right, we could definitely do that (currently we check for changes, but not restricted to the viewport).

I suspect that's considerably less than half of the work we do though, skipping display list building, layer building and painting would be more valuable.

Comment 13

[Seth Fowler [:seth] [:s2h]]

FWIW: the loading animation is a CSS background-image, so we currently don't track its visibility at all. Bug 1218990 would make us do that, but we'd still animate the image as long as it's within the displayport (and even that only after bug 1261554); we don't currently have an implementation for finer-grained visibility tracking than that.

We're going to need to implement visibility tracking that's aware of the viewport for intersection observer anyway. In practice what that means is that we'll have to send messages from the compositor process to the content process notifying us when "rects of interest" enter or leave the viewport. If we want to solve this problem without layerizing the images (which I have to say does seem like the cleanest solution), we can certainly piggyback on the same mechanism.