+++ This bug was initially created as a clone of Bug #1087530 +++ Spinning off the framebuffer changes into their own change.

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?

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.

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

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

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

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