Open Bug 911471 Opened 8 years ago Updated 8 years ago

reduce composition of occluded regions

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

People

(Reporter: gal, Assigned: gal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

When compositing, we often end up rendering pixels that will be occluded by layers we render on top of them. This patch goes through the layers of each container in reverse Z order and tries to shrink the scissor rect to avoid painting pixels that are guaranteed to not be visible anyway. This approach is not complete since we only have a scissor rect but we might have to paint a complex region of the layer since several parts of it are occluded. A complete approach would require breaking such layers into multiple quads.
Bug 833897 should use the new method introduced by this bug.
Blocks: 833897
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → gal
Attachment #798187 - Flags: review?(roc)
Here is an example of this patch in action, starting my mac desktop FF with a page:

original scissor rect: 0 0 2880 52
original scissor rect: 0 0 2880 1704
original scissor rect: 0 130 2880 1574
original scissor rect: 0 0 2880 1704
new scissor rect: 0 0 2880 130

The last rect is the UI which gets composited underneath the content. Its height is shrunk to 130, the actual height of the UI on my Retina mac, because opaque content is known to sit on top of the rest. The rect 0 130 2880 1574 is not composited, which saves some 18MB VRAM traffic every time we composite. On desktop this doesn't matter that much for performance, but might affect battery life.
Attached patch patch (obsolete) — Splinter Review
Updated patch.
Attachment #798187 - Attachment is obsolete: true
Attachment #798187 - Flags: review?(roc)
Attachment #798192 - Flags: review?(roc)
Blocks: 907048
Do we need to worry about this region getting very complicated, giving O(N^2) behavior?
We have seen problems like that before. Since I am simplifying out to the bounds of the scissor rect anyway, I could as well put a limit of 5 rects or so on the region and then start simplifying on the spot. Wdyt?
You can probably go higher than 5; in the past we've used things like 100 to prevent the _really_ bad edge cases.  We don't want to go too low, because the region-simplification in question is a SimplifyInward on "occluded", right?  So we might just miss whole chunks of opaque things once we hit the limit we set.

(On a separate note, I wonder whether a smarter algorithm for SimplifyInward than what we have now is worth thinking about.)
I wonder if wins from this optimization come up in any of our tests...
Attached patch patchSplinter Review
Simplify inward once we hit 100 rects to prevent the occluded region from exploding quadratically.
Attachment #798192 - Attachment is obsolete: true
Attachment #798192 - Flags: review?(roc)
Attachment #798306 - Flags: review?(bzbarsky)
Bas, I think at least D3D10 supports multiple scissor rects. Do you think its worth changing the signatures to hand down a region and GL will use the bounds and Windows the actual region?
glScissorArrayv is not available until GL 4.1. It would be pretty straight forward to emulate it in GLContext if we ever have to by allocating an VBO and splitting the supplied rect into sub-rects applying the scissor rects.
A couple follow-up items:
- for this to work well we want as many regions to be opaque as possible, it might be even worth it to analyze RGBA content and on the fly infer that its actually opaque even when we didn't know when drawing it
- as bz suggests, its probably time to measure our region code a bit
Filed profiling and optimizing the region code as bug 911590.
Depends on: 911590
(In reply to Boris Zbarsky [:bz] from comment #7)
> You can probably go higher than 5; in the past we've used things like 100 to
> prevent the _really_ bad edge cases.  We don't want to go too low, because
> the region-simplification in question is a SimplifyInward on "occluded",
> right?  So we might just miss whole chunks of opaque things once we hit the
> limit we set.
> 
> (On a separate note, I wonder whether a smarter algorithm for SimplifyInward
> than what we have now is worth thinking about.)

Jeff is switching our region implementation to use pixman regions which behave better and rely less on Simplify*.
Comment on attachment 798306 [details] [diff] [review]
patch

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

For most cases (not involving compositor-driven animations) we've already performed occlusion detection in the display list and the visible regions of the layers should reflect that. So, I wonder if we could just modify Layer::CalculateScissorRect to restrict the scissor rect to the bounds of the visible region (after applying the layer transform to the visible region)?

::: gfx/layers/Layers.cpp
@@ +788,5 @@
> +        aScissorRects[i] = scissorRect;
> +      }
> +      // Updated the occluded region only after we determined the clipping rect
> +      // for the current layer since the layer doesn't occlude itself.
> +      if (layer->GetContentFlags() & CONTENT_OPAQUE) {

There's a problem here: layer->GetContentFlags() & CONTENT_OPAQUE only guarantees that the layer's visible region is opaque, not necessarily the entire area of the layer inside its computed scissor rect.

::: gfx/layers/Layers.h
@@ +1476,5 @@
>    void SortChildrenBy3DZOrder(nsTArray<Layer*>& aArray);
> +  void GetScissorRectsForSortedChildren(const nsIntRect& aCurrentScissorRect,
> +                                        const gfxMatrix* aWorldTransform,
> +                                        const nsTArray<Layer*>& aLayers,
> +                                        nsTArray<nsIntRect>& aScissorRects);

This needs to be documented to make clear that we compute one scissor rect for each element of aLayers.
roc, we are definitely not doing this right whenever we cross document boundaries (XUL->content) and also process boundaries. So I don't think CalculateScissorRect will suffice, but I will redo the patch to take the visible region into account.
Although I suppose those visible regions are being used to construct the quads already, so if this scissor-rect stuff helps, the visible regions set on the layers must not be accurate enough.

Maybe that's because nsDisplayTransform::ComputeVisibility always gives up on updating aVisibleRegion to occlude the display items under it?

Peter, a full display list dump and layer tree dump for the homescreen would be really useful.
roc, apply the patch and start up a browser. I already see this help by reducing the size of the scissor rect for the XUL window under the content window (since content covers the bottom, minus the UI on top). I think thats very similar to what happens with the home screen where we have a color layer thats fully covered by the background image in a different process (maybe we don't see that thats rgb?).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> Although I suppose those visible regions are being used to construct the
> quads already, so if this scissor-rect stuff helps, the visible regions set
> on the layers must not be accurate enough.
> 
> Maybe that's because nsDisplayTransform::ComputeVisibility always gives up
> on updating aVisibleRegion to occlude the display items under it?
> 
> Peter, a full display list dump and layer tree dump for the homescreen would
> be really useful.


I created bug 907048 for one over painting case during video playback.
And bug 907109 for layer tree dump for homescreen.
(In reply to Andreas Gal :gal from comment #18)
> roc, apply the patch and start up a browser. I already see this help by
> reducing the size of the scissor rect for the XUL window under the content
> window (since content covers the bottom, minus the UI on top).

My layer tree for a browser showing about:blank looks like this:
ClientLayerManager (0x7f7c58009800)
  ClientContainerLayer (0x7f7c065c9800) [visible=< (x=0, y=0, w=1717, h=996); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=1717.000000, h=996.000000) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollId=0 }]
    ClientThebesLayer (0x7f7c065c9c00) [visible=< (x=0, y=0, w=1717, h=68); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000] [valid=< (x=0, y=0, w=1717, h=68); >]
    ClientContainerLayer (0x7f7c065ca800) [clip=(x=0, y=68, w=1717, h=928)] [visible=< (x=0, y=68, w=1717, h=928); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000]
      ClientThebesLayer (0x7f7c1691d400) [clip=(x=0, y=0, w=0, h=0)] [transform=[ 1 0; 0 1; 0 68; ]] [not visible] [isFixedPosition anchor=0.000000,0.000000]
      ClientColorLayer (0x7f7c1691d800) [transform=[ 1 0; 0 1; 0 68; ]] [visible=< (x=0, y=0, w=1717, h=928); >] [opaqueContent] [isFixedPosition anchor=0.000000,0.000000] [color=rgba(255, 255, 255, 1)]

The content of the XUL window is drawn into layer 0x7f7c065c9c00. Your patch gives it a scissor rect of 0,0,1717,68. But we already knew that its visible region is x=0, y=0, w=1717, h=68 (that comes from the display list visibility calculations, which take into account occlusion by the content window), and that determines the quad that gets drawn, so AFAIK improving the scissor rect has no effect on GPU resource usage in this case.

> I think thats
> very similar to what happens with the home screen where we have a color
> layer thats fully covered by the background image in a different process
> (maybe we don't see that thats rgb?).

Maybe. Maybe not. We need to look at the full layer tree and display list dumps to understand at what stage occlusion information is being lost, and why.
We have the tree somewhere. I will find the bug.
Attached patch 0001-avoid-overcomposition.patch (obsolete) — Splinter Review
I made a patch to detect the over-composition condition and it could skip the redundant colorlayer during video playback. 

But it had problem (skip two layers)during the animation when I switch video to background by homekey.
Attachment #798693 - Flags: feedback?(gal)
Attached patch avoid-overcomposition v2 (obsolete) — Splinter Review
fix wrong detection of opacity layer by checking the content flag.

Now it could skip the redundant colorlayer composition correctly during video playback.
Attachment #798693 - Attachment is obsolete: true
Attachment #798693 - Flags: feedback?(gal)
Attachment #798795 - Flags: feedback?(gal)
Comment on attachment 798306 [details] [diff] [review]
patch

roc is a way better reviewer here.
Attachment #798306 - Flags: review?(bzbarsky) → review?(roc)
One reason that layer visible regions can be set to include areas that are covered by other opaque layers is for layers with animated transforms; for those layers we prerender the layer contents, which we do by treating the entire contents of the layer as visible.

Other than that, I hope that display list visibility computation would avoid overdrawing.

I'd like to focus on specific test cases that exhibit compositor overdrawing and find out what the underlying problem is (e.g., whether it's due to layer prerendering). We may be able to fix the problem without doing occlusion computation again in the compositor.
Comment on attachment 798795 [details] [diff] [review]
avoid-overcomposition v2

I put my patch back to bug 907048.
Attachment #798795 - Attachment is obsolete: true
Attachment #798795 - Flags: feedback?(gal)
Comment on attachment 798306 [details] [diff] [review]
patch

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

Generally display-list visibility computation should be reducing the visible regions of layers to account for occlusion. Bug 907048 fixed a specific case where that doesn't work: when we're using RefLayers to composite together layer trees from different processes, so display list construction for one process doesn't know which areas are opaque due to the rendering of other processes. If there are other cases that bug 907048 didn't fix, I think we should track them down and try to fix them with targeted fixes rather than doing general occlusion culling in the layer tree.
Attachment #798306 - Flags: review?(roc) → review-
You need to log in before you can comment on or make changes to this bug.