1236035 - [checkerboard-experiment] Display port lags behind too much

Status: RESOLVED DUPLICATE of bug 1242609

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

Main thread painting determines what to paint based on the display port margins and the main thread scroll position. When the user scrolls using APZ, the main thread scroll position is updated through IPC messages that are sent by APZ. Painting is triggered by vsync, from the vsync IPC message.

If there's a long running paint on the main thread while the user scrolls, chances are that both scroll update ipc messages and vsync ipc messages have been queued in the main thread ipc queue. When painting is triggered, we only take into account the scroll position updates from ipc messages that have already been processed. Any scroll position updates that are already in the ipc queue but have arrived after the vsync ipc message will not be respected during the paint. This means that we're painting based on stale information.

We can work around problem this by "jumping the queue": We can get the most recent scroll position from the compositor, using the cross-process shared compositor frame metrics (which up to now have only been used for aborting progressive painting), and then we place the display port based on the up-to-date compositor scroll position.
We can't make the main thread scroll position itself use the up-to-date compositor scroll position - we need to preserve ordering of scroll position changes w.r.t. processing positioned events like mouse wheel events.

Comment 1

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1236035 - Share the compositor frame metrics even when progressive painting is disabled. r?kats

Review commit: https://reviewboard.mozilla.org/r/29291/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29291/

Comment 2

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1236035 - Take the most up-to-date scroll position into account when determining what to paint. r?kats

Review commit: https://reviewboard.mozilla.org/r/29293/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29293/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8703144 [details]
MozReview Request: Bug 1236035 - Share the compositor frame metrics even when progressive painting is disabled. r?kats

https://reviewboard.mozilla.org/r/29291/#review26065

This patch will need a try run before landing. We haven't enabled the frame metrics sharing code on Linux ever. I know also nical was concerned about the number of file descriptors that are getting used up by other shmem buffers in the tiling code, and this will add to that count because these shmems will use file descriptors as well. It might result in crashes/wonky behaviour that can be hard to pin down so we should monitor crash-stats carefully to see if there's a spike in anything after this lands.

::: gfx/thebes/gfxPlatform.cpp:2184
(Diff revision 1)
>  /*virtual*/ bool

Remove /*virtual*/ here

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8703145 [details]
MozReview Request: Bug 1236035 - Take the most up-to-date scroll position into account when determining what to paint. r?kats

https://reviewboard.mozilla.org/r/29293/#review26071

Some concerns below. In general I feel like this patch adds more complexity to a lot of already-brittle code that we've had a lot of trouble with in the past. We also don't have a lot of testing coverage for the scroll acknowledgement stuff (my fault). I do think that this change can help but it's also risky so it would be nice add a pref that people can use to diagnose issues. Thoughts?

::: layout/base/nsLayoutUtils.cpp:900
(Diff revision 1)
> +      base += *compositorScrollPos - scrollPos;

I don't understand why you shift |base| here rather than just setting |scrollPos = *compositorScrollPos;|. In particular see below the comment where it says "the base rect for root scroll frames is ..." and how the transformation into screen space is different for the base rect and the scroll position. I don't think you can add the scroll position delta like this.

::: layout/generic/nsGfxScrollFrame.cpp:3345
(Diff revision 1)
> +  if (mLastScrollOrigin && mLastScrollOrigin != nsGkAtoms::apz) {

I'm concerned about the interaction with main-thread smooth-scrolling here. In APZCCallbackHelper::ScrollFrameTo we have additional conditions that we check for to make sure we're not updating the main-thread scroll position if it's in the middle of a smooth/async scroll and I'm not sure if we need that here as well. I would err on the side of yes.

::: layout/generic/nsGfxScrollFrame.cpp:3357
(Diff revision 1)
> +  nsRect oldDisplayPort, newDisplayPort;

one declaration per line, and you can move the newDisplayPort declaration down to just before it's used.

Comment 6

[Markus Stange [:mstange]]

Bug 1242609 has a better fix for the same problem. Unassigning.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

I'll dupe it over for now. We can undupe it later if needed.