fix: default sync timing to the wall clock (restore 2.1.0 multi-room sync)#52
Merged
Conversation
…sync) 2.1.0 synchronized correctly with no offset because it timed sync against the wall clock. 2.2.x switched the timing source to the WASAPI device clock (d18dd27), which reads the DAC-rendered position — permanently lagging the buffer read pointer by the ~100ms WASAPI output prefill. That constant -100ms error pushes the player ~100ms off the shared schedule, out of sync with other players, for no upside: the DAC clock reads a 1.0000 ratio on the hardware tested (agrees with the system clock = no drift benefit), and the #33 stutter was fixed by the concealment work in #46, not the clock. Default UseDeviceClock to false (wall clock) in appsettings, the config read, and the player constructor. The device clock stays available as an opt-in for genuinely divergent DAC clocks, keeping the existing runtime wall-clock fallback. Adds MultiRoomSyncAlignmentTests: drives the real buffer + SyncCorrectionCalculator + SyncCorrectedSampleSource through a drift-free session and asserts the player holds the server schedule (sync error ~0) with no startup offset, and drifts ~100ms off schedule when an uncompensated prefill leaks in (the device-clock failure mode). Refs #33.
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
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.
The regression
2.1.0 synchronized with other players correctly, no offset needed. 2.2.x is ~100ms out of sync. Bisecting the sync pipeline across the whole 2.1.0→2.2.x range (both repos) found a single change to when audio plays:
HighPrecisionTimer.Shared.GetCurrentTimeMicroseconds()DeviceClockAnchor(commitd18dd27)Nothing in the SDK 8.0.0→9.0.4 jump moved playback timing — external correction and the sync formula already existed in 8.0.0.
Why the device clock desyncs
The device clock reads the DAC-rendered position, which permanently lags the samples read from our buffer by the ~100ms WASAPI output prefill. So
elapsed − samplesRead = −100msforever — a constant error the corrector fights by inserting samples, shoving the player off the shared schedule. The wall clock advances 1:1 with real playback, so the startup prefill gulp self-corrects and steady-state error is ~0. No offset needed, exactly like 2.1.0.It also bought us nothing: the DAC clock reports a 1.0000 ratio on the hardware tested (agrees with the system clock → zero drift benefit), and the #33 stutter was actually fixed by the concealment + output-driven feeding in #46 — not the clock.
Change
Default
Audio:SyncCorrection:UseDeviceClocktofalse(wall clock) inappsettings.json, the config read, and the player constructor. The device clock stays available as an opt-in for genuinely divergent DAC clocks, with the existing automatic runtime wall-clock fallback intact.Tests
New
MultiRoomSyncAlignmentTestsdrives the realTimedAudioBuffer+SyncCorrectionCalculator+SyncCorrectedSampleSourcethrough a drift-free session and asserts:Full suite green (97/97).
Verify
Merging publishes a dev build. The decisive check: this player back in sync with other Sendspin players with
StaticDelayat 0, as in 2.1.0.Refs #33.