1698537 - Double-tap zoom in and out transition is not smoothly performed

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Emil Ghitta, QA [:emilghitta]]

Affected versions

• Firefox 88.0a1 (BuildId:20210315091836)

Affected platforms

• macOS 10.14
• macOS 10.15
• macOS 11.1

Unaffected platforms

• Windows 10 64bit
• Ubuntu 20.04

Preconditions

• Have apz.mac.enable_double_tap_zoom_touchpad_gesture pref enabled

Steps to reproduce

1. Launch Firefox.
2. Access the following link.
3. Double-tap to zoom in a certain area of interest (ex: On the United States flag).
4. Double-tap to zoom in the same area in order to zoom back to normal.

Expected result

• The zoom in and out transition is smooth.

Actual result

• The zoom in and out transition is not that smooth. It seems that Safari and Chrome browsers have a smoother zoom transition.

Regression Range

• I don’t think that this is a regression.

Notes

• For further information regarding this issue, please observe the following screencast (Unfortunately the file size exceeds the bugzilla limit. Mozilla account needed).

Comment 1

[Botond Ballo [:botond]]

Based on the video, I assume the lack of smoothness refers to the fact that there is a flicker of white ("checkerboarding") while zooming back out.

We had a similar issue with pinch-zooming out in bug 1654290, which we addressed by adjusting our displayport / repaint heuristics. Perhaps we can do something similar here.

Comment 2

[Timothy Nikkel (:tnikkel)]

The checkerboarding in the video is happening on Safari. I don't see any checkerboarding in Firefox in the video. The problem in the video with Firefox that I see is that our zoom out animation isn't smooth: it seems like it is not smooth in two different ways. 1) we aren't drawing enough frames per second 2) the path that our animation takes doesn't seem to follow as smooth a curve as possible (things "jump around" more than necessary). I'm not the best at observing this kind of thing so I could be mistaken in that break down.

Comment 3

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #2)

The checkerboarding in the video is happening on Safari. I don't see any checkerboarding in Firefox in the video.

My bad, I mixed those up!

The problem in the video with Firefox that I see is that our zoom out animation isn't smooth: it seems like it is not smooth in two different ways. 1) we aren't drawing enough frames per second 2) the path that our animation takes doesn't seem to follow as smooth a curve as possible (things "jump around" more than necessary). I'm not the best at observing this kind of thing so I could be mistaken in that break down.

I'm also bad at observing (1), but I do see what you mean by (2), there's a jitter there in Firefox (as though a scroll offset is being adjusted back and forth) which is not present in Safari.

Comment 4

[Timothy Nikkel (:tnikkel)]

In testing this I see a couple other problems:

1. I was able to get into a state where I was had zoomed out back to the initial page zoom (using double tap to zoom back out) but it looked like there was a clip applied that only allowed what was previously visible when I was zoomed in to be shown. The scroll thumbs were much larger than even the page at initial zoom and they looked very pointy at the ends (indicating a significant async scale was being appleid). This happened two times in my limited testing.
2. when I double tap zoom in I am able to see the scroll thumb flash to a much larger size (larger than it should be at any zoom) pretty much every time.

These may not be related but I wanted to write them down so I don't forget.

In the video, when I slow it down to 0.25 speed I see that we checkerboard briefly when we zoom in, which is surprising.

Comment 5

[Martin Balfanz [:mbalfanz]]

I observe the following from my own tests and watching the video:

1. The seems to be a yank (skipped frames) when zooming in the first time. It appears like a jump before the transition start
2. when zooming back out, the page seems to shift around horizontally in some way (point 2 of :tnikkel above)
3. our transition seems to be way faster than what Safari does. I think we should try to slow it down a little bit.

Comment 6

[Martin Balfanz [:mbalfanz]]

Re: 1: A good test case for this seems to be gmail. When double-tapping into email content, the entire viewport flashes in the background color for a fraction of a second (black in my case)

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1698537. Don't allow double tap to zoom in print preview documents. r?botond

We shouldn't need mutation change notifications for print/preview changes as the static document bool looks to be set very early to document creation.

Comment 8

[Phabricator Automation]

Comment on attachment 9209628 [details]
Bug 1698537. Don't allow double tap to zoom in print preview documents. r?botond

Revision D108731 was moved to bug 1698785. Setting attachment 9209628 [details] to obsolete.

Comment 9

[Timothy Nikkel (:tnikkel)]

In AsyncPanZoomController::ZoomToRect we have the rect we want to zoom (out) to, and we have our current scrollOffset=(1012,235.2), our currentzoom=10, and our end of zoom target scrolloffset is (0,79), target zoom = 1.

