Closed Bug 1216287 Opened 6 years ago Closed 6 years ago

FPS counter causes OMTVideo to render at 60 FPS

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

(Keywords: power)

Attachments

(1 file)

Looks like a side-effect of bug 1150552. It shows that video is recompositing at 60 FPS. Doesn't effect users without the FPS counter however.
Alright so here's the issue:
- The FPS causes invalid region to be always set.
- The video code causes wake-up to check the invalid region.

Normally each on their on work fine. But when we combine them it causes us to always wake up and notice that the region is invalid. So we start painting at 60 FPS.

I think the bug is that the video code is still causing idle-wake-ups at 60 FPS to check if the frame has changed instead of scheduling the next frame which is bad for power usage. It will cause these even when the FPS counter is hidden.

In bug 1103207 I'm making it so we only wake up the compositor when we're ready to composite. Perhaps we need to do the same for video.

ni? :roc since you worked on bug 1143575.
Flags: needinfo?(roc)
Keywords: power
Summary: Bug 1150552 causes video to recomposite at 60 FPS when FPS counter is shown → OMT-Video is causing 60 FPS idle-wake-ups
(In reply to Benoit Girard (:BenWa) from comment #1)
> Alright so here's the issue:
> - The FPS causes invalid region to be always set.
> - The video code causes wake-up to check the invalid region.
> 
> Normally each on their on work fine. But when we combine them it causes us
> to always wake up and notice that the region is invalid. So we start
> painting at 60 FPS.
> 
> I think the bug is that the video code is still causing idle-wake-ups at 60
> FPS to check if the frame has changed instead of scheduling the next frame
> which is bad for power usage. It will cause these even when the FPS counter
> is hidden.

My understanding is that these wakeups occur no matter what because our hardware vsync timer code must run on every vsync. Mason, am I wrong?

If that's wrong, then we definitely need to do something. But if that's not wrong, the current approach may not impact power usage that much, and it is nice and simple.
Flags: needinfo?(roc) → needinfo?(mchang)
Ahh that's a good point.

If I recall correctly the VSync driver will keep ticking until it hasn't been needed for x frames/ms or something similar. 20 FPS video for instance would probably always leave the VSync driver running.

mchang can you confirm?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> (In reply to Benoit Girard (:BenWa) from comment #1)
> > Alright so here's the issue:
> > - The FPS causes invalid region to be always set.
> > - The video code causes wake-up to check the invalid region.
> > 
> > Normally each on their on work fine. But when we combine them it causes us
> > to always wake up and notice that the region is invalid. So we start
> > painting at 60 FPS.
> > 
> > I think the bug is that the video code is still causing idle-wake-ups at 60
> > FPS to check if the frame has changed instead of scheduling the next frame
> > which is bad for power usage. It will cause these even when the FPS counter
> > is hidden.
> 
> My understanding is that these wakeups occur no matter what because our
> hardware vsync timer code must run on every vsync. Mason, am I wrong?
> 
> If that's wrong, then we definitely need to do something. But if that's not
> wrong, the current approach may not impact power usage that much, and it is
> nice and simple.

You are correct. The underlying platform vsync timers will run at every vsync no matter what. The compositor / refresh driver can ignore the vsync signal by unobserving, but the underlying vsync signal will continue to tick until we disable hardware vsync.

(In reply to Benoit Girard (:BenWa) from comment #3)
> Ahh that's a good point.
> 
> If I recall correctly the VSync driver will keep ticking until it hasn't
> been needed for x frames/ms or something similar. 20 FPS video for instance
> would probably always leave the VSync driver running.
> 
> mchang can you confirm?

Correct. The refresh driver has logic to start/stop the timer, which will keep the hardware vsync signal alive. The compositor will listen to X vsync notifications before unobserving [1]. See [2] for why it's set to 10. We won't disable the hardware vsync signal until both the compositor and refresh driver are idle.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?case=true&from=CompositorParent.cpp#426
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPrefs.h#268
Flags: needinfo?(mchang)
Alright so we agreed that we should just fix the FPS counter to show the correct number. We WONTFIX the idle wake-ups because they are within the design restriction of the refresh driver.

Patch incoming for the FPS counter.
Summary: OMT-Video is causing 60 FPS idle-wake-ups → FPS counter causes OMTVideo to render at 60 FPS
Bug 1216287 - Properly invalidate the debug overlay. r=mattwoodrow
Attachment #8675917 - Flags: review?(matt.woodrow)
We knew the implementation in bug 1150552 was a hack. And now we've seen a case where it doesn't work. Let's implement it properly this time.
Assignee: nobody → bgirard
Comment on attachment 8675917 [details]
MozReview Request: Bug 1216287 - Properly invalidate the debug overlay. r=mattwoodrow

https://reviewboard.mozilla.org/r/22507/#review20049
Attachment #8675917 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/7dedd0f85f8b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.