1367770 - [meta] Try harder to keep scroll-linked effects in sync with APZ

Status: NEW

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

Description

[zcyzcy88]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170518000419

Steps to reproduce:

1. https://tdresser.github.io/sync-scroll-offset-test
2. Scroll around a number of times


Actual results:

elements jitter when scroll (JavaScript [scroll-linked effects]([https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Scroll-linked_effects](https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Scroll-linked_effects)))

### change element's style in `onscroll` callback
- **heavy jitter** - scroll mouse wheel
- **no jitter** - press and drag middle mouse wheel
- **a little jitter** - drag the scroll bar

### change element's style in `requestAnimationFrame` callback
- **heavy jitter** - scroll mouse wheel
- **a little jitter** - press and drag middle mouse wheel
- **a little jitter** - drag the scroll bar


Expected results:

Scroll synchronization on simple pages should be perfect.
This is such a simple page, that I'd argue there should almost NEVER be a reason for JS frame production (green) to be out-of-sync with the native CSS (red).
[Chrome already make g great work to patch it](https://bugs.chromium.org/p/chromium/issues/detail?id=468310), I think Firefox should do this also, for better user experience.

Comment 1

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

I agree that on such simple pages you shouldn't really ever see jitter. On Windows in the latest nightly I can see the two colors be slightly out of sync during wheel scrolling and scrollbar drag (because we have async scrollbar drag only turned on in Nightly, you wouldn't have seen this in 53 release).

Comment 2

[Botond Ballo [:botond]]

There are two cases here: 

  (1) Scroll input methods that kick off an APZ animation, which
      is sampled on each frame and the scroll offset updated on
      each sample. Mousewheel scrolling falls into this category.

      Here, scroll events being 1 frame behind async scrolling
      is a known issue. It actually used to be 2 frames behind,
      and that was reduced to 1 frame in bug 1234950.

      Basically, the reason there's still a 1 frame delay is that
      refresh driver ticks and composites are both aligned to
      vsync. For a given composite C:
   
        - Immediately after C happens, we sample the APZ 
          animation with the timestamp of C+1.

        - The main thread is notified of the updated scroll
          position, but that will only be painted during the
          next refresh driver tick (the one aligned with C+1).

        - The result of the paint is then sent to the compositor
          thread and composited as part of C+2, 1 frame later
          than the async scroll position which was rendered
          at C+1.

  (2) Scroll input methods that update the async scroll offset
      immediately after each input event. Touch scrolling,
      macOS trackpad scrolling, and scrollbar dragging fall into
      this category.

      Here, the notification to the main thread about the updated
      scroll offset happens whenever the input event is processed.
      The new async scroll offset will be composited on the next
      composite, while the main thread will process the new scroll
      offset and paint the result on the next refresh driver tick,
      which is then composited on the subsequent composite, 
      resulting in a similar 1 frame delay as above.

I'd like to experiment with delaying the compositor's application of async scroll offsets by one composite, to achieve scroll-linked effects being in sync with async scrolling when they can be painted within the frame budget.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> I'd like to experiment with delaying the compositor's application of async
> scroll offsets by one composite, to achieve scroll-linked effects being in
> sync with async scrolling when they can be painted within the frame budget.

Here's a quick-and-dirty proof-of-concept patch that does this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea5b265c5742a2d5e747828cef09529113739113

With this patch applied, on most frames the two scroll offsets are in sync. There are still occasional frames where content is 1 frame behind, which I'd like to investigate further, as on a simple page like this that should basically never happen.

Comment 4

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #3)
> There are still occasional frames where content is 1 frame behind, which I'd
> like to investigate further, as on a simple page like this that should
> basically never happen.

It turns out this is actually a regression from bug 1250550, which changed the "scroll" event to be fired from a FlushType::Layout refresh observer rather than a FlushType::Style refresh observer.

FlushType::Layout refresh observers are invoked after the restyle, so, as a result of that change, if a "scroll" event changes style but nothing else before the paint triggers another restyle, the style change will not go into effect on that tick.

The reason for using a FlushType::Layout observer is explained in bug 1250550 comment 3. The above suggests that we should solve that problem differently, for example by having scroll events be fired from a function called directly by nsRefreshDriver::Tick() after running all Style observers but before performing the actual restyle.

With the patch in bug 1250550 reverted locally, I'm now not seeing the scroll-linked effect be behind the async scrolling at all during wheel scrolling. Interestingly, *very* occasionally, I see it being 1 frame *ahead*, which is unexpected and needs to be investigated. In addition, during scrollbar dragging I'm still seeing the effect being occasionally behind, which also needs to be investigated.

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #4)
> The reason for using a FlushType::Layout observer is explained in bug
> 1250550 comment 3. The above suggests that we should solve that problem
> differently, for example by having scroll events be fired from a function
> called directly by nsRefreshDriver::Tick() after running all Style observers
> but before performing the actual restyle.

Markus found bug 1340684 already on file for more or less the same issue; I'll use that bug for this change.

Comment 6

[Botond Ballo [:botond]]

I'm going to turn this into a meta bug, and land specific attempts to keep scroll-linked effects in sync with APZ in dependent bugs.

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #3)
> (In reply to Botond Ballo [:botond] from comment #2)
> > I'd like to experiment with delaying the compositor's application of async
> > scroll offsets by one composite, to achieve scroll-linked effects being in
> > sync with async scrolling when they can be painted within the frame budget.
> 
> Here's a quick-and-dirty proof-of-concept patch that does this:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ea5b265c5742a2d5e747828cef09529113739113

I will continue pursuing this particular change in bug 1375949.

Comment 8

[Botond Ballo [:botond]]

There are more improvements to be done here but I'm not actively working on it.