This seems to be a regression from bug 1563717. While the problem that bug fixed only affected certain websites, this seems to affect any zoomable one.

To reproduce open up mozilla.org in geckoview example app with webrender enabled. When at steady state, the viewport clip looks correct. Pinch zooming in seems to be okay, but when pinch zooming out the bottom-right of the screen is clipped. The clip seems to jump repeatedly as you zoom, as if it is calculated correctly during painting, but doesn't take into account the async zoom.

Here is my understanding of the problem:

As it says in bug 1563717:

The clip rect is derived from the composition bounds, which is in units that
are outside the resolution, but it's applied to the ScrollFrame item which is
inside the resolution.

We noticed the problem on bugzilla.mozilla.org because the pres shell resolution is < 1.0. Therefore the clip rect (which was just the composition bounds, in outside-resolution units) is smaller than the on screen area (in inside-resolution units). Dividing the clip rect by the pres shell resolution makes them match, and the whole screen was displayed.

However, now we are seeing a similar issue whilst zooming out on most pages. This is because the pres shell resolution decreases as you zoom out, therefore the clip rect should increase so as to display more of the page. Whenever we calculate the clip rect (during display list building?) it is correct, but then we continue async zooming. More of the page should become visible, but the clip rect does not change until the next display list is built.

I believe the only reason we didn't see this before bug 1563717 was by accident. Whilst zoomed-in, because the clip rect wasn't being divided by the pres shell resolution, it was much larger than it needed to be. So it didn't matter if you asynchronously zoomed out because the clip was large enough anyway.

To fix this I believe that webrender needs to scale the clip for each frame, taking into account the async zoom. I have no idea where the correct place to do this is, however.

Botond, does that sound plausible, and do you know where best to fix this?

The way this is handled for non-WebRender, is that the clip is "outside" the zoom (when rendering a layer, we apply its transform first, and then apply the clip to the result). I feel like it would make things more consistent if WebRender behaved similarly; not sure how feasible that is.

If we can't get the clip to be outside the zoom, then yes, scaling the clip asynchronously with the async zoom would make sense.

In terms of where to do that, a couple of starting points for investigation:

• The place where APZ records the current async zoom transform for WebRender is here.
• The place where that information makes it to the Rust side of the WebRender API is here. Note, by this point we may not be able to distinguish the async zoom from the other async transforms. We may need to modify the API to give the async zoom a special label, or something like that.

Presumably, we want to apply the async zoom to the clip "per frame" (since the async transforms can change per frame), somewhere after the async transforms are sampled, and somewhere before the clip is applied. I'm afraid I don't have more specific pointers for this part :/ Perhaps someone with more knowledge of the Rust side of WebRender can help us out here?

I have tracked down where webrender is doing the clipping:

ClipManager::DefineScrollLayers() (C++) calls into DisplayListBuilder::define_scroll_frame() (rust) with clip_rect as the root composition bounds (scaled by the pres shell resolution as of bug 1576923, but I believe this only papered over the issue by fixing it for the non-zooming case. we should revert this as part of the fix).

When we flatten the display list we call ClipScrollTree::add_scroll_frame() with this rect as the frame_rect, which creates a new ScrollFrameInfo with this rect as the viewport_rect.

In TileCacheInstance::pre_update() we attempt to map viewport_rect to world and local space, giving self.world_viewport_rect and self.local_clip_rect

I believe these are then used to clip when compositing picture cache tiles, and when rendering the tiles, respectively. But the mapping was incorrect so we clip incorrectly.

local_clip_rect ends up being equal to viewport_rect, and world_viewport_rect ends up smaller than the root composition bounds we started with. So the mapping did the wrong thing. Attached is the clip_scroll tree. Here is where we choose what spatial node to map from/to. We currently chose the scroll frame's parent spatial node (4), which is the ReferenceFrame with the transform binding which is being async zoomed. I think we actually want to choose the zoom ReferenceFrame's parent (the ScrollFrame's grandparent)? Does that sound right Glenn? Because the initial value of the viewport, as set all the way back in ClipManager.cpp, is set in units outside of the zoom.

Good find!

For unrelated reasons, I'm working right now on a patch that completely removes the need for local_clip_rect and world_viewport_rect in the picture caching code.

I suspect that once I land that (hopefully today / tomorrow) it will also fix this bug. I'll link to the patch here once it's ready.

The patch I mentioned above is https://bugzilla.mozilla.org/show_bug.cgi?id=1578641.

I still need to do a full try run to see if I broke anything - but if you have time, it'd be great to know if this patch also fixes this bug.

That patch does indeed fix this bug. Excellent.

I'm unsure whether we want to revert bug 1563717 then? My gut says yes, but it doesn't seem to make any difference with your patch.