1066713 - Async scale transforms on color layers are applied incorrectly when layout.css.devPixelsPerPx is a non-integer value

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Chris Lord [:cwiiis]]

Go to the attached URL in the FirefoxOS browser and tap the red rectangle. Note that it changes size for the two-second duration of the transition (plus a little longer, while the layer remains active, I assume).

The rectangle should not change size (see the mark-up).

This only seems to apply to colour layers, if you add some text content to the div, the bug no longer manifests (as I assume it ends up as a thebes layer instead)

Comment 1

[Chris Lord [:cwiiis]]

This may of course have nothing to do with async animations, but I can't think of an easy way to force a colour layer to be active to test that. This is more likely just a problem in ColorLayerComposite, or CompositorOGL::DrawQuad.

Comment 2

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

Attachment: Layers dump during the animation

Here's the layers dump that encompasses the animation. Layer 0xaa5a6c00 is the ContainerLayer that ends up with 100x scale.

Comment 3

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

(I accidentally posted the following comment to the wrong bug. Re-posting here)

The two layer dumps where the shadow-transform on that layer changes from 1.49999 to 1.5 are probably the most interesting. There's a preScale that magically appears and the color layer changes in size. Somebody who knows about preScale should probably look at this *cough*matt*cough* :)

Comment 4

[Chris Lord [:cwiiis]]

n?mattwoodrow re comment #3.

Comment 5

[Benoit Girard (:BenWa)]

I comment in bug 1068961 which may also be relevant here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1068961#c1

Comment 6

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #5)
> I comment in bug 1068961 which may also be relevant here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1068961#c1

So I had a look at using GetEffectiveVisibleRegion and even making sure to iterate over each rect instead of assuming it's a rect, but it made no difference. Hopefully Matt will have some insight :)

Comment 7

[Chris Lord [:cwiiis]]

This is still an issue after the fix for bug 1068961 has landed.

Comment 8

[Chris Lord [:cwiiis]]

n?tn for any input re comment #3.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Lord [:cwiiis] from comment #0)
> Go to the attached URL in the FirefoxOS browser and tap the red rectangle.
> Note that it changes size for the two-second duration of the transition
> (plus a little longer, while the layer remains active, I assume).

I actually don't see this on my hamachi. The rectangle shifts one pixel but stays the same size for me.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: set the scale on color layers properly

I think this might fix it. Can you test it please Chris (or anyone who can reproduce)?

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8497785 [details] [diff] [review]
set the scale on color layers properly

Looks like kats could reproduce.

Comment 12

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

Comment on attachment 8497785 [details] [diff] [review]
set the scale on color layers properly

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

This doesn't fix the problem; in fact it makes it worse. With this patch applied I see the same behaviour as before, except right before the animation ends the entire screen goes red for a composite or two.

If you're not able to reproduce this bug on the hamachi then it's quite likely related to the device scale. Try setting your layout.css.devPixelsPerPx property to 1.5 and see if it still happens. Hopefully the device is still usable in that condition.

Comment 13

[Chris Lord [:cwiiis]]

Comment on attachment 8497785 [details] [diff] [review]
set the scale on color layers properly

Unfortunately, as kats says, this makes the situation even worse :(

Comment 14

[Chris Lord [:cwiiis]]

Playing around with this, it's worth noting that only color layers that have an async animation happening on them get this bad scale, and the scale corresponds to the device resolution (so 1.5 on a Flame).

If you apply the inverse scale to the color layer's base transform in FrameLayerBuilder, they appear correctly during async animations, but incorrectly all other times.

Could this issue be in the async composition manager code or are we certain it's a layout issue?

Comment 15

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

It could be in the async composition manager I suppose. I guessed layout based on the layer tree dumps I posted above but to be honest I'm not sure what the contract is for layer trees during an async composition. Maybe that layer tree is expected on the layout side and the composition manager is not doing the right thing on its end.

Comment 16

[Chris Lord [:cwiiis]]

Had a quick fiddle, commenting out the async animated transform code, the layer still appears at the incorrect size, so I would guess that this is a layout issue then.

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> If you're not able to reproduce this bug on the hamachi then it's quite
> likely related to the device scale. Try setting your
> layout.css.devPixelsPerPx property to 1.5 and see if it still happens.
> Hopefully the device is still usable in that condition.

Ah yes, I can reproduce with that.

Comment 18

[Chris Lord [:cwiiis]]

We're looking to enable app-grouping by default in the next week or so and that makes this bug stand out a lot. tn, do you have time to look at this? (it doesn't need to be fixed in that time-frame, but we will need it for 2.2)

Comment 19

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

Tagged this as "nice to have" for 2.2.  If the app-grouping becomes a 2.2 feature (right now it is not tagged as anything *-b2g: 2.2+), let me know and we can see if we can accommodate this.

Comment 20

[Maria Sandberg [:mushi]]

I'm noming this for 2.2 as we will need it for app grouping which is in for 2.2 (as of yesterday). 
Also ccing Katie who will be the UX lead on app grouping.

Comment 21

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

Redirecting ni to Brian as he might also know this stuff.

Comment 22

[Brian Birtles (:birtles)]

