1033538 - Opacity calculation code doesn't handle fractional content scale hitting slow paths.

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect, P1)

Description

[Benoit Girard (:BenWa)]

+++ This bug was initially created as a clone of Bug #1027231 +++

This is a spin off of bug 1027231. I'm hoping we can solve the underlying platform issue here:

- We have a 545 CSS px node.
- We get a nsDisplayBackgroundColor of 545 px * 60 = 32700 app units
- We create a layer of 818 px but 32700 app units doesn't cover 818 px so we consider the layer non opaque.

Ideally we need to fix the last part where we round out the layer pixels and compare opacity on that area.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Just to clarify: in this case the device pixel ratio is 1.5, so 545 CSS pixels == 817.5 layout device pixels. The 818px is layout device pixels, rounded out.

Comment 2

[Markus Stange [:mstange]]

(In reply to Benoit Girard (:BenWa) from comment #0)
> - We create a layer of 818 px but 32700 app units doesn't cover 818 px so we
> consider the layer non opaque.

Do you know where this happens? Is it this code? http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=e4c604b52662&mark=3418-3427#3409

Comment 3

[Andreas Gal :gal]

Nice find. Thanks.

Comment 4

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

This accounts for at least a portion of the high overdraw count we're seeing on the homescreen on Flame devices.

Comment 5

[Benoit Girard (:BenWa)]

I think the problem is here:
mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=e4c604b52662&mark=2155-2177#2138

Comment 6

[Benoit Girard (:BenWa)]

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=e4c604b52662&mark=2155-2177#2138

Comment 7

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

Sorry for the wide needinfo - but can somebody from layout help us move this one faster? It's blocking some QC testing, so every day counts.  If NZ can jump in on this, it buys us an extra day.
Benoit, can you put whatever information you collected in this bug so that others don't have to duplicate that work?  Same for anybody that looks at this - drop a note as to the status so that we can continue on Monday.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: ScaleToOutsidePixels for drawing

I'm not sure what the best way of rounding is but this fixes part of the problem. We create a proper color layer with this.

However we still have the status bar thebes layer include a single pixel in the bottom of the screen because of this half pixel problem. I believe we bail out of HWC because we have a non rectangular layer region.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: remove needless will-change

Until bug 1034347 is properly fix you'll need to apply this patch if you want to simplify the layer tree.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: Displaylist dump with the 2 above patches

Note the system app thebes layer region: [visible=< (x=0, y=0, w=480, h=36); (x=0, y=853, w=480, h=1); >]

I'm having more trouble with this one. I can't find which display item is causing us to accumulate (x=0, y=853, w=480, h=1) into our region so I haven't been able to find out where the off by one is. If someone can give me a hand with this that would be nice.

Comment 11

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

ContainerLayer 0xad554c90 and ColorLayer 0xad554c90 should be marked opaque, but they are not. I don't know why not.

Dumping the fully-composed layer tree (after RefLayers have been substituted with their actual layer subtrees) would be helpful.

I'll look more into this in a few hours. However, as I have commented in bug 1027231 I think whatever we do here we also need to fix Gaia to make application viewports default to a whole number of device pixels. Anything else is just asking for trouble. E.g. creating a <canvas> that fills the whole viewport so that there's one canvas pixel per device pixel is hard if the viewport isn't aligned to device pixel edges...

Comment 12

[Benoit Girard (:BenWa)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> ContainerLayer 0xad554c90 and ColorLayer 0xad554c90 should be marked opaque,
> but they are not. I don't know why not.

Ohh that bug might be what mstange linked in Comment 2

Comment 13

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

Another thing we need here is a dump of the display list from nsLayoutUtils::PaintFrame between ComputeVisibilityForRoot and PaintRoot. The display item visible rects in the "after optimization" dump have been recomputed by FrameLayerBuilder during painting. It would be useful to know the display item visible rects computed by ComputeVisibilityForRoot, since they're the ones used to compute layer visible regions.

Comment 14

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

I/Gecko   ( 7686):   nsDisplayTransform b35ca3a8(HTMLScroll(div)(5)) bounds(0,1440,19200,32700) visible(0,1440,19200,32700) componentAlpha(0,1440,19200,32700) clip(0,0,19200,34160)  (opaque 0,1440,19200,32700)[ 1 0; 0 1; 0 36; ] layer=b027b000
I/Gecko   ( 7686):     OwnLayer ad8adbd8(FrameOuter(iframe src=app://a98f206c-28b5-4eff-8b41-d2db0e914679/index.html)(4)) bounds(0,0,19200,32700) visible(0,0,19200,32700) componentAlpha(0,0,0,0) clip(0,0,19200,32700)  (opaque 0,0,19200,32700) layer=b027b400
I/Gecko   ( 7686):       BackgroundColor ad8adbd8(FrameOuter(iframe src=app://a98f206c-28b5-4eff-8b41-d2db0e914679/index.html)(4)) bounds(0,0,19200,32700) visible(0,0,19200,32700) componentAlpha(0,0,0,0) clip(0,0,19200,32700)  uniform (opaque 0,0,19200,32700) (rgba 1.000000,1.000000,1.000000,1.000000) layer=ad554ae0
I/Gecko   ( 7686):     Remote ad8adbd8(FrameOuter(iframe src=app://a98f206c-28b5-4eff-8b41-d2db0e914679/index.html)(4)) bounds(0,0,19200,32700) visible(0,0,19200,32700) componentAlpha(0,0,0,0) clip(0,0,19200,32700)  layer=b027b800

The OwnLayer, BackgroundColor and Remote are all generated by the <iframe>. The BackgroundColor and Remote display items have their bounds set to exactly the iframe's border box. The OwnLayer and nsDisplayTransform get their bounds from those items. The iframe's border box is clearly at y=1440 and has height 32700, so its bottom is at 34140 appunits which is 853.5 device pixels, not quite reaching to the bottom of the screen. That's why we're getting a half-pixel of the system app showing through at the bottom of the screen.

The iframe border box layout should not be subject to any rounding to device pixels *or* CSS pixels and should not be affected by any decisions of FrameLayerBuilder. So we need to figure out why it's getting this height. It does look like something computed a CSS height of 545.333333px and then rounded it down to 545px. I'm pretty sure Gecko layout didn't do that!

Comment 15

[Markus Stange [:mstange]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> So we need to figure out why it's getting this height. It
> does look like something computed a CSS height of 545.333333px and then
> rounded it down to 545px. I'm pretty sure Gecko layout didn't do that!

If I understood correctly, the value of 545px comes from the following calculation in apps/system/js/layout_manager.js:

     /**
      * Gives the possible height for a window.
      *
      * @memberOf LayoutManager
      */
     get height() {
      return window.innerHeight -
         (this.keyboardEnabled ? KeyboardManager.getHeight() : 0) -
         StatusBar.height -
         softwareButtonManager.height;
     },

Is it possible that the actual value of window.innerHeight should be fractional, and the rounding happens because window.innerHeight always returns integers?

Comment 16

[Eric Rahm [:erahm]]

:BenWa noted this will reduce graphics memory usage.

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Markus Stange [:mstange] from comment #15)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> > So we need to figure out why it's getting this height. It
> > does look like something computed a CSS height of 545.333333px and then
> > rounded it down to 545px. I'm pretty sure Gecko layout didn't do that!
> 
> If I understood correctly, the value of 545px comes from the following
> calculation in apps/system/js/layout_manager.js:
> 
>      /**
>       * Gives the possible height for a window.
>       *
>       * @memberOf LayoutManager
>       */
>      get height() {
>       return window.innerHeight -
>          (this.keyboardEnabled ? KeyboardManager.getHeight() : 0) -
>          StatusBar.height -
>          softwareButtonManager.height;
>      },
> 
> Is it possible that the actual value of window.innerHeight should be
> fractional, and the rounding happens because window.innerHeight always
> returns integers?

Let me check. If that's the case then we likely need to fork this discussion yet again since it's a useful fix but isn't the original reported issue. We can still invest in better optimizing a case where content request content that have fractional w/h.

Comment 18

[Benoit Girard (:BenWa)]

Nice find Markus, changing innerHeight/innerWidth to double as they should be per spec fixes the gaia calculation. Spinning up a few bug for that fix.

Comment 19

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

Benoit, with your fix in bug 1035474 (or my suggested alternative), what's left to do here to fix the Flame issues?

Comment 20

[Benoit Girard (:BenWa)]

Well this bug should cover fixing incorrect opacity calculations. Bug 1035474 and your alternative avoid the problem for the b2g homescreen but we can end up with fractional width/height in content. We should avoid hitting a performance cliffs because of sloppy rounding issues.

I don't think this bug should block release considering we have work around for the important gaia case.

Comment 21

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

Right. Rerequesting 2.0 triage with a view to getting a blocking-...

Comment 22

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

For this not to be blocking, bug 1035474 would need to be blocking?

Comment 23

[Benoit Girard (:BenWa)]

(In reply to Milan Sreckovic [:milan] from comment #22)
> For this not to be blocking, bug 1035474 would need to be blocking?

No, bug 1027231 should be. It looks like we're going to have to change the gaia calculation because of web compatibility issues.

Comment 24

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

OK, let's aim this at 35 at the latest then.

Comment 25

[Nicholas Nethercote [inactive]]

BenWa, is this still a big problem? Has it been worked around?

Comment 26

[Benoit Girard (:BenWa)]

It's been worked around in Gaia in certain parts.

But this bug should remain open because we can still optimize this with the platform. But it's in the backlog since it's a lot work/low reward.

Given that it's been worked around I don't believe it should be MemShrink:P1.

Comment 27

[Benoit Girard (:BenWa)]

The motivation for this bug was Gaia and content scale screen where this slow path really mattered. Let's loop back if we are hitting this in the wild.