If you build gecko at mercurial cset cc20208a6eb4 (i.e. with the original patch for bug 1034247) and using it on B2G on a Nexus 4, the layers dump ends up looking like [1] on the homescreen. This has a container layer with preScale=1,2 which demonstrates one way that we can end up with scaling factors that are not the same on the x- and y-axes. The APZ code needs to be able to deal with this stuff, and I don't think it currently does handle this case properly. More generally, we assume an axis-uniform LayoutDeviceToLayerPixel scale, but that may not be the case. [1] https://bug1034247.bugzilla.mozilla.org/attachment.cgi?id=8453779

This will be Stage 3 of the apz-css-transforms work [1]. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=993525#c3

I'm going to take this to finish off the apz-css-transforms work.

These are WIP patches. They need testing.

/r/4733 - Bug 1036967 - Introduce ScaleFactors2D. r=kats,Bas /r/4735 - Bug 1036967 - Use ScaleFactors2D instead of ScaleFactor where appropriate in APZ and surrounding code. r=kats /r/4737 - Bug 1036967 - Remove ScaleFactor::ScaleFactor(float, float). r=kats Pull down these commits: hg pull review -r 2f880cbe012ce9d3b5e1207bab8b88f6ecd2a07a

Comment on attachment 8571648 [details] [diff] [review] Part 1 - Introduce ScaleFactors2D I updated the patches to fix a few mistakes and to get them to compile on Android. Local testing looks good with a page like http://people.mozilla.org/~bballo/divtrans.html. The bug 1071018 STR also aren't problematic any more (with the workaround removed). For getting the patches reviewed, I thought I'd try out MozReview. Updated patches posted there.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c5fcc84bc17

I should note that there remains one set of places in the code where Layout provides both x- and y-resolution, but I only use the x-resolution: for the pres shell resolution (and derived quantities, like the cumulative pres shell resolution [not to be confused with FrameMetrics::mCumulativeResolution, which includes a css-driven component and is thus 2D]). This is because the pres shell resolution should never have different x- and y-scales. I filed a follow-up, bug 1139675, to modify the relevant APIs to reflect this.

https://reviewboard.mozilla.org/r/4733/#review3861 A few suggestions but I'd be fine with it landing as-is too. ::: gfx/2d/ScaleFactors2D.h (Diff revision 1) > + std::streamsize oldPrecision = aStream.precision(3); I'm not a fan of hard-coding a precision like this; in general I'd prefer the caller do that if they want a specific precision. I don't feel too strongly about this though, I suspect in practice it won't matter much. ::: gfx/2d/ScaleFactors2D.h (Diff revision 1) > + return ScaleFactors2D<src, other>(xScale * aOther.scale, yScale * aOther.scale); Could probably also do this as |return this * ScaleFactor2D(aOther);| if you prefer. I find that a bit less repetitive but I'm fine either way. ::: gfx/2d/ScaleFactors2D.h (Diff revision 1) > + return fabs(xScale - yScale) < 1e-6; I'd rather use FuzzyEqualsMultiplicative from mfbt/FloatingPoint.h here ::: gfx/2d/ScaleFactors2D.h (Diff revision 1) > + // Divide two scales of the same units, yielding a scale with no units, Do we need this one? I'd rather not add it until we need it, and would also prefer if it returned a ScaleFactor2D<src, src> or a ScaleFactor2D<dst, dst>. Either of those would be better than a gfxSize, I think. Note that in layout/base/Units.h we do have a ParentLayerToParentLayerScale so it's not without precedent.

