1605155 - Opening a video in Picture-in-Picture still causes the originating tab to compute DisplayLists at the video frame rate while the tab is the foreground

Status: NEW

(Core :: Graphics: WebRender, defect, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1. Ensure WebRender is enabled
2. Enable the WebRender profiler (gfx.webrender.debug.profiler)
3. Visit any YouTube video
4. Star the video, notice the activity in the profiler HUD
5. Open the video in a Picture-in-Picture player window

ER:

HUD activity should drop once the video moves into its own window.

AR:

The HUD still shows the same activity despite the originating tab showing a static image in place of the video

Comment 1

[Kris Taeleman (:ktaeleman)]

Note: Pausing the video Picture-in-picture video causes the youtube page to stop updating as well.

@sotaro: any thoughts?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

A couple things we discovered when looking at this:

• The video is actually still playing in the original tab, it's just covered with an opaque shadow dom element during Picture-in-picture.
• With WebRender off I suspect we detect that the video is not visible and end up preventing the paints either during ComputeVisiblity or Layer invalidation. We don't really have a mechanism in WebRender that would prevent paints in the same way
• A possible way to fix this would be to be to know earlier on in the pipeline that the video isn't visible and avoid causing refresh driver ticks in the first place.

Comment 3

[Sotaro Ikeda [:sotaro]]

If nsDisplayVideo(original video) is set invisible, it is easier to drop AsyncImagePipeline. The nsVideoFrame was still visible(IsVisibleForPainting() = true) even when it was hidden in Tab.

Comment 4

[Sotaro Ikeda [:sotaro]]

WebRender seems not occlude video rendering by cpu even when it is behind other items. It is expected to be dropped by using z-id by GPU

Comment 5

[Sotaro Ikeda [:sotaro]]

:gw, is it possible to detect if video is covered by display items at WR?

Comment 6

[Glenn Watson [:gw]]

There's no current mechanism in WR to detect this is occluded. It's possible we might be able to do it once they are promoted to picture cache slices, but it'd be quite fragile (e.g. anything that prevents promotion to a cache slice, such as a rounded border clip etc would prevent that working).

Without knowing any of the details, it does sound like we should handle this much earlier in the pipeline, to avoid other redundant work. Is that likely to be feasible?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Yeah, the video element knows this is happening we should be able to communicate that down the video frame and avoid invalidation. Matt, any thoughts for the best way to do this?

Comment 8

[Matt Woodrow (:mattwoodrow)]

VideoFrameContainer::InvalidateWithFlags calls into HTMLVideoElement::Invalidate.

I think we can skip calling the baseclass function (which is what requests a displaylist build) if we have mVisualCloneTarget, aImageSizeChanged is false, aNewIntrinsicSize is Nothing() and aForceInvalidate is false.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Mike, is that something you can make a patch for?

Comment 10

[Matt Woodrow (:mattwoodrow)]

I also would expect this not to compute display lists, even without these changes.

HTMLMediaElement::Invalidate is supposed to be calling InvalidateLayer, which should detect that we have a Layer for this video (or the WebRender equivalent). It then won't schedule painting since we've already sent the frame across the video bridge and the compositor should be able to handle that without further input.

If that's not working with WebRender, then we may be spending a lot of time building display lists when playing video normally (without PiP), which isn't great.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

You're right we don't seem to be getting new display lists. With and without PnP. It seems like this is probably a problem deeper in WebRender then.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Is it still worth writing the patch suggested in comment 8 given comment 11?

Comment 13

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #12)

Is it still worth writing the patch suggested in comment 8 given comment 11?

Let's not worry about it for now.