Open Bug 1092360 Opened 5 years ago Updated Last year

Avoid redundant framebuffer switches

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

People

(Reporter: BenWa, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file, 13 obsolete files)

25.74 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1087530 +++

Spinning off the framebuffer changes into their own change.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8516232 - Flags: review?(nical.bugzilla)
Attachment #8516232 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8516232 [details] [diff] [review]
patch

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +554,5 @@
>    MOZ_ASSERT(aInvertEffect || aGrayscaleEffect || aContrastEffect != 0.0);
>  
> +  if (aPreviousTarget) {
> +    mCompositor->SetRenderTarget(aPreviousTarget);
> +  }

Why is this here? Let's not do it.
Comment on attachment 8516232 [details] [diff] [review]
patch

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +333,5 @@
>    RenderLayers(aContainer, aManager, RenderTargetPixel::FromUntyped(aClipRect));
> +
> +  // The caller is responsible for binding the new render target.
> +  // This allows the prepare phase to render to multiple intermediate
> +  // surface without rebinding to the screen inbetween.

This function should stop getting the previousTarget.

I actually wonder if this function should just be inlined into it's callers for clarity. That makes it so that binding and unbinding can still be paired and we don't have the weirdness of the function changing state.

It also moves the 'if (!surface)' checks closer to the surface creation checks which feels better.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +611,1 @@
>  

What is this change for?
Attachment #8516232 - Flags: review-
I've had to remove this assertion because we now run the prepare phase outside of begin frame by design:

@@ -858,17 +858,16 @@ CompositorOGL::DrawQuad(const Rect& aRec
                         const Rect& aClipRect,
                         const EffectChain &aEffectChain,
                         Float aOpacity,
                         const gfx::Matrix4x4 &aTransform)
 {
   PROFILER_LABEL("CompositorOGL", "DrawQuad",
     js::ProfileEntry::Category::GRAPHICS);
 
-  MOZ_ASSERT(mFrameInProgress, "frame not started");
   MOZ_ASSERT(mCurrentRenderTarget, "No destination");
 
   Rect clipRect = aClipRect;
   // aClipRect is in destination coordinate space (after all
   // transforms and offsets have been applied) so if our
   // drawing is going to be shifted by mRenderOffset then we need
   // to shift the clip rect by the same amount.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> ::: gfx/layers/opengl/CompositorOGL.cpp
> @@ +611,1 @@
> >  
> 
> What is this change for?

This to to make sure we can 1) Easily track the surface change for debugging, 2) track the bind with the profiler. Really the current line just bi-passes the function for no good reason.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8516232 - Attachment is obsolete: true
Attachment #8516937 - Flags: review?(jmuizelaar)
Comment on attachment 8516937 [details] [diff] [review]
patch v2

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +335,5 @@
>  
>      if (aContainer->mPrepared->mNeedsSurfaceCopy) {
>        // we needed to copy the background so we waited until now to render the intermediate
>        surface = CreateTemporaryTargetAndCopyFromBackground(aContainer, aManager);
> +      RefPtr<CompositingRenderTarget> previousTarget = compositor->GetCurrentRenderTarget();

Move this above the call to CreateTemporaryTarget so that if comes after it.

@@ +340,5 @@
> +      if (surface) {
> +        compositor->SetRenderTarget(surface);
> +        RenderLayers(aContainer, aManager, RenderTargetPixel::FromUntyped(aClipRect));
> +      }
> +      compositor->SetRenderTarget(previousTarget);

Move this SetRenderTarget into the if so that it more obviously matches with the other SetRenderTarget.
Attachment #8516937 - Flags: review?(jmuizelaar) → review+
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8516937 - Attachment is obsolete: true
Attachment #8519125 - Flags: review+
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8519125 - Attachment is obsolete: true
Attachment #8519126 - Flags: review+
Attached patch patch v4 (rebased) (obsolete) — Splinter Review
Attachment #8520782 - Flags: review+
Attachment #8519126 - Attachment is obsolete: true
Attached patch patch v4 (rebased) (obsolete) — Splinter Review
Attachment #8520782 - Attachment is obsolete: true
Attachment #8520785 - Flags: review+
Attachment #8520908 - Attachment is obsolete: true
Attachment #8520993 - Flags: review?(jmuizelaar)
Comment on attachment 8520993 [details] [diff] [review]
part 2: Refactor how basic intermediate surfaces are created

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

Benoit and I talked about this. We agreed that it would be better to keep Prepare inside BeginFrame and move the SetRenderTarget out of BeginFrame into a separate explicit call that happens after Prepare.

::: gfx/layers/basic/BasicCompositor.h
@@ +128,5 @@
>    // Widget associated with this compositor
>    nsIWidget *mWidget;
>    gfx::IntSize mWidgetSize;
>  
> +  // The draw target type to us when creating an intermediate surface.

What does that mean?

@@ +129,5 @@
>    nsIWidget *mWidget;
>    gfx::IntSize mWidgetSize;
>  
> +  // The draw target type to us when creating an intermediate surface.
> +  RefPtr<gfx::DrawTarget> mDrawTargetType;

mReferenceDrawTarget?
Attachment #8520993 - Flags: review?(jmuizelaar) → review-
Attached patch patch v5 (obsolete) — Splinter Review
Attachment #8520785 - Attachment is obsolete: true
Attachment #8520993 - Attachment is obsolete: true
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #8521722 - Attachment is obsolete: true
Attachment #8521753 - Flags: review?(jmuizelaar)
Comment on attachment 8521753 [details] [diff] [review]
patch v6

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

::: gfx/layers/Compositor.h
@@ +473,5 @@
>    }
>  
> +  /**
> +   * This is called to set the composition target (the screen or an offscreen target)
> +   * after we've ran the prepare phase. This msut be called after BeginFrame.

must instead of msut

@@ +478,5 @@
> +   */
> +  virtual void SetCompositionTarget() {
> +    MOZ_ASSERT(mCompositingTarget);
> +    SetRenderTarget(mCompositingTarget);
> +  }

