Closed
Bug 1056944
Opened 10 years ago
Closed 10 years ago
Create an image layer if opaque image covers previous display items
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1055821 +++ Right now we only get a color layer if the only display item against a layer is an image layer. In cases like the b2g homescreen the image covers previous display item but we fail to optimize it.
Assignee | ||
Comment 2•10 years ago
|
||
reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1055821#c2 https://tbpl.mozilla.org/?tree=Try&rev=a36f35de975c
Comment 3•10 years ago
|
||
Comment on attachment 8477016 [details] [diff] [review] patch Review of attachment 8477016 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +2339,5 @@ > + */ > + if (aClippedOpaqueRegion.Contains(mVisibleRegion) && > + mVisibleRegion.IsEqual(aClippedOpaqueRegion) && > + clipMatches && > + aItem->SupportsOptimizingToImage()) { This will break the single item in a layer case.
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #3) > Comment on attachment 8477016 [details] [diff] [review] > patch > > Review of attachment 8477016 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/FrameLayerBuilder.cpp > @@ +2339,5 @@ > > + */ > > + if (aClippedOpaqueRegion.Contains(mVisibleRegion) && > > + mVisibleRegion.IsEqual(aClippedOpaqueRegion) && > > + clipMatches && > > + aItem->SupportsOptimizingToImage()) { > > This will break the single item in a layer case. Can you be more specific? It's now passing layout/reftests/invalidation/test-image-layers.html which has a single Image display item: ClientContainerLayer (0x1230abc00) [clip=(x=0, y=79, w=1905, h=1984)] [transform=[ 1 0 0 0; 0 1 0 0; -136 -215 1 -1; 8 87 0 1; ]] [visible=< (x=0, y=0, w=256, h=256); >] ClientThebesLayer (0x1230ac000) [clip=(x=0, y=0, w=0, h=0)] [not visible] ClientImageLayer (0x1230ac800) [visible=< (x=0, y=0, w=256, h=256); >] Info: Accumulating dp=Image(1231369c8), f=1230d7378 against tld=115d2dbb0 Tracking image: nsDisplayImageContainer covers the layer Layer is not a solid color: Display item is not uniform over the visible bound Selecting layer for tld=115d2dbb0 Solid=0, hasImage=1, canOptimizeAwayThebes=1 Selected image layer=1230ac800
Comment 5•10 years ago
|
||
Oh, I guess it works if aClippedOpaqueRegion is empty but it will fail otherwise. If mVisibleRegion is empty (since this is the first/only item), and aClippedOpaqueRegion is non-empty, then we won't set mImage.
Comment 6•10 years ago
|
||
Yeah, I think you just applied Matt's comment about aClippedOpaqueRegion.Contains(mVisibleRegion) to the wrong line. Compared to the patch quoted in bug 1055821 comment 2 you changed mVisibleRegion.IsEqual(nsIntRegion(aVisibleRect) instead of mVisibleRegion.IsEqual(aClippedOpaqueRegion).
Assignee | ||
Comment 7•10 years ago
|
||
- Removed clip matches - Changed the first condition to nsIntRegion(aVisibleRect).Contains(mVisibleRegion). - Implement GetOpaqueRegion for nsImageFrame.h (I'm afraid it might be missing some cases).
Attachment #8483103 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•10 years ago
|
Attachment #8477016 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
Comment on attachment 8483103 [details] [diff] [review] patch Review of attachment 8483103 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: layout/generic/nsImageFrame.h @@ +399,5 @@ > + { > + *aSnap = false; > + bool animated; > + // REVIEW: Will mImage return FrameIsOpaque during progressive decode? > + if (mImage && mImage->GetAnimated(&animated) == NS_OK && !animated && mImage->FrameIsOpaque(imgIContainer::FRAME_CURRENT)) { Do we need to make this fail for animated images?
Attachment #8483103 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d330dc5f83df Adjusted the opaque bound to reflect what we actually paint. Maybe we need to do this as well for the image bounds?
Attachment #8483103 -
Attachment is obsolete: true
Attachment #8483688 -
Flags: review?(matt.woodrow)
Comment 11•10 years ago
|
||
Why did you need to make that change? It should be fine for GetOpaqueRegion to return area outside of what is actually visible.
Assignee | ||
Comment 12•10 years ago
|
||
As we discussed on IRC the frame size incldues the margin/border/padding which we don't paint on. Here's a revision to update the bounds. Passing the regression prone tests locally. https://tbpl.mozilla.org/?tree=Try&rev=112caf408b5d
Attachment #8483688 -
Attachment is obsolete: true
Attachment #8483688 -
Flags: review?(matt.woodrow)
Attachment #8483963 -
Flags: review?(matt.woodrow)
Updated•10 years ago
|
Attachment #8483963 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 13•10 years ago
|
||
reftest failure is caused by transparent bitmap. When using bitmap compression we can use 'delta' to skip over pixels. Pixels that haven't been coded will be transparent. http://msdn.microsoft.com/en-us/library/dd183383%28v=vs.85%29.aspx We need to fix the BMP decoder before this can land.
Assignee | ||
Comment 14•10 years ago
|
||
push with the patch from 1063084: https://tbpl.mozilla.org/?tree=Try&rev=aff27d218f5a
Assignee | ||
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d624029199a
https://hg.mozilla.org/mozilla-central/rev/3d624029199a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•