feat(android): Start frames on vsync with its time by MartinZikmund · Pull Request #24981 · unoplatform/uno

MartinZikmund · 2026-10-05T10:34:06Z

GitHub Issue: Part of #24963

Based on #24968 (dev/mazi/scroll-vsync-timestamps). Review only this PR's commits. Completes the Android step of #24963.

PR Type:

✨ Feature

What changed? 🚀

Before

Vulkan view: started frames on a Choreographer vsync, but dropped frameTimeNanos.
Canvas (GL) view: a GLSurfaceView that drew as soon as it was asked, held back only by eglSwapBuffers back-pressure.
WebGPU view: its render thread woke as soon as it was asked, plus every 100 ms while idle.
Frame times: none of the views passed a vsync time.

After

One frame source. All three views go through ChoreographerFramePacer:
- repeated render requests share one callback, and nothing is posted while idle;
- DoFrame stores the vsync time and wakes the render thread;
- the frame passes that time to OnNativePlatformFrameRequested(..., vsyncTimestamp:).
Time conversion: frameTimeNanos is converted through its age (JavaSystem.NanoTime() - frameTimeNanos), the same way the other hosts do it.
Stale times: a stored time older than 100 ms is dropped. After the app comes back from the background, the first frame then isn't dated before the gap.
WebGPU idle: the WebGPU render thread no longer wakes every 100 ms while idle.
Test: When_Host_Reports_Vsync_Then_Frame_Time_Is_The_Vsync now runs on SkiaAndroid. It checks that frame intervals sit within 0.3 ms of whole multiples of the median. The old aheadOfClock <= 0 check could never fail, because frame times are clamped to now.

Trade-off: the Canvas view now waits for the next vsync instead of drawing at once, the way Android's own views do. This can add up to a frame to one-off redraws after input. Starting a frame at once when the current vsync interval has none yet is a follow-up. It will reuse the shared frame scheduler from the macOS PR.

Validation

