1024148 - Flatten Opacity and BackgroundColor display items

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benoit Girard (:BenWa)]

We sometimes get a sequence of display item of the form:
  Opacity 12ad0a1c0(Block(div)(1)) bounds(480,5220,960,960) visible(480,5220,960,960) componentAlpha(0,0,0,0) clip(0,4740,57600,62040)  (opacity 0.800000) layer=12a091800
    Opacity 12ad0a5e0(Block(div)(1)) bounds(480,5220,960,960) visible(480,5220,960,960) componentAlpha(0,0,0,0) clip()  (opacity 0.300000) layer=12a095c00
      BackgroundColor 12ad0a928(Block(div)(1)) bounds(480,5220,960,960) visible(480,5220,960,960) componentAlpha(0,0,0,0) clip()  uniform (opaque 480,5220,960,960) (rgba 255,0,0,255) layer=12a096c00


These end up creating a separate ThebesLayer for something that could either get a color layer or be folded into the parent ThebesLayer.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: testcase

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Part 1: Flatten nsDisplayOpacity together

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Part 1: Flatten nsDisplayOpacity together

As I discussed with matt we actually can't report nsDisplayOpacity as opaque if we have will-change. Updating patch and comment to reflect that.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Part 2: Flatten nsDisplayBackgroundColor

Note that the error can accumulate every time we merge.

Comment 5

[Benoit Girard (:BenWa)]

    Opacity 1331ac1c0(Block(div)(1)) bounds(480,5220,960,960) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,4740,57600,62040)  (opacity 0.800000)
      Opacity 1331ac5e0(Block(div)(1)) bounds(480,5220,960,960) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  (opacity 0.300000)
        Opacity 1331ac7d8(Block(div)(1)) bounds(480,5220,960,960) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  (opacity 0.300000)
          BackgroundColor 1331acb20(Block(div)(1)) bounds(480,5220,960,960) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  uniform (opaque 480,5220,960,960) (rgba 255,0,0,255) layer=130939800

->

  BackgroundColor 1331acb20(Block(div)(1)) bounds(480,5220,960,960) visible(480,5220,960,960) componentAlpha(0,0,0,0) clip(0,4740,57600,62040)  uniform (rgba 255,0,0,18) layer=130939800

Comment 6

[Benoit Girard (:BenWa)]

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

Comment 7

[Benoit Girard (:BenWa)]

Attachment: Part 2: Flatten nsDisplayBackgroundColor v2

Address try failure, non trivial changes so re-requesting review:

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

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Part 2: Flatten nsDisplayBackgroundColor v2

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8439490 [details] [diff] [review]
Part 2: Flatten nsDisplayBackgroundColor v2

>@@ -2881,21 +2883,21 @@ nsCSSRendering::PaintBackgroundColorWith
>   // Determine whether we are drawing background images and/or
>   // background colors.
>   bool drawBackgroundImage;
>   bool drawBackgroundColor;
>-  nscolor bgColor = DetermineBackgroundColor(aPresContext,
>-                                             aBackgroundSC,
>-                                             aForFrame,
>-                                             drawBackgroundImage,
>-                                             drawBackgroundColor);
>+  DetermineBackgroundColor(aPresContext,
>+                           aBackgroundSC,
>+                           aForFrame,
>+                           drawBackgroundImage,
>+                           drawBackgroundColor);

Ignoring the return value of DetermineBackgroundColor means this is probably going to screw up printing.