We calculate an end of zoom display port and we request a content repaint ( https://searchfox.org/mozilla-central/rev/f07a609a76136ef779c65185165ff5ac513cc172/gfx/layers/apz/src/AsyncPanZoomController.cpp#5373 ) before the zoom starts with these end of zoom metrics, and the (0,79) scrolloffset comes back to apz from content in a NotifyLayersUpdated call. And then the zoom animation starts and over writes the (0,79) scroll offset.

So it seems like we need to calculate this end of zoom display port based on the end of zoom scroll offset, but then express it relative to the current scroll offset and that displayport (along with the current scrolloffset, not the end of zoom offset) is what we should send to the main thread before the zoom happens.

Comment 10

[Timothy Nikkel (:tnikkel)]

Getting this displayport computed so that we don't have obvious artifacts is harder then I expected, but seems like the right approach and it should fix the other issues that I observed.

Comment 11

[Timothy Nikkel (:tnikkel)]

There is a lot going on here but I think I have untangled it all.

If one reads the source code now one would probably come away with the following understanding:

1. at the start of a double tap zoom we calculate a displayport for the "after" zoom with the "after" scroll offset. Then we send that displayport, along with a FrameMetrics that has the after zoom and the after scroll offset to content. https://searchfox.org/mozilla-central/rev/08013752b4638f01d41d2a38ca7bd741bc572c86/gfx/layers/apz/src/AsyncPanZoomController.cpp#5574
2. that we do not ask content to repaint during the animation (since WantsRepaints of ZoomAnimation returns false here https://searchfox.org/mozilla-central/rev/08013752b4638f01d41d2a38ca7bd741bc572c86/gfx/layers/apz/src/AsyncPanZoomController.cpp#661 )
3. combining 1 and 2 one could conclude that we have all the content we need painted to render the zoom animation completely on the compositor already, since there would be no point of rendering the after zoom content at the start of the animation, then blowing that away to render something we need for the zoom animation.

None of this is true. The content that we ask content to render at 1 gets blown away by the content we ask content to render during the zoom animation, so it's useless. This is because even though we don't ask for repaints during the zoom animation we get them in a different, roundabout away. And finally, it is not so easy to get all of the content we might need to render the full zoom animation at one time, and in general may not be possible given texture limits or memory limits (mostly only a problem on mobile I'm guessing).

The reason that we get repaints for almost every frame (at least on desktop, mobile might not be able to keep up, I haven't looked) is that the following:
-repaint request with the after resolution gets applied by content, it paints layers/computes frame metrics
-the new layers get sent to apz
-NotifyLayersUpdated notices that the presshell resolution has changed, this makes it send a repaint request
-apz still thinks we are at or near (if the zoom animation has started) the before zoom, so the repaint request is different from what content thinks is the resolution
-content updates to the new resolution
-apz gets the new layers
-at this point things could stop, but they don't because the zoom animation is running, so the zoom keeps changing, each repaint request to content generates another

The reason it's not so easy to render all content before the zoom animation is that in the case of zooming out we'd need to render all of the content visible before, during, and after the zoom animation at the zoomed in resolution, otherwise it would look fuzzy. This could be a lot of content.

So repainting during the animation actually seems like a good thing, we can render the content we need at that time and at the resolution. We are however, I believe, rendering the full display port we would have in a static situation where we compute the display port so that scrolling works well. This means we are rendering quite a bit more content then is visible, and is an area to optimize in the future.

My initial efforts to remove the initial repaint request at the after resolution stumped me because we were checkerboarding through the whole zoom animation, but this explains why: with that first repaint request at the after resolution we did not paint content during the animation so it checkerboarded. The old code wasn't necessarily computing/rendering a displayport/resolution that was perfect, it just repainted every frame to hide any problem it might have had.

So to fix this bug in a low risk manner I am proposing:
-we keep the repaints during zoom that are already happening (by making zoom animation return true from WantsRepaints)
-we stop sending the after zoom resolution/displayport to content at the start of the zoom animation

Once those two things are fixed the non-smoothness here is still a problem because the main issue this bug was initially filed about is actually a different problem then anything I've described above! But luckily it is also easy to understand. The way that we compute the scrolled rect at https://searchfox.org/mozilla-central/rev/08013752b4638f01d41d2a38ca7bd741bc572c86/layout/generic/nsGfxScrollFrame.cpp#7101 involves getting the presshell resolution via GetPaintedLayerScaleForFrame and then multiplying coords by it and then rounding. So as the resolution changes the scrolled rect changes slightly. When content sends metrics to apz NotifyLayersUpdated notices this change and says we need to reclamp the scroll offset https://searchfox.org/mozilla-central/rev/08013752b4638f01d41d2a38ca7bd741bc572c86/gfx/layers/apz/src/AsyncPanZoomController.cpp#5035 This in turn calls ClampVisualScrollOffset on the sampled state (the sampled state is basically the scroll offset and the zoom to use to paint on the next two frames in the future, which we sampled already in the past). ClampVisualScrollOffset in the sampled state uses the zoom from the metrics on the apzc. This zoom is newer than the zoom on the sampled state. So it can actually move the scroll offset. Since we are going to use the zoom on the sampled state when we present the frame (and not from the apzc) this is incorrect. Note that earlier in NotifyLayersUpdated we call ZoomBy on the sampled state to update the zoom https://searchfox.org/mozilla-central/rev/08013752b4638f01d41d2a38ca7bd741bc572c86/gfx/layers/apz/src/AsyncPanZoomController.cpp#5016 however the scale we compute there is 1, because the change in cumulative resolution is equal to the change in presshell resolution, because we (apz) are the ones who requested the changes in the presshell resolution, so we already know about it.

Although it took a huge amount of words all of these changes should pretty small and relatively low risk.

Comment 12

[Botond Ballo [:botond]]

That's an epic diagnosis :)

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

None of this is true.

:D

So to fix this bug in a low risk manner I am proposing:
-we keep the repaints during zoom that are already happening (by making zoom animation return true from WantsRepaints)
-we stop sending the after zoom resolution/displayport to content at the start of the zoom animation

+1

Comment 13

[Timothy Nikkel (:tnikkel)]

The three dependant bugs that I filed will all have patches shortly that fix all of the problems here in a manner that is still fairly similar to how things currently work.

Comment 14

[Timothy Nikkel (:tnikkel)]

Dependant bugs have all landed now. This should be fixed.

Comment 15

[Emil Ghitta, QA [:emilghitta]]

This is verified fixed using Firefox 89.0b9 (BuildId:20210506185706) and Firefox 90.0a1 (BuildId:20210506214311) on macOS 10.14 & 10.15.

The zoom in & out transition is way more smoother. Thanks!