1092360 - Avoid redundant framebuffer switches

Status: RESOLVED WONTFIX

(Core :: Graphics, defect, P3)

Description

[Benoit Girard (:BenWa)]

+++ This bug was initially created as a clone of Bug #1087530 +++

Spinning off the framebuffer changes into their own change.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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 3

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 4

[Benoit Girard (:BenWa)]

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.

Comment 5

[Benoit Girard (:BenWa)]

(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.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: patch v3

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch v3

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch v4 (rebased)

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch v4 (rebased)

Comment 12

[Benoit Girard (:BenWa)]

Attachment: part 2: Refactor how basic intermediate surfaces are created

Comment 13

[Benoit Girard (:BenWa)]

Attachment: part 2: Refactor how basic intermediate surfaces are created

Comment 14

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 15

[Benoit Girard (:BenWa)]

Attachment: patch v5

Comment 16

[Benoit Girard (:BenWa)]

Attachment: patch v6

Comment 17

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=42175cddd6b1

Comment 18

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 19

[Benoit Girard (:BenWa)]

Attachment: patch v7

Comment 20

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 21

[Benoit Girard (:BenWa)]

Attachment: patch v8

Comment 22

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=797857e8ca1f

Comment 23

[Benoit Girard (:BenWa)]

Attachment: patch v9

https://tbpl.mozilla.org/?tree=Try&rev=d891a48293fa

Comment 24

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48667a14f94d

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Benoit Girard (:BenWa)]

Attachment: patch v10

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=28d627f2dbcc

Comment 27

[Sylvestre Ledru [:Sylvestre]]

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

Comment 28

[Sylvestre Ledru [:Sylvestre]]

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