https://reviewboard.mozilla.org/r/4735/#review3865 ::: gfx/layers/FrameMetrics.h (Diff revision 1) > - mZoom.scale *= aFactor; > + ZoomBy(gfxSize(aScale, aScale)); Can we use a ParentLayerToParentLayerScale2D here? ::: gfx/layers/LayersLogging.cpp (Diff revision 1) > - aStream << nsPrintfCString("] [z=%.3f] }", m.GetZoom().scale).get(); > + aStream << nsPrintfCString("] [z=%s] }", ToString(m.GetZoom()).c_str()).get(); AppendToString(aStream, m.GetZoom(), "] [z=", "] }"); (You'll need to add an AppendToString overload for ScaleFactor2D) ::: gfx/layers/LayersLogging.cpp (Diff revision 1) > - aStream << nsPrintfCString("] [z=(ld=%.3f r=%.3f cr=%.3f z=%.3f er=%.3f)", > + aStream << nsPrintfCString("] [z=(ld=%.3f r=%.3f cr=%s z=%s er=%s)", Break this up into a handful of AppendToString calls ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp (Diff revision 1) > - EXPECT_EQ(2.5f, fm.GetZoom().scale); > + EXPECT_EQ(2.5f, fm.GetZoom().xScale); If you do fm.GetZoom().ToScaleFactor().scale here it will also check that the x- and y-scales are equal which is probably better. Ditto for the others below.

Comment on attachment 8572911 [details] MozReview Request: bz://1036967/botond Hm, I think I screwed up the MozReview flow on the second patch. I meant to "Ship it" with comments but accidentally opened issues, which I think blocks the auto-r+ process. I've dropped the issues (hopefully you can still see the comments though). This is basically an r+ with comments addressed.

Comment on attachment 8572911 [details] MozReview Request: bz://1036967/botond /r/4733 - Bug 1036967 - Introduce ScaleFactors2D. r=kats,Bas /r/4735 - Bug 1036967 - Use ScaleFactors2D instead of ScaleFactor where appropriate in APZ and surrounding code. r=kats /r/4737 - Bug 1036967 - Remove ScaleFactor::ScaleFactor(float, float). r=kats Pull down these commits: hg pull review -r 353e24eccb1a214d4d3c8e5db689c699b6fe5e2b

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10) > ::: gfx/2d/ScaleFactors2D.h > (Diff revision 1) > > + std::streamsize oldPrecision = aStream.precision(3); > > I'm not a fan of hard-coding a precision like this; in general I'd prefer > the caller do that if they want a specific precision. I don't feel too > strongly about this though, I suspect in practice it won't matter much. The idea was to preserve the precision used to print scales in LayersLogging. I ended up changing LayersLogging to use a custom AppendToString overload instead of ToString, and moving the precision-setting code, so I removed it from here. (I kept ToString() itself because it had a non-LayersLogging call site.) > ::: gfx/2d/ScaleFactors2D.h > (Diff revision 1) > > + return ScaleFactors2D<src, other>(xScale * aOther.scale, yScale * aOther.scale); > > Could probably also do this as |return this * ScaleFactor2D(aOther);| if you > prefer. I find that a bit less repetitive but I'm fine either way. Good idea - done. > ::: gfx/2d/ScaleFactors2D.h > (Diff revision 1) > > + return fabs(xScale - yScale) < 1e-6; > > I'd rather use FuzzyEqualsMultiplicative from mfbt/FloatingPoint.h here Done. > ::: gfx/2d/ScaleFactors2D.h > (Diff revision 1) > > + // Divide two scales of the same units, yielding a scale with no units, > > Do we need this one? I'd rather not add it until we need it, and would also > prefer if it returned a ScaleFactor2D<src, src> or a ScaleFactor2D<dst, > dst>. Either of those would be better than a gfxSize, I think. Note that in > layout/base/Units.h we do have a ParentLayerToParentLayerScale so it's not > without precedent. We use it in AsyncPanZoomController::GetTransformToLastDispatchedPaint(), and to compute the resolution change in NotifyLayersUpdated(). I'm not convinced about returning a typed ScaleFactor2D here. First, we'd have to make an arbitrary choice between <dst, dst> and <src, src>. Second, as discussed on IRC, we've found that using a typed scale in the past for the zoom change between two frames doesn't work very well (see https://bugzilla.mozilla.org/show_bug.cgi?id=923431#c9 where we removed it, a ScreenToScreenScale at the time). (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11) > ::: gfx/layers/FrameMetrics.h > (Diff revision 1) > > - mZoom.scale *= aFactor; > > + ZoomBy(gfxSize(aScale, aScale)); > > Can we use a ParentLayerToParentLayerScale2D here? As per the above, staying with untyped units here. > ::: gfx/layers/LayersLogging.cpp > (Diff revision 1) > > - aStream << nsPrintfCString("] [z=%.3f] }", m.GetZoom().scale).get(); > > + aStream << nsPrintfCString("] [z=%s] }", ToString(m.GetZoom()).c_str()).get(); > > AppendToString(aStream, m.GetZoom(), "] [z=", "] }"); > > (You'll need to add an AppendToString overload for ScaleFactor2D) Done. (I'm not thrilled about the code duplication between AppendToString and ToString. We should probably unify these at some point.) > ::: gfx/layers/LayersLogging.cpp > (Diff revision 1) > > - aStream << nsPrintfCString("] [z=(ld=%.3f r=%.3f cr=%.3f z=%.3f er=%.3f)", > > + aStream << nsPrintfCString("] [z=(ld=%.3f r=%.3f cr=%s z=%s er=%s)", > > Break this up into a handful of AppendToString calls Done. > ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp > (Diff revision 1) > > - EXPECT_EQ(2.5f, fm.GetZoom().scale); > > + EXPECT_EQ(2.5f, fm.GetZoom().xScale); > > If you do fm.GetZoom().ToScaleFactor().scale here it will also check that > the x- and y-scales are equal which is probably better. Ditto for the others > below. Done.

