943041 - OMTC compositing does not respect ASAP mode

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Markus Stange [:mstange]]

Attachment: Make compositor scheduling respect layout.frame_rate

When layout.frame_rate is zero, we want to paint and composite as fast as possible. CompositorParent currently has a hardcoded frame interval which does not respect this pref.

Comment 1

[Markus Stange [:mstange]]

Attachment: Make compositor scheduling respect layout.frame_rate

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8338001 [details] [diff] [review]
Make compositor scheduling respect layout.frame_rate

Review of attachment 8338001 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +513,5 @@
>    TimeDuration delta;
>    if (!initialComposition)
>      delta = TimeStamp::Now() - mLastCompose;
>  
> +  int32_t rate = gfxPlatform::GetPrefLayoutFrameRate();

This can call |InitLayersAccelerationPrefs();| which can init preferences off the main thread. We need to guarantee we wont call it and likely at the same time remove the call to InitLayersAccelerationPrefs(); there.

@@ +514,5 @@
>    if (!initialComposition)
>      delta = TimeStamp::Now() - mLastCompose;
>  
> +  int32_t rate = gfxPlatform::GetPrefLayoutFrameRate();
> +  if (rate < 0) {

<= to remove divide by zero.

@@ +518,5 @@
> +  if (rate < 0) {
> +    rate = kDefaultFrameRate;
> +  }
> +  TimeDuration minFrameDelta = TimeDuration::FromMilliseconds(
> +    rate > 0 ? std::max(0.0, 1000.0 / rate - 2.0) : 0);

Why - 2?

Comment 3

[Markus Stange [:mstange]]

(In reply to Benoit Girard (:BenWa) from comment #2)
> Comment on attachment 8338001 [details] [diff] [review]
> Make compositor scheduling respect layout.frame_rate
> 
> Review of attachment 8338001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +513,5 @@
> >    TimeDuration delta;
> >    if (!initialComposition)
> >      delta = TimeStamp::Now() - mLastCompose;
> >  
> > +  int32_t rate = gfxPlatform::GetPrefLayoutFrameRate();
> 
> This can call |InitLayersAccelerationPrefs();| which can init preferences
> off the main thread. We need to guarantee we wont call it and likely at the
> same time remove the call to InitLayersAccelerationPrefs(); there.

Maybe gfxPlatform::GetPrefLayoutFrameRate() should wrap its call to InitLayersAccelerationPrefs in a main-thread check, and return -1 if it's called on a non-main thread before the prefs have been initialized.
We have the same problem at http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderCGL.mm#157

> 
> @@ +514,5 @@
> >    if (!initialComposition)
> >      delta = TimeStamp::Now() - mLastCompose;
> >  
> > +  int32_t rate = gfxPlatform::GetPrefLayoutFrameRate();
> > +  if (rate < 0) {
> 
> <= to remove divide by zero.

Actually, no. rate == 0 needs to be handled differently than rate == -1. 0 needs to activate ASAP mode and -1 needs to use the default frame rate.
I avoid division by zero by checking for > 0 in the calculation below.

> 
> @@ +518,5 @@
> > +  if (rate < 0) {
> > +    rate = kDefaultFrameRate;
> > +  }
> > +  TimeDuration minFrameDelta = TimeDuration::FromMilliseconds(
> > +    rate > 0 ? std::max(0.0, 1000.0 / rate - 2.0) : 0);
> 
> Why - 2?

Arbitrary. I'm open to suggestions on this one. Why did the original code use 15 and not 16.6?

Comment 4

[Avi Halachmi (:avih)]

FWIW, the refresh driver uses the following logic for the pref layout.frame_rate:

If > 0, use this value as frequency.

Else if < 0 (-1 is the default), if we can read the HW refresh rate, use it, else, use hardcoded 60 (this happens on each refresh driver iteration since it could change in runtime, and it also reads and uses the timestamp for the next vsync).

Else if 0 (ASAP), internally use an arbitrarily high frequency which we know we can't follow, effectively iterating as fast as possible, while avoiding special-handling of 0 (this arbitrary frequency is 10000).


To test if HW refresh API is available (e.g. it isn't on XP), check for not-null: WinUtils::dwmGetCompositionTimingInfoPtr

Reading the HW refresh rate requires to include WinUtils.h, and then the value is read at GetVBlankInfo here: http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsrefreshdriver.cpp#328

This function fails if HW refresh info is not available (e.g. is DWM is disabled due to theme etc).

Note that GetVBlankInfo at the refresh driver reads the HW refresh info for the main monitor (HWND == 0 == desktop window), because the actual HWND is not easy to access elegantly from the refresh driver timer callback.

It's probably worth using a similar logic here, or even refactor the code a bit and have both use a single GetVBlankInfo function.

Comment 5

[Markus Stange [:mstange]]

OK, it seems like those issues need some work. Let's just make ASAP mode work in this bug so we can land bug 920123 and bug 924403, and then work on using the precise refresh rate in another bug.

Comment 6

[Markus Stange [:mstange]]

Attachment: Make compositor scheduling respect layout.frame_rate

With more comments and a main thread check.

Comment 7

[Markus Stange [:mstange]]

Attachment: Make compositor scheduling respect layout.frame_rate

Comment 8

[Markus Stange [:mstange]]

Attachment: Make compositor scheduling respect layout.frame_rate

Comment 9

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93d937df1409

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93d937df1409