> void
> nsDisplayBackgroundColor::Paint(nsDisplayListBuilder* aBuilder,
>-                                nsRenderingContext* aCtx) 
>+                                nsRenderingContext* aCtx)
> {
>   if (mColor == NS_RGBA(0, 0, 0, 0)) {
>     return;
>   }

Comparing gfxRGBA and nscolor here?

Comment 10

[Matt Woodrow (:mattwoodrow)]

(In reply to Timothy Nikkel (:tn) from comment #9)
> Comment on attachment 8439490 [details] [diff] [review]
> Part 2: Flatten nsDisplayBackgroundColor v2
> 
> >@@ -2881,21 +2883,21 @@ nsCSSRendering::PaintBackgroundColorWith
> >   // Determine whether we are drawing background images and/or
> >   // background colors.
> >   bool drawBackgroundImage;
> >   bool drawBackgroundColor;
> >-  nscolor bgColor = DetermineBackgroundColor(aPresContext,
> >-                                             aBackgroundSC,
> >-                                             aForFrame,
> >-                                             drawBackgroundImage,
> >-                                             drawBackgroundColor);
> >+  DetermineBackgroundColor(aPresContext,
> >+                           aBackgroundSC,
> >+                           aForFrame,
> >+                           drawBackgroundImage,
> >+                           drawBackgroundColor);
> 
> Ignoring the return value of DetermineBackgroundColor means this is probably
> going to screw up printing.

Can you explain more here please. Why would DetermineBackgroundColor return something different to what our display list code thinks the background color is? Isn't that potentially buggy in other ways?

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> Can you explain more here please. Why would DetermineBackgroundColor return
> something different to what our display list code thinks the background
> color is? Isn't that potentially buggy in other ways?

We create nsDisplayBackgroundColor's with the color that comes back from DetermineBackgroundColor, so this should be fine after all. Sorry for the confusion. I was didn't know that we had already called DetermineBackgroundColor and this call is essentially redundant already.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's track what kinds of results we get here and consider this for Aurora (32) uplift.

Comment 13

[Benoit Girard (:BenWa)]

Alright I fixed the invalidation problems:

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

But now we're exposing ourselves to all sorts of rounding issues with reftest.

Comment 14

[Benoit Girard (:BenWa)]

Previous run is looking surprising good. Pushing a full try run to be safe:
https://tbpl.mozilla.org/?tree=Try&rev=670f96615b89

Comment 15

[Benoit Girard (:BenWa)]

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

Comment 16

[Benoit Girard (:BenWa)]

Attachment: Part 2: Flatten nsDisplayBackgroundColor v3 (Fixed invalidation)

All done!

Comment 17

[Benoit Girard (:BenWa)]

Comment on attachment 8440308 [details] [diff] [review]
Part 2: Flatten nsDisplayBackgroundColor v3 (Fixed invalidation)

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

::: layout/reftests/bugs/759036-1.html
@@ +6,5 @@
>        height: 300px;
>        width: 300px;
>        overflow:hidden;
>        border-radius:30px;
> +      opacity: 0.6;

I adjusted these values to have better rounding behavior.

Comment 18

[Benoit Girard (:BenWa)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f6188b4794
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1218c119a5d4

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5f6188b4794
https://hg.mozilla.org/mozilla-central/rev/1218c119a5d4

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 8439260 [details] [diff] [review]
Part 1: Flatten nsDisplayOpacity together

Approval Request Comment
[Feature/regressing bug #]: In this bug only. This is not a regression.
[User impact if declined]: See bug Comment 104. This saves two fullscreen buffer on the b2g homescreen and will save memory for apps that use css opacity in some very specific ways.
[Describe test coverage new/current, TBPL]: Has been on central for over a month. This change is enabled on all platforms and has integration coverage.
[Risks and why]: Even given all the exposure of this change there's still some risk. The alternative is we can remove the opacity effect on the gaia homescreen to get the memory back without this change and make a lower risk gaia said change. Of course this means we can't get the desired effect for v2.0.
[String/UUID change made/needed]: None

I didn't request an uplift before because I didn't think it would make such a difference.

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 8440308 [details] [diff] [review]
Part 2: Flatten nsDisplayBackgroundColor v3 (Fixed invalidation)

Approval Request Comment
Same as part 1

Comment 22

[Benoit Girard (:BenWa)]

Comment on attachment 8439260 [details] [diff] [review]
Part 1: Flatten nsDisplayOpacity together

Wrong approval

Comment 23

[Benoit Girard (:BenWa)]

Requesting blocking per Bug 1038884 Comment 104.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fee2d2f0ece5
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a94ff8d9f9af