Avoid double buffering in BasicCompositor when guaranteed that layers do not overlap

ASSIGNED
Assigned to

Status

()

Firefox
General
--
enhancement
ASSIGNED
4 years ago
2 years ago

People

(Reporter: handyman, Assigned: handyman)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
From bug 994554,

> Only double buffering overlapping layers. We only need to double buffer when 
> there actually is overlap. BasicLayerManager does some analysis to figure out 
> if any layers actually overlap within the current invalid region and which 
> minimal sub-tree of the layer tree needs to be double buffered to fix it. 
> This massively reduces the rate that we have to double buffer, and reduces 
> the area when we do.
> 
> The BasicLayerManager code for this is: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicLayerManager.cpp#469

In short, we want the BasicCompositor to do the double-buffering work that BasicLayerManager would if it ran with the Compositor.  Since only regions with overdraw/underdraw need double-buffering (to avoid flickering), we want to only double-buffer those regions.

The BasicLayerManager allocates a surface for each overdraw region.  We should also consider atlasing the overdraw regions instead of creating textures for each -- we can avoid texture sampler changes this way.  We could also reduce draw calls by batching the copy of the overdraw regions to the draw target that way.  Simple rectangle-packing is very fast and pretty simple (see Wikipedia ( http://en.wikipedia.org/wiki/Packing_problems#Different_rectangles_in_a_rectangle ).
(Assignee)

Comment 1

4 years ago
(Assignee)

Comment 2

4 years ago
(Assignee)

Comment 3

4 years ago
Created attachment 8456240 [details] [diff] [review]
BasicCompositor without full-screen double-buffering

This patch works and passes test suites.
(Assignee)

Updated

4 years ago
Attachment #8456240 - Flags: review?(matt.woodrow)
Comment on attachment 8456240 [details] [diff] [review]
BasicCompositor without full-screen double-buffering

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

For the most part this looks really good!

Could you please split this into a few smaller patches (each of which compile on their own preferably). A lot of the changes are fairly mechanical (trailing whitespace removal, DrawQuad signature change, DefaultComputeEffectiveTransforms changes) and would be much easier to review as standalone patches. This also makes like much easier for anyone looking through hg history at a later data. If you're using mq to manage patches then I can help you with doing this.

::: gfx/layers/Compositor.h
@@ +315,5 @@
>     * required, these will be in the primary effect in the effect chain.
>     */
>    virtual void DrawQuad(const gfx::Rect& aRect, const gfx::Rect& aClipRect,
>                          const EffectChain& aEffectChain,
> +                        const gfx::DrawOptions &drawOptions, const gfx::Matrix4x4 &aTransform) = 0;

I don't like using DrawOptions here, since it implies that we (might) support the AntialiasMode as well. We already have EffectBlendMode as part of the effect chain for setting blend mode, this is how SetMixBlendMode is implemented.

@@ +516,5 @@
> +   * up into the intermediate surface created, false otherwise.
> +   */
> +  virtual void ComputeSupportsComponentAlphaChildren(ContainerLayerComposite &layer, bool *aNeedsSurfaceCopy) {
> +    if (aNeedsSurfaceCopy)  {
> +      *aNeedsSurfaceCopy = false;

Why default to false? Surface copying is used for all the accelerated compositors to be able to render component alpha layers correctly.

::: gfx/layers/basic/BasicCompositor.cpp
@@ +154,5 @@
> +  // CopySurface ignores the destination transform so we apply it manually.
> +  Matrix destTransform = aDest->GetTransform();
> +  Point destPos = destTransform * Point(aDestRect.x, aDestRect.y);
> +  aDest->PushClipRect(destTransform.TransformBounds(aDestRect));
> +  aDest->CopySurface(aSource, sourceIntRect, IntPoint(destPos.x, destPos.y));

CopySurface also ignores clip, so I don't think this will work.

Why do we need this instead of just using DrawSurfaceWithTextureCoords?

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +453,5 @@
>      effectChain.mPrimaryEffect = new EffectRenderTarget(surface);
>  
>      gfx::Rect rect(visibleRect.x, visibleRect.y, visibleRect.width, visibleRect.height);
>      gfx::Rect clipRect(aClipRect.x, aClipRect.y, aClipRect.width, aClipRect.height);
> +    aManager->GetCompositor()->DrawQuad(rect, clipRect, effectChain, DrawOptions(opacity, gfx::CompositionOp::OP_SOURCE),

I don't think this is a correct change, if the intermediate surface wasn't opaque then we would normally expect it to be blended with the background.

@@ +696,5 @@
> +}
> +
> +void
> +ContainerLayerComposite::CompositeComputeSupportsComponentAlphaChildren(bool* aNeedsSurfaceCopy)
> +{

Why do we need to modify this for BasicCompositor?

BasicCompositor doesn't ever support component alpha children properly, so if anything this overloaded version should be setting everything to false.
Attachment #8456240 - Flags: review?(matt.woodrow)
(In reply to David Parks [:handyman] from comment #0)

> The BasicLayerManager allocates a surface for each overdraw region.  We
> should also consider atlasing the overdraw regions instead of creating
> textures for each -- we can avoid texture sampler changes this way.  We
> could also reduce draw calls by batching the copy of the overdraw regions to
> the draw target that way.  Simple rectangle-packing is very fast and pretty
> simple (see Wikipedia (
> http://en.wikipedia.org/wiki/
> Packing_problems#Different_rectangles_in_a_rectangle ).

We could, but for software 2d libraries, the number of draw calls isn't usually a large component of the performance, instead the run time is fairly proportional to the number of pixels drawn.

Texture atlasing is probably a big win for our 3d Compositor backends, if we can find easy situations to use it.
Comment on attachment 8456240 [details] [diff] [review]
BasicCompositor without full-screen double-buffering

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

::: gfx/layers/Compositor.h
@@ +506,5 @@
> +   * Set the effective transforms on layer, based on this
> +   * Compositor's buffering mechanism.
> +   */
> +  virtual void ComputeEffectiveTransforms(LayerComposite &layer,
> +                                          const gfx::Matrix4x4& aTransformToSurface);

I'd rather avoid exposing Layers to the Compositor, since it's not supposed to know/care about them.

Can we instead expose the condition that we care about (NeedsBufferedOutput maybe?) and do the branching in the calling code?
(Assignee)

Comment 7

4 years ago
Thanks Matt.  I'm working on these issues now.  Some notes in anticipation of the updated patch:

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Could you please split this into a few smaller patches (each of which
> compile on their own preferably).

Sure.  I had intended to do that but I didn't see a neat partitioning.  But then, I didn't look that hard.


> ::: gfx/layers/Compositor.h
> @@ +315,5 @@
> >     * required, these will be in the primary effect in the effect chain.
> >     */
> >    virtual void DrawQuad(const gfx::Rect& aRect, const gfx::Rect& aClipRect,
> >                          const EffectChain& aEffectChain,
> > +                        const gfx::DrawOptions &drawOptions, const gfx::Matrix4x4 &aTransform) = 0;
> 
> I don't like using DrawOptions here, since it implies that we (might)
> support the AntialiasMode as well.
> We already have EffectBlendMode as part
> of the effect chain for setting blend mode, this is how SetMixBlendMode is
> implemented.

You're right.  This was a concession to get the blend mode into DrawQuad (more on why I need this below) but its already possible with the effect chain.

> 
> @@ +516,5 @@
> > +   * up into the intermediate surface created, false otherwise.
> > +   */
> > +  virtual void ComputeSupportsComponentAlphaChildren(ContainerLayerComposite &layer, bool *aNeedsSurfaceCopy) {
> > +    if (aNeedsSurfaceCopy)  {
> > +      *aNeedsSurfaceCopy = false;
> 
> Why default to false? Surface copying is used for all the accelerated
> compositors to be able to render component alpha layers correctly.

I didn't get the purpose of this function when I wrote this.  You are correct -- this is wrong.  Additionally... its not used anywhere.
     
> ::: gfx/layers/basic/BasicCompositor.cpp
> @@ +154,5 @@
> > +  // CopySurface ignores the destination transform so we apply it manually.
> > +  Matrix destTransform = aDest->GetTransform();
> > +  Point destPos = destTransform * Point(aDestRect.x, aDestRect.y);
> > +  aDest->PushClipRect(destTransform.TransformBounds(aDestRect));
> > +  aDest->CopySurface(aSource, sourceIntRect, IntPoint(destPos.x, destPos.y));
> 
> CopySurface also ignores clip, so I don't think this will work.

Also true.  Should have been removed.

> 
> Why do we need this instead of just using DrawSurfaceWithTextureCoords?

DrawSurfaceWithTextureCoords could be used if it supported OP_SOURCE blend mode (I believe our OGL Compositor, for example, currently doesn't).  I also went with CopySurface because its faster (if render-target modes are compatible for blitting) and can't end up aliasing.  But I think the key thing you want to know is why I want OP_SOURCE in the first place.

The first thing to note is that this is only relevant when the ContainerLayerComposite needs to useIntermediateSurface.  Second, either the ContainerLayerComposite has only one child which is opaque (ie it needs no blending), or it sets needsSurfaceCopy in ContainerLayer::DefaultComputeSupportsComponentAlphaChildren.  When that is set, the ContainerLayerComposite copies the framebuffer to the off-screen "double" buffer before rendering the layer's children to it.  The rendering is blended as it is drawn.  Once the double-buffer is drawn to, it is then written to the framebuffer in the call to DrawQuad we are discussing.  This is how things currently work.

Now, its ok that the rendering in that DrawQuad call is done with OP_OVER because the blending is always blending to a transparent black background -- ie the extra double-buffer that BasicCompositor::BeginFrame created.  That secondary framebuffer is then blitted (not drawn) back to our actual destination in BasicCompositor::EndFrame.

But now, we dont have that secondary framebuffer and the call to DrawQuad blends into our already-partially-rendered framebuffer.  This causes things to blend weird but it also causes the window manager to blend weird in Windows Aero.  Using 'Copy' fixes all of that.


> ::: gfx/layers/composite/ContainerLayerComposite.cpp
> @@ +453,5 @@
> >      effectChain.mPrimaryEffect = new EffectRenderTarget(surface);
> >  
> >      gfx::Rect rect(visibleRect.x, visibleRect.y, visibleRect.width, visibleRect.height);
> >      gfx::Rect clipRect(aClipRect.x, aClipRect.y, aClipRect.width, aClipRect.height);
> > +    aManager->GetCompositor()->DrawQuad(rect, clipRect, effectChain, DrawOptions(opacity, gfx::CompositionOp::OP_SOURCE),
> 
> I don't think this is a correct change, if the intermediate surface wasn't
> opaque then we would normally expect it to be blended with the background.

Same as above.   The deal is that we have already blended into the intermediate surface (same as always) and the old DrawQuad operated as a de-facto OP_SOURCE draw, which we now need to genuinely do.

> @@ +696,5 @@
> > +}
> > +
> > +void
> > +ContainerLayerComposite::CompositeComputeSupportsComponentAlphaChildren(bool* aNeedsSurfaceCopy)
> > +{
> 
> Why do we need to modify this for BasicCompositor?
> 
> BasicCompositor doesn't ever support component alpha children properly, so
> if anything this overloaded version should be setting everything to false.

I didn't realize that.  This, like the similar function at the top, is totally wrong.  The BasicCompositor does need ComputeSupportsComponentAlphaChildren, tho, to set needsSurfaceCopy as discussed above.  I think the function's purpose grew beyond its name.  Regardless, this function is also not called so, easy fix.

Let me know if you have any comments on the strategy.  I'll let you know when I've got your recommendations implemented.
(Assignee)

Comment 8

4 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
>
> ::: gfx/layers/Compositor.h
> @@ +506,5 @@
> > +   * Set the effective transforms on layer, based on this
> > +   * Compositor's buffering mechanism.
> > +   */
> > +  virtual void ComputeEffectiveTransforms(LayerComposite &layer,
> > +                                          const gfx::Matrix4x4& aTransformToSurface);
> 
> I'd rather avoid exposing Layers to the Compositor, since it's not supposed
> to know/care about them.
> 
> Can we instead expose the condition that we care about (NeedsBufferedOutput
> maybe?) and do the branching in the calling code?

I can do that.  The dilemma here was that I needed the compositor to be able to decide the strategy for computing transforms (since BasicCompositor requires different routines than others now).  I wanted to implement it as a virtual function in the Layer but the LayerComposite multiple-inheritance thing got in the way.  I'm pretty sure that Layer should be a virtual base class of LayerComposite to get rid of a lot of forwarding and casting stuff but thats a fairly colossal deal that tons of people would have an opinion on.  I'll find a way to do it without exposing LayerComposite to the Compositor (...but it will also be hacky).
(In reply to David Parks [:handyman] from comment #7)
> DrawSurfaceWithTextureCoords could be used if it supported OP_SOURCE blend
> mode (I believe our OGL Compositor, for example, currently doesn't).  I also
> went with CopySurface because its faster (if render-target modes are
> compatible for blitting) and can't end up aliasing.  But I think the key
> thing you want to know is why I want OP_SOURCE in the first place.

I'm not sure why the OGL Compositor is relevant here, since this code lives entirely within the software compositor. DrawSurfaceWithTextureCoords should be able to support OP_SOURCE very easily, just need to pass the operator down into it.

It is possibly true that CopySurface is faster, but I doubt the extra complexity to detect when we can use it is worth it in this case. If it turns out to be important then we should probably do this detection and optimization within the Moz2D backend rather than in layers code.

> 
> The first thing to note is that this is only relevant when the
> ContainerLayerComposite needs to useIntermediateSurface.  Second, either the
> ContainerLayerComposite has only one child which is opaque (ie it needs no
> blending), or it sets needsSurfaceCopy in
> ContainerLayer::DefaultComputeSupportsComponentAlphaChildren.  When that is
> set, the ContainerLayerComposite copies the framebuffer to the off-screen
> "double" buffer before rendering the layer's children to it.  The rendering
> is blended as it is drawn.  Once the double-buffer is drawn to, it is then
> written to the framebuffer in the call to DrawQuad we are discussing.  This
> is how things currently work.

I agree with the first thing here, but not the second. We only set needsSurfaceCopy to true under some conditions (that we have opaque content behind us, that the layer transform is just a translation and that we don't have complex blend modes). It's very possible for us to have a set of children that aren't opaque, and not do a surface copy. This is the path where we get CreateRenderTarget(rect, INIT_MODE_CLEAR).

You should be able to reproduce this fairly easy by animation a rotation on an element that isn't opaque, like text with no background color .

It appears that we set needsSurfaceCopy = true in all cases that we can currently, I think that's a bug, and we should only do so if we have the CONTENT_COMPONENT_ALPHA content flag on the layer. Copying up the background isn't cheap and we shouldn't be doing it unless we really need to.

> 
> Now, its ok that the rendering in that DrawQuad call is done with OP_OVER
> because the blending is always blending to a transparent black background --
> ie the extra double-buffer that BasicCompositor::BeginFrame created.  That
> secondary framebuffer is then blitted (not drawn) back to our actual
> destination in BasicCompositor::EndFrame.
> 
> But now, we dont have that secondary framebuffer and the call to DrawQuad
> blends into our already-partially-rendered framebuffer.  This causes things
> to blend weird but it also causes the window manager to blend weird in
> Windows Aero.  Using 'Copy' fixes all of that.

Using OP_SOURCE when drawing directly into the native window object makes perfect sense, since there is likely existing content there. We can't use clear+over since that would expose us to the same flickering that double buffering exists to prevent.

> 
> 
> > ::: gfx/layers/composite/ContainerLayerComposite.cpp
> > @@ +453,5 @@
> > >      effectChain.mPrimaryEffect = new EffectRenderTarget(surface);
> > >  
> > >      gfx::Rect rect(visibleRect.x, visibleRect.y, visibleRect.width, visibleRect.height);
> > >      gfx::Rect clipRect(aClipRect.x, aClipRect.y, aClipRect.width, aClipRect.height);
> > > +    aManager->GetCompositor()->DrawQuad(rect, clipRect, effectChain, DrawOptions(opacity, gfx::CompositionOp::OP_SOURCE),
> > 
> > I don't think this is a correct change, if the intermediate surface wasn't
> > opaque then we would normally expect it to be blended with the background.
> 
> Same as above.   The deal is that we have already blended into the
> intermediate surface (same as always) and the old DrawQuad operated as a
> de-facto OP_SOURCE draw, which we now need to genuinely do.

As above, there's plenty of situations where the intermediate surface isn't opaque, and we haven't copied up opaque content in order to fix that. We still need OVER blending in those cases.
(In reply to Matt Woodrow (:mattwoodrow) from comment #9) 
> It appears that we set needsSurfaceCopy = true in all cases that we can
> currently, I think that's a bug, and we should only do so if we have the
> CONTENT_COMPONENT_ALPHA content flag on the layer. Copying up the background
> isn't cheap and we shouldn't be doing it unless we really need to.

I'm going to fix this with one of the patches in bug 1023677.
Also filed bug 1039145 to make DefaultComputeSupportsComponentAlpha always return false when using BasicCompositor.
(Assignee)

Comment 12

4 years ago
Created attachment 8461791 [details] [diff] [review]
basic-comp-dbuff.patch

WIP: Further work on double buffering
Attachment #8456240 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8461794 [details] [diff] [review]
WIP: Work on making opaque calculations produce the desired result

Fixes to opacity calculations and intermediate-surface logic.  Works on Windows but seems to have affected GL implementations by using an unimplemented blend mode.
Looks like there's some promising stuff in here but no activity for a little while. What blocks the current patches from landing? David, id you are busy I can pick this up and finish it, if you give me some pointers to where things stopped and what's left.
Flags: needinfo?(davidp99)
(Assignee)

Comment 15

4 years ago
I gave up on this when I realized that there were a lot of places where the layers were in very slight overlap (for example, one large chrome layer and one large content layer in sub-pixel overlap... the union of which covered nearly the entire window)... and that double-buffering these overlapping regions was devastating to performance because it meant so many more compositing draw calls.  I'd started to dive into the actual layer calculations but I was in way over my head (this was my second Firefox bug).  I'll be on PTO until January but I'd be happy to help when I get back.  But I had the wrong idea about a lot of fundamental stuff at the point I wrote this so 'buyer beware'.

So, to be precise, I had the double-buffering working fine (that *should* be in the patch here) but the performance was *much more* terrible (like 5-10x or something) so it didn't make sense to submit without fixing the layers.
Flags: needinfo?(davidp99)
You need to log in before you can comment on or make changes to this bug.