fix(h264): start the blank detector's clocks when EGFX goes ready - #196
Merged
Merged
Conversation
The per-connection EGFX context is built when the server attaches channels, at TCP accept, before TLS and authentication. Its clocks started then, so a slow login counted as connected time. Seen live on the Mac mini (2026-10-01): Windows App logins took 7-11 s from accept to auth, waiting on the client's own prompt. By the first frame, blank recovery's 3 s arm delay and 4 s wall-clock threshold had already run out, so it ran a deactivation-reactivation 0.2-0.4 s after that frame, on sessions that hadn't yet had a chance to present. The same flaw would hit slow NLA over a distant link. It also started the adaptive controller's 3 s no-ack grace early. `ConnectionContext::start_clocks` restarts every connection-start baseline on the first transition to ready. It does not run on a mid-session capabilities re-advertise, which would delay detection, and a declined no-AVC client never goes ready. Nothing ships or is acknowledged before ready, so this is exactly "the connection starts now". This has been present since before v0.9.6; it is not a pin-bump regression. Tests drive the real on_ready handler against a context built through build_server_with_handle: - a_slow_login_does_not_count_toward_the_connection_clocks - a_mid_session_ready_keeps_the_connection_clocks - a_declined_client_does_not_start_the_clocks Each was checked against a mutation, and each mutation failed at least one test: removing the restart, removing the first-ready guard, and leaving one clock out of start_clocks.
clintcan
added a commit
that referenced
this pull request
Oct 2, 2026
The Mac mini live tests so far: the passes, the two fixes that landed on main (#195, #196), and what's still to do. Also a Deferred entry for the intermittent client-to-mini clipboard gap: it happens on v0.9.6 too, so it isn't a bump regression. It includes the test gotchas behind yesterday's false reading.
antonmos
added a commit
to antonmos/macrdp
that referenced
this pull request
Oct 3, 2026
Upstream clintcan#196 (start_clocks) fixes the same bug — the blank detector's connect-time clocks starting at channel build, before the handshake — by restarting every connection-start baseline, including `epoch`, when EGFX goes ready. That is a superset of this branch's `ready_at`, so the conflicting files take upstream's version and this branch no longer carries a change of its own. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Blank recovery could run a needless deactivation-reactivation right after any slow login.
Cause. The per-connection EGFX context is built when the server attaches channels, which happens at TCP accept, before TLS and authentication. Its clocks started then, so login time counted as connected time.
Seen live on the Mac mini (2026-10-01). Windows App logins took 7–11 s from accept to auth, waiting on the client's own prompt. By the first frame, the detector's 3 s arm delay and 4 s wall-clock threshold had already run out. It then ran a reactivation 0.2–0.4 s after that frame, on sessions that hadn't yet had a chance to present. The same flaw would hit slow NLA over a distant link. It also started the adaptive controller's 3 s no-ack grace early.
Fix.
ConnectionContext::start_clocksrestarts every connection-start baseline on the first transition to ready. Nothing ships or is acknowledged before ready, so this is exactly "the connection starts now". It doesn't run on a mid-session capabilities re-advertise, and a declined no-AVC client never goes ready.This has been present since before v0.9.6; it isn't a regression from the IronRDP pin bump (#194).
Tests drive the real
on_readyhandler against a context built throughbuild_server_with_handle:a_slow_login_does_not_count_toward_the_connection_clocksa_mid_session_ready_keeps_the_connection_clocksa_declined_client_does_not_start_the_clocksEach was checked against a mutation, and each mutation failed at least one test: removing the restart, removing the first-ready guard, and leaving one clock out of
start_clocks.Checked: fmt (stable and nightly), clippy
-D warnings, 265 tests (also on 1.95.0 with-D warnings), and a release build.Not yet verified live: that a slow login on the mini no longer triggers a reactivation. I'll check that with the pin-bump live tests.