Comment on attachment 8572911 [details] MozReview Request: bz://1036967/botond Carrying r+ from Kats (not sure what the proper MozReview workflow is for doing that).

Comment on attachment 8572911 [details] MozReview Request: bz://1036967/botond /r/4733 - Bug 1036967 - Introduce ScaleFactors2D. r=kats,Bas /r/4735 - Bug 1036967 - Use ScaleFactors2D instead of ScaleFactor where appropriate in APZ and surrounding code. r=kats /r/4737 - Bug 1036967 - Remove ScaleFactor::ScaleFactor(float, float). r=kats Pull down these commits: hg pull review -r fe3604d3c095aa8fb8e40275e723d05334d34387

Comment on attachment 8572911 [details] MozReview Request: bz://1036967/botond While working on bug 1139675, I realized that I missed a 1D -> 2D conversion in a couple of spots. Updated patch to add these in. Carrying r+ from Kats.

https://reviewboard.mozilla.org/r/4733/#review4251 Looks good!

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa4b3571ae88 https://hg.mozilla.org/integration/mozilla-inbound/rev/78dcb3a426f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/faf1d0ec530e

https://hg.mozilla.org/mozilla-central/rev/fa4b3571ae88 https://hg.mozilla.org/mozilla-central/rev/78dcb3a426f2 https://hg.mozilla.org/mozilla-central/rev/faf1d0ec530e

Follow-up to fix debug logging: https://hg.mozilla.org/integration/mozilla-inbound/rev/96482314a464

Follow-up to fix wacky FrameMetrics output in layer dumps where it was showing things like "[scrollId=21.5]": https://hg.mozilla.org/integration/mozilla-inbound/rev/834936a75fb2

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25) > Follow-up to fix wacky FrameMetrics output in layer dumps where it was > showing things like "[scrollId=21.5]": > > https://hg.mozilla.org/integration/mozilla-inbound/rev/834936a75fb2 Whoops! Thanks for catching that.

Weird.. I just got bugzilla e-mail for the review request from this bug..

(In reply to Bas Schouten (:bas.schouten) from comment #28) > Weird.. I just got bugzilla e-mail for the review request from this bug.. From https://groups.google.com/d/msg/mozilla.dev.version-control/M65guOWfERs/E0Q1Ty7D3jQJ: "You may have received some bugmail about changes to MozReview attachments; no actual changes were committed, so please ignore them. Apologies for the inconvenience."