Sorry, I can't load the test URL so I can't even guess what might be happening. Also, it sounds like it's not animation-related based on comment 16. Chris, got a new test URL? Is your server down?

Comment 23

[Chris Lord [:cwiiis]]

Attachment: Minimal test-case

(In reply to Brian Birtles (:birtles) from comment #22)
> Sorry, I can't load the test URL so I can't even guess what might be
> happening. Also, it sounds like it's not animation-related based on comment
> 16. Chris, got a new test URL? Is your server down?

Link works for me, but here's the file at that address. Click on the red rectangle.

Expected: Clicking the rectangle toggles it between its original position and one 1 pixel down

Actual: The rectangle changes in size for 2 seconds before settling at its new position.

Comment 24

[Chris Lord [:cwiiis]]

To be absolutely clear, this is on b2g (where OMTC + OMTA are enabled).

Comment 25

[Brian Birtles (:birtles)]

(In reply to Chris Lord [:cwiiis] from comment #23)
> Created attachment 8518870 [details]
> Minimal test-case
> 
> (In reply to Brian Birtles (:birtles) from comment #22)
> > Sorry, I can't load the test URL so I can't even guess what might be
> > happening. Also, it sounds like it's not animation-related based on comment
> > 16. Chris, got a new test URL? Is your server down?
> 
> Link works for me, but here's the file at that address. Click on the red
> rectangle.

Thanks, works for me now. I can reproduce on my Windows machine at home with OMTA turned on.

I've spent a few hours digging into this but haven't been able to work it out yet.

It seems like the animation code is setting the correct transform. I wonder if we end up setting the display resolution transform twice. For a while I wondered if it was because other callers of SetShadowTransform seem to be careful to invert the resolution scale first but we don't for animation. However, that doesn't seem to help.

I'll dig into it some more later but I think someone who is more familiar with this code could do it a lot faster. Matt, where do we normally apply device resolution scaling? I'm getting a bit lost between what goes on the color layer and what goes on the container layer, and base transform vs local transform vs inherited scale etc.

One thing to mention: when we calculate interpolated values we incorporate the device pixel ratio into our calculations already. I'm pretty sure that only applies to the translation component of matrices and to transform origin etc. e.g.

  http://hg.mozilla.org/mozilla-central/file/d380166816dd/gfx/layers/composite/AsyncCompositionManager.cpp#l408
  http://hg.mozilla.org/mozilla-central/file/d380166816dd/layout/style/nsStyleTransformMatrix.cpp#l75

We do tweak the scale component at one point but we revert it too:

  http://hg.mozilla.org/mozilla-central/file/d380166816dd/layout/style/nsStyleTransformMatrix.cpp#l620

Comment 26

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

Apparently SFE team would like to land app-grouping feature this depends on early in 2.2.

Comment 27

[Jonathan Watt [:jwatt]]

I can reproduce this on Mac if layout.css.devPixelsPerPx is set to a non-integer value. Experimenting with the value of that pref showed that the change in size of the div may be an increase or a decrease in size depending on the value. That made it look like we may be encountering rounding error. I also notice that when active (when the bug occurs) the layer was 2x2 in size vs. 200x200 when inactive.

Talking to Matt today he pointed me to GetMinAndMaxScaleForAnimationProperty and we stepped through the code there. What's happening is that nsLayoutUtils::ComputeSuitableScaleForAnimation defaults its maxScale/minScale gfxSize variables to 1x1 before calling GetMinAndMaxScaleForAnimationProperty. That provides an artificial minimum for the lines:

          aMinScale.width = std::min<float>(aMinScale.width, from.width);
          aMinScale.height = std::min<float>(aMinScale.height, from.height);

in GetMinAndMaxScaleForAnimationProperty so it ends up setting its aMinScale argument to 1x1 instead of 100x100. We then return to ComputeSuitableScaleForAnimation where we pass the minScale variable with this incorrect 1x1 size to GetSuitableScale which returns 1 instead of 100. Hence why we end up with a 2x2 layer that we scale up to 200x200 instead of creating a 200x200 layer that we don't scale. Hence if the layer size is being rounded due to non-integer layout.css.devPixelsPerPx that rounding error is scaled up, which is why we get such a big jump in layer size.

Comment 28

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 29

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8531907 [details] [diff] [review]
patch

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

In some cases GetMinAndMaxScaleForAnimationProperty can return without modifying the sizes, we should make sure that we still return (1,1) in that case.

Comment 30

[Jonathan Watt [:jwatt]]

Comment on attachment 8531907 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc36463e09b

Comment 31

[Jonathan Watt [:jwatt]]

Fix to broken attempt to address review comment:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f81b210f5ce

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bfc36463e09b
https://hg.mozilla.org/mozilla-central/rev/4f81b210f5ce

Comment 33

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.2.

Result: The red rectangle does NOT re-size when tapped. There is no change made.

Device: Flame 2.2 (319mb, KK, Full Flash)
BuildID: 20141208040202
Gaia: 0e429d970c160e580e19e61ad8ff5612de159f00
Gecko: c4c7442e9113
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 34

[bhavana bajaj [:bajaj]]

Clearing the blocking nom for 2.2? as this is already fixed/verified on that branch per the status flag