feat(composition): Snap frame times to the display interval - #24966
MartinZikmund wants to merge 3 commits into
Conversation
|
Claude finished @MartinZikmund's task in 2m 32s —— View job Review in progress
|
9efc4a9 to
f7277e1
Compare
549b543 to
43adc32
Compare
|
Claude finished @MartinZikmund's task in 5m 25s —— View job Review complete ✅
Verdict: fix-first2 findings posted inline, both test-coverage/quality issues — no bugs found in
Cross-cutting notes
|
| /// </summary> | ||
| [TestMethod] | ||
| [RunsOnUIThread] | ||
| public void When_Frames_Hitch_Then_Frame_Clock_Does_Not_Freeze() | ||
| { | ||
| const long Vsync = TimeSpan.TicksPerSecond / 60; | ||
|
|
||
| var clock = new Uno.UI.Composition.FrameClock(); | ||
|
|
||
| var stamps = new long[200]; | ||
| var raw = TimeSpan.TicksPerSecond; | ||
| for (var i = 0; i < stamps.Length; i++) | ||
| { | ||
| // 60Hz, then a hitch slow enough to flip the median yet short of the idle-gap rejection at four | ||
| // periods, then 60Hz again. | ||
| raw += i is >= 60 and < 80 ? 58 * TimeSpan.TicksPerMillisecond : Vsync; |
There was a problem hiding this comment.
This test's comment claims the hitch is "short of the idle-gap rejection at four periods", but it overlooks the other, independent guard in FrameClock.NextTimestamp:
if (delta > MaxFrameIntervalInTicks || (period > 0 && delta >= period * IdleGapPeriods))MaxFrameIntervalInTicks = TimeSpan.TicksPerSecond / 20 is 50 ms, but the hitch delta here is 58 * TimeSpan.TicksPerMillisecond = 58 ms, which exceeds it. delta > MaxFrameIntervalInTicks is true on every one of the 20 "hitch" frames, independent of the period check — so each one takes the early-return/gap-passthrough branch (FrameClock.skia.cs:62-65) and never reaches Sample()/the median update at all.
Concretely: the median never flips during this test (it stays pinned to the pre-hitch ~60 Hz value), the banked-lead/maxLead clamp this test is named for never executes, and each hitch stamp is just raw passed straight through (strictly increasing, so worst >= 0 and frozen <= 2 hold trivially). The test currently passes, but for a reason unrelated to the "median flips, lead correction kicks in" scenario it documents — a regression in the maxLead clamp logic (FrameClock.skia.cs:91-97) would not be caught by this test.
To actually exercise that path, the hitch delta needs to stay ≤ MaxFrameIntervalInTicks (50 ms) while still being large enough, relative to the then-current median, to shift it — e.g. something like 45–48 ms repeated enough times to move the median, which is also comfortably under the period-relative idle threshold (66.67 ms at steady 60 Hz).
|
|
||
| /// <summary>A notch arriving mid-motion restarts the curve over what is left plus the notch, so a spin only speeds up.</summary> | ||
| [TestMethod] | ||
| public async Task When_Wheel_Spins_Then_Velocity_Never_Drops_At_A_Notch() |
There was a problem hiding this comment.
Per the PR description this test is known flaky (failed 2/5 locally without the grid, 1/11 with it), with the suspected root cause (inertia handler's first tick returning early while HasEntered is false) still "to be investigated" and not fixed in this PR.
Landing a self-reported-flaky test active (not [Ignore]d) risks intermittent CI failures/red herrings for unrelated PRs. The repo already has a convention for this — see Given_ScrollViewerTests/Given_ScrollViewer_ScrollChaining.cs:103:
[Ignore("Flaky on all targets, not one platform - https://github.com/unoplatform/uno/issues/24489")]Consider either quarantining this test the same way with a tracking issue link until the root cause above is fixed, or filing that tracking issue now and referencing it in a comment so the flakiness isn't silently reintroduced/forgotten.
Frames present one per vsync, but the UI thread reaches each frame with milliseconds of jitter, and motion evaluated against that raw instant turns it into v·Δt of position error. Phase-lock the frame times onto the median frame interval: advance by whole frames, correct a sixteenth of the phase error per frame, cap the lead at half a period, and never step back. Idle gaps re-anchor on the real clock. This compensates for sampling the time on the UI thread after the dispatcher hop; native vsync timestamps (#24963) would make it a fallback for hosts that have none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014tR98SJixdXBRVuRo69Pek
Restores the grid tests split out of #24735: even steps under jitter, no drift, a dropped frame stepping twice, re-anchoring after idle, never stepping back, and no freeze after a hitch or a slower motion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014tR98SJixdXBRVuRo69Pek
Moved here from the WinUI wheel curve PR: it asserts frame-to-frame smoothness, which only holds once frame times sit on the display interval. On raw frame times two frames can land almost together, and the step between them collapses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014tR98SJixdXBRVuRo69Pek
f7277e1 to
fe26434
Compare
43adc32 to
a36ed5a
Compare
|
Claude finished @MartinZikmund's task in 1m 12s —— View job Review complete ✅
Verdict: fix-first (unchanged)HEAD is still
No new issues found. Branch · |
GitHub Issue: Part of #23985, related to #24963
PR Type:
✨ Feature
What changed? 🚀
Before
#24735 gives every frame one timestamp, but that timestamp is the raw clock, read on the UI thread after the native frame callback has been marshalled through the dispatcher. It carries a few ms of random queuing delay, and motion evaluated against it (a scroll at speed v) turns that into v·Δt of uneven stepping.
After
FrameClock.NextTimestampphase-locks the frame times to the display interval it estimates from the median of the last 32 frame intervals:This was part of #24735 and was split out so the stack can land without it, and so it can be reworked into a timestamp-driven solution.
Known limitations (why this is a stepping stone)
The other frameworks don't need this, because they take the frame time from the vsync signal itself (WinUI's compositor scheduler thread, Flutter's
handleBeginFrame, Android'sChoreographer, browserrequestAnimationFrame). Here the grid has to be rebuilt from samples, which misbehaves when the refresh rate changes. A simulation of this exact algorithm with ±1.5 ms of jitter, where raw sampling gives about 1.2 ms RMS step error:So on ProMotion and variable-refresh displays it is worse than the raw clock for up to 0.8 s after each rate change. It is also meaningless on hosts that aren't paced to vsync (the Linux framebuffer measures sub-ms intervals).
Plan: rework this PR along #24963. Hosts that have a vsync timestamp pass it into
OnNativePlatformFrameRequestedand bypass the grid: iOSCADisplayLink, WASMrequestAnimationFrameand Linux DRM page-flip already receive one and drop it. Snapping stays only as the fallback for hosts that have none, and gets a fast reset when the intervals change.PR Checklist ✅
Screenshots Compare Test Runresults.Tests:
Given_Compositorframe-clock tests: even steps under jitter, no drift, a dropped frame stepping twice, re-anchoring after idle, never stepping back, no freeze after a hitch or after a slower motion.Given_ScrollView.When_Wheel_Spins_Then_Velocity_Never_Drops_At_A_Notch, moved here from fix(scrollview): Scroll by wheel like WinUI's ScrollView #24738 because it asserts frame-to-frame smoothness. Known flaky: it failed 2/5 locally without the grid and 1/11 with it, always as a near-zero step at a notch ("slowed from 1.58 to 0.01 DIP/frame"). The suspected cause is in the stack, not the grid: the new inertia handler's first tick returns early whileInteractionTracker.State.HasEnteredis false, so one frame doesn't move. To be investigated.Local runs (Skia Win32):
Given_Compositor+Given_CompositionTarget+Given_ScrollView35/36, with the one failure being the flake above.🤖 Generated with Claude Code
https://claude.ai/code/session_014tR98SJixdXBRVuRo69Pek