956263 - Create a pref for full-tilt compositing

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Markus Stange [:mstange]]

This would be useful for compositor performance testing. There's a patch in bug 949692 you can apply to get the compositor to re-composite all the time, even if nothing changed. Being able to flip a pref instead would be nicer.

Comment 1

[Benoit Girard (:BenWa)]

Taking this, this will be useful for perf evaluation on desktop.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch

fix crash on shutdown

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8365395 [details] [diff] [review]
patch

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

When I tested with the patch from bug 949692 a few weeks ago, I noticed that we stopped compositing whenever window focus changes forced a sync composite. Idle compositing started again when the next normal async composition happened. Does this patch have the same behavior?

::: gfx/layers/ipc/CompositorParent.cpp
@@ +549,5 @@
> +      rate = 0;
> +    } else {
> +      rate = gfxPlatform::GetPrefLayoutFrameRate();
> +    }
> +  }

I don't understand this at all. Are some of these supposed to be gfxPlatform::GetPrefLayersCompositionFrameRate()? Also, please introduce additional variables if necessary so that you don't need to call each GetPref variant more than once - not for perf reasons, but for code clarity reasons.

::: modules/libpref/src/init/all.js
@@ +4083,5 @@
>  pref("layers.scroll-graph", false);
>  
>  // Set the default values, and then override per-platform as needed
>  pref("layers.offmainthreadcomposition.enabled", false);
> +// Number of ms to use between composites.

This comment seems wrong.

Comment 5

[Benoit Girard (:BenWa)]

(In reply to Markus Stange [:mstange] from comment #4)
> I don't understand this at all. Are some of these supposed to be
> gfxPlatform::GetPrefLayersCompositionFrameRate()? Also, please introduce
> additional variables if necessary so that you don't need to call each
> GetPref variant more than once - not for perf reasons, but for code clarity
> reasons.
> 

The default value of this new preference is 0 which means default to matching the layout frame behavior. This is what we want. Then -1 means run as fast as you can which may still be blocked by the OS vsync.

Perhaps I'd be simpler to just have a full-tilt perference as bool. For a platform like OSX where we vsync you'd have to set layout.frame_rate to uncap the glSwapInterval vsync.

Comment 6

[Benoit Girard (:BenWa)]

(In reply to Benoit Girard (:BenWa) from comment #5)
> (In reply to Markus Stange [:mstange] from comment #4)
> > I don't understand this at all. Are some of these supposed to be
> > gfxPlatform::GetPrefLayersCompositionFrameRate()? Also, please introduce
> > additional variables if necessary so that you don't need to call each
> > GetPref variant more than once - not for perf reasons, but for code clarity
> > reasons.
> > 
> 
> The default value of this new preference is 0 which means default to
> matching the layout frame behavior. This is what we want. Then -1 means run
> as fast as you can which may still be blocked by the OS vsync.
> 
> Perhaps I'd be simpler to just have a full-tilt perference as bool. For a
> platform like OSX where we vsync you'd have to set layout.frame_rate to
> uncap the glSwapInterval vsync.

Actually I think the preference may be confusing but I think tweaking the transaction settings of the main thread and compositor independently is very useful. I'm open on suggestions to make this easier to understand but think switching this to a bool and loose the ability to target a frame-rate on the compositor is a bad idea.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch

Fixed the sync composite case. Nice catch!

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8365575 [details] [diff] [review]
patch

(In reply to Benoit Girard (:BenWa) from comment #5)
> The default value of this new preference is 0 which means default to
> matching the layout frame behavior. This is what we want. Then -1 means run
> as fast as you can which may still be blocked by the OS vsync.

Hmm, can we turn this around, so that it's consistent with the layout frame rate special values? layout.frame_rate == 0 means "as fast as possible" and layout.frame_rate == -1 means "use default".

I agree that it's good to have explicit control of the composition frame rate and not just a boolean switch.

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

>   int32_t rate = gfxPlatform::GetPrefLayoutFrameRate();
>   if (rate < 0) {
>     // TODO: The main thread frame scheduling code consults the actual monitor
>     // refresh rate in this case. We should do the same.
>     rate = kDefaultFrameRate;
>   }
>+  if (gfxPlatform::GetPrefLayoutFrameRate() != 0) {
>+    if (gfxPlatform::GetPrefLayoutFrameRate() == -1) {
>+      rate = 0;
>+    } else {
>+      rate = gfxPlatform::GetPrefLayoutFrameRate();
>+    }
>+  }

This calculation still doesn't use the value of gfxPlatform::GetPrefLayersCompositionFrameRate().

How about something like this:

static int32_t
CalculateCompositionFrameRate()
{
  int32_t compositionFrameRatePref = gfxPlatform::GetPrefLayersCompositionFrameRate();
  if (compositionFrameRatePref < 0) {
    // Use the same frame rate for composition as for layout.
    int32_t layoutFrameRatePref = gfxPlatform::GetPrefLayoutFrameRate();
    if (layoutFrameRatePref < 0) {
      // TODO: The main thread frame scheduling code consults the actual
      // monitor refresh rate in this case. We should do the same.
      return kDefaultFrameRate;
    }
    return layoutFrameRatePref;
  }
  return compositionFrameRatePref;
}

...

  int32_t rate = CalculateCompositionFrameRate();

   // If rate == 0 (ASAP mode), minFrameDelta must be 0 so there's no delay.
   TimeDuration minFrameDelta = TimeDuration::FromMilliseconds(
     rate == 0 ? 0.0 : std::max(0.0, 1000.0 / rate));

...

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>@@ -4079,16 +4079,20 @@ pref("layers.frame-counter", false);
> pref("layers.max-active", -1);
> // When a layer is moving it will add a scroll graph to measure the smoothness
> // of the movement. NOTE: This pref triggers composites to refresh
> // the graph.
> pref("layers.scroll-graph", false);
> 
> // Set the default values, and then override per-platform as needed
> pref("layers.offmainthreadcomposition.enabled", false);
>+// Number of ms to use between composites.

The comment is still wrong. It's a frequency, not a time interval.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch v2

Agreed with your feedback.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch v2.1 rebased r=mstange

Comment 11

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5f29b7bd75

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a5f29b7bd75