866265 - Transformation equation cleanups

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

We should take a look at the transformation equations in APZC/CompositorParent and do a proper "this is how it should work" rewrite of them. We should add unit tests for these equations by creating standalone APZC instances and nested APZC instances attached to a known layer tree to ensure that we don't regress this.

Comment 1

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

Attachment: Part 1 - Fix resolution-setting on HiDPI B2G devices

I think I missed a spot in bug 883646.

Comment 2

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

Attachment: Part 2 - Fix up some comments in FrameMetrics.h

Comment 3

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

Attachment: Part 3 - Use the layer's reported resolution where possible instead of inferring it

Comment 4

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

Attachment: Part 4 - Add a test for AsyncPanZoomController::SampleContentTransformForFrame

This patch depends on the patches in bug 775459.

Comment 5

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

Btw I pushed the first three patches here to try at https://tbpl.mozilla.org/?tree=Try&rev=3b7538b3fd51 along with one from bug 885030. They look pretty green.

Comment 6

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

Comment on attachment 767319 [details] [diff] [review]
Part 1 - Fix resolution-setting on HiDPI B2G devices

I missed this in bug 883646, utils->SetResolution should be passed a LayoutDeviceToLayerScale but we were passing it a CSSToScreenScale.

Comment 7

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

Attachment: Part 3 - Revert a bad change in bug 883646

I believe I made a mistake in bug 883646; I assumed the presShell resolution was always the same as the scale in the nsDisplayItem::ContainerParameters. I don't think this is true - the ContainerParameters scale might have additional modifications based on CSS transforms (I think). I'm not 100% sure on this but I'd like to revert this line to the original version since it was only meant to be a small optimization and if there's any chance it's wrong I'd rather not do it at all.

Comment 8

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

Landed these three on inbound. Marking leave open because I have a couple more patches to come here.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d2e3b21982ad
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4d5c493179a8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f76508949f

Comment 9

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

Attachment: Part 4 - Use the resolution from the metrics object instead of inferring it from the transform

So there's this code that's been around since OMTC began on Fennec, that reads the scale transform from the layer and inverts it to get the resolution. That code has been copied to a bunch of places, but it's wrong. It's wrong because the transform on the layer can be affected by things other than the zoom (for example, CSS transforms). Previously this code worked fine on Fennec because we would always read the transform from the root layer, which is where we were always setting it in Fennec. On B2G and with multi-apzc this will no longer be the case, so this needs to be cleaned up. This patch identifies all the locations where we are doing this and corrects them, in some cases adding a #ifdef for the Fennec behaviour. Once bug 732971 is fixed (which should be doable now, just needs some more time spent on it) we can remove the ifdefs and everything should just work correctly.

As it is, this patch also fixes bug 860162 without breaking B2G. That is, it fixes the behaviour on Fennec+APZC because it reads the mResolution (which is always populated correctly) instead of inverting the transform from a layer which is correct on B2G but wrong for Fennec.

Comment 10

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

Attachment: Part 5 - Add a test for the transformation equations

This needs the stuff in bug 775459 to land first, I will wait until that is in before requesting review.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d2e3b21982ad
https://hg.mozilla.org/mozilla-central/rev/4d5c493179a8
https://hg.mozilla.org/mozilla-central/rev/c5f76508949f

Comment 13

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

Part 4 landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/b11787e3544b

Comment 14

[(no longer active)]

Regression: Mozilla-Inbound - Robocop Pan Benchmark - Android 4.0.4 - 225% increase
-----------------------------------------------------------------------------------
    Previous: avg 1111.729 stddev 248.061 of 12 runs up to revision 887ae544b828
    New     : avg 3607.562 stddev 512.240 of 12 runs since revision c5f76508949f
    Change  : +2495.833 (225% / z=10.061)
    Graph   : http://mzl.la/19YUQvD

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=887ae544b828&tochange=c5f76508949f

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/d2e3b21982ad
    : Kartikaya Gupta <[e-mail]> - Bug 866265 - Fix setting the resolution on hi-dpi B2G devices (followup to bug 883646). r=kentuckyfriedtakahe
    : http://bugzilla.mozilla.org/show_bug.cgi?id=866265

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/4d5c493179a8
    : Kartikaya Gupta <[e-mail]> - Bug 866265 - Fix up some comments in FrameMetrics.h. r=kentuckyfriedtakahe
    : http://bugzilla.mozilla.org/show_bug.cgi?id=866265

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/c5f76508949f
    : Kartikaya Gupta <[e-mail]> - Bug 866265 - Revert a line from bug 883646 because it was incorrect. r=kentuckyfriedtakahe
    : http://bugzilla.mozilla.org/show_bug.cgi?id=866265

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=866265 - Transformation equation cleanups
_______________________________________________
dev-tree-management mailing list
[e-mail]
https://lists.mozilla.org/listinfo/dev-tree-management

Comment 15

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

I already replied to dev.tree-mgmt about the m-c version of this notice. Same applies to inbound.

Comment 16

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b11787e3544b

Comment 17

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

Attachment: Part 5 - Add a test for the transformation equations

Comment 18

[Benoit Girard (:BenWa)]

Comment on attachment 772105 [details] [diff] [review]
Part 5 - Add a test for the transformation equations

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +142,5 @@
> +  gfx3DMatrix transforms[] = {
> +    gfx3DMatrix(),
> +    gfx3DMatrix(),
> +  };
> +  transforms[0].ScalePost(0.5f, 0.5f, 1.0f);

Your comment above doesn't describe this.

Comment 19

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

Added a comment to explain that and landed:

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

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ef387716ec2b

Comment 21

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

Comment on attachment 770276 [details] [diff] [review]
Part 3 - Revert a bad change in bug 883646

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 883646
User impact if declined: bug 896802
Testing completed (on m-c, etc.): this has been on m-c for a while
Risk to taking this patch (and alternatives if risky): low-risk. it backs out an incorrect change in a previous bug
String or IDL/UUID changes made by this patch: none

Comment 22

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/81a5eb02a7ee