961887 - Make FindOpaqueBackgroundColorFor search parent layers for a background color

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[Matt Woodrow (:mattwoodrow)]

Currently FindOpaqueBackgroundColorFor only searches ThebesLayer within the same ContainerLayer.

It would be nice if we could also walk up the layer tree and find siblings of an ancestor to use as the background color.

A simple example that would benefit from this is:

<div style="opacity:0.5; will-change:opacity;>Text</div>

This will force an active ContainerLayer for the opacity, and the text will have no background color, resulting in requiring component alpha.

Comment 1

[Mats Palmgren (inactive)]

Should this bug block bug 961871 perhaps?

Comment 2

[Matt Woodrow (:mattwoodrow)]

I don't think it needs to, the same effect can be had with transitions/animations too. will-change was just the easiest to write for an example.

Comment 3

[Markus Stange [:mstange]]

Attachment: something like this

Comment 4

[Markus Stange [:mstange]]

This patch applies on top of the ones in bug 1018464.

Comment 5

[Markus Stange [:mstange]]

Attachment: v1

This gives each ContainerLayer a backdrop color that it can pull up into its ThebesLayers, but only if the ContainerLayer itself has a uniform opaque color behind it. This should address the most important use cases.
One thing I hope this will achieve (but which I haven't actually tested yet) is pulling colors through nsDisplayScrollLayer ContainerLayers. As far as I can tell, scrollboxes with display ports have never been able to benefit from the background color optimization.

Comment 6

[Markus Stange [:mstange]]

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

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8432215 [details] [diff] [review]
v1

Looks like I need to debug some reftest failures on Windows first.

Comment 8

[Markus Stange [:mstange]]

Attachment: v2

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

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8439873 [details] [diff] [review]
v2

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

Seems to me we could simplify this patch a lot if we made it work only when we're pulling a background color into the first layer of its container. And wouldn't that catch most of the cases we care about?

::: layout/base/FrameLayerBuilder.cpp
@@ +732,5 @@
>    AutoLayersArray                  mNewChildLayers;
>    nsTArray<nsRefPtr<ThebesLayer> > mRecycledThebesLayers;
>    nsDataHashtable<nsPtrHashKey<Layer>, nsRefPtr<ImageLayer> >
>      mRecycledMaskImageLayers;
> +  nsIntRegion                      mVisibleAboveBackgroundRegion;

Add a comment explaining what this means

@@ +737,3 @@
>    uint32_t                         mNextFreeRecycledThebesLayer;
>    nscoord                          mAppUnitsPerDevPixel;
> +  nscolor                          mContainerUniformBackgroundColor;

and this

@@ +740,2 @@
>    bool                             mSnappingEnabled;
> +  bool                             mAllDrawingAboveBackground;

and this

@@ +2541,5 @@
>        }
>  
> +      // Prerendered transform items can be updated without layer building
> +      // (async animations or an empty transaction).
> +      bool mayChangeTransform = item->GetType() == nsDisplayItem::TYPE_TRANSFORM &&

use itemType

@@ +2548,5 @@
> +                                                              false);
> +      // 3D-transformed layers don't necessarily draw in the order in which
> +      // they're added to their parent container layer.
> +      bool mayDrawOutOfOrder = (item->Frame()->Preserves3D() ||
> +                                item->Frame()->Preserves3DChildren());

can we condition this on the item being TYPE_TRANSFORM first? These might not be cheap.

Comment 10

