The flame has a content scale of 1.5x. It sizes the app container as 545px which in screen pixels is 817.5px. This causes many optimizations to be skipped. For the best performance on the flame and other 1.5x content scale devices everything needs to be sized as multiple of 1.5x. This fixes bug 1022164.

This should block 2.0 for the important performance implication of this. It means that instead of getting a Color layer we get a transparent thebes layer for a fullsize screen layer. This means more memory, more power, no HWC, jankier scrolling.

It's not trivial to find what part of the system app hard codes these values, but they appear to be set from CSS directly on the element. Can someone who knows the system app track this down? We need the layout to be a multiple of 1.5. You can confirm by dumping the display list and making sure that the display item bounds are multiples of 40.

I suspect alive can help us here

Alive, Assigning this to you per James' comment 3. Please reply here.

It seems like we are trying to change the system UI to accommodate a fast path here? I find it highly likely that third party developers can still run into this problem (nested iframe or similar), so I don't really find this fix ideal. Benoit - What are the implications of creating a larger color layer than needed, to the next largest 1.5x multiple? Would this be better or worse than the transparent layer?

Just changing the status bar height to 2.3rem should be enough for BenWa to test. But I agree with Kevin that this is really not ideal to work around that in gaia. diff --git a/apps/system/style/statusbar/statusbar.css b/apps/system/style/statusbar/statusbar.css index fa209d9..d8e2a8f 100644 --- a/apps/system/style/statusbar/statusbar.css +++ b/apps/system/style/statusbar/statusbar.css @@ -8,7 +8,7 @@ #statusbar-background { width: 100%; - height: 2.4rem; + height: 2.3rem; background-color: #000; opacity: 1; padding: 0; @@ -20,7 +20,7 @@ #statusbar { position: fixed; width: 100%; - height: 2.4rem; + height: 2.3rem; top: 0; left: 0; }

We are sizing the app container on demand. Usually it's calc(100% - 2.4rem), but there's something happen to change the system layout we will use window.innerWidth/window.innerHeight to calculate the value dynamically. https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L1246 I am not sure what to do with this bug - does use rem for app width/height help? I am not sure how does gaia know the '1.5x' factor.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #7) > help? I am not sure how does gaia know the '1.5x' factor. It's in window.devicePixelRatio

