Transformation equation cleanups

RESOLVED FIXED in Firefox 24

Status

()

Core
Graphics: Layers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks: 1 bug)

25 Branch
mozilla25
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 fixed, firefox25 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

1.30 KB, patch
kentuckyfriedtakahe
: review+
kats
: checkin+
Details | Diff | Splinter Review
2.17 KB, patch
kentuckyfriedtakahe
: review+
kats
: checkin+
Details | Diff | Splinter Review
1.03 KB, patch
kentuckyfriedtakahe
: review+
kats
: checkin+
Details | Diff | Splinter Review
7.71 KB, patch
kentuckyfriedtakahe
: review+
kats
: checkin+
Details | Diff | Splinter Review
5.55 KB, patch
BenWa
: review+
kats
: checkin+
Details | Diff | Splinter Review
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.

Updated

5 years ago
Depends on: 865735

Updated

5 years ago
Depends on: 875630
Assignee: nobody → bugmail.mozilla
Version: 23 Branch → 25 Branch
Created attachment 767319 [details] [diff] [review]
Part 1 - Fix resolution-setting on HiDPI B2G devices

I think I missed a spot in bug 883646.
Created attachment 767320 [details] [diff] [review]
Part 2 - Fix up some comments in FrameMetrics.h
Created attachment 767321 [details] [diff] [review]
Part 3 - Use the layer's reported resolution where possible instead of inferring it
Created attachment 767324 [details] [diff] [review]
Part 4 - Add a test for AsyncPanZoomController::SampleContentTransformForFrame

This patch depends on the patches in bug 775459.
Depends on: 775459
Blocks: 875630
No longer depends on: 875630
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 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.
Attachment #767319 - Flags: review?(ajones)
Attachment #767320 - Flags: review?(ajones)
Created attachment 770276 [details] [diff] [review]
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.
Attachment #767321 - Attachment is obsolete: true
Attachment #767324 - Attachment is obsolete: true
Attachment #770276 - Flags: review?(ajones)
Attachment #767319 - Flags: review?(ajones) → review+
Attachment #767320 - Flags: review?(ajones) → review+
Attachment #770276 - Flags: review?(ajones) → review+
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
Whiteboard: [leave open]
Attachment #767319 - Flags: checkin+
Attachment #767320 - Flags: checkin+
Attachment #770276 - Flags: checkin+
Created attachment 770775 [details] [diff] [review]
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.
Attachment #770775 - Flags: review?(ajones)
Created attachment 770778 [details] [diff] [review]
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.
Duplicate of this bug: 860162
Attachment #770775 - Flags: review?(ajones) → review+
Attachment #770775 - Flags: checkin+

Comment 14

5 years ago

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 <kgupta@mozilla.com> - 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 <kgupta@mozilla.com> - 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 <kgupta@mozilla.com> - 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
dev-tree-management@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-tree-management
Flags: needinfo?(bugmail.mozilla)
I already replied to dev.tree-mgmt about the m-c version of this notice. Same applies to inbound.
Flags: needinfo?(bugmail.mozilla)
Created attachment 772105 [details] [diff] [review]
Part 5 - Add a test for the transformation equations
Attachment #770778 - Attachment is obsolete: true
Attachment #772105 - Flags: review?(bgirard)
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.
Attachment #772105 - Flags: review?(bgirard) → review+
Added a comment to explain that and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef387716ec2b
Whiteboard: [leave open]
Attachment #772105 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/ef387716ec2b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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
Attachment #770276 - Flags: approval-mozilla-aurora?
Blocks: 896802

Updated

5 years ago
Attachment #770276 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox24: --- → fixed
status-firefox25: --- → fixed
You need to log in before you can comment on or make changes to this bug.