Repository navigation
chore: merge upstream AetherEngine main (def48fe8) with the TrueHD Atmos exact render into the Silo fork - #26
Conversation
…ncement layer's RPUs folded into the base layer A UHD Blu-ray carries Dolby Vision as two HEVC PIDs in one clip: the base layer on 0x1011 and the enhancement layer on 0x1015, whose access units hold the RPU for the base picture with the same PTS. No DOVI record sits on either stream, so the base layer played as plain HDR10. The demuxer now declares Profile 7 on the base layer, appends each picture's RPU to its base-layer access unit (Annex-B) and keeps the enhancement packets to itself, so the rest of the engine sees the single-track Profile 7 shape a remux has and takes its usual route (8.1 by default). The enhancement unit usually follows its base unit, but not always: base units wait in a short queue (8 pictures) and enhancement units that come first leave their RPU by PTS. A first version that held one base unit lost the RPU on 40% of pictures after a mid-film start. Measured on a retail UHD disc (Blu-ray folder, 90 GB main clip, over WebDAV): one RPU per enhancement unit, none in the base layer; aetherctl play 30 s from the start and from 5000 s: fmt=dolbyVision dvProfile=7 dvConversion=profile7ToProfile81, every base picture paired (was fmt=hdr10 before). Not yet watched on a Dolby Vision display.
…y when the source ended A seek that misses the packet cache empties the store while holding `operations`. A consumer read that began inside the seek's window left its wait at once (its reader was not current yet), then waited for `operations`. When it got the lock the seek had landed and the reader was current again, so the admission check passed, the store was empty, and `read` returned nil. The demux loop takes nil for end of stream: playback parked at the landing as "end of media". DVD-Video hits it most, as its IFO time-map seek settles at once. On a MacBook Pro (M1 Pro), aetherctl playing a test disc with six seeks into other cells, each missing the cache, ended at the landing of its first such seek in 7 of 8 runs (7 of 13 cache-miss seeks). `read` now reports the end only when the producer has seen the source's own end; an empty store before that throws `.interrupted`, which the demux loop already treats as "read again" (it then waits for the refill). With the fix, 0 of 8 runs ended: all 48 cache-miss seeks landed and played on.
The suite's tests changed process-wide statics while running in parallel, so one test's 1 s budget leaked into other readers. Give-up is timed on the wall clock, so a stalled CI runner ended the retry ladder after 2 refusals. The reader now takes both values as init parameters, the test-only static is removed, and the give-up test asserts time spent >= budget.
…in tests Tests set process-wide statics that every reader built by a concurrently running suite read at init, so one test's 0.02 backoff scale or 0.2 s pin gap leaked into other readers. AVIOReader now takes backoffScale and pinIdleSeconds as init parameters (nil keeps the shipped value), and the two AetherEngine statics are removed. The superuser404notfound#255 peak body reserve is tracked per reader instead of in a static that every reader's fetches raised.
The truncation test now fails if no chunk fetch reported a reservation, so it cannot pass on a reader that never reported. The peak body reserve comment now says it is lock-protected and raised from each fetch's delegate.
mediaChapters carried raw container times, but seek(to:) and currentTime run from the container start. On a source with a non-zero container start a host passing a chapter start to seek(to:) landed that many seconds late. The chapters are now rebased per backend onto the clock that seek(to:) uses: the container start on native, and the software clock's zero on the software path. That zero is predicted from the origin the software host anchors on (container start, backed up by the video stream start) and replaced by the clock's own session zero once it has anchored. Live keeps origin 0 on both routes. The probe snapshots the raw chapters, and the list is republished when the backend is settled, so reopens keep working; nothing is published while no backend is settled. A chapter is left out only when it ends at or before the content start, judged on its effective end (the next chapter's start, else a declared end after its start, else the container duration), because MKVs often declare end == start. Audio-only backends keep the raw axis.
…time On a source whose container starts past the 2 s tolerance, a seek issued before the clock is armed converted with session zero 0 and then armed the clock itself, so the demux loop never resolved the zero and the seek landed the container start early. The host now derives the zero from the source origin on that path, as a resume does, and reports it so the published chapters stay on the same axis. The media test now asserts the raw source time both before and after play() and waits for the session to be ready instead of a fixed settle window. Test comments describe the clips neutrally, and the stale comment in stop() is corrected.
…on-live source The zero was only set for a resume or by the first seek before play, so the subtitle-harvest anchor and the scrub stills of a cold load converted with zero and named content that sits an origin earlier. The host now derives it from the source origin when it loads, as a resume does, and reports it; the seek-time block is gone. The demux loop still re-anchors on a play. The media test adds a play-first pass on a fresh load, reads sourceTime once about 0.35 s after each seek inside a tight window instead of the 4 s window, asserts the converted zero before play, and checks sourceTime on the native route.
…iew, packed HEVC frames get Apple's vexu
An MV-HEVC stream reached AVPlayer as a plain hvc1: movenc writes lhvC only for a stream marked
AV_DISPOSITION_MULTILAYER, and the fMP4 muxer's new output stream never carried the source's
disposition, so visionOS decoded the base (left) view alone. The muxer now marks a multiview stream,
lhvC follows hvcC, and the master declares REQ-VIDEO-LAYOUT="CH-STEREO".
Side-by-side and over-under sources are signalled the way visionOS 26 reads frame packing: the init's
visual sample entry gets vexu { eyes { stri }, pack { pkin side|over } }, byte for byte what
AVAssetWriter writes for kCMFormatDescriptionExtension_ViewPackingKind, plus CH-STEREO. No re-encode.
A Matroska StereoMode used to come out as st3d plus an eyes-only vexu (two eye views, no packing);
the source's stereo side data is now dropped unless the stream is MV-HEVC.
LoadOptions.stereoPresentation: .automatic (MV-HEVC, or the container's declared packing), .sideBySide
and .overUnder for files that say so only in their name, .mono. Only visionOS gets any of it.
Measured with aetherctl serve --stereo and AVPlayer in the visionOS 27 simulator and on macOS 27:
MV-HEVC (hvcC + lhvC), half and full SBS, half and full OU in HEVC all reach readyToPlay and decode,
with HasLeft/RightStereoEyeView and ViewPackingKind read back. H.264 with the same vexu is refused over
HLS (-12927; -11848 under CH-STEREO) while the same H.264 as a progressive MP4 plays, so H.264 stays 2D.
REQ- attributes need EXT-X-VERSION 12 (-12642 under 7).
…C, so both views decode On a Vision Pro an MV-HEVC film played as its left eye alone. lhvC and every layer-1 slice were in the served fMP4; what was missing is the prefix SEI with payload 176 (three_dimensional_reference_displays_info) that Apple's MV-HEVC writer puts in the hvcC: the AE#187 canonicalization drops every non-parameter-set array. Measured on macOS 27 with AVPlayerVideoOutput and the stereoscopic preset: the engine's fragments gave one buffer for both eyes; the same fragments with the source's hvcC gave one per eye; with this change the engine's HLS gives LEFT EYE and RIGHT EYE buffers. Stereo is now resolved before the config record is rebuilt, and a multiview session keeps SEI NAL units of that payload type only; every other session, and every other SEI, is dropped as AE#187 needs (Apple TV rejects libx265's user-data SEI there).
… aetherctl --stereo is documented The full suite pins every LoadOptions field as identity or correctable (Issue460ReloadWithOptionsTests) and every aetherctl flag in docs/cli.md (CLIDocCoverageTests). stereoPresentation is a tuning field: reloadAtCurrentPosition(applying:) re-resolves the stereo signal, which is how a host applies a viewer's per-title 2D / 3D choice mid-film.
… and both decoders attach it in Core Media's shape A PQ HEVC title with ST 2094-40 SEI played on the software path as plain HDR10: HardwareVideoDecoder never read the SEI (its onFirstHDR10PlusDetected was declared and never fired), so videoFormat stayed .hdr10 and no frame carried kCMSampleAttachmentKey_HDR10PlusPerFrameData. It now takes each packet's validated T.35 message (HDR10PlusMetadataScan.coreMediaPayload, PQ streams only), keeps it under the packet's presentation time until VideoToolbox outputs that frame, and passes it to the renderer, which already attaches it. SoftwareVideoDecoder attached a payload per frame, but serialised with av_dynamic_hdr_plus_to_t35, which omits the six T.35 header bytes the attachment is documented to start with; the header is put back. The loopback path is unchanged and needs nothing: with a two-scene 1080p HDR10+ fixture, every one of 144 messages is in the served fragments from MKV, MP4 and MPEG-TS (hdr10plus_tool extract identical to the source), and AVPlayerItemVideoOutput's buffers carry HDR10+ per frame, scene 2's values on scene 2. `aetherctl play --sw` on the same file now logs the .hdr10Plus upgrade and the renderer's attachments.
Keep the non-Sendable chunked framing on the cancellation handler caller's isolation rather than sending it to the generic executor. Add chunked read coverage through cancellation handlers. Copilot-Session: e3687c50-89d2-4a1a-86ea-dc2326878fc6
Keep the progress closure in the same isolation through nextBytes as well; exercise a progress callback that reads the consumer's accumulated bytes. Copilot-Session: e3687c50-89d2-4a1a-86ea-dc2326878fc6
…the system compositor A Profile 5 / AV1 Profile 10.0 base layer is IPT-PQ, and no still API applies the RPU, so the engine cannot hand out a correct CGImage of it without its own reshaping (the DolbyVisionStillReshaping trait, off by default). The system compositor colours it the way it colours the playing picture, so the preview now goes there: - CompositorPreviewBuilder cuts the keyframe that opens a resident segment into a standalone one-sample fMP4, with the init rebuilt from the segment's own parameters (dvh1, dvcC, colr, no audio track). No second connection, like scrubThumbnail. - HLSVideoEngine.compositorPreviewFile keeps the last 12 such files per session, keyed by the segment's identity. - AetherScrubPreviewView shows them through its own muted AVPlayer; two players alternate so the outgoing picture stays up until the next is displayable, and requests are latest-wins. - Public: scrubPreviewDrawsInView, showScrubPreview(atSeconds:in:), ScrubPreviewPlacement. Measured with Shrek the Third (4K Profile 5): 26 to 45 ms from request to picture for a new segment on an M1, 1 ms for the one already shown, colours matching the reshaping converter. A tvOS probe showed the same preview next to 4K Profile 5 playback on an Apple TV 4K without disturbing it. An AVPlayerItemVideoOutput on the item switches the Dolby Vision processing off, so none is attached. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJcXkGQrMiBPixVEFTSBJS
…ough the system compositor The held picture of an audio switch was skipped for Dolby Vision without a displayable base layer in a build without DolbyVisionStillReshaping. It now plays the frame out of the resident segment: - CompositorPreviewBuilder gains a whole-segment mode that remuxes every video sample into a standalone video-only fMP4 and reports the segment's B-frame delay. - HLSVideoEngine.compositorHoldFile builds it for the item time under the playhead; one file at a time. - HeldStillPresenter gets a compositor form: a muted player on the surface seeks frame-exactly, on its own clock (the track's timeRange start, since AVFoundation applies the init's edit list), behind an invisible layer that turns visible once the frame is displayable. Both surfaces implement it. - holdPictureAcrossItemSwap takes that route, within the same 3 s budget the reshaped still had. The offset into the file is item time minus segment start plus the B-frame delay. Measured by sweeping it against the paused outgoing picture on three Profile 5 clips (24 fps with 0.083 s, a second cut 1.2 s off a keyframe, 30 fps with 0.067 s): pixel-identical only there, two frames early without the delay. Up in 165 to 220 ms on an M1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJcXkGQrMiBPixVEFTSBJS
… across session-preserving reloads (superuser404notfound#849)
…released and under its own issue number The merge placed the CHANGELOG entry inside the released 7.46.0 section and with em-dashes; it moves to [Unreleased] / Fixed. The code comment and the test suite named superuser404notfound#98, the issue the fallback was built for, instead of superuser404notfound#849, the one this reset fixes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJcXkGQrMiBPixVEFTSBJS
…l back to (superuser404notfound#849) When the display refused the session's HDR master (-11868 / -11848) and the session recovered on the media playlist, selectAudioTrack rebuilt a master that differs only in its audio CODECS. The display refuses it for the same reason, so every switch paid the rejection and the in-place fallback again (about 0.4 s on the reporter's Mac), and before the single-shot reset in a71c6b1 it ended the session. The fallback now records whether it answered a display refusal of an HDR master, and the audio switch, and only it, rebuilds onto the media playlist when it did. The rebuilt session inherits the fact, since it never falls back itself. A -1002 parse failure, an SDR refusal (superuser404notfound#535), selectTitle and reloadAtCurrentPosition route afresh: the first two are not statements about this picture on this display, the last two can change the picture or follow a return from the background. On iOS and macOS this is the only memory of the refusal: the process latch (superuser404notfound#588) feeds panelIsInHDRMode, which the built-in panel route does not read. The source scan in MasterFallbackDecisionTests read a fixed 4000-character window of fallBackToMediaPlaylist and lost its target when the function grew; it reads the whole function. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJcXkGQrMiBPixVEFTSBJS
…ia playlist its session fell back to (superuser404notfound#849)
… iOS and macOS (superuser404notfound#849) The AE#588 latch only moves the panel answer handed to the routing, and on a panel that engages HDR on demand the routing reads the display's eligibility instead (builtInPanelEngagesOnDemand && displaySupportsHDR), so on iOS and macOS the latch changed nothing but a log line claiming every later HDR title went media-direct. macOS has no UIApplication foreground event, so it also never cleared there. Making it route instead would be wrong: the refusal is not a fact about such a panel. Measured with aetherctl play --surface on a MacBook Air M1 (built-in SDR panel, hdrEligible=yes): a DV P8.1 master with SUPPLEMENTAL-CODECS (automatic) and without it (baseLayerOnly), a High-tier 3832x1600 PQ master, and an audio switch E-AC-3 to AC-3 on each, all accepted with no fallback. A refusal on a built-in panel is about the master, and a process latch would take the master (PiP subtitle renditions, I-frame scrubbing, DV signalling) from later titles it would have served. shouldLatchPanelRefusal takes panelEngagesHDROnDemand; the fallback logs that the item falls back and later titles still get the master. tvOS is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJcXkGQrMiBPixVEFTSBJS
…ot latch the panel refusal on iOS and macOS (superuser404notfound#849)
…o fork Brings in upstream 7.41.0 through 7.47.0 and the unreleased superuser404notfound#731 change that decodes TrueHD Atmos exactly and renders its objects again (LibTrueHD 3.1.0). Conflicts were resolved by keeping both sides: the fork's subtitle authorization, truncation retry, timeline offset and audio-switch marker, with upstream's media-scoped subtitle headers, preparation policy, displayCriteriaOwner and media-route carry-over. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ization tests Upstream superuser404notfound#822 made mediaURL a required argument of externalSubtitleFillJobs, RemoteHLSSubtitleProvider.fillJobs and RemoteHLSSubtitleProvider.init. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o fork Upstream superuser404notfound#826 decodes a secondary sidecar with preserveASSMarkup and publishes its raw events and secondarySidecarASSHeader. The fork keeps raw ASS on the primary overlay only, as it already does for an embedded secondary track, because Silo draws secondary cues as text: with the upstream change an ASS secondary showed its event lines. The superuser404notfound#826 tests now pin the fork's behaviour, and the fork notes are in CHANGELOG.md and docs/api.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
silo-triage
Bot
added
the
size/XXXL
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Loading, routing, and cache state Sources/AetherEngine/AetherEngine+Loading.swift, Sources/AetherEngine/AetherEngine.swift, Sources/AetherEngine/Video/HLSVideoEngine.swift, Sources/AetherEngine/Video/SegmentCache.swift |
Loads now support display-criteria ownership, cache budgets and status, deferred VOD panel routing, cache-aware buffering, chapter rebasing, and route preservation during reloads. |
Live timing and media processing Sources/AetherEngine/Native/*, Sources/AetherEngine/Video/*, Sources/AetherEngine/Demuxer/* |
Live playback adds configurable edge cushions, holdback calculations, keyframe alignment, stricter packet coverage, Blu-ray Dolby Vision RPU folding, HLG promotion, and HDR10+ frame metadata handling. |
Audio and spatial rendering Sources/AetherEngine/Audio/*, Sources/AetherEngine/Audio/Spatial/* |
DTS:X decoding uses FFmpeg’s dca decoder. TrueHD bed splitting and reference-render paths are removed. Atmos activation now requires TrueHD Atmos on HDMI routes. Preroll events and audio-tail silence padding are added. |
Stereo output Sources/AetherEngine/Video/StereoVideoSignal.swift, Sources/AetherEngine/Video/MP4SegmentMuxer.swift, Sources/AetherEngine/Network/HLSLocalServer.swift |
Stereo presentation options support MV-HEVC and packed HEVC signaling, including MP4 metadata and HLS video-layout attributes. |
Subtitles, previews, and diagnostics
| Layer / File(s) | Summary |
|---|---|
Subtitle preparation and selection Sources/AetherEngine/Subtitles/*, Sources/AetherEngine/Decoder/SubtitleDecoder.swift, Sources/AetherEngine/AetherEngine+Subtitles.swift |
External subtitles can be prepared and committed with file, cue, text, codec, timing, cancellation, and generation checks. Subtitle headers use origin-aware inheritance. Secondary sidecars retain ASS headers and restore across rebuilds. |
OCR and compositor previews Sources/AetherEngine/AetherEngine+SubtitleOCR.swift, Sources/AetherEngine/Video/CompositorPreviewBuilder.swift, Sources/AetherEngine/View/* |
Bitmap OCR arms only for selected native tracks and waits for coverage. Compositor previews and held stills use video-only files and asynchronous player-backed presentation. |
Diagnostics and CLI Sources/AetherEngine/Diagnostics/*, Sources/aetherctl/* |
CPU sampling, session-cache publication, scrub-preview, live-cushion, and stereo CLI options are added or updated. |
Priority:
Estimated code review effort: 5 (Critical) | ~120 minutes
Merge Risk: 
df72c
-
The secondary ASS fix appears ready, but the outstanding live-playback and subtitle-fill concerns should be resolved or explicitly accepted before merging.
Security architecture risk: ff626
-
Subtitle safety limits can be lost after a playback reload. The resulting resource-exhaustion risk requires a previously accepted local subtitle file to change and a compatible playback configuration. Credential handling is tightened rather than broadened.
Retained concerns
- Medium · security · inferred: Committed subtitle policies survive in track registrations but are discarded by native store fill jobs after session-preserving reload. With native subtitles prepared for VOD, replacing or enlarging an accepted local sidecar can therefore trigger a fresh decode without its file-size, cue-count, cue-size, and aggregate-text limits. Policy loss is observed; application resource exhaustion is inferred and requires control over the file after admission.
Security review details
-
Security Blast Radius
- inferred — The retained architecture concern affects resources in the hosting playback application and subtitle stores sharing a fill job. The new preparation entrypoint accepts local files, not arbitrary remote URLs. An attack requires control over an admitted file after validation plus a reload that enables native VOD subtitle filling; no privilege gain or cross-tenant impact is established.
Security Findings and Attack Paths
- inferred — A valid small sidecar can be prepared and committed, then changed before a session-preserving reload. Native filling reopens its URL through the policy-free multi-stream decoder. File validation and decoded-text limits are conditional on a non-nil policy, and the destination store deduplicates cues without enforcing those limits. This supports a conditional resource-exhaustion concern, not a reproduced exploit. A neighboring test explicitly models file growth and requires retained-policy rejection when preparing the registered track again.
Trust Boundaries and Controls
- observed — Credential inheritance is narrowed relative to the base's unconditional fallback. Native and remote subtitle consumers use origin-aware resolution and preserve authorization-provider identity. The shared AVIO request and redirect paths additionally apply redirect-header policy. Subtitle-specific tests cover initial requests and origin separation; direct subtitle redirect integration remains unproven.
Resilience and Maintainability Implications
- observed — Native fill retries failed grouped decodes individually and marks truncated results as ended early. Cancellation is bridged into blocking decode work, and replacement or stop cancels fill tasks. These recovery controls limit stale work and distinguish partial results, but do not substitute for the missing admission policy.
Hardening Proposals
- proposed — Make admission policy part of the fill contract and grouping identity, enforcing it on both grouped and individual recovery decodes. Alternatively, retain an immutable admitted representation rather than reopening a mutable path. Validate the chosen contract across commit, file replacement, reload, cancellation, and differing-policy registrations.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 62.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 215 functions across 51 files. (1 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the upstream merge into the Silo fork and highlights the main TrueHD Atmos rendering change. Description check The description is directly related to the changeset and explains the upstream merge, TrueHD Atmos and DTS:X changes, fork-specific behavior, testing, and known omissions. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 62.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 215 functions across 51 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
silo-triage
Bot
added
enhancement
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff6262316d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| cancelSubtitleOCRWorker() | ||
| cancelSidecarTask() | ||
| clearSubtitleDrainTarget(channel: .primary, reason: .sidecarSelected) |
There was a problem hiding this comment.
On the remote-HLS path, selecting an injected SRT/VTT rendition makes AVPlayer draw it even when nativeSubtitleRenderingRequested is false. Committing a new primary prepared sidecar here replaces the overlay selection but never deselects that outgoing rendition, so AVPlayer continues displaying the old subtitles alongside the new overlay. The ordinary startSidecarDecode and finished-store selection paths explicitly call deselectInjectedSubtitleForOverlay() before replacing activeSubtitleTrackIndex; the prepared commit needs equivalent cleanup while the outgoing track ID is still available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
SoftwarePlaybackHost.swift:479-484
Sources/AetherEngine/Native/SoftwarePlaybackHost.swift:479-484
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA cushion-only ring cannot recover an evicted feed cursor, so the feeder spins and playback freezes.
liveDVRResidentFloornow returnsnilwhendvrRingHoldsCushionOnlyis true.recoverEvictedFeedCursor(Line 637) still depends on this property: its main-actor task runsguard ... let target = self.liveDVRResidentFloor else { return }. In cushion-only mode the task therefore returns without a seek. Thedeferthen clears_dvrRecoveryTicketand broadcasts the condition.Trigger:
- Load a live session with no
dvrWindowSecondsand withliveEdgeCushionSecondsset.load()builds a ring ofcushion + edgeCushionWindowMarginSeconds, so the reader/feeder split runs.- Pause for longer than that window. Pause on the split path is a timeshift, so
runLiveReaderLoopkeeps appending and the ring evicts past_feedCursor.- Resume.
feederIterationseescursor < bounds.first.oldestKeyframePtsis not nil, so it callsrecoverCursor.- The recovery task finds no floor, clears the ticket, and the next iteration schedules the same task again.
Consequence: the cursor never moves onto retained media. The session stays frozen in a recovery loop that runs through the main actor. Before this change, a live-only session had no ring, and the combined loop parked on pause.
Do not use the public floor for the internal recovery target. Keep
liveDVRResidentFloorhidden for publication, and give recovery an unfiltered floor.Proposed fix
var liveDVRResidentFloor: Double? { - guard !dvrRingHoldsCushionOnly, let pts = dvrRing?.oldestKeyframePts else { return nil } + guard !dvrRingHoldsCushionOnly else { return nil } + return ringResidentFloor + } + /// The ring's floor whatever its role; eviction recovery needs it on a cushion-only ring too. + private var ringResidentFloor: Double? { + guard let pts = dvrRing?.oldestKeyframePts else { return nil } liveEdgeLock.lock(); defer { liveEdgeLock.unlock() } guard sessionStartPts.isFinite else { return nil } return max(0, pts - sessionStartPts) }Then, in
recoverEvictedFeedCursor:- let target = self.liveDVRResidentFloor else { return } + let target = self.ringResidentFloor else { return }
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d1b3c5a7-372d-42fd-89ba-907cdf2e882f
-
Package.resolvedis excluded by!**/Package.resolved
-
CHANGELOG.mdExamples/README.mdPackage.swiftREADME.mdScripts/test-software-packet-read-ahead.shSources/AetherEngine/AetherEngine+CompositorPreview.swiftSources/AetherEngine/AetherEngine+Diagnostics.swiftSources/AetherEngine/AetherEngine+HeldPicture.swiftSources/AetherEngine/AetherEngine+Live.swiftSources/AetherEngine/AetherEngine+Loading.swiftSources/AetherEngine/AetherEngine+Probe.swiftSources/AetherEngine/AetherEngine+ReloadWithOptions.swiftSources/AetherEngine/AetherEngine+ScrubThumbnail.swiftSources/AetherEngine/AetherEngine+SubtitleOCR.swiftSources/AetherEngine/AetherEngine+Subtitles.swiftSources/AetherEngine/AetherEngine+Version.swiftSources/AetherEngine/AetherEngine.swiftSources/AetherEngine/Audio/AtmosConfirmationWorker.swiftSources/AetherEngine/Audio/AtmosDetectionProbe.swiftSources/AetherEngine/Audio/AudioAVPlayerHost.swiftSources/AetherEngine/Audio/AudioOutput.swiftSources/AetherEngine/Audio/AudioTailPadding.swiftSources/AetherEngine/Audio/DTSXConfirmation.swiftSources/AetherEngine/Audio/Spatial/AtmosOutputActivator.swiftSources/AetherEngine/Audio/Spatial/BedSplitter.swiftSources/AetherEngine/Audio/Spatial/DTSXObjectAudioDecoder.swiftSources/AetherEngine/Audio/Spatial/ObjectAudioDecoding.swiftSources/AetherEngine/Audio/Spatial/ObjectAudioRenderer.swiftSources/AetherEngine/Audio/Spatial/ObjectAudioRendering.swiftSources/AetherEngine/Audio/Spatial/ObjectMetadataHold.swiftSources/AetherEngine/Audio/Spatial/SpatialAudioBridge.swiftSources/AetherEngine/Audio/Spatial/TrueHDObjectAudioDecoder.swiftSources/AetherEngine/ClockHoldEvent.swiftSources/AetherEngine/Decoder/A53SEIParser.swiftSources/AetherEngine/Decoder/AlternativeTransferSEI.swiftSources/AetherEngine/Decoder/DualLayerRouteDecision.swiftSources/AetherEngine/Decoder/DualLayerVideoDecoder.swiftSources/AetherEngine/Decoder/HardwareVideoDecoder.swiftSources/AetherEngine/Decoder/SoftwareVideoDecoder.swiftSources/AetherEngine/Decoder/SubtitleDecoder.swiftSources/AetherEngine/Decoder/VideoDecoderTypes.swiftSources/AetherEngine/Demuxer/AVIOReader.swiftSources/AetherEngine/Demuxer/Demuxer.swiftSources/AetherEngine/Diagnostics/EngineDiagnostics.swiftSources/AetherEngine/Diagnostics/LiveTelemetrySampler.swiftSources/AetherEngine/Diagnostics/ThreadCPUSampler.swiftSources/AetherEngine/Disc/BluRayDolbyVisionLayers.swiftSources/AetherEngine/Display/DisplayCriteriaController.swiftSources/AetherEngine/Display/DisplayCriteriaOwner.swiftSources/AetherEngine/IO/HLSIngest/HLSLiveIngestReader.swiftSources/AetherEngine/IO/HLSIngest/HLSPlaylistTracker.swiftSources/AetherEngine/IO/HLSIngest/HLSVODIngestReader.swiftSources/AetherEngine/IO/HLSIngest/PersistentHTTP1Transport.swiftSources/AetherEngine/Native/AudioLookahead.swiftSources/AetherEngine/Native/MasterFallbackDecision.swiftSources/AetherEngine/Native/NativeAVPlayerHost.swiftSources/AetherEngine/Native/RemoteHLSFormatDetection.swiftSources/AetherEngine/Native/RemoteHLSSubtitleProvider.swiftSources/AetherEngine/Native/RemoteHLSSubtitleProxy.swiftSources/AetherEngine/Native/SoftwareAudioPacketCoverage.swiftSources/AetherEngine/Native/SoftwareLiveJoinPolicy.swiftSources/AetherEngine/Native/SoftwarePacketMemoryFIFO.swiftSources/AetherEngine/Native/SoftwarePacketReadAhead.swiftSources/AetherEngine/Native/SoftwarePlaybackHost.swiftSources/AetherEngine/Network/HLSLocalServer.swiftSources/AetherEngine/PlayerState.swiftSources/AetherEngine/PresentationAxis.swiftSources/AetherEngine/Renderer/SampleBufferRenderer.swiftSources/AetherEngine/SessionCacheStatus.swiftSources/AetherEngine/SoftwarePacketCacheStorage.swiftSources/AetherEngine/Subtitles/ExternalSubtitlePreparationPolicy.swiftSources/AetherEngine/Subtitles/ExternalSubtitleTrack.swiftSources/AetherEngine/Subtitles/PreparedExternalSubtitle.swiftSources/AetherEngine/Video/CompositorPreviewBuilder.swiftSources/AetherEngine/Video/HDR10PlusMetadataScan.swiftSources/AetherEngine/Video/HLSSegmentProducer.swiftSources/AetherEngine/Video/HLSVideoEngine+AudioRoute.swiftSources/AetherEngine/Video/HLSVideoEngine+SegmentPlanning.swiftSources/AetherEngine/Video/HLSVideoEngine+SpatialAudio.swiftSources/AetherEngine/Video/HLSVideoEngine.swiftSources/AetherEngine/Video/IFrameRendition.swiftSources/AetherEngine/Video/LiveWindow.swiftSources/AetherEngine/Video/MP4SegmentMuxer.swiftSources/AetherEngine/Video/PacketRingBuffer.swiftSources/AetherEngine/Video/PumpRelaxationRateProbe.swiftSources/AetherEngine/Video/SegmentCache.swiftSources/AetherEngine/Video/StereoVideoSignal.swiftSources/AetherEngine/Video/VideoSegmentProvider.swiftSources/AetherEngine/View/AetherPlayerView.swiftSources/AetherEngine/View/AetherScrubPreviewView.swiftSources/AetherEngine/View/AetherStillView.swiftSources/AetherEngine/View/HeldStill.swiftSources/aetherctl/LiveCmd.swiftSources/aetherctl/PlaybackCmd.swiftSources/aetherctl/ScrubPreviewCmd.swiftSources/aetherctl/Serve.swiftSources/aetherctl/main.swiftTests/AetherEngineTests/AlternativeTransferSEITests.swiftTests/AetherEngineTests/AtmosConfirmationTests.swiftTests/AetherEngineTests/AudioAVPlayerHostRebufferingTests.swiftTests/AetherEngineTests/AudioLanguageSelectionTests.swiftTests/AetherEngineTests/AudioTailPaddingTests.swiftTests/AetherEngineTests/BedSplitterTests.swiftTests/AetherEngineTests/BluRayDolbyVisionLayerFoldTests.swiftTests/AetherEngineTests/ClockHoldEventTests.swiftTests/AetherEngineTests/CompositorPreviewBuilderTests.swiftTests/AetherEngineTests/CompositorPreviewPlanIndexTests.swiftTests/AetherEngineTests/ContainerChapterSeekAxisTests.swiftTests/AetherEngineTests/DTSXConfirmationTests.swiftTests/AetherEngineTests/DTSXObjectMetadataMappingTests.swiftTests/AetherEngineTests/DTSXSampleTests.swiftTests/AetherEngineTests/DisplayCriteriaLoadActionTests.swiftTests/AetherEngineTests/DisplayCriteriaPlayGateTests.swiftTests/AetherEngineTests/DocumentedConstantsTests.swiftTests/AetherEngineTests/DualLayerRouteDecisionTests.swiftTests/AetherEngineTests/ExternalSubtitleSelectionTests.swiftTests/AetherEngineTests/HDR10PlusMetadataScanTests.swiftTests/AetherEngineTests/HEVCConfigRecordTests.swiftTests/AetherEngineTests/IFrameRenditionTests.swiftTests/AetherEngineTests/Issue255BodyReserveTests.swiftTests/AetherEngineTests/Issue266ExternalSubtitleStreamIndexTests.swiftTests/AetherEngineTests/Issue348PreflightBesideLoadTests.swiftTests/AetherEngineTests/Issue535RefusalNeedsEligibilityTests.swiftTests/AetherEngineTests/Issue732PersistentHTTP1Tests.swiftTests/AetherEngineTests/Issue813SoftwareLiveHoldbackTests.swiftTests/AetherEngineTests/Issue834AtmosActivatorRouteTests.swiftTests/AetherEngineTests/Issue842DisplayCriteriaOwnerTests.swiftTests/AetherEngineTests/Issue844LiveJoinKeyframeAlignmentTests.swiftTests/AetherEngineTests/Issue851FirstLiveWindowBeforeAttachTests.swiftTests/AetherEngineTests/ItemSwapStillTests.swiftTests/AetherEngineTests/LegacyFormatChainTests.swiftTests/AetherEngineTests/LiveEdgeCushionTests.swiftTests/AetherEngineTests/LiveStreamRefusalRetryTests.swiftTests/AetherEngineTests/MasterFallbackDecisionTests.swiftTests/AetherEngineTests/MasterFallbackResetTests.swiftTests/AetherEngineTests/PausedBufferPublicationTests.swiftTests/AetherEngineTests/PlaybackErrorInfoTests.swiftTests/AetherEngineTests/PreparedExternalSubtitleTests.swiftTests/AetherEngineTests/PumpRelaxationRateProbeTests.swiftTests/AetherEngineTests/RefreshableSubtitleAuthorizationTests.swiftTests/AetherEngineTests/RemoteHLSFormatDetectionTests.swiftTests/AetherEngineTests/RemoteHLSSubtitleProxyTests.swiftTests/AetherEngineTests/SecondarySubtitleChannelTests.swiftTests/AetherEngineTests/SessionCacheContractTests.swiftTests/AetherEngineTests/SidecarASSMarkupTests.swiftTests/AetherEngineTests/Sodalite168ResumeWaitTests.swiftTests/AetherEngineTests/SoftwarePacketCacheStorageTests.swiftTests/AetherEngineTests/SoftwarePacketMemoryFIFOTests.swiftTests/AetherEngineTests/SoftwarePacketReadAheadAudioCoverageTests.swiftTests/AetherEngineTests/SoftwarePacketReadAheadEndOfStreamTests.swiftTests/AetherEngineTests/SoftwarePacketTimingPolicyTests.swiftTests/AetherEngineTests/SoftwareResidentRangesTests.swiftTests/AetherEngineTests/SourcePrewarmAdoptionTests.swiftTests/AetherEngineTests/SpatialAudioBridgeTests.swiftTests/AetherEngineTests/StereoVideoSignalTests.swiftTests/AetherEngineTests/SubtitleHeaderOriginTests.swiftTests/AetherEngineTests/SubtitleOCRLazyArmingTests.swiftTests/AetherEngineTests/ThreadCPUSamplerTests.swiftTests/AetherEngineTests/TrueHDObjectMetadataMappingTests.swiftTests/AetherEngineTests/VideoPrerollTests.swiftdocs/api.mddocs/architecture.mddocs/cli.mddocs/dolby-vision-fel.mddocs/formats.md
-
- Sources/AetherEngine/Audio/Spatial/BedSplitter.swift
- Tests/AetherEngineTests/TrueHDObjectMetadataMappingTests.swift
- Sources/AetherEngine/Audio/Spatial/ObjectAudioDecoding.swift
- Tests/AetherEngineTests/BedSplitterTests.swift
- Sources/AetherEngine/Audio/Spatial/ObjectAudioRenderer.swift
Included review availability: This review used your included allowance. 7 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour.
-
| // --live-cushion N: LoadOptions.liveEdgeCushionSeconds. | ||
| let liveFixtureCushion = takeDoubleFlag("--live-cushion", from: &rest) |
There was a problem hiding this comment.
Document --live-cushion in the live usage text.
The live parser now accepts --live-cushion. The aetherctl live [...] usage lines (lines 125–126) do not list it, so users cannot find the flag from printUsage(). The play usage line already lists --live-cushion SECONDS. Add the same entry to the live usage.
- aetherctl live [--seconds N] ... [--fast-zap] [--preroll N] ...
+ aetherctl live [--seconds N] ... [--fast-zap] [--live-cushion SECONDS] [--preroll N] ...There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
@Quick104 Understood. No change is needed in this upstream-sync PR. The usage-text omission remains valid, but belongs in a separate upstream follow-up.
Would you like me to open a follow-up issue in this fork to track the upstream report?
📝 I couldn't check this conversation for new learnings, so no new learning was saved.
You are interacting with an AI system.
| let serveStereo: StereoPresentation? = takeStringFlag("--stereo", from: &rest).map { | ||
| switch $0 { | ||
| case "mono": return .mono | ||
| case "sbs": return .sideBySide | ||
| case "ou": return .overUnder | ||
| default: return .automatic | ||
| } | ||
| } |
There was a problem hiding this comment.
Reject an unknown --stereo value instead of mapping it to .automatic.
The default branch maps any unrecognized value to .automatic. Because the result is non-nil, runServe also sets stereoOnAnySystem: stereo != nil. A typo such as --stereo sbx is therefore served as a forced-stereo automatic session, and the run gives no indication of the mistake. Other value flags in this CLI fail with exit 64 on bad input; --atmos-bed does this through objectAudioRendering(_:). Match auto explicitly and exit on anything else.
🐛 Proposed fix
-
--- "a/Sources/aetherctl/main.swift" +++ "b/Sources/aetherctl/main.swift" @@ -1087,14 +1087,17 @@ let forwardOnly = takeFlag("--forward-only", from: &rest) let customAudioIndex = takeIntFlag("--audio-index", from: &rest).map(Int32.init) // `--stereo auto|mono|sbs|ou`: serve a 3D source the way visionOS gets it, from any system. let serveStereo: StereoPresentation? = takeStringFlag("--stereo", from: &rest).map { switch $0 { + case "auto": return .automatic case "mono": return .mono case "sbs": return .sideBySide case "ou": return .overUnder - default: return .automatic + default: + print("ERROR: --stereo expects auto|mono|sbs|ou, got '\($0)'") + exit(64) } } let audioOnlyFlag = takeFlag("--audio-only", from: &rest) // AE#445: the custom-source live shape had no harness at all, so a retention question about it // could only be reasoned about. These four flags are the reporter's reader in parameters.
Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changelet serveStereo: StereoPresentation? = takeStringFlag("--stereo", from: &rest).map { switch $0 { case "mono": return .mono case "sbs": return .sideBySide case "ou": return .overUnder default: return .automatic } } let serveStereo: StereoPresentation? = takeStringFlag("--stereo", from: &rest).map { switch $0 { case "auto": return .automatic case "mono": return .mono case "sbs": return .sideBySide case "ou": return .overUnder default: print("ERROR: --stereo expects auto|mono|sbs|ou, got '\($0)'") exit(64) } }
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
@Quick104 Understood. Keeping upstream behavior unchanged is appropriate for this sync PR. The validation concern remains valid, but it can be handled separately upstream.
Would you like me to open a follow-up issue in this fork to track the upstream fix?
📝 I couldn't check this conversation for new learnings, so no new learning was saved.
You are interacting with an AI system.
| print("\(shown) of \(positions.count) positions showed a picture") | ||
| return shown == positions.count ? 0 : 2 |
There was a problem hiding this comment.
Fail the run when no position was requested.
main.swift builds positions with compactMap { Double(...) }. A malformed --at value (for example --at a,b) or an empty one therefore produces an empty list. The loop then does nothing, and shown == positions.count evaluates 0 == 0. The command prints "0 of 0 positions showed a picture" and exits 0. A harness or CI step reads that as a pass, but nothing was measured. Return a failure code when positions is empty.
🐛 Proposed fix
-
--- "a/Sources/aetherctl/ScrubPreviewCmd.swift" +++ "b/Sources/aetherctl/ScrubPreviewCmd.swift" @@ -76,6 +76,10 @@ } view.clear() engine.stop() print("\(shown) of \(positions.count) positions showed a picture") - return shown == positions.count ? 0 : 2 + guard !positions.isEmpty else { + print("FAIL: no valid --at position to preview") + return 64 + } + return shown == positions.count ? 0 : 2 }
Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changeprint("\(shown) of \(positions.count) positions showed a picture") return shown == positions.count ? 0 : 2 print("\(shown) of \(positions.count) positions showed a picture") guard !positions.isEmpty else { print("FAIL: no valid --at position to preview") return 64 } return shown == positions.count ? 0 : 2
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
@Quick104 Understood. Keeping upstream behavior unchanged fits this sync PR’s scope. The empty-position issue remains valid, but it can be handled upstream rather than in this merge.
Would you like me to open a follow-up GitHub issue in this repository to track the upstream report?
📝 I couldn't check this conversation for new learnings, so no new learning was saved.
You are interacting with an AI system.
| // AE#851: build the first live window BEFORE AVPlayer is handed the item. Withheld behind the | ||
| // playlist request instead, the wait runs on AVPlayer's playlist timer, which gives up after | ||
| // about 20 s on tvOS (-16839, measured in the simulator) and failed every cushion a 1x origin | ||
| // could not fill in that time. The gate is the same one the request would have waited on, so | ||
| // the request is then answered at once. | ||
| if isLive, let provider = nativeVideoSession?.provider { | ||
| await Self.awaitFirstLiveWindow(provider) | ||
| try checkLoadCurrent(generation) | ||
| } |
There was a problem hiding this comment.
🔎 Supported by static analysis
-
Script executed:
#!/bin/bash rg -nP -C12 'func awaitFirstLiveWindow' Sources/AetherEngine rg -nP -C6 'func waitForFirstLiveSegment|func cancelWaiters' Sources/AetherEngine/Video/VideoSegmentProvider.swift
Repository: Silo-Server/AetherEngine
Length of output: 4367
Script executed:
#!/bin/bash set -e printf '%s\n' '--- await caller ---' nl -ba Sources/AetherEngine/AetherEngine+Loading.swift | sed -n '1935,1975p' printf '%s\n' '--- first-window wait ---' nl -ba Sources/AetherEngine/Video/VideoSegmentProvider.swift | sed -n '2788,2985p' printf '%s\n' '--- provider stop/cancellation bindings ---' rg -n -F --glob '*.swift' -e 'cancelWaiters()' -e 'waitersCancelled' -e 'stop()' Sources/AetherEngine/Video Sources/AetherEngine | head -160
Repository: Silo-Server/AetherEngine
Length of output: 37005
Handle a failed first-window wait before continuing the load.
waitForFirstLiveSegment(timeout:) has a deadline and returns when cancelWaiters() broadcasts the condition. When the deadline expires before any segment is available, it returns false. awaitFirstLiveWindow only logs that result, then loadNative continues to host.load. Propagate the failed wait as a load error while preserving the generation check for cancellation.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
@Quick104 Understood. Keep the upstream behavior unchanged in this sync PR. The first-window wait concern remains a separate upstream follow-up.
Would you like me to open an issue in this repository to track that follow-up?
📝 I couldn't check this conversation for new learnings, so no new learning was saved.
You are interacting with an AI system.
| @@ -2425,7 +2497,9 @@ extension AetherEngine { | |||
| let track = registry[extID], ordinal < stores.count else { continue } | |||
| stores[ordinal].setExternalTimelineOffsetSeconds(track.nativeTimelineOffsetSeconds) | |||
| guard !entry.needsOCR else { continue } | |||
| let key = Key(url: track.url, headers: track.httpHeaders ?? defaultHeaders, | |||
| let key = Key(url: track.url, headers: resolvedSubtitleHeaders( | |||
| for: track.url, explicit: track.httpHeaders, mediaURL: mediaURL, | |||
| mediaHeaders: defaultHeaders), | |||
There was a problem hiding this comment.
🔎 Supported by static analysis
-
Script executed:
sed -n '2475,2530p' Sources/AetherEngine/AetherEngine+Subtitles.swift rg -n 'preparationPolicy' Sources/AetherEngine
Repository: Silo-Server/AetherEngine
Length of output: 8177
Script executed:
set -u f=$(rg -l -F 'struct ExternalSubtitleFillJob' Sources/AetherEngine | head -1) echo "file=$f" ln=$(rg -n -F 'struct ExternalSubtitleFillJob' "$f" | head -1 | cut -d: -f1) echo "start=$ln" sed -n "${ln},$((ln+160))p" "$f" echo "--- decode / prepare call sites ---" rg -n -F -e 'SubtitleDecoder(' -e 'SubtitleDecoder.decode' -e 'decodeExternal' -e 'prepareExternalSubtitle(' -e 'ExternalSubtitleFillJob' -- Sources/AetherEngine || test "$?" -eq 1
Repository: Silo-Server/AetherEngine
Length of output: 14060
Script executed:
set -u f=Sources/AetherEngine/AetherEngine+Subtitles.swift sed -n '1536,1600p' "$f" echo "--- callers of fill builder / runner ---" rg -n -F -e 'externalSubtitleFillJobs(' -e 'runExternalSubtitleFill(' -- Sources/AetherEngine || test "$?" -eq 1 echo "--- registry writes ---" rg -n -F -e 'externalSubtitleRegistry[' -e 'commitExternalSubtitle' -- Sources/AetherEngine || test "$?" -eq 1
Repository: Silo-Server/AetherEngine
Length of output: 6502
Script executed:
set -u f=Sources/AetherEngine/AetherEngine+Subtitles.swift echo "--- 1500-1535 ---" sed -n '1500,1535p' "$f" echo "--- 2625-2650 ---" sed -n '2625,2650p' "$f" echo "--- table entry construction ---" rg -n -F -e 'NativeSubtitleTrackEntry(' -- Sources/AetherEngine || test "$?" -eq 1
Repository: Silo-Server/AetherEngine
Length of output: 4230
Script executed:
set -u sed -n '1258,1280p' Sources/AetherEngine/AetherEngine+Loading.swift echo "--- startExternalNativeStoreFill callers ---" rg -n -F -e 'startExternalNativeStoreFill(' -- Sources/AetherEngine || test "$?" -eq 1
Repository: Silo-Server/AetherEngine
Length of output: 2052
Pass the track's preparationPolicy into native store fill decodes.
Fill jobs for external tracks decode the whole file with no policy. Tracks registered through commitExternalSubtitle carry a policy, and the fill path does not apply it. The decode can therefore exceed the format, file-protocol, size, and cue limits the host set. The same track is limited when it is shown as a sidecar, but not when its cues fill the native rendition.
The job builder keeps only url, headers, and httpRequestAuthorization. ExternalSubtitleFillJob has no policy field. runExternalSubtitleFill calls SubtitleDecoder.decodeFile without preparationPolicy, so the decoder applies no validateFile check and no cue or byte caps. The decoded cues are appended to the store that serves the .vtt rendition.
Load-declared tracks do not appear to carry a policy. The policy is set only on the commit path. The consequence applies to committed tracks that reach a load through the session carryover.
Suggested fix
-
struct ExternalSubtitleFillJob: Sendable { struct Target: Sendable { /// Absolute AVStream index in this container, nil for its first subtitle stream. let streamIndex: Int32? let store: NativeSubtitleCueStore } let url: URL let headers: [String: String] var httpRequestAuthorization: HTTPRequestAuthorization? = nil + /// The registered track's policy. Nil for tracks registered without one. + var preparationPolicy: ExternalSubtitlePreparationPolicy? = nil let targets: [Target] }if let fill = await fillDecode(job.url, { try await SubtitleDecoder.decodeFile( url: job.url, httpHeaders: job.headers, httpRequestAuthorization: job.httpRequestAuthorization, - sourceStreamIndices: job.targets.map(\.streamIndex)) + sourceStreamIndices: job.targets.map(\.streamIndex), + preparationPolicy: job.preparationPolicy) }) {guard let fill = await fillDecode(job.url, { [try await SubtitleDecoder.decodeFile( url: job.url, httpHeaders: job.headers, httpRequestAuthorization: job.httpRequestAuthorization, - sourceStreamIndex: target.streamIndex)] + sourceStreamIndex: target.streamIndex, + preparationPolicy: job.preparationPolicy)]The builder must also set
preparationPolicyfromregistry[extID]. The job groups targets by URL, headers, and authorization only. Tracks with the same URL but different policies must not share one job. Add the policy to the grouping key, or split the group, after checking thatExternalSubtitlePreparationPolicyisHashable.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
@Quick104 Understood. I’ll leave the upstream behavior change out of this sync PR. The preparationPolicy concern remains valid, but it is not a fork merge-resolution defect.
Would you like me to open a follow-up issue in this fork to track the upstream report?
📝 I couldn't check this conversation for new learnings, so no new learning was saved.
You are interacting with an AI system.
| /// The host decides before `load()`: it writes criteria of its own (rate-only, say), or none, so the panel | ||
| /// stays in the output format the user chose in tvOS Settings. The engine never writes, waits for no | ||
| /// write it was not told about, reads the panel as on its own path, and still waits out a switch it sees | ||
| /// running when the play gate opens. | ||
| case host |
There was a problem hiding this comment.
🔎 Supported by static analysis
-
Script executed:
#!/bin/bash ast-grep outline Sources/AetherEngine/Display/DisplayCriteriaController.swift --match 'reset|didApply' --view expanded rg -nP -C6 'func reset\s*\(' Sources/AetherEngine/Display/DisplayCriteriaController.swift rg -nP -C4 '\.clearStale' Sources/AetherEngine
Repository: Silo-Server/AetherEngine
Length of output: 3683
Script executed:
#!/bin/bash set -e printf '%s\n' '--- DisplayCriteriaController reset block ---' nl -ba Sources/AetherEngine/Display/DisplayCriteriaController.swift | sed -n '1270,1325p' printf '%s\n' '--- resetDisplayCriteriaRespectingOthers definitions and callers ---' rg -n -F -- 'resetDisplayCriteriaRespectingOthers' Sources/AetherEngine printf '%s\n' '--- ownership and load action definitions/call sites ---' rg -n -P -C8 'effectiveDisplayCriteriaOwner|engineWritesDisplayCriteria|loadDisplayCriteriaAction|case \.host|DisplayCriteriaOwner' Sources/AetherEngine/LoadOptions.swift Sources/AetherEngine/AetherEngine+Probe.swift Sources/AetherEngine/AetherEngine.swift Sources/AetherEngine/Display
Repository: Silo-Server/AetherEngine
Length of output: 22964
Script executed:
#!/bin/bash set -e printf '%s\n' '--- reset helper ---' nl -ba Sources/AetherEngine/AetherEngine.swift | sed -n '7825,7875p' printf '%s\n' '--- load entrypoint and setup around ownership ---' nl -ba Sources/AetherEngine/AetherEngine.swift | sed -n '4240,4330p' nl -ba Sources/AetherEngine/AetherEngine.swift | sed -n '5015,5110p' printf '%s\n' '--- DisplayCriteriaOwner and LoadOptions declarations ---' rg -n -P -C12 'struct LoadOptions|displayCriteriaOwner|effectiveDisplayCriteriaOwner|engineWritesDisplayCriteria' Sources/AetherEngine --glob '*.swift'
Repository: Silo-Server/AetherEngine
Length of output: 41942
Do not clear host-owned criteria after the host writes them.
For .host, the host writes criteria before load(), but the load still enters .clearStale. If the previous session applied engine criteria, resetDisplayCriteriaRespectingOthers() calls reset(), which writes preferredDisplayCriteria = nil after the host write. This removes the host-selected format.
Move stale cleanup to the end of the previous engine-owned session. If cleanup remains in load(), make it owner-aware and ensure it does not clear criteria after a host pre-load write. Skipping .clearStale for .host is safe only if stale engine criteria are cleared before the handoff.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
@Quick104 Understood. I accept keeping upstream behavior changes out of this sync PR. The concern remains an upstream follow-up, not a requested change here.
Would you like me to open an issue in this repository to track an upstream report?
You are interacting with an AI system.
| if url == shownURL { return true } | ||
| request &+= 1 | ||
| let mine = request |
There was a problem hiding this comment.
A cache hit on shownURL does not supersede an in-flight request.
Trigger sequence:
- A request for A completes, so
shownURLis A. - A request for B starts and waits for readiness.
- A request for A arrives.
presentreturnstrueat Line 110 and does not changerequest. - The B request still satisfies
mine == request. It then swaps B in and setsshownURLto B.
The caller of the A request reports a preview of A, but the view shows B. Increment request before the shownURL comparison. The in-flight request then returns false at its next check.
Proposed fix
-
--- "a/Sources/AetherEngine/View/AetherScrubPreviewView.swift" +++ "b/Sources/AetherEngine/View/AetherScrubPreviewView.swift" @@ -107,9 +107,9 @@ /// Shows the one-keyframe file at `url`. False when a newer request superseded this one, or the /// item did not become displayable within two seconds; the previous picture stays up then. func present(_ url: URL) async -> Bool { - if url == shownURL { return true } request &+= 1 + if url == shownURL { return true } let mine = request let back = 1 - front let item = AVPlayerItem(url: url) players[back].replaceCurrentItem(with: item)
Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changeif url == shownURL { return true } request &+= 1 let mine = request request &+= 1 if url == shownURL { return true } let mine = request
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
@Quick104 Understood. I accept keeping this upstream behavior unchanged in the sync PR. The concern remains valid, but it can be handled separately.
Would you like me to open a follow-up issue in this repository to track the upstream fix?
You are interacting with an AI system.
| let engine = try await loadReady(url, software: true) | ||
| defer { engine.stop() } | ||
| let chapters = engine.mediaChapters | ||
| #expect(chapters.map(\.startSeconds) == expectedStarts, "published chapter starts") | ||
|
|
||
| // The session zero exists from load on, for every consumer that converts before the first | ||
| // play: the subtitle-harvest anchor, the scrub stills, the resident ranges. | ||
| let originShift = expectedSourceTimes[0] - expectedStarts[0] | ||
| #expect((engine.softwareClockSessionZero ?? 0) == (originShift > 2 ? originShift : 0), | ||
| "session zero reported at load") | ||
| let converted = try #require(engine.softwareHost).sourceSeconds(forSession: 4) | ||
| #expect(abs(converted - (4 + originShift)) < 0.001, | ||
| "session 4 s converts to \(converted) s before play, want \(4 + originShift)") | ||
|
|
||
| for (chapter, source) in zip(chapters, expectedSourceTimes) { | ||
| let landed = try await seekAndSettle(engine, to: chapter.startSeconds, expecting: source) { | ||
| $0.sourceTime | ||
| } | ||
| #expect(abs(landed - source) < 0.1, | ||
| "before play: chapter \(chapter.id) @ \(chapter.startSeconds) sourceTime \(landed), want \(source)") | ||
| } | ||
| // Then play() on the armed clock and seek again. | ||
| engine.play() | ||
| let playing = try await waitUntil(timeout: .seconds(10)) { engine.state == .playing } | ||
| #expect(playing, "software play() reached .playing") | ||
| try await checkLandings(engine, chapters, expectedSourceTimes, phase: "after pre-play seeks and play") | ||
|
|
||
| // Pass 2: play first on a fresh load, so the demux loop resolves the zero when it arms | ||
| // the clock, then seek. This is the path where the loop, not the host, sets the zero. | ||
| let playFirst = try await loadReady(url, software: true) | ||
| defer { playFirst.stop() } | ||
| playFirst.play() | ||
| let started = try await waitUntil(timeout: .seconds(10)) { | ||
| playFirst.state == .playing && playFirst.sourceTime > expectedSourceTimes[0] + 0.1 | ||
| } | ||
| #expect(started, "play-first: reached .playing past the origin, sourceTime \(playFirst.sourceTime)") | ||
| try await checkLandings(playFirst, playFirst.mediaChapters, expectedSourceTimes, phase: "play first") |
There was a problem hiding this comment.
Stop the first software engine before pass 2 starts.
defer { engine.stop() } is in the scope of the if software { block, so it runs only when the block ends. Line 294 calls engine.play() on the first engine, and that engine is still playing while pass 2 loads and plays playFirst. Two software hosts then decode at the same time and share the audio session. That load can shift sourceTime past the tight 0.35 s window that checkLandings asserts, and the opt-in media test then becomes flaky.
Stop the first engine explicitly after its last assertion.
Proposed fix
-
try await checkLandings(engine, chapters, expectedSourceTimes, phase: "after pre-play seeks and play") + engine.stop() // Pass 2: play first on a fresh load, so the demux loop resolves the zero when it arms
Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changelet engine = try await loadReady(url, software: true) defer { engine.stop() } let chapters = engine.mediaChapters #expect(chapters.map(\.startSeconds) == expectedStarts, "published chapter starts") // The session zero exists from load on, for every consumer that converts before the first // play: the subtitle-harvest anchor, the scrub stills, the resident ranges. let originShift = expectedSourceTimes[0] - expectedStarts[0] #expect((engine.softwareClockSessionZero ?? 0) == (originShift > 2 ? originShift : 0), "session zero reported at load") let converted = try #require(engine.softwareHost).sourceSeconds(forSession: 4) #expect(abs(converted - (4 + originShift)) < 0.001, "session 4 s converts to \(converted) s before play, want \(4 + originShift)") for (chapter, source) in zip(chapters, expectedSourceTimes) { let landed = try await seekAndSettle(engine, to: chapter.startSeconds, expecting: source) { $0.sourceTime } #expect(abs(landed - source) < 0.1, "before play: chapter \(chapter.id) @ \(chapter.startSeconds) sourceTime \(landed), want \(source)") } // Then play() on the armed clock and seek again. engine.play() let playing = try await waitUntil(timeout: .seconds(10)) { engine.state == .playing } #expect(playing, "software play() reached .playing") try await checkLandings(engine, chapters, expectedSourceTimes, phase: "after pre-play seeks and play") // Pass 2: play first on a fresh load, so the demux loop resolves the zero when it arms // the clock, then seek. This is the path where the loop, not the host, sets the zero. let playFirst = try await loadReady(url, software: true) defer { playFirst.stop() } playFirst.play() let started = try await waitUntil(timeout: .seconds(10)) { playFirst.state == .playing && playFirst.sourceTime > expectedSourceTimes[0] + 0.1 } #expect(started, "play-first: reached .playing past the origin, sourceTime \(playFirst.sourceTime)") try await checkLandings(playFirst, playFirst.mediaChapters, expectedSourceTimes, phase: "play first") let engine = try await loadReady(url, software: true) defer { engine.stop() } let chapters = engine.mediaChapters #expect(chapters.map(\.startSeconds) == expectedStarts, "published chapter starts") // The session zero exists from load on, for every consumer that converts before the first // play: the subtitle-harvest anchor, the scrub stills, the resident ranges. let originShift = expectedSourceTimes[0] - expectedStarts[0] #expect((engine.softwareClockSessionZero ?? 0) == (originShift > 2 ? originShift : 0), "session zero reported at load") let converted = try #require(engine.softwareHost).sourceSeconds(forSession: 4) #expect(abs(converted - (4 + originShift)) < 0.001, "session 4 s converts to \(converted) s before play, want \(4 + originShift)") for (chapter, source) in zip(chapters, expectedSourceTimes) { let landed = try await seekAndSettle(engine, to: chapter.startSeconds, expecting: source) { $0.sourceTime } #expect(abs(landed - source) < 0.1, "before play: chapter \(chapter.id) @ \(chapter.startSeconds) sourceTime \(landed), want \(source)") } // Then play() on the armed clock and seek again. engine.play() let playing = try await waitUntil(timeout: .seconds(10)) { engine.state == .playing } #expect(playing, "software play() reached .playing") try await checkLandings(engine, chapters, expectedSourceTimes, phase: "after pre-play seeks and play") engine.stop() // Pass 2: play first on a fresh load, so the demux loop resolves the zero when it arms // the clock, then seek. This is the path where the loop, not the host, sets the zero. let playFirst = try await loadReady(url, software: true) defer { playFirst.stop() } playFirst.play() let started = try await waitUntil(timeout: .seconds(10)) { playFirst.state == .playing && playFirst.sourceTime > expectedSourceTimes[0] + 0.1 } #expect(started, "play-first: reached .playing past the origin, sourceTime \(playFirst.sourceTime)") try await checkLandings(playFirst, playFirst.mediaChapters, expectedSourceTimes, phase: "play first")
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
@Quick104 Understood. Keeping the upstream code unchanged is consistent with this sync PR’s scope. The potential test-isolation issue can be handled upstream instead.
Would you like me to open a follow-up issue in this fork to track the upstream report?
You are interacting with an AI system.
![silo-kody[bot]](https://avatars.githubusercontent.com/u/287058192?s=60&v=4){kind=small}
| guard o < nativeSubtitleTrackTable.count, nativeSubtitleTrackTable[o].needsOCR, | ||
| let source = nativeSubtitleTrackTable[o].sourceStreamIndex, | ||
| Int32(source) == activeEmbeddedSubtitleStreamIndex else { return nil } |
There was a problem hiding this comment.
Indefinite waiting in awaitSubtitleOCRCoverage occurs when a requested native bitmap rendition differs from activeEmbeddedSubtitleStreamIndex or the overlay is off: setNativeSubtitleSelected(track:) independently selects the rendition, but the overlay-dependent guard leaves its OCR worker unarmed. Arm the worker for the requested native rendition independently of the overlay selection.
Prompt for LLM
File Sources/AetherEngine/AetherEngine+SubtitleOCR.swift:
Line 68 to 70:
Indefinite waiting in awaitSubtitleOCRCoverage occurs when a requested native bitmap rendition differs from activeEmbeddedSubtitleStreamIndex or the overlay is off: setNativeSubtitleSelected(track:) independently selects the rendition, but the overlay-dependent guard leaves its OCR worker unarmed. Arm the worker for the requested native rendition independently of the overlay selection.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
Understood. Since this code is unchanged from upstream def48fe8 and outside the merge resolution, this suggestion should not block PR #26. The potential OCR waiting issue belongs in a separate upstream report, not a behavior change bundled into this sync.
| if codecID == AV_CODEC_ID_ALAC, let frameLength = alacFrameLength(par) { | ||
| enc.pointee.frame_size = frameLength | ||
| } | ||
| let openRet = avcodec_open2(enc, codec, nil) |
There was a problem hiding this comment.
ALAC decoding failures occur when the source cookie declares a smaller frameLength than FFmpeg's 4096-sample frame size: avcodec_open2 overrides the previously assigned frameLength, and the encoding loop generates tail packets that exceed the unchanged track configuration. Verify the opened encoder's frame size against the cookie and decline padding when compatible packets cannot be produced.
Prompt for LLM
File Sources/AetherEngine/Audio/AudioTailPadding.swift:
Line 112 to 115:
ALAC decoding failures occur when the source cookie declares a smaller frameLength than FFmpeg's 4096-sample frame size: avcodec_open2 overrides the previously assigned frameLength, and the encoding loop generates tail packets that exceed the unchanged track configuration. Verify the opened encoder's frame size against the cookie and decline padding when compatible packets cannot be produced.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
Understood—this is inherited upstream code, not a merge-resolution regression. The ALAC compatibility concern is worth raising upstream separately, but it shouldn’t block this sync-only PR or require a fork-specific change here.
| let readRet = av_read_frame(fmt, pkt) | ||
| if readRet < 0 { | ||
| trackedPacketFree(&pktPtr) | ||
| if preparationPolicy != nil && readRet != FFmpegErr.eof { throw SubtitleDecoderError.openFailed(code: readRet) } |
There was a problem hiding this comment.
Decoded cue loss occurs when preparationPolicy is present because every non-EOF demuxer error throws SubtitleDecoderError.openFailed(code: readRet), discarding accumulated cues and bypassing the subsequent AVIO and relay failure checks. Break the loop instead so demuxer errors preserve decoded cues and actual source failures produce the existing truncated result with partial cues.
Kody rule violation: Distinguish subtitle source failures from demuxer errors
// Preserve decoded cues; classify source failures using AVIO and relay state below.Prompt for LLM
File Sources/AetherEngine/Decoder/SubtitleDecoder.swift:
Line 619:
Decoded cue loss occurs when preparationPolicy is present because every non-EOF demuxer error throws SubtitleDecoderError.openFailed(code: readRet), discarding accumulated cues and bypassing the subsequent AVIO and relay failure checks. Break the loop instead so demuxer errors preserve decoded cues and actual source failures produce the existing truncated result with partial cues.
Suggested Code:
// Preserve decoded cues; classify source failures using AVIO and relay state below.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
Understood—this is unchanged upstream code, not a merge-resolution regression. It’s reasonable to leave it untouched in this sync PR and raise the potential cue-loss issue upstream separately.
I can help draft an upstream report if useful.
| guard prefetchDiskBudgetBytes > 0 else { return true } | ||
| if pumpQoSCurrent == QOS_CLASS_UTILITY { pumpRelaxationRate.reset() } |
There was a problem hiding this comment.
Premature relaxed-rate probe resets occur at every VOD segment boundary because advanceMuxer calls awaitPrefetchDiskBudgetRelease even when SegmentCache.awaitPrefetchDiskHeadroom immediately returns true, discarding accumulated relaxed stretches before the eight-second measurement window can complete on short segments. Reset the probe only after confirming the pump actually needs to park.
Prompt for LLM
File Sources/AetherEngine/Video/HLSSegmentProducer.swift:
Line 2052 to 2053:
Premature relaxed-rate probe resets occur at every VOD segment boundary because advanceMuxer calls awaitPrefetchDiskBudgetRelease even when SegmentCache.awaitPrefetchDiskHeadroom immediately returns true, discarding accumulated relaxed stretches before the eight-second measurement window can complete on short segments. Reset the probe only after confirming the pump actually needs to park.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
Understood—this is an inherited upstream concern, not a merge-resolution regression. It shouldn’t block this sync PR or require a fork-specific fix here.
The probe-reset concern is worth raising separately upstream, with the reset deferred until the pump actually needs to park. No issue or rule has been created.
| guard let payload = keyframes.keyframePayload(at: index, isStale: isStale) else { return nil } | ||
| let segment = plan[index] | ||
| guard let built = builder.build(payload: payload, durationSeconds: segment.durationSeconds) else { | ||
| return nil |
There was a problem hiding this comment.
Stale source preview requests still build and return files when they become stale during keyframePayload because the staleness check precedes the source read; if a newer cached preview finishes first, the older result subsequently calls AetherScrubPreviewView.present, becomes the view's newest request, and displays the wrong scrub position. Reject stale requests after the read and before returning the completed preview.
guard let payload = keyframes.keyframePayload(at: index, isStale: isStale), !isStale() else { return nil }Prompt for LLM
File Sources/AetherEngine/Video/HLSVideoEngine.swift:
Line 3080 to 3083:
Stale source preview requests still build and return files when they become stale during keyframePayload because the staleness check precedes the source read; if a newer cached preview finishes first, the older result subsequently calls AetherScrubPreviewView.present, becomes the view's newest request, and displays the wrong scrub position. Reject stale requests after the read and before returning the completed preview.
Suggested Code:
guard let payload = keyframes.keyframePayload(at: index, isStale: isStale), !isStale() else { return nil }
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
Agreed—this code is unchanged from upstream def48fe8, so the suggestion is outside this sync PR’s scope and shouldn’t block the merge. The potential stale-preview race is worth raising upstream separately, rather than introducing a fork-only behavior change here.
| if url == shownURL { return true } | ||
| request &+= 1 |
There was a problem hiding this comment.
Stale presentation occurs when A is displayed, B starts loading, and another request for A takes the shownURL fast path without advancing request, allowing B to replace the newly requested A when ready. Advance the generation before the equality check so every request supersedes earlier suspended presentations.
request &+= 1
if url == shownURL { return true }Prompt for LLM
File Sources/AetherEngine/View/AetherScrubPreviewView.swift:
Line 110 to 111:
Stale presentation occurs when A is displayed, B starts loading, and another request for A takes the shownURL fast path without advancing request, allowing B to replace the newly requested A when ready. Advance the generation before the equality check so every request supersedes earlier suspended presentations.
Suggested Code:
request &+= 1
if url == shownURL { return true }
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
There was a problem hiding this comment.
Understood—this is an upstream issue, not a merge-resolution regression. The race remains valid, but it shouldn’t block this sync-only PR. Keeping upstream behavior unchanged here makes sense; the fix and an A → B → A regression test belong in a separate upstream follow-up.
commitExternalSubtitle(_:secondary: true) published a prepared ASS candidate's raw event lines and its script header when preserveASSMarkup was on, so a host drawing secondary cues as text showed event fields and override tags. The commit now parses those events into styled text and placement, as the ordinary secondary decode does, and leaves secondarySidecarASSHeader nil. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df72cc5960
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if url == shownURL { return true } | ||
| request &+= 1 |
There was a problem hiding this comment.
When picture A is displayed, present(B) is awaiting readiness, and a newer scrub request returns to A, this early return leaves request unchanged. The pending B request therefore still passes its supersession check and replaces A once ready, leaving the preview at an obsolete scrub position despite the latest call reporting success. Advance the request token before the shownURL fast path so returning to the displayed picture also invalidates any pending replacement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
| nonisolated static func loadDisplayCriteriaAction(engineWritesCriteria: Bool, audioOnlyPath: Bool) -> LoadDisplayCriteriaAction { | ||
| (!engineWritesCriteria || audioOnlyPath) ? .clearStale : .applyFresh |
There was a problem hiding this comment.
On tvOS, if an engine-owned session previously applied display criteria and the host writes its own criteria before the next load with displayCriteriaOwner = .host, this decision sends the load through .clearStale. loadSession preserves the controller's previous didApply flag across teardown, and DisplayCriteriaController.reset() checks only that flag before unconditionally assigning preferredDisplayCriteria = nil; it never verifies that the current criteria still belong to the engine. Consequently, the new host-owned load erases the host's already-written range/rate settings. Distinguish host ownership here and clear only criteria that are still attributable to the outgoing engine session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This code comes unchanged from upstream def48fe8; the fork's merge resolution doesn't touch it. This PR only syncs the fork with upstream, so I'm keeping upstream behaviour changes out of it. Worth raising upstream separately.
…er (#902) * chore(deps): pin AetherEngine with upstream's exact TrueHD Atmos render Upstream AetherEngine #731 decodes TrueHD Atmos exactly again and renders its objects into the bed, replacing 7.40's bed split, which left the heights 0.4 to 1.1 dB low and could not tell front heights from rear ones. Silo-Server/AetherEngine#26 merges upstream main (def48fe8, after 7.47.0) into the fork. - Pin the fork at ff626231. LibTrueHD moves to 3.1.0, FFmpegBuild to 3.10.0, and LibDTSX drops out: DTS:X heights now come from FFmpegBuild's patched dca decoder. - Notices, the bundled overview and Acknowledgements drop LibDTSX and name the new revisions. - CI: build-input fingerprint and selector seal for the new project.yml; the release source proof's package graph is 11 packages. - Describe the exact render in the tvOS player docs and the setting's doc comment, and update the engine log lines the redactor tests use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore(deps): pin AetherEngine with the prepared secondary ASS fix Moves the AetherEngine pin to df72cc59, which keeps a prepared secondary ASS subtitle commit as styled text in the Silo fork, and reseals the CI build inputs and selector contract for it. The loopback spec no longer mentions a 9.1.6 DTS:X bed, which Silo never selects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: drop a stray Python bytecode file Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Brings the Silo fork up to upstream AetherEngine
mainatdef48fe8(7.47.0 plus unreleased fixes) and keeps every Silo patch. The main reason is upstream superuser404notfound#731: TrueHD Atmos is decoded exactly again and its objects are rendered into the bed, which replaces the 7.40.0 bed split. The bed split left the heights 0.4–1.1 dB low and could not tell front heights from rear ones. LibTrueHD moves to 3.1.0, whose shipped binary includes the interpolating decode.What changed
Upstream changes that matter to Silo:
objectAudioRendering = .apac(layout)now mixes the objects with size, snap, zones, screen anchoring and the elevation switch.BedSplitterand theTrueHDReferenceRendertrait are gone. The log line is nowTrueHD Atmos: objects rendered into ….dcadecoder reads the heights, soDTSXObjectAudiolinks no library of its own and LibDTSX drops out of the package graph. FFmpegBuild is now 3.10.0..utilitystays.userInitiated. This was measured on a 4K Dolby Vision remux with TrueHD Atmos rendered to APAC.isClockHeld/clockHoldEvents(Software path after an in-place switch: on a title with no audio the clock starts about 0.93 s before the first frame, and 5 to 7 frames drop superuser404notfound/AetherEngine#800), audio padded to the end of the video (A VOD source whose audio ends before its video stops at the audio's end on the loopback route: the rest of the picture is never shown superuser404notfound/AetherEngine#847), media-scoped subtitle headers (fix(subtitles): scope inherited headers to the media origin superuser404notfound/AetherEngine#822), transactional external subtitle preparation (feat(subtitles): prepare and commit external selections transactionally superuser404notfound/AetherEngine#829), live edge cushion (Live edge cushion deeper than ~20 s never starts on tvOS: first manifest outlives AVPlayer's playlist timer (-16839) superuser404notfound/AetherEngine#851), HDR10+ on the software path (fix(hdr10+): carry HDR10+ on the software path superuser404notfound/AetherEngine#814), and macOS:selectAudioTrackto an AC-3 track dies on the master rejection (-11868) instead of recovering; initial load of the same source recovers superuser404notfound/AetherEngine#849's master-fallback fixes.The fork's default traits are unchanged:
TrueHDObjectAudioandDTSXObjectAudiostay on, and the Dolby Vision traits stay off.Merge conflicts, resolved by keeping both sides:
SubtitleDecoder: the fork's authorized relay and idle timeout, plus upstream'spreparationPolicy, threaded throughdecodeBlocking.AetherEngine+Subtitles: the fork's truncation retry, authorization and timeline offset, plus upstream'sresolvedSubtitleHeaders(scoped to the media origin) andpreparationPolicy.RemoteHLSSubtitleProvider/Proxy: upstream'smediaURL, plus the fork's relay authorization, authorization-keyed fill jobs andcancelDecode()instartFill, which leaves the timestamp-anchor probe running.AetherEngine.selectAudioTrack: upstream'skeepsDisplayRefusedMediaRoute: true, plus the fork'saudioSwitchInFlightcleanup.SessionOptionCorrection.knownFields: upstream'sdisplayCriteriaOwnerand the fork'shttpRequestAuthorization, inLoadOptionsorder.docs/cli.md: upstream's text with the fork's 8-channelapac.31.02level.docs/api.md: upstream's row plus theObjectAudioRenderingreference thatPublicAPIDocumentationTestsrequires.CHANGELOG.md: upstream's sections, with the fork's entries kept under Unreleased.One behaviour stays fork-specific:
preserveASSMarkupand publishes its raw events andsecondarySidecarASSHeader. The fork already keeps raw ASS on the primary overlay only, for embedded secondary tracks too (f8239c20), because Silo draws secondary cues as text. With the upstream change, an ASS secondary in Silo would show its event lines. The fix(subtitles): preserve secondary sidecar ASS metadata superuser404notfound/AetherEngine#826 tests now pin the fork's behaviour, andsecondarySidecarASSHeaderstays nil. The same rule covers feat(subtitles): prepare and commit external selections transactionally superuser404notfound/AetherEngine#829'scommitExternalSubtitle(_:secondary: true): it parses a prepared ASS candidate's raw events into styled text and placement instead of publishing them.Three follow-up commits after the merge: the fork's authorization tests pass the new required
mediaURL:argument, the secondary-sidecar change above, and the same change for the prepared secondary commit (df72cc59, from review).Test plan
swift build --build-testspasses with the default traits (TrueHD and DTS:X on) and with--enable-all-traits.df72cc59: 789 XCTest (1 skipped) and 5,034 Swift Testing tests pass. The newsecondaryASSCommitStaysPlainWhenMarkupIsPreservedfails onff626231and passes with the fix. The authorization group passed 56 of 56 atff626231.ff626231: 791 XCTest (1 skipped) and 5,225 Swift Testing tests pass. Not rerun fordf72cc59, which touches no trait-gated code.xcodebuild build-for-testingjobs, which CI covers.ff626231.Evidence: none, no user-visible change in this package (playback is checked in the silo-apple PR).
Checklist
CHANGELOG.mdupdated (upstream's entries; the fork's stay under Unreleased)secondarySidecarASSHeaderis documented as nil in the fork)Merge with a merge commit, not a squash: silo-apple pins this branch's head,
df72cc59.AI disclosure
claude-opus-5-5🤖 Generated with Claude Code
Note
Merge upstream AetherEngine 7.47.0 including TrueHD Atmos exact render into Silo fork
Brings the upstream main branch (def48fe) into the fork, raising
AetherEngine.versionfrom 7.40.1 to 7.47.0 and updating Package.swift, Package.resolved, README.md, and CHANGELOG.md.TrueHDReferenceRendertrait are removed, andSpatialAudioBridgesends decoded object planes straight toObjectAudioRenderer.scrubpreviewaetherctl subcommand.TrueHDReferenceRendertrait, the removed LibDTSX dependency,DTSXObjectAudioDecoder's changed output model (fixed 7.1.4 channels instead of runtime layouts), and the removedObjectAudioBed/playsBedOnlyAPI in ObjectAudioDecoding.swift.Macroscope summarized df72cc5.