Closed Bug 882447 Opened 7 years ago Closed 6 years ago

Only composite changed areas with BasicCompositor

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: mattwoodrow, Assigned: dvander)

References

(Depends on 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 6 obsolete files)

Currently the BasicCompositor always composites the entire screen, which is proving to be too slow.

For working this out on the main thread we use this code:

* Before we start layer construction, we take a snapshot of the existing layer tree
 - http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1125
 - Or alternatively (fast path when only a video or canvas has changed frame) http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5540

* Then we run layer construction and DLBI. This modifies the existing layer tree (which could have been empty if it's the first paint) and DLBI generates an invalid region for each ThebesLayer (relative to the ThebesLayer), retrievable via Layer::GetInvalidRegion.

* Then we compare the new layer tree to the snapshot and generate an invalid region in screen pixels
 - http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1198

* We send that to the widget, and it queues a paint event.

* Paint events arrives, specifies a region to draw (At least the region we generated earlier, but possibly more). We composite that.

The BasicCompositor still currently relies on the paint events coming from the OS, so we still need to generate the invalid region in layout. Once we can composite entirely without main-thread interaction, then this could go away.

I think what we want to do is to add the invalid region to the set of properties that gets sent across IPC for each layer.

Then any time we receive a transaction on the compositor side, we can run the same snapshot/compare code to see what changes were made, and how much recompositing is required. Obviously we can skip this if we're using an accelerated compositor.
I talked with Matt a little more about this, and here's some additional explanation for my memory.

Layer::GetInvalidRegion returns the region that has changed within the given layer. It doesn't account for new/removed layers or for a layer being transformed. It doesn't actually compute anything itself; it just returns a region that's been built up by calls to Layer::AddInvalidRect or Layer::SetInvalidRectToVisibleRegion.

The patch would change things so that the Layer::AddInvalidRect calls still happen on the main thread. The compositor thread would call Layer::GetInvalidRegion, which would return a region that was sent over the IPC channel from the main thread.
Thinking about this more, we're going to have real issues if the OS specified repaint area, and the compositor side computed ones don't match up. This can happen during a resize or window focus change event, since the OS will add areas to the invalid region.

We could fix this by just passing the OS area to the compositor, that would fix the in-process case.

For the OOP case, we have no easy way of computing the change area of the sub-process content to give to the OS. Currently we just always pass the entire region.

I think we really need to fix the problem that BasicCompositor can't composite outside of a paint event. That's going to be easier, and a lot more useful than trying to figure out a crazy solution to the above problem.

:(

I'll file a bug for that too.
Blocks: 865104
Filed bug 882523.
Attached patch first try (obsolete) — Splinter Review
I tried to do something like what you described in comment 0. Could you let me know if this is at all reasonable? The data it's printing out seems a little weird, so I doubt it's correct.

Also, once I have the invalid region, what should I do with it? I'm guessing it should be passed to BeginFrame somehow?
Attachment #763037 - Flags: feedback?(matt.woodrow)
Attached patch correct patch (obsolete) — Splinter Review
...and I had the wrong patch.
Attachment #763037 - Attachment is obsolete: true
Attachment #763037 - Flags: feedback?(matt.woodrow)
Attachment #763038 - Flags: feedback?(matt.woodrow)
Comment on attachment 763038 [details] [diff] [review]
correct patch

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

(In reply to Bill McCloskey (:billm) from comment #4)
>
> Also, once I have the invalid region, what should I do with it? I'm guessing
> it should be passed to BeginFrame somehow?

Yeah, that seems reasonable. BeginFrame already takes a clip rect parameter, but I think that's a required clip, whereas our computed one is an optional one. We don't want to require the accelerated backends to try clip to a complex region, since they need a mask to do that.

Overall it looks good, about what I expected. Handling out-of-process content is slightly more complex, I'll dump what I know about that in the next comment.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +126,5 @@
>  void
>  LayerManagerComposite::BeginTransaction()
>  {
>    mInTransaction = true;
> +  mClonedLayerTreeProperties = LayerProperties::CloneFrom(GetRoot());

The compositor does multiple transactions for each content change.

We do one transaction when we receive the new layers across IPC (since layer trees can only be modified within a transaction), and a separate one to composite the layers to the screen.

There's no point running the layer tree comparison for the latter, so you'd probably be better off having this code inside LayerTransactionParent::RecvUpdate.

::: gfx/layers/ipc/LayerTransaction.ipdlh
@@ +192,5 @@
>   };
>  
>  struct ThebesLayerAttributes {
>    nsIntRegion validRegion;
> +  nsIntRegion invalidRegion;

All layer types can have an invalid region, so this should probably be part of CommonLayerAttributes.
Attachment #763038 - Flags: feedback?(matt.woodrow) → feedback+
When we have content processes, we end up with a master layer tree, and 1 or more sub-trees (one for each content process).

The main layer manger (for the chrome process - layer_manager()->GetRoot()) has all the layers for drawing the UI, as well as a 'RefLayer' attachment point for the content process' sub tree.

The content process sub trees are are stored in sIndirectLayerTrees.

When we get to composite time, we use AutoResolveRefLayers to match the RefLayers to the subtree they refer to, and combine the layer trees together (RefLayer is a subclass of ContainerLayer).

The problem here is that during LayerTransactionParent::RecvUpdate, the subtrees aren't attached, so running layer tree comparison on GetRoot() won't step into the subtrees looking for changes.

I can think of two possible options here:

One is to keep an invalid rect for each layer tree separately. When we get a call to RecvUpdate, determine which tree it's operating on (mShadowLayersManager knows this), and use the root of that, rather than the GetRoot().

The downside of this is that the stored invalid region for subtrees are in coordinates relative to the content process' frame, not in screen coordinates. So at some point you'd need to transform these (by walking up the layer tree to the true root) applying the layer transform to convert them to screen coords.

The other option would be to use AutoResolveRefLayers inside RecvUpdate, so that the layer tree comparison can find the subtrees to compare.

Downside here is that each RecvUpdate call is for a single subtree, or the main layer tree. That means that we'd be running comparisons on a bunch of layers that we know haven't changed.

It's not obvious which of those approaches is superior. Might be easier to implement the latter, and worry about performance later if it turns out to be an issue. I'd expect layer tree comparison to be fairly cheap, and it's not blocking the main thread. Or we could think of something that just gets the best of both.
Assignee: nobody → dvander
Status: NEW → ASSIGNED
Attached patch very rough patch (obsolete) — Splinter Review
Very rough draft. This patch builds on top of Bill's by changing the compositor to clip to the bounding box of the invalid region, and then only copying those pixels. For non-compositing window managers, we also have to send the invalid region of paint events to the compositor, before any composites are scheduled, but this can be done asynchronously.
Attachment #763038 - Attachment is obsolete: true
Attached patch invalid.patch (obsolete) — Splinter Review
This version allocates a surface sized just for the invalidated area. It also clips the draw target to the rectangles in the invalidation region.

With this, my microbenchmark composites in 1.1ms instead of 4.2ms and the CPU usage improves about 20%. It doesn't seem as good as the main-thread basic layers yet though.
Attachment #822614 - Attachment is obsolete: true
Attached patch invalid.patch (obsolete) — Splinter Review
Windows support (tested on XP and 7)
Attachment #822726 - Attachment is obsolete: true
mwu, can we steal this for the gonk widget code? This would probably make a pretty big difference on crap hardware where the compositor is memory bandwidth limited, not to mention the power savings.
We already have a bit of logic which determines what we actually need to draw. This probably does a better job, though I'm not sure how to integrate it based on the changes to other widget backends.
The widget changes are for non-compositing window managers, where if you overlap the window the OS loses the pixels. Since the actual layers don't change, we have to tell the compositor about the pixels the OS lost.

I'm not sure whether or not that applies to Gonk. For example it doesn't for OS X so no changes were needed there.
Ah ok.

On gonk, we are the only thing running on screen, so that isn't a concern.
Attached patch invalid.patch (obsolete) — Splinter Review
rebased
Attachment #823731 - Attachment is obsolete: true
Attachment #830575 - Flags: review?(matt.woodrow)
Comment on attachment 830575 [details] [diff] [review]
invalid.patch

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

Adding nrc to the reviewers, since I think he'll want to check the nsWindowGfx.cpp change.

::: gfx/layers/basic/BasicCompositor.cpp
@@ +566,5 @@
> +  mInvalidRegion = aInvalidRegion;
> +
> +  if (aRenderBoundsOut) {
> +    *aRenderBoundsOut = Rect();
> +  }

Why do we need to do this? We set it further down.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +144,5 @@
>  void
>  LayerManagerComposite::BeginTransaction()
>  {
>    mInTransaction = true;
> +  mClonedLayerTreeProperties = LayerProperties::CloneFrom(GetRoot());

Can we only do this if mCompositor is LAYERS_BASIC, pretty pointless work otherwise.
Attachment #830575 - Flags: review?(ncameron)
Attachment #830575 - Flags: review?(matt.woodrow)
Attachment #830575 - Flags: review+
Comment on attachment 830575 [details] [diff] [review]
invalid.patch

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

I only looked at nsWindowGfx, so apologies if I got the wrong end of the stick with what you are doing here. I fear that the scheduled composite is not early enough (it is now after ::BeginPaint, and I'm not sure how long that takes, it is possible for that to trigger windows messages, but I'm not sure if it does for us).

The early composite is for smooth resizing of the window from the top or left (not needed when you resize from the bottom or right) with OMTC on windows. The bad behaviour is obvious if you comment out that code and try to resize. Please can you test your patch for that and check it does not break the fix. Details are in bug 912352. Thanks!

::: widget/windows/nsWindowGfx.cpp
@@ +257,5 @@
>    bool forceRepaint = nullptr != aDC;
>  #endif
>    nsIntRegion region = GetRegionToPaint(forceRepaint, ps, hDC);
> +
> +  ClientLayerManager *clientLayers =

I would prefer clientLayerManager to clientLayers

@@ +266,5 @@
> +  if (clientLayers && mCompositorParent) {
> +    // We need to paint to the screen even if nothing changed, since if we
> +    // don't have a compositing window manager, our pixels could be stale.
> +    clientLayers->SetNeedsComposite(true);
> +    clientLayers->SendInvalidRegion(region);

I'm not sure if this is OK in the window resizing case - we might need the whole window to be invalid.

@@ +275,5 @@
> +    listener->WillPaintWindow(this);
> +  }
> +  // Re-get the listener since the will paint notification may have killed it.
> +  listener = GetPaintListener();
> +  if (!listener)

nit: {}

@@ +278,5 @@
> +  listener = GetPaintListener();
> +  if (!listener)
> +    return false;
> +
> +  if (clientLayers && mCompositorParent && clientLayers->NeedsComposite()) {

I think we need to do this even if NeedsComposite is false because that does not take account of the window size, and that is why we do this early composite. Also, please leave the comment which explained that (feel free to improve it though).
Attached patch invalid.patchSplinter Review
Thanks for catching that, indeed I janked up resizing from the top left. I put the early composite back but put it behind a resize test, and now it seems to work again.

This actually seems to make resizing on Windows work a little better on Windows XP. With trunk nightly the resized area is transparent, but with this patch it's a solid color.
Attachment #830575 - Attachment is obsolete: true
Attachment #830575 - Flags: review?(ncameron)
Attachment #831915 - Flags: review?(ncameron)
Comment on attachment 831915 [details] [diff] [review]
invalid.patch

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

Assuming you've just changed nsWindowGfx.cpp and you're carrying r=mattwoodrow for the rest.

::: widget/windows/nsWindowGfx.cpp
@@ +218,5 @@
> +      (GetLayerManager()->GetBackendType() == LAYERS_CLIENT)
> +      ? static_cast<ClientLayerManager*>(GetLayerManager())
> +      : nullptr;
> +
> +  if (clientLayerManager && mCompositorParent && !mBounds.IsEqualEdges(mLastPaintBounds)) {

nit: please break this line
Attachment #831915 - Flags: review?(ncameron) → review+
https://hg.mozilla.org/mozilla-central/rev/f5120348a27f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Duplicate of this bug: 913505
Blocks: 966679
Whiteboard: [qa-]
Depends on: 1005568
Blocks: 1017353
You need to log in before you can comment on or make changes to this bug.