Closed Bug 943041 Opened 9 years ago Closed 9 years ago

OMTC compositing does not respect ASAP mode

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 4 obsolete files)

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.
Attachment #8337999 - Flags: review?(bgirard)
Attachment #8338001 - Flags: review?(bgirard)
Attachment #8337999 - Attachment is obsolete: true
Attachment #8337999 - Flags: review?(bgirard)
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?
Attachment #8338001 - Flags: review?(bgirard) → review-
(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?
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.
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.
With more comments and a main thread check.
Attachment #8338001 - Attachment is obsolete: true
Attachment #8338741 - Flags: review?(bgirard)
Attachment #8338741 - Attachment is obsolete: true
Attachment #8338741 - Flags: review?(bgirard)
Attachment #8338767 - Flags: review?(bgirard)
Attachment #8338767 - Attachment is obsolete: true
Attachment #8338767 - Flags: review?(bgirard)
Attachment #8338770 - Flags: review?(bgirard)
Attachment #8338770 - Flags: review?(bgirard) → review+
Blocks: 943859
https://hg.mozilla.org/mozilla-central/rev/93d937df1409
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.