Bug 232077 – [GTK][WPE] Use the display refresh to drive scrolling animations (sync scroll)

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Chris Lord]

At the moment, both ScrollAnimator (used for synchronous scrolling) and ScrollingTreeScrollingNodeDelegateNicosia (used for asynchronous scrolling on GTK/WPE (and maybe other ports?)) use a timer running at 1/60 seconds to drive scrolling animations (flings and smooth scrolling, the latter also used for CSS scroll snapping). This means that if your screen isn't running at exactly 60Hz (or the timer is not exact) that you are liable to get perceptibly jittery scrolling as frames are duplicated or skipped. These animations should be driven by the display refresh.

[Chris Lord]

Retitling this bug as it makes sense to split the work between sync (ScrollAnimator) and async (ScrollingTreeScrollingNodeDelegateNicosia) paths.

[Chris Lord]

Created attachment 442496 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 442496 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442496&action=review

> Source/WebCore/ChangeLog:9
> +        Use the DisplayRefreshMonitor instead of a Timer to drive scrolling
> +        animations in ScrollAnimator.

Things that animate on the main thread at the same frequency as Page::updateRendering() should, ideally, just be driven by Page::updateRendering which is already driven by a DisplayRefreshMonitor. So I'm not sold that we need two more DisplayRefreshMonitor clients for these animations.

However, we don't have plumbing to keep Page::updateRendering() firing when a scroll animation is active, nor do we have code in document.runScrollSteps() that updates scroll animations, although this is probably the right place for it.

[Chris Lord]

Created attachment 442580 [details]
Patch

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 442496 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442496&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Use the DisplayRefreshMonitor instead of a Timer to drive scrolling
> > +        animations in ScrollAnimator.
> 
> Things that animate on the main thread at the same frequency as
> Page::updateRendering() should, ideally, just be driven by
> Page::updateRendering which is already driven by a DisplayRefreshMonitor. So
> I'm not sold that we need two more DisplayRefreshMonitor clients for these
> animations.
> 
> However, we don't have plumbing to keep Page::updateRendering() firing when
> a scroll animation is active, nor do we have code in
> document.runScrollSteps() that updates scroll animations, although this is
> probably the right place for it.

hmm, so I didn't use document.runScrollSteps for this, it didn't feel like the right place but I'm happy to make the change if you think it is. I think it'd mean re-architecting this patch a fair bit; Chrome felt like a more suitable place to me.

[Chris Lord]

Created attachment 442583 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 442583 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442583&action=review

> Source/WebCore/page/Chrome.h:99
> +    void registerActiveScrollAnimator(ScrollAnimator&) override;
> +    void serviceScrollAnimations();

It feels wrong to go via Chrome for this; Chrome is normally used to communicate with the WebKit layer, and there should be no need to do this here.

startAnimationCallback() should be able to invoke Page-level stuff via FrameView/RenderLayerScrollableaArea ScrollableArea subclasses.

[Chris Lord]

Created attachment 442686 [details]
Patch

[Chris Lord]

