Benoit Girard (:BenWa) Assignee
	Description • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 1 • 9 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 3 • 9 years ago

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?

	Mason Chang [Inactive] [:mchang]
	Comment 4 • 9 years ago

(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

	Benoit Girard (:BenWa) Assignee
	Comment 5 • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 6 • 9 years ago

Bug 1216287 - Properly invalidate the debug overlay. r=mattwoodrow

	Benoit Girard (:BenWa) Assignee
	Comment 7 • 9 years ago

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.

	Matt Woodrow (:mattwoodrow)
	Comment 8 • 9 years ago

Comment on attachment 8675917 [details]
MozReview Request: Bug 1216287 - Properly invalidate the debug overlay. r=mattwoodrow

https://reviewboard.mozilla.org/r/22507/#review20049

	Benoit Girard (:BenWa) Assignee
	Comment 9 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dedd0f85f8b1be2230287e73b4906c5b1165996
Bug 1216287 - Properly invalidate the debug overlay. r=mattwoodrow

	Carsten Book [:Tomcat]
	Comment 10 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7dedd0f85f8b

	Carsten Book [:Tomcat]
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/7dedd0f85f8b