Closed Bug 1217560 Opened 7 years ago Closed 7 years ago

Only compute the backbuffer's damage region on composite

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #8677651 - Flags: review?(matt.woodrow)
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.
Attachment #8677651 - Flags: review?(matt.woodrow) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8677651 - Attachment is obsolete: true
Attachment #8677716 - Flags: review?(matt.woodrow)
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
Attached patch v3 (obsolete) — Splinter Review
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.
Attachment #8677716 - Attachment is obsolete: true
Attachment #8677716 - Flags: review?(matt.woodrow)
Attachment #8677772 - Flags: review?(matt.woodrow)
Attachment #8677772 - Flags: review?(matt.woodrow) → review+
Attached patch v4Splinter Review
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.
Attachment #8677772 - Attachment is obsolete: true
Attachment #8678699 - Flags: review?(matt.woodrow)
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).
Attachment #8678699 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/fbe8ec51aa38
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.