Closed Bug 1098495 Opened 10 years ago Closed 10 years ago

Retain a container's intermediate surface's content

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(3 files, 4 obsolete files)

With bug 1087530 we retain the intermediate surface but we clear it every time. We should track if the container layer' child haven't changed and retain the content (skip the render) for the container layers.
I don't necessarily think this will hit 2.2 but we should keep an eye on this for better app transition performance.
Attached patch patch (obsolete) — Splinter Review
This works for my simple testcase
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attached file testcase
I did a blind test for app transition performance with 3 people using two Flame devices. Each were able to easily identify which phone had this patch applied.
Blocks: 1063708
Attachment #8524025 - Flags: review?(matt.woodrow)
Comment on attachment 8524025 [details] [diff] [review]
patch

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +154,5 @@
>        // If we don't have to generate invalidations separately for child
>        // layers then we can just stop here since we've already invalidated the entire
>        // old and new bounds.
> +      bool hasIntermediateSurface = mLayer->AsContainerLayer() && mLayer->AsContainerLayer()->UseIntermediateSurface();
> +      if (!aCallback && !hasIntermediateSurface) {

One of our descendant ContainerLayers might have an intermediate surface, and have changed children. I think we need to make sure we don't bail out early if we care about children changes.

::: gfx/layers/Layers.h
@@ +1936,5 @@
>    bool mSupportsComponentAlphaChildren;
>    bool mMayHaveReadbackChild;
> +  // This is updated by ComputeDifferences. This will be true if we need to invalidate
> +  // the intermediate surface.
> +  bool mHasChildrenChanged;

I'd prefer mChildrenChanged.

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +201,5 @@
> +
> +      if (!surface) {
> +        // If we don't need a copy we can render to the intermediate now to avoid
> +        // unecessary render target switching. This brings a big perf boost on mobile gpus.
> +        surface = CreateTemporaryTarget(aContainer, aManager);

It's not very obvious that CreateTemporaryTarget will attempt to re-use mLastIntermediateSurface, rather than always creating one.

Maybe pass mChildrenChanged into the function and figure it out within there.
Attached patch patch v2 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=ebe810b66428
Attachment #8524025 - Attachment is obsolete: true
Attachment #8524025 - Flags: review?(matt.woodrow)
Attachment #8524899 - Flags: review?(matt.woodrow)
Comment on attachment 8524899 [details] [diff] [review]
patch v2

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

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +96,5 @@
> +{
> +  nsIntRect visibleRect = aContainer->GetEffectiveVisibleRegion().GetBounds();
> +  gfx::IntRect surfaceRect = gfx::IntRect(visibleRect.x, visibleRect.y,
> +                                          visibleRect.width, visibleRect.height);
> +  return surfaceRect;

Can this just be
return gfx::ToIntRect(aContainer->GetEffectiveVisibleRegion().GetBounds());
?
Yes, changed locally.
Attachment #8524899 - Flags: review?(matt.woodrow) → review+
Attached patch patch v3 (obsolete) — Splinter Review
Carrying r+, discussed the changes with matt on IRC:

https://tbpl.mozilla.org/?tree=Try&rev=4697e3d545bb

Passing the reftest failures locally.
Attachment #8524899 - Attachment is obsolete: true
Attachment #8525781 - Flags: review+
Attached patch patch v4 (obsolete) — Splinter Review
https://hg.mozilla.org/try/rev/fef3f0e52de4
Attachment #8525781 - Attachment is obsolete: true
Attachment #8525805 - Flags: review+
Attached patch patch v5Splinter Review
Run the invalidation code on windows as well.
Attachment #8525805 - Attachment is obsolete: true
Attachment #8526213 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7f9005cad6e0
https://hg.mozilla.org/mozilla-central/rev/6309710dd71d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1109314
Depends on: 1237056
You need to log in before you can comment on or make changes to this bug.