[Markus Stange [:mstange]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Seems to me we could simplify this patch a lot if we made it work only when
> we're pulling a background color into the first layer of its container. And
> wouldn't that catch most of the cases we care about?

My initial motivation for this patch came from the iframe.svg test that's part of tscrollx, which has a display list tree that looks like this:

[...]
  CanvasBackgroundColor 11b735b28(Canvas(svg)(-1)) (rgba 255,255,255,255)
  nsDisplayTransform 11ba6ad68(Block(foreignObject)(5))
    Border 11ba6b0d0(FrameOuter(iframe src=drac.htm)(1))
    SubDocument 11c83a458(Viewport(-1))
      CanvasBackgroundColor 11c83b070(Canvas(html)(-1)) (rgba 0,0,0,0)
      [...]

I'd like to be able to pull the white canvas background of the outer document into the scrolled layer of the iframe document, and the ContainerLayer of the iframe's nsDisplaySubDocument is the second child layer of the foreignObject transform ContainerLayer because there's a ThebesLayer for the iframe's border in between. So pulling only into the first layer of a container wouldn't fix this case.

Comment 11

[Markus Stange [:mstange]]

I'm going to attach new patches that are on top of the patch in bug 1134311.

Comment 12

[Markus Stange [:mstange]]

Attachment: part 1: refactor mVisibleAboveRegion management

Comment 13

[Markus Stange [:mstange]]

Attachment: part 2: make FindOpaqueBackgroundColorFor take a region instead of a PaintedLayerData

Comment 14

[Markus Stange [:mstange]]

Attachment: part 3: find uniform opaque background colors under ContainerLayers

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8566258 [details] [diff] [review]
part 2: make FindOpaqueBackgroundColorFor take a region instead of a PaintedLayerData

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

::: layout/base/FrameLayerBuilder.cpp
@@ +736,1 @@
>     * has a single opaque color behind it, over the entire bounds of its visible

Need to say what coordinate system this region is in.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8566259 [details] [diff] [review]
part 3: find uniform opaque background colors under ContainerLayers

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2999,5 @@
>  
> +      // 3D-transformed layers don't necessarily draw in the order in which
> +      // they're added to their parent container layer.
> +      bool mayDrawOutOfOrder = itemType == nsDisplayItem::TYPE_TRANSFORM &&
> +        (item->Frame()->Preserves3D() || item->Frame()->Preserves3DChildren());

What if the container layer has a possibly async transform on it? That should prevent us from pulling up a background color, right? I don't see how that is prevented.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8566259 [details] [diff] [review]
part 3: find uniform opaque background colors under ContainerLayers

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

::: layout/base/FrameLayerBuilder.cpp
@@ +3002,5 @@
> +      bool mayDrawOutOfOrder = itemType == nsDisplayItem::TYPE_TRANSFORM &&
> +        (item->Frame()->Preserves3D() || item->Frame()->Preserves3DChildren());
> +
> +      // Pull up a uniform background color into the layer if possible.
> +      mParameters.mBackgroundColor = (prerenderedTransform || mayDrawOutOfOrder)

The prerenderedTransform check here should at least cover the OMTA transform case. But I haven't really thought about the interaction with APZ. I'll think about that some more.

I also need to think about how this interacts with blend modes. I think it's possible that we pull a color into a container layer that has a non-OVER blend mode, and then we see different results after blending with that background color vs. blending with transparent background.

Comment 18

[Markus Stange [:mstange]]

Attachment: part 3: find uniform opaque background colors under ContainerLayers

This fixes the blend mode case.

I haven't added any checks for async scrolling. There are existing ones that prevent us from pulling background colors *from* async scrolled painted layers, but none that prevent pulling *into* async scrolled layers. So it looks like the latter needs to be fixed for painted layers anyway.

Comment 19

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd82968a7b95

Comment 20

[Markus Stange [:mstange]]

Attachment: part 2: make FindOpaqueBackgroundColorFor take a region instead of a PaintedLayerData

I had missed one "break;" here, which caused a bug. I'm adding a reftest for that bug in the part 3 patch.

Comment 21

[Markus Stange [:mstange]]

Attachment: part 3: find uniform opaque background colors under ContainerLayers

with the added test

Comment 22

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7b54bcc6746

Comment 23

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f02c5c1f7704
https://hg.mozilla.org/integration/mozilla-inbound/rev/97c8184c3058
https://hg.mozilla.org/integration/mozilla-inbound/rev/1684d29858e1

Comment 24

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

All three patches backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/96d7198bec0a for reftest orange:

https://treeherder.mozilla.org/logviewer.html#?job_id=6945789&repo=mozilla-inbound

Comment 25

[Markus Stange [:mstange]]

Repushed with a slightly tweaked test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/88283631c931
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c87822f9ae6
https://hg.mozilla.org/integration/mozilla-inbound/rev/70dbb97a096d

If the test still fails, please ping me before backing out and I'll just fuzz it. The test is not worth wasting more time and try hours on.

Comment 26

[Markus Stange [:mstange]]

Added a little fuzz:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01d879d05fa1

Apparently we're missing one pixel of text subpixel antialiasing when calculating the text's bounds.

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/88283631c931
https://hg.mozilla.org/mozilla-central/rev/0c87822f9ae6
https://hg.mozilla.org/mozilla-central/rev/70dbb97a096d
https://hg.mozilla.org/mozilla-central/rev/01d879d05fa1

Comment 28

[Markus Stange [:mstange]]

Setting status-b2g-v2.2 to wontfix, because the actual change here is in part 3, which was not uplifted in the rollup patch for bug 1146137. Only part 1 was, which only was a refactoring.
I didn't uplift the other two patches because they cause a regression with overlay scrollbars and async scrolling, which was now fixed on firefox 40 / b2g 3.0 in bug 1136304.