Skip to content

fix(audio): preserve explicit pause authority across clock writes - #902

Open
jonluca wants to merge 7 commits into
superuser404notfound:mainfrom
jonluca:fix/software-pause-clock-authority
Open

jonluca wants to merge 7 commits into
superuser404notfound:mainfrom
jonluca:fix/software-pause-clock-authority

Conversation

@jonluca

@jonluca jonluca commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

A software-host pause can still be followed by a decoder arm or rebuffer release that already read the old playback intent. #867 reconciles queued arm callbacks, but the stale worker can restart the clock before that callback runs. Latch explicit transport authority inside AudioOutput under the same lock as every clock write, so these stale positive-rate commands cannot start it at all.

A paused seek still updates its anchor at zero rate, and explicit play enables normal preroll again. Enabling transport does not advance the clock-command revision: if play arrives after the feeder reads that revision but before it publishes the armed latch, the queued arm correction still supplies the initial start. Internal buffering holds leave transport intent intact. Progress-aware preroll and clock-hold events remain unchanged, including a cancelled event when an explicit pause retires a pending start. Stop still retires the enqueue epoch and flushes queued audio, while avoiding a clock reset when the output is already paused.

Stacked on #867, with its exact tip 3da8803 preserved as an ancestor. Upstream main c728c71 is also merged. The prerequisite merge changed no files, and the main merge required only a CHANGELOG resolution; the complete source, tests and documentation patch is unchanged. The prerequisite changes can be dropped after #867 merges.

Validation on macOS 27, Xcode 27 / Swift 6.4:

  • Strengthened the pre-arm-pause regression: it fails on fix(swhost): serialize clock-arm correction with transport changes #867 alone, observing rate 1 after both a stale arm and a stale rebuffer release; it passes with this change.
  • A new deterministic regression reproduces play between the feeder's revision read and armed-latch publication: the initial PR head leaves the clock stopped; the corrected head starts it.
  • 68 tests across 7 suites passed with warnings as errors in an isolated package containing the exact complete engine sources and selected existing test files. Coverage includes clock revision ordering, paused seeks, explicit play, preroll cancellation/progress, clock-hold events, renderer resume and end-of-media behavior.
  • swift build --target AetherEngine --force-resolved-versions -Xswiftc -warnings-as-errors passed.
  • Documentation links and git diff --check passed.

The isolated test manifest excludes the diagnostic CLI and unrelated tests with pre-existing compiler warnings. Xcode 26 and physical-device playback were not available for this validation.

Copilot AI balanced review requested due to automatic review settings October 26, 2026 14:33

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review

The PR latches explicit pause inside AudioOutput, checked under the same lock as every clock write, so a stale decoder arm, rebuffer release or preroll completion cannot restart a clock the viewer paused. It also carries the #867 commit it is stacked on. The direction is right: the fix is in the engine, it keeps the #107 rule (no rate change on an un-anchored synchronizer, because setPlaybackEnabled only stops a clock whose rate is already non-zero), and it does not ask hosts to compensate.

I traced these paths and found no defect:

  • play() re-enables the latch before RendererClockResume.onPlay.
  • Paused seeks still anchor at rate 0.
  • A host setRate while paused stays refused, which matches the engine's #730 handling.
  • AudioPlaybackHost shares AudioOutput but never disables the latch, so only its stop() changes, and that output is discarded right after.

Findings

  • Should fix: the new latch is a rule a later contributor will trip over: while paused, every positive-rate write without a revision is dropped silently, and only play() lifts that. The architecture paragraph only describes #867's revision check (docs/architecture.md:124).
  • Should fix: the diff includes all of #867, including its CHANGELOG entry. Once #867 merges, this branch needs a rebase so only the final commit and its own entry remain. Otherwise the CHANGELOG gets two entries for one change.
  • Nit: a viewer pause on the software route no longer writes a [AudioOutput] pause at t= line, and a refused positive write leaves no trace in the log either (Sources/AetherEngine/Audio/AudioOutput.swift:219).

Checklist: CHANGELOG yes, docs yes (partial, see above), tests yes, traits both ways n.a.

Needs a device run: pause and resume around first-frame arming and around a live or DVR rebuffer on an Apple TV, to confirm video no longer catches up on resume. The tests do not cover how the synchronizer behaves when a stopped output skips its .zero reset in stop().

Thanks for the careful write-up and the deterministic regression tests.

Automated review by Claude of 6ab1e45. It reads the code, it does not build or run it; a maintainer reviews and merges.

