Closed
Bug 1098495
Opened 10 years ago
Closed 10 years ago
Retain a container's intermediate surface's content
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
Attachments
(3 files, 4 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
I don't necessarily think this will hit 2.2 but we should keep an eye on this for better app transition performance.
Blocks: gfx-target-2.2
Assignee | ||
Comment 2•10 years ago
|
||
This works for my simple testcase
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Attachment #8524025 -
Flags: review?(matt.woodrow)
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8524025 -
Attachment is obsolete: true
Attachment #8524025 -
Flags: review?(matt.woodrow)
Attachment #8524899 -
Flags: review?(matt.woodrow)
Comment 7•10 years ago
|
||
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());
?
Assignee | ||
Comment 8•10 years ago
|
||
Yes, changed locally.
Updated•10 years ago
|
Attachment #8524899 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8525781 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8525805 -
Flags: review+
Assignee | ||
Comment 12•10 years ago
|
||
Run the invalidation code on windows as well.
Attachment #8525805 -
Attachment is obsolete: true
Attachment #8526213 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8526265 -
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
You need to log in
before you can comment on or make changes to this bug.
Description
•