I think things would be cleaner if you didn't try to share this code between backends and just implemented it as required. That would let you avoid adding mCompositingTarget to the base class which let's you remove the cast in BasicCompositor.cpp. If you want more convincing of this I can do that tomorrow.
Attachment #8521753 - Flags: review?(jmuizelaar) → review-
Attached patch patch v7 (obsolete) — Splinter Review
Attachment #8521753 - Attachment is obsolete: true
Attachment #8522441 - Flags: review?(jmuizelaar)
Comment on attachment 8522441 [details] [diff] [review]
patch v7

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

Change the setter and member names to FinalDestinationTarget.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +712,5 @@
>    // Allow widget to render a custom background.
>    mCompositor->GetWidget()->DrawWindowUnderlay(this, nsIntRect(actualBounds.x,
>                                                                 actualBounds.y,
>                                                                 actualBounds.width,
>                                                                 actualBounds.height));

DrawWindowUnderlay should be under the new hunk.
Attachment #8522441 - Flags: review?(jmuizelaar) → review+
Attached patch patch v8 (obsolete) — Splinter Review
Attachment #8522441 - Attachment is obsolete: true
Attachment #8522529 - Flags: review+
Attached patch patch v9 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=d891a48293fa
Attachment #8522529 - Attachment is obsolete: true
Attachment #8522681 - Flags: review+
sorry had to back this out for test failures on Linux like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3892467&repo=mozilla-inbound
Flags: needinfo?(bgirard)
Attached patch patch v10Splinter Review
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=28d627f2dbcc
Attachment #8522681 - Attachment is obsolete: true
Flags: needinfo?(bgirard)
Attachment #8523115 - Flags: review+
Assignee: bgirard → nobody
Status: ASSIGNED → NEW
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
You need to log in before you can comment on or make changes to this bug.