defer { lock.unlock() }
playbackEnabled = enabled
_clockCommandRevision &+= 1
guard !enabled else { return }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: SoftwarePlaybackHost.pause() used to call audioOutput?.pause(), which logged [AudioOutput] pause at t=.... It now calls setPlaybackEnabled(false), which logs nothing. Only the feeder's rebuffer pauses still produce that line, so a field log can no longer show where the clock stopped for a viewer pause. Positive writes that the latch refuses in setRate and seekClock are also silent, and that is exactly what someone debugging "play did nothing" would want to see. Suggestion: capture the source time under the lock and emit one line after unlocking (for example [AudioOutput] playback disabled at t=...). Optionally log a refused positive rate once per disable.

Comment thread docs/architecture.md
end-of-media tail playback to its owner unless the host explicitly paused. It also carries the output's clock-command revision:
`AudioOutput` compares that revision and applies the rate under its existing lock, so a later
rebuffer pause, seek or rate command supersedes the queued correction. Preroll completion belongs
to its original command and keeps the revision. Paused intent cancels a pending preroll start,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph explains #867's revision check, but not the rule this PR's own commit adds. While the host is paused, AudioOutput refuses every positive rate (stale or new, with or without a revision, from setRate, seekClock and preroll completion), and only setPlaybackEnabled(true) from play() lifts the latch. Feeder rebuffer pauses do not touch it. Anyone who later adds a clock start on the software route will find it silently dropped while paused. One or two sentences here naming setPlaybackEnabled and who calls it would prevent that.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Enabling playback can invalidate the pending arm correction while skipping its own resume command, leaving playback active with the clock stopped.

1 open issue
What changed in this PR

This PR strengthens software-playback pause handling by giving AudioOutput explicit transport authority over clock writes, building on #867’s queued clock-arm reconciliation.

Changes:

  • Blocks stale decoder, rebuffer and preroll starts after explicit pause.
  • Preserves paused-seek anchors and avoids resetting an already-paused clock during stop.
  • Adds regression coverage and documents clock-command ordering.
File Description
Tests/​AetherEngineTests/​VideoPrerollTests.swift Tests pause authority, resumed preroll and paused-stop behavior.
Tests/​AetherEngineTests/​Sodalite104PausedFirstFrameTests.swift Tests clock-arm ordering against transport and newer clock commands.
Sources/​AetherEngine/​Native/​SoftwarePlaybackHost.swift Connects transport intent to output authority and serializes arm reconciliation.
Sources/​AetherEngine/​Audio/​AudioOutput.swift Adds locked playback authority and clock-command revision checks.
docs/​architecture.md Documents arm reconciliation and revision ordering.
CHANGELOG.md Records explicit-pause and clock-arm fixes.

Review effort: Balanced

Comment on lines +218 to +219
_clockCommandRevision &+= 1
guard !enabled else { return }

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review

I re-checked the PR at 24b64f3. The direction is still right: explicit pause authority lives in AudioOutput under the clock lock, and the #107 rule (no rate change on an un-anchored synchronizer) still holds.

Since the last review

  • Fixed: docs/architecture.md now has a paragraph on setPlaybackEnabled. It covers who calls it, what it refuses, and the fact that only play() lifts it.
  • Fixed: setPlaybackEnabled(false) now logs [AudioOutput] pause at t=... (host transport disabled), so a viewer pause shows up in field logs again.
  • Fixed: enabling playback no longer advances the clock-command revision. A play that lands between the feeder's revision read and its clockArmed publication keeps the queued correction valid. playBetweenRevisionReadAndArmedLatch covers this case. I traced it end to end: arm seekClock at rate 0, revision read, play() skips setRate because the clock is unarmed, then the main-actor correction passes the revision check and starts the clock.

Findings

  • Should fix (still open): the branch still carries all of #867, including its CHANGELOG entry ("A software playback clock stays stopped when first-frame arming overlaps a pause"). #867 is not on main yet. Rebase after it merges so only this PR's commit and its own entry remain.

Checklist: CHANGELOG yes, docs yes, tests yes, traits both ways n.a.

Needs a device run: on an Apple TV, pause and resume around first-frame arming and around a live or DVR rebuffer, and check that video no longer catches up on resume. Also check that skipping the .zero reset in stop() for an already-paused clock does not affect the next load.

Thanks for turning the earlier comments around so quickly, with a regression test for each.

Automated review by Claude of 24b64f3. It reads the code, it does not build or run it; a maintainer reviews and merges.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review

I checked the new head, 5ec5c13. It merges main and #867's tip, with only a CHANGELOG conflict resolved, and nothing my last review covered has changed. One point is still open: #867 is not on main yet, so this PR still carries it and its CHANGELOG entry. Rebase once #867 merges.

Automated review by Quint (Claude) of 5ec5c13. It reads the code, it does not build or run it; a maintainer reviews and merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants