1613144 - Content jumps when pinch zooming on webrender on android

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Jamie Nicol [:jnicol]]

STR: zoom in to a page, then quickly pinch zoom back out. As you reach nearly zoomed out, the content might quickly jump down then up.

This is a regression from bug 1609002. That change made APZCTreeManager::SampleForWebRender() use only the translation from the layout component of the async transfrom for the async scroll offset it sends to webrender. (And then includes the translation of the visual component of the async transfrom in the animated transform binding which zooms the contents of the scroll frame.) Previously, the animated transform binding had only the zoom, and the translation of the combined (layout and visual) async transform was used for the scroll offset.

The consequence of using only the layout component rather than the combined layout+visual, is that the translation returned by GetCurrentAsyncTransform() is now scaled by the paint resolution rather than the current zoom. When we then divide the translation by GetCurrentPinchZoomScale(), to convert from ParentLayerPoint to LayoutDevicePoint, it's in a different coordinate space than it used to be: we're effectively out by a factor of FrameMetrics::GetAsyncZoom().

I can kinda see how this conceptually makes sense: the layout transform is unaffected by async zoom. But it causes this bug. It's also confusing that it's a ParentLayerPoint when I believe it really should be a LayerPoint, but we coerced it by multiplying by LayerToParentLayerScale(1.0f).

Changing GetCurrentAsyncTransform() to always scale by GetEffectiveZoom(), even for layout-only transforms, fixes this issue. But I'd imagine would cause other issues elsewhere, as well as potentially being conceptually wrong. So perhaps we should be dividing the layout async translation not by GetCurrentPinchZoomScale(), but rather by something that doesn't include the async zoom. (I believe that something might be FrameMetrics::mCumulativeResolution, to convert a LayerPoint masquerading as a ParentLayerPoint into a LayoutDevicePoint)

Comment 1

[Botond Ballo [:botond]]

Thanks for the analysis, Jamie!

Changing GetCurrentAsyncTransform() to always scale by GetEffectiveZoom(), even for layout-only transforms, fixes this issue. But I'd imagine would cause other issues elsewhere, as well as potentially being conceptually wrong.

Yeah, this would break non-WebRender, which applies the eLayout transform to scrolling layers, and then the eVisual transform to the async zoom container that contains the scrolling layers. If the eLayout and eVisual transforms both included the async zoom, we'd be applying the async zoom twice.

So perhaps we should be dividing the layout async translation not by GetCurrentPinchZoomScale(), but rather by something that doesn't include the async zoom.

+1, this sounds like the right fix.

(I believe that something might be FrameMetrics::mCumulativeResolution, to convert a LayerPoint masquerading as a ParentLayerPoint into a LayoutDevicePoint)

Yup, exactly! We can multiply it by LayerToParentLayerScale(1.0f) to get the units to work out for the division.

It's also confusing that it's a ParentLayerPoint when I believe it really should be a LayerPoint, but we coerced it by multiplying by LayerToParentLayerScale(1.0f).

Yeah :( We definitely have more coordinate spaces than we have unique names for at this point. Also, the eLayout and eVisual transforms are in different coordinates, but GetCurrentAsyncTransform() can only have one return type. We could potentially split the function into two, with different return types.

Removing non-WebRender will allow us to simplify things a bit here (for example, if WebRender wants the async translation it LayoutDevice pixels, we can just return it from GetCurrentAsyncTransform() in LayoutDevice pixels.)

Comment 2

[Botond Ballo [:botond]]

Taking a brief look through our test coverage of async zooming, it looks like all our reftests that use reftest-async-zoom either:

• are not also using reftest-async-scroll-* (so they don't catch this bug because the translation they're multiplying by the wrong scale is 0)
• are testing scrollbar behaviour, with the content being blank / white
• are testing fixed / sticky content, which is not affected by the layout transform

It should be straightforward to write a simple reftest with some scrolled content, and both reftest-async-zoom and reftest-async-scroll-x/y, that fails on WebRender without a fix here.

Comment 3

[Jamie Nicol [:jnicol]]

Thanks Botond. Yes, figured it would be because a function can only return one type. Probably not worth splitting the function until post-webrender when we can really think about what we need. As part of this patch, however, I might add a comment to GetCurrentAsyncTransform noting the different co-ordinate spaces of the return value. Once that idea clicked this bug wasn't so hard to figure out.

I can confirm dividing by CumulativeResolution * LayerToParentLayerScale(1.0f) works perfectly. (Had to add a getter for the cumulative resolution to AsyncPanZoomController).

Will work on a test-case next :)

Comment 4

[Jamie Nicol [:jnicol]]

Attachment: Bug 1613144 - Ensure async scroll offset is in correct coordinate space whilst async zooming. r?botond

Bug 1613144 made it so that the async scroll offset sent to webrender
is taken from only the layout component of the async transform, rather
than the combined layout and visual components. The consequence of
this is that the layout-only transform is in LayerPixel space (and
unnaffected by the async zoom) rather than ParentLayerPixel
space (which is affected by async zoom).

To convert the value to LayoutDevicePixel space, as webrender expects,
we were dividing by the pinch zoom scale, which includes the async
zoom. This was causing content to jump around whilst async panning and
zooming, as the scroll offset was incorrectly scaled. This is fixed
by instead dividing by the cumulative resolution, which does not
include the async zoom.

Comment 5

[Jamie Nicol [:jnicol]]

Attachment: Bug 1613144 - Add reftest. r?botond

Add a reftest that would fail in webrender before the corresponding
fix landed.

Ensures that there is an async zoom and that both the layout and
visual viewports have async scroll offsets. To pass, we must apply
each of the layout and visual offsets in their correct coordinate
spaces.

Depends on D61787

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5d9bf5b8d20
Ensure async scroll offset is in correct coordinate space whilst async zooming. r=botond
https://hg.mozilla.org/integration/autoland/rev/8124309e0bbe
Add reftest. r=botond

Comment 7

[Stefan Hindli [:stefan_hindli]]

Backed out 2 changesets (Bug 1613144) for causing reftest failures CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=c2c1012de608a476b039b7ab4155d0038a391613&selectedJob=287972581

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=287972581&repo=autoland&lineNumber=1452

Comment 8

[Jamie Nicol [:jnicol]]

Wrong reftest syntax. I thought I'd fixed that before landing. Sorry!

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bbfac21b3384
Backed out 2 changesets for causing reftest failures CLOSED TREE

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb480c346a72
Ensure async scroll offset is in correct coordinate space whilst async zooming. r=botond
https://hg.mozilla.org/integration/autoland/rev/b936ad51dd87
Add reftest. r=botond

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/eb480c346a72
https://hg.mozilla.org/mozilla-central/rev/b936ad51dd87