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
Categories
(Core :: Graphics: WebRender, defect, P3)
Tracking
()
People
(Reporter: mconley, Unassigned)
References
(Depends on 1 open bug, Blocks 3 open bugs)
Details
(Keywords: perf, reproducible)
STR:
- Ensure WebRender is enabled
- Enable the WebRender profiler (
gfx.webrender.debug.profiler
) - Visit any YouTube video
- Star the video, notice the activity in the profiler HUD
- 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•5 years ago
|
||
Note: Pausing the video Picture-in-picture video causes the youtube page to stop updating as well.
@sotaro: any thoughts?
Comment 2•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
:gw, is it possible to detect if video is covered by display items at WR?
Comment 6•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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 10•5 years ago
|
||
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•5 years ago
|
||
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.
Reporter | ||
Comment 12•5 years ago
|
||
Is it still worth writing the patch suggested in comment 8 given comment 11?
Comment 13•5 years ago
|
||
(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.
Updated•5 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•2 years ago
|
Description
•