1217560 - Only compute the backbuffer's damage region on composite

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Currently we compute the compositor's backbuffer damage region on every shadow layer transaction. This is unnecessary and can lead to over-invalidation. We should only compute the damage region before each composite.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8677651 [details] [diff] [review]
patch

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +307,3 @@
>      mInvalidRegion.Or(mInvalidRegion, invalid);
> +
> +    mClonedLayerTreeProperties = LayerProperties::CloneFrom(GetRoot());

This should be outside the mClonedLayerTreeProperties condition! I think we can just return from this function immediately if !mRoot.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch v2

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8677716 [details] [diff] [review]
patch v2

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +299,5 @@
> +
> +void
> +LayerManagerComposite::UpdateAndRender()
> +{
> +  if (mClonedLayerTreeProperties) {

I think we want && !mTarget in this condition so that we don't overwrite the 'window invalid area' when compositing to a target.

@@ +307,3 @@
>      mInvalidRegion.Or(mInvalidRegion, invalid);
>    } else if (!mTarget) {
>      mInvalidRegion.Or(mInvalidRegion, mRenderBounds);

We could add a 3rd condition here for when mTarget is true, and set mInvalidRegion to mTargetBounds.

Then we can get rid of the !mTarget check on line 313, and then if condition in ::Render (line 722).

@@ +307,5 @@
>      mInvalidRegion.Or(mInvalidRegion, invalid);
>    } else if (!mTarget) {
>      mInvalidRegion.Or(mInvalidRegion, mRenderBounds);
>    }
> +  mClonedLayerTreeProperties = LayerProperties::CloneFrom(GetRoot());

Same with this (if !mTarget), we don't want to clobber our snapshot when compositing to a target

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v3

Addresses review comments. This relegates mInvalidRegion to just accumulate the window manager's invalid region, which we consume on composites to the window. The compositor invalid region is communicated through a parameter now.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v4

Turns out ComputeDifferences also invalidates intermediate surfaces in ContainerLayers. The v3 patch only calls ComputeDifferences when compositing to windows, so a ComposeToTarget() call can result in stale pixels if we don't first composite the same layers to the window.

This patch always calls ComputeDifferences in a composite, but if we're not compositing to a window, the damage region gets accumulated until the next composite to the window.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8678699 [details] [diff] [review]
v4

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +308,5 @@
> +    // immediately use the resulting damage area, since ComputeDifferences
> +    // also invalidates intermediate surfaces in ContainerLayers.
> +    nsIntRegion changed = mClonedLayerTreeProperties->ComputeDifferences(mRoot, nullptr, &mGeometryChanged);
> +
> +    if (mTarget) {

I think we could combine this if (mTarget) {} else {} with the one below. 'changed' will be empty if !mCloneLayerTreeProperties, but that shouldn't be a problem (and the operations should be really cheap).

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe8ec51aa38

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbe8ec51aa38