Open Bug 1605155 Opened 5 years ago Updated 2 years ago

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)

defect

Tracking

()

People

(Reporter: mconley, Unassigned)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: perf, reproducible)

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

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

@sotaro: any thoughts?

Blocks: wr-perf
Flags: needinfo?(sotaro.ikeda.g)
Keywords: perf, reproducible
Priority: -- → P3

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.

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.

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

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

Flags: needinfo?(sotaro.ikeda.g) → needinfo?(gwatson)

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?

Flags: needinfo?(gwatson)

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?

Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)

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

Flags: needinfo?(mconley)

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.

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.

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

Flags: needinfo?(mconley) → needinfo?(jmuizelaar)

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

Flags: needinfo?(jmuizelaar)
Blocks: 161338, videopip
No longer blocks: wr-displaylist-perf
Blocks: wr-displaylist-perf
No longer blocks: 161338
Depends on: 1743875
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.