1056944 - Create an image layer if opaque image covers previous display items

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

+++ 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.

Comment 1

[Benoit Girard (:BenWa)]

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

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1055821#c2

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

Comment 3

[Matt Woodrow (:mattwoodrow)]

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.

Comment 4

[Benoit Girard (:BenWa)]

(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

[Matt Woodrow (:mattwoodrow)]

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

[Markus Stange [:mstange]]

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).

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch

- 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).

Comment 8

[Matt Woodrow (:mattwoodrow)]

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?

Comment 9

[Benoit Girard (:BenWa)]

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

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch v3

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?

Comment 11

[Matt Woodrow (:mattwoodrow)]

Why did you need to make that change? It should be fine for GetOpaqueRegion to return area outside of what is actually visible.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch v4

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

Comment 13

[Benoit Girard (:BenWa)]

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.

Comment 14

[Benoit Girard (:BenWa)]

push with the patch from 1063084:
https://tbpl.mozilla.org/?tree=Try&rev=aff27d218f5a

Comment 15

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d624029199a

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3d624029199a