Closed Bug 1036967 (apz-css-trans-stage3) Opened 6 years ago Closed 5 years ago

Make APZ code properly handle transforms and scales that are different on the x- and y-axes

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: kats, Assigned: botond)

References

Details

Attachments

(3 files, 4 obsolete files)

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
Alias: apz-css-trans-stage3
I'm going to take this to finish off the apz-css-transforms work.
Assignee: nobody → botond
These are WIP patches. They need testing.
Attached file MozReview Request: bz://1036967/botond (obsolete) —
/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
Attachment #8572911 - Flags: review?(bugmail.mozilla)
Attachment #8572911 - Flags: review?(bas)
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.
Attachment #8571648 - Attachment is obsolete: true
Attachment #8571649 - Attachment is obsolete: true
Attachment #8571650 - Attachment is obsolete: true
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.
Attachment #8572911 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #8572911 - Flags: review+ → review?(bugmail.mozilla)
(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).
Attachment #8572911 - Flags: review?(bugmail.mozilla) → review+
Attachment #8572911 - Flags: review+ → review?(bugmail.mozilla)
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.
Attachment #8572911 - Flags: review?(bugmail.mozilla) → review+
Attachment #8572911 - Flags: review?(bas) → review+
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.
Depends on: 1149060
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."
Attachment #8572911 - Attachment is obsolete: true
Attachment #8618225 - Flags: review+
Attachment #8618226 - Flags: review+
Attachment #8618227 - Flags: review+
You need to log in before you can comment on or make changes to this bug.