Closed Bug 963821 Opened 7 years ago Closed 7 years ago

Port FPS counter to the Compositor API

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Works well on Windows now and should work fine when the basic compositor is ready.
Attachment #8365375 - Flags: review?(bas)
Comment on attachment 8365375 [details] [diff] [review]
patch

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +316,5 @@
> +		       int aOffsetX, int aOffsetY,
> +                       Compositor* aCompositor,
> +		       EffectChain& aEffectChain)
> +{
> +  unsigned int divisor = 100;

NOTE TO SELF: > 1000 -> 999
Comment on attachment 8365375 [details] [diff] [review]
patch

Vlad are you happy with going with this version instead of yours?

Our approach are very different. I'm not sure how they compare in performance but my patch is simpler and the performance is good enough. We could speed it up by extending Compositor API to support a DrawQuad API and possible add a way to retain an abtraction over VBO for accelerated backends.
Attachment #8365375 - Flags: feedback?(vladimir)
See Also: → 874781
Comment on attachment 8365375 [details] [diff] [review]
patch

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

Things that make things more cross-platform are generally good...

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +440,5 @@
>    /** Our more efficient but less powerful alter ego, if one is available. */
>    nsRefPtr<Composer2D> composer2D = mCompositor->GetWidget()->GetComposer2D();
>  
>    if (composer2D && composer2D->TryRender(mRoot, mWorldMatrix)) {
> +    double fps = mFPS->mCompositionFps.AddFrameAndGetFps(TimeStamp::Now());

shouldn't this only be called if mFPS != nullptr?
Yes, another other review comments?
Attached patch patch - rebasedSplinter Review
Assignee: nobody → bgirard
Attachment #8365375 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8365375 - Flags: review?(bas)
Attachment #8365375 - Flags: feedback?(vladimir)
Attachment #8373639 - Flags: review?(bas)
Attachment #8373639 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/058f4ed4d33b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
BenWa, the following code causes the regression as not to use HWComposer at all. Can you fix it?

------------------------------------------------
 
   /** Our more efficient but less powerful alter ego, if one is available. */
   nsRefPtr<Composer2D> composer2D = mCompositor->GetWidget()->GetComposer2D();
 
-  if (composer2D && composer2D->TryRender(mRoot, mWorldMatrix)) {
+  if (mFPS && composer2D && composer2D->TryRender(mRoot, mWorldMatrix)) {
+    double fps = mFPS->mCompositionFps.AddFrameAndGetFps(TimeStamp::Now());
+    if (gfxPlatform::GetPrefLayersDrawFPS()) {
+      printf_stderr("HWComposer: FPS is %g\n", fps);
+    }
     mCompositor->EndFrameForExternalComposition(mWorldMatrix);
     return;
   }
Flags: needinfo?(bgirard)
(In reply to Benoit Girard (:BenWa) from comment #4)
> Comment on attachment 8365375 [details] [diff] [review]
> patch
> 
> Vlad are you happy with going with this version instead of yours?
> 
> Our approach are very different. I'm not sure how they compare in
> performance but my patch is simpler and the performance is good enough. We
> could speed it up by extending Compositor API to support a DrawQuad API and
> possible add a way to retain an abtraction over VBO for accelerated backends.

Oh yeah, go nuts.  Getting something in is way better than blocking; the world has changed quite a bit since my patches too.  It shouldn't hard to get any of the interesting bits from my patches back in again at some point.
Blocks: 971875
Opps, filed bug 971875 to fix the regression.

It's really bad that we don't have any test for HWC on checkin :(
Flags: needinfo?(bgirard)
You need to log in before you can comment on or make changes to this bug.