Repository navigation
fix(software-host): recover from system flushes, failed renderers and refused session rebuilds, with three small engine fixes - #912
Conversation
… refused session rebuilds, with three small engine fixes Flush and failure recovery. When the system flushes or fails the host's video queue or its audio renderer, the host flushes the failed queue and the video decoder and resumes at the next keyframe while the audio keeps playing, seeking to the playhead only when that keyframe is further away than the queued audio. A video failure while only audio plays in the background is paid on the foreground return; a live source with no rewind ring is only flushed; a recovery that did not help is not repeated until a frame gets through; the decoder is fed again only from the next keyframe; a failing audio renderer is flushed at most three times in ten seconds. Refused session rebuilds. When VideoToolbox refuses to rebuild an invalidated hardware session, the host retries at each keyframe fed while the session is lost (at least two seconds apart), up to four times, then publishes a playback failure. In-band parameter-set scan budget. The scan that rebuilds an empty hvcC from in-band VPS/SPS/PPS also stops at 16 MiB of input, with the same ceiling armed on the demuxer input. LoadOptions.initialSubtitleTrackID. A load can name the subtitle track to activate by id, ahead of preferredSubtitleLanguages; an id that names no track falls through to the language preference. selectChapter on a disc title no longer applies the container start twice on the software route, and no longer adds the playlist shift on the native route. Tests. Suites that set the origin budget's quiet-period cap hold a counted pin so the cap comes back, and a new test pins that a paused software session and its remote come back after a background teardown.
ed02a77 to
d46a283
Compare
…ving up on a wall-clock budget The wake-restore, flush-recovery and chapter-seek tests waited for positive events with a deadline of their own (a Date() loop whose try? sleep swallowed the time limit's cancellation, waitFor(upTo: 10 s), a 1 s task poll). They now use the unbounded waitFor and the suite time limit catches a real hang; the chapter-seek wiring suite gets a one-minute limit.
![quint-aether[bot]](https://avatars.githubusercontent.com/in/5482133?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review
The PR teaches the software host to recover when the system flushes or fails its video queue or audio renderer, and to retry a hardware-session rebuild that VideoToolbox refused. It also bundles three engine fixes: a byte budget on the in-band hvcC scan, LoadOptions.initialSubtitleTrackID, and the selectChapter axis fix. The direction is right: the recovery lives in the engine, and nothing asks hosts to compensate.
I checked the selectChapter change against seek(to:) on both routes. On a native disc title, sourcePresentationOrigin equals the playlist shift, so the 0-based target is already what seek(to:) expects. On the software route, subtracting the session zero matches SWClockAnchorPolicy.sourceSeconds. The keyframe ledger records PTS after the discontinuity fold, so the ledger and the clock use the same axis.
Findings
- Should fix: After the fourth refused rebuild, the host publishes a failure. That moves the engine to
.error. The code comment says a later foreground return "still gets its own try", butfailureis never cleared. A rebuild that later succeeds leaves the session errored, and flush recovery is disabled whilefailureis set. The retries also run whenever the host is not in audio-only mode, which includes a retained paused session in the background, where VideoToolbox refuses by design. (Sources/AetherEngine/Native/SoftwarePlaybackHost.swift:1902) - Should fix:
FlushRecoveryTestsusesTask.sleepsix times to check that nothing happened.CONTRIBUTING.mdforbids sleeps in tests. These checks can be made exact because a reposition setsflushRecoveryPendingsynchronously. (Tests/AetherEngineTests/FlushRecoveryTests.swift:582) - Should fix: One commit carries six unrelated changes: software-host recovery, the rebuild retry, the hvcC scan budget, a new public
LoadOptionsfield, the chapter axis fix and test hygiene. The subtitle option, the chapter fix and the quiet-cap pin each stand on their own. Splitting them would make review and any later revert or bisect much cheaper, and would leave the #703 overlap to the chapter PR alone. This is the maintainer's call. - Nit: A DVR session's live feeder has no keyframe ledger, so
videoGapSecondsis always nil there. Every video flush on a DVR session will therefore be a reposition. That may be the intended conservative choice; one sentence inFlushRecoveryPolicysaying so would help. - Nit: The
selectChapterCHANGELOG entry is a single unwrapped line. The entries around it are wrapped.
Checklist: CHANGELOG yes, docs yes (docs/api.md row for initialSubtitleTrackID, source map in docs/architecture.md), tests yes, traits both ways n.a. (no trait-guarded code; the PR reports a DualLayerDolbyVision run)
Needs a device run: Real requiresFlushToResumeDecoding and .failed video queues on tvOS, iOS and macOS (background and foreground, lock and unlock). An audio renderer failure on an HDMI or AirPlay route change. A refused rebuild after a screen lock, including a retained paused session in the background, to confirm the retries cannot reach give-up there. selectChapter on a disc title that starts past 2 s, on both routes. The PR's own device list covers these well.
Thank you for a careful and well-documented PR, and for the thorough device checklist.
Automated review by Quint (Claude) of 92b8ae8. It reads the code, it does not build or run it; a maintainer reviews and merges.
| category: .swPlayback) | ||
| // `videoSessionRecoveryOwed` stays set on purpose: the failure is published, but a later | ||
| // foreground return still gets its own try (and a fresh budget). | ||
| if failure == nil { |
There was a problem hiding this comment.
After four refusals this sets failure. wireCommonHostSinks turns that into publishError, which moves the engine to .error. The comment above says a later foreground return "still gets its own try", but nothing clears failure. If exitBackgroundAudioOnly -> recoverVideoSession then succeeds and seeks, the engine stays in .error. flushRecoveryState also reports hasSource == false while failure is set, so every later flush trigger is ignored.
The retries are gated only on !backgroundAudioOnly. A paused session retained in the background (#870) still feeds keyframes, and VideoToolbox refuses session creation in the background by design. Could those keyframes reach .giveUp and publish an error while the user is away, when the foreground return would have recovered?
Suggested fix: (a) do not count attempts, or give up, while the app is not active. (b) Either drop the comment's promise or make it true by not publishing a terminal failure that a later successful rebuild cannot clear.
| #expect(decoder.feedEpoch > 0) | ||
| // The floor restarts at the playhead, not at whatever frame arrives first. | ||
| #expect(decoder.skipUntilPTS.map { abs($0.seconds - 20) < 0.01 } == true) | ||
| try await Task.sleep(for: .milliseconds(50)) |
There was a problem hiding this comment.
Task.sleep is used here and at 714, 744, 767, 783 and 803 to check that no reposition happened. CONTRIBUTING.md says to wait with waitFor and never with a sleep. A 50 ms sleep also proves nothing on an oversubscribed runner: the spawned seek Task may simply not have run yet.
For the reposition cases this can be made exact without waiting. handleFlushTrigger sets flushRecoveryPending = true synchronously before it spawns the seek Task. Asserting !host.recoveryPendingForTesting right after the call, together with the returned action, shows that no seek was scheduled.
The AE#549 test at 783 posts a notification that hops queues. There, wait with waitFor for a positive witness (for example the AudioOutput flush line or its counter), then check that flushRecoveryLog is empty.
…the session in error A rebuild VideoToolbox refuses is retried at each keyframe in the foreground. After the fourth refusal the host published a playback failure, which moves the engine to .error. The engine keeps that state until the next load, so a rebuild that succeeded later left the session errored, and flush recovery stayed off while the failure was set. The retries also ran while the app was in the background (a retained paused session), where a refusal is expected. The host no longer publishes that failure: after the budget is spent it waits for the next foreground return, which starts a fresh budget. Nothing is spent while the app is in the background. The engine reports background and foreground to the host. Also: the flush recovery tests check exact state after each synchronous trigger instead of sleeping, the AE#549 case waits on the audio output's own count, FlushRecoveryPolicy says why a DVR session always repositions, and the selectChapter changelog entry is wrapped.
…ead of giving up A refused rebuild now keeps being retried at keyframes: four quick attempts, then one every 30 seconds, so a session the system gives back later recovers without a reload. Nothing is retried while the app is in the background without picture-in-picture, where VideoToolbox refuses by design. The engine tells the host when the picture is off screen, and a foreground return starts a fresh quick budget. No failure is published for a refused rebuild. The give-up that stopped the retries froze the picture for good on platforms that never return to the foreground (macOS), with no error shown. The give-up also did not publish a failure: the engine's .error state is terminal until the next load. The DVR flush-recovery doc now names the rules that can degrade a reposition to a plain flush.
There was a problem hiding this comment.
Automated review
I re-reviewed head fe8b453. Both Should-fix findings from the last review are fixed. The direction still holds: the recovery lives in the engine, and nothing asks hosts to compensate.
Since the last review
- Fixed: a refused rebuild no longer publishes a failure. Once the four quick attempts are spent, the host keeps retrying every 30 s, so
.errorcan no longer stick after a later rebuild succeeds. Retries are gated on a newappInBackgroundflag (background with no PiP). That flag also covers the retained paused session from #870, and a foreground return resets the budget. The code comment and the CHANGELOG now match the behaviour. - Fixed: the
Task.sleepcalls inFlushRecoveryTestsare gone. - Fixed: the
selectChapterCHANGELOG entry is now wrapped. - Still open, and the maintainer's call: one commit carries six unrelated changes.
Findings
- Nit: A software host built while the app is already in the background starts with
appInBackground == false. The engine only reports the flag when the background, foreground or PiP state changes. So a reload or title switch in the background can still spend retries there. The impact is small now that nothing is published. (Sources/AetherEngine/Native/SoftwarePlaybackHost.swift:950) - Nit: The PR body still says section 2 "publishes a playback failure" after four tries. Worth updating so the merge record matches the code.
Checklist: CHANGELOG yes, docs yes, tests yes, traits both ways n.a.
Needs a device run: Real requiresFlushToResumeDecoding and failed queues on tvOS, iOS and macOS. An audio renderer failure on an HDMI or AirPlay route change. A refused rebuild after a screen lock: the picture should return at a keyframe in the foreground, and no retry lines should appear while the app is backgrounded without PiP. selectChapter on a disc title that starts past 2 s, on both routes.
Thanks for working through the earlier findings so quickly and carefully.
Automated review by Quint (Claude) of fe8b453. It reads the code, it does not build or run it; a maintainer reviews and merges.
| /// Nothing shows the picture: the app is in the background with no picture-in-picture window. That covers | ||
| /// audio-only mode and a retained paused session, where VideoToolbox refuses a rebuild by design. The engine | ||
| /// reports it through `noteAppInBackground(_:)`. Main actor only. | ||
| private var appInBackground = false |
There was a problem hiding this comment.
Nit: this starts as false on every new host. The engine only pushes the real value from noteVideoPresentationToHost(), which runs on background entry, foreground return and PiP changes. A software host built while the app is already backgrounded (a reload, a selectTitle, a load that finishes after didEnterBackground) never hears it. Until the next foreground return it retries refused rebuilds in the background, where VideoToolbox refuses by design. That contradicts the CHANGELOG line "Nothing is retried while the app is in the background". The impact is small now that give-up publishes nothing. Suggested fix: call noteVideoPresentationToHost() right after self.softwareHost = host in loadSoftware.
Robustness fixes for the software host, and three small engine items. One commit; the sections below are its parts. None of it touches Dolby Vision code. None of the changed code is platform-guarded (the only new
#ifis#if DEBUGfor test seams), so every section applies on tvOS, iOS and macOS alike.What changes
1. Flush and failure recovery (software host). The system can flush or fail the host's video queue (
requiresFlushToResumeDecodingafter the app lost the right to decode in the background) or its audio renderer. Nothing observed it, so the picture stayed frozen until the next seek. The host now:2. A refused video-session rebuild is retried at the next keyframes. When the system invalidated the hardware video session and VideoToolbox refused the rebuild, the decoder dropped every packet and the picture froze beside live audio until the next foreground return. The host now retries at each keyframe fed while the session is still lost (at least two seconds apart), up to four times, then publishes a playback failure.
3. In-band parameter-set scan budget. The scan that rebuilds an empty HEVC hvcC from in-band VPS/SPS/PPS was bounded in packets only. It now also stops at 16 MiB of input, arms the same ceiling on the demuxer input so one oversized read cannot overshoot it, and leaves the record as it was.
4.
LoadOptions.initialSubtitleTrackID. A load can name the subtitle track to activate (an embedded stream index or an external synthetic id) on the same post-load path aspreferredSubtitleLanguages, ahead of it. An id that names no track falls through to the language preference, so a stale server index cannot fail a load. Additive;docs/api.mdand the CHANGELOG name it.5.
selectChapteron a disc title applied the container start twice. The software route seeks on the session axis and the host adds the clock's session zero back, so adding the whole container start as well landed the chapter late by the start (a start past the 2 s anchor tolerance is that zero). Only the part the zero did not take is added now, through a pure helper with no-media tests. A native disc title already seeks on the 0-based display axis, soselectChapterno longer adds the playlist shift there either.6. Test hygiene. Two suites set
quietPeriodCapForTestingto 0 on the shared origin budget and never reset it, so every later test ran without the quiet ladder; they now hold a counted pin and the cap comes back when the last pin goes (no public API change). A new test pins that a software session paused with the engine-owned Now Playing card comes back, with its remote, after a background teardown.Overlap with open PRs
recoverAfterSuspensionIfNeeded()for a failed queue after a retained pause. Section 1 covers the other triggers (a failure or flush while playing, and the audio renderer); the two do not conflict.selectChapterfor the software route in a different way (session zero only); section 5 is the same defect fixed by the pure helper plus the native-route change. Whichever lands first, the other needs a small rebase.SoftwarePlaybackHost/AudioOutput), fix(lifecycle): reapply background policy after retained seeks and reloads #903 (lifecycle), fix(video): reject retired VP9 packets before session resize #904, fix(video): preserve immutable AV1 output when copying fails #907, fix(video): guard mobile supplemental decoder declarations #900 (HardwareVideoDecoder) touch files this PR touches. A trial merge of this branch with each of them has no conflicts apart from the CHANGELOG.Tested on Mac
swift test(macOS): 5156 tests in 707 suites pass, plus 17 XCTest, no failures.LIBDISPATCH_COOPERATIVE_POOL_STRICT=1 swift test --skip-build --disable-xctest: 5156 tests pass, no failures.FlushRecovery*(policy, keyframe ledger, renderer, host),InBandParameterSetRebuildandHDR10PlusScanInputBudget(including the synthetic junk-tail cases on a custom reader and a local file),VideoSessionRetry,SoftwareHostWakeRestore,OriginBudgetQuietCapPin,DiscChapterSeekTarget,InitialSubtitleTrack: 92 tests in 12 suites pass, also with theDualLayerDolbyVisiontrait compiled in.build-for-testing: both succeed.waitFor, the suite's time limit catches a hang);SoftwareHostWakeRestore,FlushRecovery*andDiscChapterSeekTarget(60 tests in 7 suites) passed three times in a row, 3.3 to 3.6 s each.Still needs a device run (Apple TV, iPhone/iPad, Mac)
The changes are not platform-guarded, so the same code runs on all three. Apple TV is the primary target; the iOS and macOS checks below are what the Mac tests above do not cover.
Apple TV
selectChapterlands within 0.5 s of the chapter mark.initialSubtitleTrackIDon a title with two subtitle tracks: the log names the requested stream.iPhone / iPad
requiresFlushToResumeDecodingafter backgrounding is the most common trigger here (sections 1 and 2).selectChapteron a Blu-ray title that starts past 2 s: lands within 0.5 s of the mark (section 5).Mac
selectChapteron a disc title and one load withinitialSubtitleTrackID(sections 4 and 5).Known follow-ups (not in this PR)
audioQueueEmptiedcan leave the audio lead reading 0 until the next audio PTS rises past the emptied value; this only biases toward a seek (benign).selectChapterduring.loadingand audio-only containers whose first sample is past 2 s keep their old chapter axis; separate fixes.