(In reply to Chris Lord from comment #8)
> Created attachment 442686 [details]
> Patch

Ah, I hadn't realised that ScrollView was abstract and FrameView was its only implementation. This version uses Document as originally suggested.

[Simon Fraser (smfr)]

Bug 232534 does the equivalent hookup on the scrolling thread for macOS.

[Simon Fraser (smfr)]

Comment on attachment 442686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442686&action=review

> Source/WebCore/dom/Document.h:2167
> +    Vector<WeakPtr<ScrollAnimator>> m_activeScrollAnimators;

It might be OK to just traverse the set of FrameView::m_scrollableAreas. Or at least store this as a WeakHashSet<ScrollableArea> in FrameView so it's next to the other one.

> Source/WebCore/page/FrameView.h:742
> +    void scrollAnimatorBecameActive() final;

Please use the ...withActiveScrollAnimations/setScrollAnimationInProgress terminology that's already in the scrolling tree.

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 442686 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442686&action=review
> 
> > Source/WebCore/dom/Document.h:2167
> > +    Vector<WeakPtr<ScrollAnimator>> m_activeScrollAnimators;
> 
> It might be OK to just traverse the set of FrameView::m_scrollableAreas. Or
> at least store this as a WeakHashSet<ScrollableArea> in FrameView so it's
> next to the other one.

I'd certainly like to avoid traversal if it's not necessary given this will be happening every frame, even if the tree is unlikely to ever be particularly deep. No objection to this living in FrameView.

> > Source/WebCore/page/FrameView.h:742
> > +    void scrollAnimatorBecameActive() final;
> 
> Please use the ...withActiveScrollAnimations/setScrollAnimationInProgress
> terminology that's already in the scrolling tree.

There is no setScrollAnimationInProgress present in the tree at all... I see ScrollableArea::setScrollAnimationStatus which has a comment that it applies to CSSOM smooth scrolling (maybe that should have a different name?)

I also don't see withActiveScrollAnimations - I see nodesWithActiveScrollSnap, but again, that's a different thing... If you have something very specific in mind, more detail would definitely help.

Maybe I need to rebase again...

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #11)
> Please use the ...withActiveScrollAnimations/setScrollAnimationInProgress
> terminology that's already in the scrolling tree.

Ok, neither of these things were in the tree when this patch was submitted or even when this comment was written.

Perhaps I need to learn to be less sensitive, but I'm not hugely happy about the wording here that suggests I haven't done due-diligence and used an obviously named API when I couldn't have possibly done that.

I'll rebase my patch and work on top of the work that has landed in the interim period between initial submission and now.

[Simon Fraser (smfr)]

(In reply to Chris Lord from comment #13)
> (In reply to Simon Fraser (smfr) from comment #11)
> > Please use the ...withActiveScrollAnimations/setScrollAnimationInProgress
> > terminology that's already in the scrolling tree.
> 
> Ok, neither of these things were in the tree when this patch was submitted
> or even when this comment was written.
> 
> Perhaps I need to learn to be less sensitive, but I'm not hugely happy about
> the wording here that suggests I haven't done due-diligence and used an
> obviously named API when I couldn't have possibly done that.
> 
> I'll rebase my patch and work on top of the work that has landed in the
> interim period between initial submission and now.

Sorry about that; you're right that I was referring to something that was very new. But the tree did already have code that referred to "active animated scrolls".

[Chris Lord]

Created attachment 443078 [details]
Patch

[Chris Lord]

(In reply to Chris Lord from comment #15)
> Created attachment 443078 [details]
> Patch

If this isn't what you mean, I'd really appreciate some very specific feedback so that we can progress here because I'm otherwise clearly not understanding something.

[Chris Lord]

Created attachment 443081 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 443081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=443081&action=review

> Source/WebCore/dom/Document.cpp:4358
> +    if (auto* animator = scrollableArea->existingScrollAnimator())
> +        animator->serviceScrollAnimations();
> +    return scrollableArea->userScrollAnimationInProgress();

Maybe serviceScrollAnimations should return an enum that's like "stop" or "keep going". Not necessary in this patch.

> Source/WebCore/platform/ScrollAnimator.cpp:462
> +        m_scrollController.animationCallback(MonotonicTime::now());

Ideally the entire rendering update would use the same snapshotted time. As a stepping stone, pass the MonotonicTime in to serviceScrollAnimations().

> Source/WebCore/platform/ScrollableArea.h:356
> +    bool userScrollAnimationInProgress() { return m_userScrollAnimationInProgress; }

Let's make this
bool hasActiveScrollAnimation() const

> Source/WebCore/platform/ScrollableArea.h:357
> +    virtual void setUserScrollAnimationInProgress(bool inProgress) { m_userScrollAnimationInProgress = inProgress; }

I think it would be better named didStartScrollAnimation() - I don't think you care about the "did end" case.

> Source/WebCore/platform/ScrollableArea.h:419
> +    bool m_userScrollAnimationInProgress { false };

I don't think we need this state here; ScrollingEffectsController already has m_isRunningAnimatingCallback, so ScrollableArea can just call through ScrollAnimator to get this state from its ScrollingEffectsController.

[Chris Lord]

Created attachment 443188 [details]
Patch

[EWS]

Committed r285205 (243831@main): <https://commits.webkit.org/243831@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443188 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84973758>