(In reply to Kevin Grandon :kgrandon from comment #5) > It seems like we are trying to change the system UI to accommodate a fast > path here? I find it highly likely that third party developers can still run > into this problem (nested iframe or similar), so I don't really find this > fix ideal. > > Benoit - What are the implications of creating a larger color layer than > needed, to the next largest 1.5x multiple? Would this be better or worse > than the transparent layer? Benoit, I've discussed to Alive and we think there is no easy solution in Gaia System. 1) The patch in window management will be non-trivial, as comment 7 pointed out, the iframe size is calculated sometimes in CSS and sometimes in JS. We can move all calculation to JS and remove the calc(), however it might trigger undesired reflow in some places. 2) Even if we could do (1), window.devicePixelRatio is device dependent. For Frame, implementing a logic in JS to have the resulting CSS pixel always dividable by 2 can fix this, but such logic will die on Madai, which comes with 2.25dppx and the height need to be dividable by 4. (Don't ask me why it was being set to that -- I would object if I was looped in the discussion). I don't know if such logic will result any bad things to these devices. I thought about having the workaround, instead, implemented in shell.html in B2G runtime. Cut off 1 screen px there can fix this. This is still device dependent though. Again, I think what you want (and what we against) is A) move calculation in CSS like |calc(100% - 2.4rem)| to JS. B) instead of writing |height = window.innerHeight - 24|, do var shouldDivide = 1 / (window.devicePixelRatio % 1); height = Math.ceil((window.innerHeight - 24) / shouldDivide) * shouldDivide;

Deassign myself.

> var shouldDivide = 1 / (window.devicePixelRatio % 1); > height = Math.ceil((window.innerHeight - 24) / shouldDivide) * shouldDivide; BTW there should be a |if ((window.devicePixelRatio % 1) === 0)| check before these lines.

Moving forward as this is a blocker. Given that we systematically resize on launch and that only the height should cause issues (we use the innerWidth for the width) I'm keeping this change small in layoutManager.height. Before asking for review I'd like to confirm - that this is still an issue - that the patch correctly fixes it Benoit, I had 50+ fps with and without this patch when running crystalskull on master. Can you check it / tell me how to test it?

This patch is still going to fail for some resolutions. You should multiply the height by the resolution, snap that value, and then divide it back down by the resolution.

Patch v2. Just confirming: setting a float height is OK as long as it translates to an integral device pixel value? There's no truncation happening somewhere and screwing things up?

(In reply to Etienne Segonzac (:etienne) from comment #12) > Created attachment 8448835 [details] [diff] [review] > 0001-Bug-1027231-Making-sure-windows-height-are-integers-.patch > > Moving forward as this is a blocker. > > Given that we systematically resize on launch and that only the height > should cause issues (we use the innerWidth for the width) I'm keeping this > change small in layoutManager.height. > Exactly we only resize on demand unless something change while I am away. Resize on launch will lose launch time performance. Trade-off if really want. The size of the newly created app container is statically inside window.css, but it's no way to use Math.ceil in css.

(In reply to Kevin Grandon :kgrandon from comment #5) > It seems like we are trying to change the system UI to accommodate a fast > path here? I find it highly likely that third party developers can still run > into this problem (nested iframe or similar), so I don't really find this > fix ideal. It's not ideal but in the short term we have too much performance at stake to overlook this optimization. It's what we get for using a fractional content scale without looking closer into the implications. > > Benoit - What are the implications of creating a larger color layer than > needed, to the next largest 1.5x multiple? Would this be better or worse > than the transparent layer? The problem isn't with the layer, it's with the display item calculations. The code building the layer tree doesn't know that we're going to be snapping to pixels so there's some mismatch here. We need to find someone to take a look at that. I'm going to file about the underlying platform issues but in the mean time we really need this work around in gaia ASAP while we build the proper fix in the platform.

> but in the mean time we really need this work > around in gaia ASAP while we build the proper fix in the platform. Hey, can you take a look at Comment 14, and Comment 12?

(In reply to Etienne Segonzac (:etienne) from comment #12) > Before asking for review I'd like to confirm > - that this is still an issue Yes, bug 1033538 will mitigate it but we don't have a candidate patch yet. > - that the patch correctly fixes it > > Benoit, I had 50+ fps with and without this patch when running crystalskull > on master. > Can you check it / tell me how to test it? It wont improve FPS directly. But it will improve memory usage, reduce compositing bandwidth (hard to measure) and let us run with HWC on the flame with Crystal Skull (but we need to work around other issues that don't have patches yet). Right now the best way to test is to build with |export B2G_DUMP_PAINTING=1| and to run with layout.display-list.dump and to careful analyze the display list and layers dump. (In reply to Etienne Segonzac (:etienne) from comment #14) > Created attachment 8449281 [details] [diff] [review] > 0001-Bug-1027231-Making-sure-windows-height-are-integers-.patch > > Patch v2. > > Just confirming: setting a float height is OK as long as it translates to an > integral device pixel value? There's no truncation happening somewhere and > screwing things up? I believe as long as it's an integer when multiple by 1.5 then we're good. Again we'd have to confirm with a display-list dump to make sure since we can't test because other regressions are in the way as well.

Benoit is 100% correct. Fractional CSS pixel values should not be a problem. Fractional *device* pixel values are a problem. Although I think we can probably fix bug 1033538, fractional device pixel height for app viewports is always going to be at risk of triggering performance bugs. E.g. if the app wants to draw a <canvas> filling the viewport (e.g. a game) giving it exactly the right canvas buffer size is going to be hard (solvable via canvas.renderedPixelWidth/Height, but hard). So I think we should also find a way to make it easy for Gaia to do that. For example maybe we can extend calc() with a function to round the result up (or down) to the nearest device pixel. But for now, any hack will do :-)

Alright I filed bug 1035474. The gaia calculation are technically correct: innerWidth/innerHeight -should- return a float/double and the calculation in gaia would size the app frame correctly. However depending on the outcome of bug 1035474 we may not want to fix it for web compatibility. Perhaps we want to do what roc suggests in bug 1035474 comment 3.

Starting the review process on this one while we confirm the fix.

Here are some display list dumps, forgive my newbieness :)

The phone is only running crystaskull, all system app will-change are temporarily remove to focus on the sizing issue. Without the patch: Painting --- retained layer tree: ClientLayerManager (0xaeec2500) ClientContainerLayer (0xb1927c00) [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) cb=(x=0, y=0, w=480, h=854) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) critdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollableRect=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) scrollId=0 }] ClientThebesLayer (0xb092b1a0) [clip=(x=0, y=0, w=0, h=0)] [not visible] ClientColorLayer (0xb092bbc0) [not visible] [opaqueContent] [color=rgba(0, 0, 0, 1)] ClientContainerLayer (0xb1928000) [clip=(x=0, y=0, w=480, h=854)] [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] ClientThebesLayer (0xb092b6b0) [visible=< (x=0, y=0, w=480, h=36); (x=0, y=853, w=480, h=1); >] [opaqueContent] [valid=< (x=0, y=0, w=480, h=36); (x=0, y=853, w=480, h=1); >] ClientContainerLayer (0xb15af400) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; 0 36; ]] [visible=< (x=0, y=0, w=480, h=818); >] ClientContainerLayer (0xb1946800) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] ClientThebesLayer (0xb2a81850) [visible=< (x=0, y=0, w=480, h=818); >] [valid=< (x=0, y=0, w=480, h=818); >] ClientRefLayer (0xb15af000) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [id=5] With the patch: Painting --- retained layer tree: ClientLayerManager (0xb13bc400) ClientContainerLayer (0xb1913800) [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) cb=(x=0, y=0, w=480, h=854) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) critdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollableRect=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) scrollId=0 }] ClientThebesLayer (0xaf5141a0) [clip=(x=0, y=0, w=0, h=0)] [not visible] ClientColorLayer (0xaf514bc0) [not visible] [opaqueContent] [color=rgba(0, 0, 0, 1)] ClientContainerLayer (0xb1913c00) [clip=(x=0, y=0, w=480, h=854)] [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] ClientThebesLayer (0xaf5146b0) [visible=< (x=0, y=0, w=480, h=36); >] [opaqueContent] [valid=< (x=0, y=0, w=480, h=36); >] ClientContainerLayer (0xaf2a0800) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; 0 36; ]] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent] ClientContainerLayer (0xaf2a0c00) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent] ClientThebesLayer (0xaf514d70) [clip=(x=0, y=0, w=0, h=0)] [not visible] ClientColorLayer (0xaf514f20) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent] [color=rgba(255, 255, 255, 1)] ClientRefLayer (0xae9d9800) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [id=5] Looks like the parent of the ClientRefLayer is gaining an `[opaqueContent]`, I guess that's a good thing right? :)

Yes that's important. Also note that two of the *ThebesLayer are now [not visible] with w=h=0 whichs mean they consume no more memory (or bandwidth each frame). We're left with a single visible thebes layer which x=0, y=0, w=480, h=36 tells us its the status bar above which is perfectly normal. In fact that should may be opaque but that's for another bug to look into. We're left with simple color fills, the status bar and whatever the app provides via the RefLayer placeholder which give us low overhead for the system app.

https://github.com/mozilla-b2g/gaia/commit/7853334901db0a92d900bb841c4ed546553ddaef Thanks for the clear explanation!

v2.0: https://github.com/mozilla-b2g/gaia/commit/3316542e36084a8d1f6a5446abe9bf199765f3d7

No steps to reproduce, unable to perform bug verification.