Compile: net10.0-android SamplesApp.
Runtime, Android 35 arm64 emulator: Given_CompositionTarget + Given_Compositor + Given_InteractionTracker on the Canvas view (software and GLES) and the Vulkan view.
- The vsync test fails on the base branch and passes with this PR.
- When_Rendering_Then_Layout_Of_The_Tick_Is_Current fails the same way before and after.
- Emulator timing is not representative of devices.
WebGPU view: code review only; it can't be selected on the emulator.
Not run: the strengthened test on WASM and UIKit, and a background/resume regression test (the runtime-test harness can't background the app).

PR Checklist ✅

🧪 Added Runtime tests, UI tests, or a manual test sample (for bug fixes / features, if applicable)
📚 Docs have been added/updated following the documentation template (for bug fixes / features)
🖼️ Validated PR Screenshots Compare Test Run results.
❗ Contains NO breaking changes
👀 Reviewed 2 other open pull requests (optional but appreciated!)

🤖 Generated with Claude Code

https://claude.ai/code/session_01JmE5zrbCJiMAv1E5yChjQp

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JmE5zrbCJiMAv1E5yChjQp

github-actions · 2026-10-05T10:34:24Z

Claude finished @MartinZikmund's task in 4m 6s —— View job

Review complete ✅

Verdict: ship — the change is a solid, self-contained improvement (one shared ChoreographerFramePacer driving all three Android render views, vsync time threaded through to OnNativePlatformFrameRequested), consistent with the pattern already established for macOS/WASM/UIKit in the base branch. No blocking issues found.

Findings posted inline (2, both low severity / nitpicks):

UnoWebGpuView.cs — Dispose(bool) disposes _framePacer before joining the render thread, unlike UnoVulkanView.cs which disposes it only after the thread is guaranteed stopped. Likely benign today (no JNI call in TakeVsyncTimestamp()), but an inconsistent/riskier ordering worth aligning.
Given_CompositionTarget.cs — the new vsync-spacing assertion divides by the median interval (period); a duplicate/zero timestamp landing at the median would silently produce NaN instead of a clear failure message.

Cross-cutting observations (not inline, don't map to a changed line):

UnoVulkanView's render loop still uses _renderEvent.Wait(TimeSpan.FromMilliseconds(100)) (periodic wake even while idle) — the same pattern this PR explicitly removed from UnoWebGpuView ("WebGPU idle" fix). Pre-existing code, untouched by this diff, but worth a follow-up for consistency since the same request/vsync plumbing now backs both views.
Thread-safety of ChoreographerFramePacer itself checked carefully (the Interlocked-guarded _vsyncTimestamp handoff between the main/Choreographer thread and each view's render thread, plus the MaxVsyncAge staleness guard) — no race found; DoFrame/RequestFrame are correctly serialized by the Choreographer's own looper.
Conventional Commits, file placement (Android-only project, no suffix needed), events (no event Action introduced), and comment style all match AGENTS.md/.claude/rules.

Not independently re-validated (relying on the PR description's own validation section): the Android-emulator runtime pass and the "fails before / passes after" vsync test behavior — I did not have an Android emulator available in this review environment.
· Branch: dev/mazi/vsync-android → dev/mazi/scroll-vsync-timestamps

mergify · 2026-10-05T10:34:42Z

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

Queue this pull request

Copilot

Copilot review overview

🟡 Changes recommended

The strengthened test has an insufficient timeout and incorrectly assumes a fixed refresh cadence.

Review effort: Balanced
Findings: 2 · 1

Open (3)

Increase timeout for 40-frame sample collection · New
Support variable refresh-rate regimes in cadence validation · New
Add GitHubWorkItem metadata for issue #24963 test · New

What changed in this PR

Adds Android Choreographer-based frame pacing and forwards native vsync timestamps into the composition frame clock.

Changes:

Shares frame pacing across Canvas, Vulkan, and WebGPU renderers.
Removes WebGPU’s periodic idle wake-up.
Extends and strengthens Android vsync runtime coverage.

File	Description
`Given_CompositionTarget.cs`	Expands vsync timing validation to Android.
`UnoWebGpuView.cs`	Adds paced rendering and removes idle polling.
`UnoVulkanView.cs`	Forwards Choreographer timestamps.
`UnoCanvasView.cs`	Gates GL rendering on Choreographer callbacks.
`ChoreographerFramePacer.cs`	Converts, stores, and expires vsync timestamps.

 		try
 		{
-			await TestServices.WindowHelper.WaitFor(() => ticks >= 10, message: "the driver should keep ticking");
+			await TestServices.WindowHelper.WaitFor(() => timestamps.Count >= 40, message: "the driver should keep ticking");


+		var intervals = timestamps.Skip(5).Zip(timestamps.Skip(6), (a, b) => b - a).ToArray();
+		var period = intervals.OrderBy(i => i).ElementAt(intervals.Length / 2);
+		var tolerance = TimeSpan.TicksPerMillisecond * 0.3;
+		var worst = intervals.Max(i => Math.Abs(i - Math.Max(1, Math.Round(i / (double)period)) * period));


 	[TestMethod]
 	[RunsOnUIThread]
-	[PlatformCondition(ConditionMode.Include, RuntimeTestPlatforms.SkiaWasm | RuntimeTestPlatforms.SkiaUIKit)]
+	[PlatformCondition(ConditionMode.Include, RuntimeTestPlatforms.SkiaWasm | RuntimeTestPlatforms.SkiaUIKit | RuntimeTestPlatforms.SkiaAndroid)]


github-actions · 2026-10-05T10:38:21Z

 		if (disposing)
 		{
 			_disposed = true;
+			_framePacer.Dispose();


Disposal ordering here differs from UnoVulkanView.Dispose(bool) (lines 361-385 of that file), which disposes _framePacer only after _renderThread?.Join(...) has returned — i.e. only once the render thread is guaranteed to have stopped calling into it.

Here _framePacer.Dispose() runs before _renderEvent.Set()/_renderThread?.Join(...), so the render thread can still be inside RenderFrame() → _framePacer.TakeVsyncTimestamp() while the pacer is being disposed (Choreographer.RemoveFrameCallback on the Java peer). It likely doesn't crash today since TakeVsyncTimestamp() is pure C# state (no JNI call), but it's a latent inconsistency — a future change to ChoreographerFramePacer.Dispose() that touches more state would introduce a real race here. Consider moving _framePacer.Dispose() after the Join call to mirror the (correct) ordering in UnoVulkanView.

Fix this →

github-actions · 2026-10-05T10:38:30Z

+		var intervals = timestamps.Skip(5).Zip(timestamps.Skip(6), (a, b) => b - a).ToArray();
+		var period = intervals.OrderBy(i => i).ElementAt(intervals.Length / 2);
+		var tolerance = TimeSpan.TicksPerMillisecond * 0.3;
+		var worst = intervals.Max(i => Math.Abs(i - Math.Max(1, Math.Round(i / (double)period)) * period));


period is the median interval (line 241). If two consecutive vsync timestamps ever come back equal (e.g. a duplicate/deduped tick), that 0 has a good chance of landing at the median index, making i / (double)period divide by zero → NaN → Math.Max(1, NaN) is NaN (Math.Max propagates NaN) → worst becomes NaN, and NaN <= tolerance is false. The test would then fail with a NaN-filled message instead of a clear diagnostic, making a real flake hard to distinguish from this edge case.

Not a correctness bug in the production code, just a robustness gap in the new assertion — worth an explicit guard/assert that period > 0 before using it as a divisor, so a degenerate case fails loudly with its own message rather than producing NaN noise.

Fix this →

MartinZikmund and others added 4 commits October 5, 2026 10:47

feat(android): Start frames on vsync with its time

1867f19

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JmE5zrbCJiMAv1E5yChjQp

test(android): Cover host vsync time on Android

dadfd23

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JmE5zrbCJiMAv1E5yChjQp

fix(android): Drop stale vsync times, idle WebGPU wait

487bdb3

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JmE5zrbCJiMAv1E5yChjQp

test(android): Assert frame times are even vsyncs

573fcf4

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JmE5zrbCJiMAv1E5yChjQp

Copilot AI balanced review requested due to automatic review settings October 5, 2026 10:34

github-actions Bot added platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform area/skia ✏️ Categorizes an issue or PR as relevant to Skia labels Oct 5, 2026

github-actions Bot assigned MartinZikmund Oct 5, 2026

Copilot started reviewing on behalf of MartinZikmund October 5, 2026 10:34 View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

github-actions Bot reviewed Oct 5, 2026

View reviewed changes

Navigation Menu

feat(android): Start frames on vsync with its time - #24981

feat(android): Start frames on vsync with its time#24981
MartinZikmund wants to merge 4 commits into
dev/mazi/scroll-vsync-timestampsfrom
dev/mazi/vsync-android

MartinZikmund commented Oct 5, 2026

Uh oh!

github-actions Bot commented Oct 5, 2026 •

edited

Loading

Uh oh!

mergify Bot commented Oct 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot Oct 5, 2026

Uh oh!

github-actions Bot Oct 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

MartinZikmund commented Oct 5, 2026

PR Type:

What changed? 🚀

Before

After

Validation

PR Checklist ✅

Uh oh!

github-actions Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete ✅

Uh oh!

mergify Bot commented Oct 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

github-actions Bot Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Oct 5, 2026 •

edited

Loading