From 2e7bf96988a4247d0c7f834323ea74084fc199b0 Mon Sep 17 00:00:00 2001 From: Vincent Herbst Date: Sun, 9 Aug 2026 17:45:28 +0200 Subject: [PATCH 1/3] fix(engine): drop the previous session's sinks before wiring the new one's softwareCancellables.removeAll() stood in the middle of loadSoftware's wiring, between two groups of .store(in:) calls, so every sink attached above it was cancelled the moment it ran. What was above it is the SW-PiP cue mirror: after the frame compositor is armed, the CombineLatest over subtitleCues + secondarySubtitleCues never delivered again, and the only cues the compositor ever saw were the snapshot pushed by the pictureInPictureActive didSet. Subtitles in a software-path PiP window therefore froze at whatever was on screen when PiP started. Hoisted above the wiring, where a reset of the outgoing session belongs. The teardown in stopInternal already covers the ordinary path; this one covers a load that reaches here without one. Nothing catches this in unit tests, because both halves work in isolation: the publisher publishes, the sink is correct, and the cancel is invisible from either end. It surfaced while measuring a new sink on the same path with aetherctl play, which is where a session runs long enough to notice that nothing arrives. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HX5zV3Fcf7Nzq4DYGdXvQE --- CHANGELOG.md | 8 +++++++- Sources/AetherEngine/AetherEngine+Loading.swift | 6 +++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98d37658..ade7add2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,13 @@ the public-API contract. ## [Unreleased] -_Nothing yet._ +### Fixed + +- The software load path cancelled every Combine sink it had already wired. + `softwareCancellables.removeAll()` stood between two groups of `.store(in:)` + calls, so the SW-PiP cue mirror never delivered a cue after the frame + compositor was armed, and subtitles in a software-path PiP window froze at + whatever was on screen when PiP started. ## [6.16.2] - 2026-08-09 diff --git a/Sources/AetherEngine/AetherEngine+Loading.swift b/Sources/AetherEngine/AetherEngine+Loading.swift index c41db024..a99a93d8 100644 --- a/Sources/AetherEngine/AetherEngine+Loading.swift +++ b/Sources/AetherEngine/AetherEngine+Loading.swift @@ -1235,6 +1235,11 @@ extension AetherEngine { } activateRendererAudioSession(audioSourceStreamIndex: audioSourceStreamIndex) + // Drop the previous session's sinks BEFORE anything wires this one's. Standing further down, + // between two groups of `.store(in:)` calls, this cancelled everything wired above it: the + // SW-PiP cue mirror never delivered a cue after the frame compositor was armed. Both halves + // of such a wiring work in isolation, which is why a dead sink here reads as a working one. + softwareCancellables.removeAll() let host = SoftwarePlaybackHost() host.deinterlaceConfig = DeinterlaceConfig( mode: loadedOptions.deinterlaceMode, @@ -1296,7 +1301,6 @@ extension AetherEngine { self.playlistShiftSeconds = 0 self.setPresentationAxis(PresentationAxisMap()) - softwareCancellables.removeAll() host.$currentTime .sink { [weak self] value in guard let self = self else { return } From 998650e32eea144955e677fad148dffe9957c0fc Mon Sep 17 00:00:00 2001 From: Vincent Herbst Date: Sun, 9 Aug 2026 17:46:23 +0200 Subject: [PATCH 2/3] feat(engine): publish the display size the software path settled on A host laying something out over the picture (a libass overlay, the case #311 gave a clock and frame boundaries to) needs the rectangle the frames land in. The only public size was sourceVideoWidth / sourceVideoHeight, which are the CODED dimensions, so anamorphic content was laid out against the wrong rectangle: 720x576 at 64:45 presents as 1024x576, and an overlay sized 5:4 sits inside a 16:9 picture. There is nothing to measure on the layer either, AVSampleBufferDisplayLayer has no videoRect. A host cannot compute it. The ratio is resolved per frame across three sources, first sane wins (#177), and one whose display aspect is impossible is dropped in favour of square pixels (#290), so a host reconstructing it from container metadata disagrees with the screen in exactly the cases that policy exists for. softwareDisplaySize is therefore read off the format description the renderer enqueues, via CMVideoFormatDescriptionGetPresentationDimensions, rather than recomputed from the SAR: that description is the object the layer is handed, and the cache key it lives behind already invalidates on a dimension or PAR change, which is exactly when the size has to be republished. Optional rather than .zero, the argument that shaped SoftwareVideoFrameTime in #311: there is no display size before the first frame is built, and a placeholder in a non-optional field is indistinguishable from a measurement at the call site. Mirrored rather than latched, unlike hasFirstFrameReadyForDisplay: a live source that switches resolution re-shapes the rectangle under a host that already laid out against it. Cleared with the session, so the next source is not laid out against this one's picture. Measured with aetherctl play --sw --frame-times, which now reports disp=WxH per tick and the settled size next to the coded one: 720x576 SAR 64:45 -> disp=1024x576 (coded 720x576) 1280x720 square -> disp=1280x720 (coded 1280x720) Re #353 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HX5zV3Fcf7Nzq4DYGdXvQE --- CHANGELOG.md | 15 ++ README.md | 9 +- .../AetherEngine/AetherEngine+Loading.swift | 24 +++ Sources/AetherEngine/AetherEngine.swift | 27 +++ .../Native/SoftwarePlaybackHost.swift | 24 +++ .../Renderer/SampleBufferRenderer.swift | 42 ++++ Sources/aetherctl/PlaybackCmd.swift | 20 ++ .../Issue353SoftwareDisplaySizeTests.swift | 200 ++++++++++++++++++ 8 files changed, 360 insertions(+), 1 deletion(-) create mode 100644 Tests/AetherEngineTests/Issue353SoftwareDisplaySizeTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index ade7add2..88602f3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,21 @@ the public-API contract. ## [Unreleased] +### Added + +- `AetherEngine.softwareDisplaySize`: the size the software path's picture + presents at, the coded frame under the pixel aspect ratio the decoder attached + (#353). A host laying an overlay out over the picture had only + `sourceVideoWidth` / `sourceVideoHeight`, which are the CODED size, so + anamorphic content was laid out against the wrong rectangle (720x576 at 64:45 + presents as 1024x576), and `AVSampleBufferDisplayLayer` carries no `videoRect` + to measure instead. Nor could a host compute it: the ratio is resolved per + frame across three sources (#177) and one whose display aspect is impossible + is dropped in favour of square pixels (#290). Read off the format description + the renderer enqueues rather than recomputed from the SAR, so it cannot + disagree with the screen. nil off the software path and before the first + frame; it follows a mid-stream format change and is cleared with the session. + ### Fixed - The software load path cancelled every Combine sink it had already wired. diff --git a/README.md b/README.md index 4ee827fa..4b008c33 100644 --- a/README.md +++ b/README.md @@ -250,9 +250,16 @@ player.softwarePresentationTimebase // the master clock, on the so player.setSoftwareVideoFrameTimeObserver { frame in frame.presentation; frame.generation } + +// The rectangle those frames land in (#353): coded dimensions under the pixel aspect ratio the +// decoder attached, read off the format description the renderer enqueues. `sourceVideoWidth` and +// `sourceVideoHeight` are the CODED size, so anamorphic content laid out against them is off by the +// pixel aspect (720x576 at 64:45 presents as 1024x576). nil off the software path and before the +// first frame; it follows a mid-stream format change and is cleared with the session. +player.softwareDisplaySize // CGSize?, @Published ``` -Subtitle cues land in raw source PTS; render the overlay against `player.sourceTime` (see [docs/formats.md › Subtitles](docs/formats.md#subtitles)). A host compositing its own overlay onto the native path (libass and friends) needs the item axis too, since that is what the compositor pairs its samples against: `presentationAxisMap` converts arbitrary positions, `setNativeVideoFrameTimeObserver` reports the frames themselves. On the software path neither is needed: `softwarePresentationTimebase` hands out the clock the frames are presented against and `setSoftwareVideoFrameTimeObserver` reports them, both on the same axis as the cues. Both return nothing rather than a guess when no axis is established, because a defaulted shift is indistinguishable from a measured one at the call site. The 1 Hz diagnostics snapshot lives on `player.diagnostics.liveTelemetry`, off-the-engine for the same render-stability reason. Frame extraction, authored-ASS styling, and the full published surface are documented in [docs/formats.md](docs/formats.md). +Subtitle cues land in raw source PTS; render the overlay against `player.sourceTime` (see [docs/formats.md › Subtitles](docs/formats.md#subtitles)). A host compositing its own overlay onto the native path (libass and friends) needs the item axis too, since that is what the compositor pairs its samples against: `presentationAxisMap` converts arbitrary positions, `setNativeVideoFrameTimeObserver` reports the frames themselves. On the software path neither is needed: `softwarePresentationTimebase` hands out the clock the frames are presented against and `setSoftwareVideoFrameTimeObserver` reports them, both on the same axis as the cues, and `softwareDisplaySize` gives the rectangle to lay the overlay out in (the native path measures its own on `AVPlayerLayer.videoRect`). Both return nothing rather than a guess when no axis is established, because a defaulted shift is indistinguishable from a measured one at the call site. The 1 Hz diagnostics snapshot lives on `player.diagnostics.liveTelemetry`, off-the-engine for the same render-stability reason. Frame extraction, authored-ASS styling, and the full published surface are documented in [docs/formats.md](docs/formats.md). Install via Swift Package Manager: diff --git a/Sources/AetherEngine/AetherEngine+Loading.swift b/Sources/AetherEngine/AetherEngine+Loading.swift index a99a93d8..b09cc66b 100644 --- a/Sources/AetherEngine/AetherEngine+Loading.swift +++ b/Sources/AetherEngine/AetherEngine+Loading.swift @@ -146,6 +146,27 @@ extension AetherEngine { hasFirstFrameReadyForDisplay = true } + /// #353: mirror a software host's settled picture size onto the public `softwareDisplaySize`. + /// + /// A mirror rather than the latch `hasFirstFrameReadyForDisplay` gets, because the two answer + /// different questions. A picture that exists cannot stop existing for the rest of the load, but + /// the size it presents at can change under it: a live source that switches resolution + /// mid-stream re-shapes the rectangle a host already laid out against, and a latched first value + /// would keep the overlay on the old one. + /// + /// No `dropFirst()` here either, and that is a property of this path rather than a style choice: + /// the software path builds a new host per load (one construction site, and `stopInternal` nils + /// it), so what a fresh mirror replays is that host's own nil and not the outgoing item's size. + /// The native hosts, which are the ones reused across a load, have no size to mirror. + func mirrorSoftwareDisplaySize( + from publisher: Published.Publisher, + storeIn cancellables: inout Set + ) { + publisher + .sink { [weak self] size in self?.softwareDisplaySize = size } + .store(in: &cancellables) + } + /// `videoReadyForDisplay` is the host's raw layer level (#315); nil on the audio hosts, which /// have nothing to display. It is folded, never mirrored: the engine's published flag is latched /// for the load, so the seams that reuse a host and briefly lose the picture do not surface. @@ -1256,6 +1277,9 @@ extension AetherEngine { // #311: a load builds a new host and a new renderer, so an observer installed once by the // host app has to be carried across the seam, exactly as the native session does at load. host.setVideoFrameTimeObserver(softwareVideoFrameTimeObserver) + // #353: the settled picture size, wired next to the frame times because a host laying out an + // overlay needs the rectangle as well as the clock, and both come off this renderer. + mirrorSoftwareDisplaySize(from: host.$videoDisplaySize, storeIn: &softwareCancellables) // SW-PiP: publish the bridge once the session owns its layer (the layer object is stable for // the session; the host attaches it to the view and, on PiP start, to the system window). softwarePiPSource = SoftwarePiPSource(layer: host.displayLayer, isLive: isLive, engine: self) diff --git a/Sources/AetherEngine/AetherEngine.swift b/Sources/AetherEngine/AetherEngine.swift index c3898f8b..d56af418 100644 --- a/Sources/AetherEngine/AetherEngine.swift +++ b/Sources/AetherEngine/AetherEngine.swift @@ -604,6 +604,30 @@ public final class AetherEngine: ObservableObject { /// display layer, nil on teardown. Hosts build their sample-buffer PiP ContentSource from it. @Published public internal(set) var softwarePiPSource: SoftwarePiPSource? + /// #353: the size the software path's picture presents at, in pixels: the coded frame under the + /// pixel aspect ratio the decoder attached. nil on every other path, before the first frame is + /// built, and on sources with no video. + /// + /// What it is for is laying something out over the picture. A host derives the picture rect from + /// an aspect under the active `videoGravity`, and `sourceVideoWidth`/`sourceVideoHeight` are the + /// CODED dimensions, so anamorphic content lays out against the wrong rectangle: 720x576 at + /// 64:45 presents as 1024x576, and an overlay sized 5:4 sits inside a 16:9 picture. There is + /// nothing to measure on the layer either, since `AVSampleBufferDisplayLayer` has no `videoRect` + /// the way `AVPlayerLayer` does. + /// + /// Nor can a host compute it. The ratio is resolved per frame across three sources, first sane + /// wins (#177), and one whose display aspect is impossible is dropped in favour of square pixels + /// (#290), so a host reconstructing it from container metadata disagrees with the screen in + /// exactly the cases that policy exists for. This is read off the format description the + /// renderer enqueues, so it is what the layer was handed rather than a second opinion about it. + /// + /// Mirrored, not latched, unlike `hasFirstFrameReadyForDisplay`: a live source that switches + /// resolution mid-stream changes the shape of the picture under a host that already laid out + /// against it, and it is cleared with the session so the next source cannot be laid out against + /// this one's rectangle. The native and bypass paths mount an `AVPlayerLayer`, which measures its + /// own `videoRect` and carries `AVPlayerItem.presentationSize`; this stays nil there. + @Published public internal(set) var softwareDisplaySize: CGSize? + /// #288: the native-path counterpart of `softwarePiPSource.layer`. `AVPictureInPictureController` /// wants the layer, not the player, so a host presenting its own PiP on the native path (tvOS has /// no reachable AVKit affordance behind suppressed chrome) cannot get there from `currentAVPlayer`. @@ -4769,6 +4793,9 @@ public final class AetherEngine: ObservableObject { } softwareCancellables.removeAll() + // #353: the picture belongs to the session. Left standing, the next source would be laid out + // against this one's rectangle for as long as it takes its own first frame to arrive. + softwareDisplaySize = nil // #314: same detach on the software path, where the outgoing renderer's decode thread is what // can still hand a frame over while the next host comes up. softwareHost?.setVideoFrameTimeObserver(nil) diff --git a/Sources/AetherEngine/Native/SoftwarePlaybackHost.swift b/Sources/AetherEngine/Native/SoftwarePlaybackHost.swift index f6d6b733..b415ece1 100644 --- a/Sources/AetherEngine/Native/SoftwarePlaybackHost.swift +++ b/Sources/AetherEngine/Native/SoftwarePlaybackHost.swift @@ -69,6 +69,12 @@ final class SoftwarePlaybackHost { /// with the session. private var readyForDisplayObserver: NSObjectProtocol? + /// #353: the size this session's picture presents at, coded dimensions under the pixel aspect + /// ratio the decoder attached; nil until the renderer builds its first sample buffer and on + /// sources with no video. The engine mirrors it as `AetherEngine.softwareDisplaySize`, which is + /// what hosts read; this host is built per load, so it starts unknown by construction. + @Published private(set) var videoDisplaySize: CGSize? + /// Fires (off-main) once per session the first time HDR10+ dynamic /// metadata appears on a decoded frame. Hooked by `AetherEngine` to /// upgrade the published `videoFormat` from `.hdr10` → `.hdr10Plus`. @@ -458,6 +464,24 @@ final class SoftwarePlaybackHost { // Default to the software decoder; load() swaps it for the // VT-backed one when the source's video codec is HEVC. self.videoDecoder = SoftwareVideoDecoder() + armDisplaySizeObserver() + } + + /// #353: the renderer settles the picture size on the decode thread, where it builds the format + /// description; publish it on the main actor like every other mirror on this host. Armed in init + /// rather than at load: the renderer is this host's own and lives exactly as long as it does. + private func armDisplaySizeObserver() { + renderer.setDisplaySizeObserver { [weak self] size in + Task { @MainActor in + guard let self, self.videoDisplaySize != size else { return } + self.videoDisplaySize = size + EngineLog.emit( + "[SWHost] picture settles at \(Int(size.width))x\(Int(size.height)) " + + "after \(self.framesEnqueued) frames", + category: .swPlayback + ) + } + } } // MARK: - Audio stream resolution (#133) diff --git a/Sources/AetherEngine/Renderer/SampleBufferRenderer.swift b/Sources/AetherEngine/Renderer/SampleBufferRenderer.swift index 75818f2f..cf56868c 100644 --- a/Sources/AetherEngine/Renderer/SampleBufferRenderer.swift +++ b/Sources/AetherEngine/Renderer/SampleBufferRenderer.swift @@ -111,6 +111,33 @@ final class SampleBufferRenderer: @unchecked Sendable { return _newestEnqueuedPtsSeconds } + /// #353: the size the picture presents at, which is the coded frame under the pixel aspect ratio + /// the decoder attached; nil before the first sample buffer is built. Read off the description + /// that is enqueued rather than recomputed from the SAR: the ratio is resolved per frame across + /// three sources (#177) and a ratio whose display aspect is impossible is dropped (#290), so a + /// second computation of the same answer is a second thing that can disagree with the screen. + /// Guarded by `reorderLock`. + private var _displaySize: CGSize? + var displaySize: CGSize? { + reorderLock.lock() + defer { reorderLock.unlock() } + return _displaySize + } + + /// #353: fires when the settled display size CHANGES, on the decode thread, plus once on + /// installation if the picture already settled. Compared against the value and not against the + /// description, because `flush()` drops the cached description and every seek therefore rebuilds + /// one for a picture that never changed shape. The late-installation call is what a host relies + /// on: on a source with one format, the only report ever due has already happened. + private var _displaySizeObserver: (@Sendable (CGSize) -> Void)? + func setDisplaySizeObserver(_ observer: (@Sendable (CGSize) -> Void)?) { + reorderLock.lock() + _displaySizeObserver = observer + let settled = _displaySize + reorderLock.unlock() + if let settled { observer?(settled) } + } + init() { displayLayer = Self.makeDisplayLayer(isHDR: false) } @@ -378,6 +405,14 @@ final class SampleBufferRenderer: @unchecked Sendable { } } + /// #353: what the layer will draw the description at. Pixel aspect ratio and clean aperture are + /// extensions of the description itself, so this asks the description what it presents at + /// instead of repeating the decision that built it. + static func presentationSize(of desc: CMVideoFormatDescription) -> CGSize { + CMVideoFormatDescriptionGetPresentationDimensions( + desc, usePixelAspectRatio: true, useCleanAperture: true) + } + /// Internal (not private) for #177 regression tests: the PAR-keyed cache behavior is the fix. func createSampleBuffer(from pixelBuffer: CVPixelBuffer, pts: CMTime) -> CMSampleBuffer? { // Cache hit avoids CMVideoFormatDescriptionCreateForImageBuffer allocation + CF refcount churn on every frame. @@ -410,10 +445,17 @@ final class SampleBufferRenderer: @unchecked Sendable { formatDescriptionOut: &formatDesc ) guard status == noErr, let new = formatDesc else { return nil } + // #353: a new description is the only moment the picture can change shape, so the + // settled size is taken here and reported outside the lock. + let settled = Self.presentationSize(of: new) reorderLock.lock() cachedFormatDesc = new cachedFormatKey = key + let changed = settled != _displaySize + if changed { _displaySize = settled } + let sizeObserver = changed ? _displaySizeObserver : nil reorderLock.unlock() + sizeObserver?(settled) desc = new } diff --git a/Sources/aetherctl/PlaybackCmd.swift b/Sources/aetherctl/PlaybackCmd.swift index 609e4374..c820b07c 100644 --- a/Sources/aetherctl/PlaybackCmd.swift +++ b/Sources/aetherctl/PlaybackCmd.swift @@ -363,6 +363,11 @@ private func playSmokeTest(url: URL, seconds: Double, live: Bool, nativeHLS: Boo defer { seekEventSub?.cancel() } var overlapVerdicts: [String] = [] + // #353: sampled during the session, because the engine clears the size with the session and the + // summary below prints after teardown. Paired with the coded dimensions read at the same moment. + var observedDisplaySize: CGSize? + var observedCodedSize: (Int32, Int32) = (0, 0) + let ticks = max(1, Int(seconds)) for tick in 1...ticks { try? await Task.sleep(nanoseconds: 1_000_000_000) @@ -390,6 +395,13 @@ private func playSmokeTest(url: URL, seconds: Double, live: Bool, nativeHLS: Boo if let timebase = engine.softwarePresentationTimebase { line += String(format: " tb=%.3fs", timebase.time.seconds) } + // #353: the rectangle the frames land in. Read next to the coded dimensions on purpose: + // on anamorphic content the two differ, and that difference IS the defect being watched. + if let size = engine.softwareDisplaySize { + line += " disp=\(Int(size.width))x\(Int(size.height))" + observedDisplaySize = size + observedCodedSize = (engine.sourceVideoWidth, engine.sourceVideoHeight) + } } print(line) // DVR-seek smoke: rewind 20 s mid-session, then live-edge return 15 s later, so the @@ -500,6 +512,14 @@ private func playSmokeTest(url: URL, seconds: Double, live: Bool, nativeHLS: Boo } if let frameProbe { print("frame times: \(frameProbe.summary())") + // #353: coded next to settled. Equal on square-pixel sources, and on anamorphic content the + // gap is exactly what a host laying out against `sourceVideoWidth` would have got wrong. + if let size = observedDisplaySize { + print("display size: \(Int(size.width))x\(Int(size.height)) " + + "(coded \(observedCodedSize.0)x\(observedCodedSize.1))") + } else { + print("display size: never published (not the software path, or no frame built)") + } } if !finalSubtitleTracks.isEmpty { let listed = finalSubtitleTracks diff --git a/Tests/AetherEngineTests/Issue353SoftwareDisplaySizeTests.swift b/Tests/AetherEngineTests/Issue353SoftwareDisplaySizeTests.swift new file mode 100644 index 00000000..a5b6e1d6 --- /dev/null +++ b/Tests/AetherEngineTests/Issue353SoftwareDisplaySizeTests.swift @@ -0,0 +1,200 @@ +import Combine +import CoreMedia +import CoreVideo +import Foundation +import Testing +@testable import AetherEngine + +/// AE#353: the picture size the software path settled on, so a host overlay lays out against the +/// picture rather than the coded frame. +/// +/// The value has to be read off the format description the renderer enqueues, not recomputed from +/// the SAR policy. `SoftwareVideoDecoder` resolves the ratio per frame (frame -> codec ctx -> +/// stream, #177) and drops one whose display aspect is impossible (#290), so a second computation +/// of the same answer is a second thing that can disagree with the screen. +@Suite("Settled software display size (#353)") +struct Issue353SoftwareDisplaySizeTests { + + private final class SizeCollector: @unchecked Sendable { + private let lock = NSLock() + private var storage: [CGSize] = [] + + var values: [CGSize] { + lock.lock(); defer { lock.unlock() }; return storage + } + + func append(_ value: CGSize) { + lock.lock(); storage.append(value); lock.unlock() + } + } + + private func makeBuffer(width: Int = 720, height: Int = 576, par: (Int, Int)?) -> CVPixelBuffer { + var pb: CVPixelBuffer? + CVPixelBufferCreate( + kCFAllocatorDefault, width, height, + kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange, + [kCVPixelBufferIOSurfacePropertiesKey: NSDictionary()] as NSDictionary, + &pb + ) + let buffer = pb! + if let par { + let aspect: NSDictionary = [ + kCVImageBufferPixelAspectRatioHorizontalSpacingKey: par.0, + kCVImageBufferPixelAspectRatioVerticalSpacingKey: par.1, + ] + CVBufferSetAttachment(buffer, kCVImageBufferPixelAspectRatioKey, aspect, .shouldPropagate) + } + return buffer + } + + private func pts(_ frame: Int64) -> CMTime { + CMTime(value: frame * 3600, timescale: 90000) + } + + // MARK: - The settled value + + @Test("no size is claimed before the first frame is built") + func nothingSettledBeforeTheFirstFrame() { + #expect(SampleBufferRenderer().displaySize == nil) + } + + @Test("square pixels settle at the coded dimensions") + func squarePixelsSettleAtCodedDimensions() throws { + let renderer = SampleBufferRenderer() + + _ = try #require(renderer.createSampleBuffer(from: makeBuffer(par: nil), pts: pts(0))) + + #expect(renderer.displaySize == CGSize(width: 720, height: 576)) + } + + /// The case the whole issue is about: 720x576 at 64:45 is PAL 16:9. A host laying out against + /// the coded frame draws into a 5:4 rect inside a 16:9 picture. + @Test("an anamorphic PAR settles at the width the picture actually has") + func anamorphicPARSettlesAtTheDisplayWidth() throws { + let renderer = SampleBufferRenderer() + + _ = try #require(renderer.createSampleBuffer(from: makeBuffer(par: (64, 45)), pts: pts(0))) + + #expect(renderer.displaySize == CGSize(width: 1024, height: 576)) + } + + // MARK: - When it is reported + + @Test("the settled size is reported once, not once per frame") + func reportedOnChangeNotPerFrame() throws { + let renderer = SampleBufferRenderer() + let collector = SizeCollector() + renderer.setDisplaySizeObserver { collector.append($0) } + + for frame in 0..<5 { + _ = try #require(renderer.createSampleBuffer( + from: makeBuffer(par: (64, 45)), pts: pts(Int64(frame)))) + } + + #expect(collector.values == [CGSize(width: 1024, height: 576)]) + } + + /// A mid-stream PAR change at identical geometry is exactly what the format cache invalidates + /// on (#177), and it is the one moment the picture changes shape under a host. + @Test("a PAR change at identical geometry reports the new size") + func parChangeReportsTheNewSize() throws { + let renderer = SampleBufferRenderer() + let collector = SizeCollector() + renderer.setDisplaySizeObserver { collector.append($0) } + + _ = try #require(renderer.createSampleBuffer(from: makeBuffer(par: nil), pts: pts(0))) + _ = try #require(renderer.createSampleBuffer(from: makeBuffer(par: (64, 45)), pts: pts(1))) + + #expect(collector.values == [ + CGSize(width: 720, height: 576), + CGSize(width: 1024, height: 576), + ]) + #expect(renderer.displaySize == CGSize(width: 1024, height: 576)) + } + + /// A seek flushes the format-description cache, so the next frame rebuilds a description that + /// describes the same picture. Reporting that as a change would have every seek re-lay-out an + /// overlay that never moved. + @Test("a format rebuild after a flush reports nothing new") + func flushRebuildIsNotAChange() throws { + let renderer = SampleBufferRenderer() + let collector = SizeCollector() + renderer.setDisplaySizeObserver { collector.append($0) } + + _ = try #require(renderer.createSampleBuffer(from: makeBuffer(par: (64, 45)), pts: pts(0))) + renderer.flush(removingDisplayedImage: false) + _ = try #require(renderer.createSampleBuffer(from: makeBuffer(par: (64, 45)), pts: pts(1))) + + #expect(collector.values == [CGSize(width: 1024, height: 576)]) + #expect(renderer.displaySize == CGSize(width: 1024, height: 576)) + } + + /// An observer installed after the picture settled still has to learn the size it missed: + /// nothing else will change until the format does, which on most sources is never. + @Test("an observer installed after the fact is told the settled size") + func lateObserverIsToldTheSettledSize() throws { + let renderer = SampleBufferRenderer() + _ = try #require(renderer.createSampleBuffer(from: makeBuffer(par: (64, 45)), pts: pts(0))) + + let collector = SizeCollector() + renderer.setDisplaySizeObserver { collector.append($0) } + + #expect(collector.values == [CGSize(width: 1024, height: 576)]) + } + + // MARK: - The engine's public mirror + + /// Stands in for the software host's `@Published private(set) var videoDisplaySize`. + @MainActor + private final class HostDouble { + @Published var videoDisplaySize: CGSize? + init(_ initial: CGSize?) { videoDisplaySize = initial } + } + + @MainActor + @Test("the settled size reaches the engine's published mirror") + func settledSizeReachesTheEngine() async throws { + let engine = try AetherEngine() + var cancellables = Set() + let host = HostDouble(nil) + engine.mirrorSoftwareDisplaySize(from: host.$videoDisplaySize, storeIn: &cancellables) + + #expect(engine.softwareDisplaySize == nil) + + host.videoDisplaySize = CGSize(width: 1024, height: 576) + #expect(engine.softwareDisplaySize == CGSize(width: 1024, height: 576)) + } + + /// Mirrored rather than latched, unlike the first-frame flag of #315. The software path builds a + /// new host per load, so a new session's mirror replays nil and the previous picture cannot be + /// inherited; a latch here would hand the next source the last one's rectangle. + @MainActor + @Test("a fresh session's mirror does not inherit the previous session's picture") + func freshSessionStartsWithNoSize() async throws { + let engine = try AetherEngine() + var cancellables = Set() + let outgoing = HostDouble(nil) + engine.mirrorSoftwareDisplaySize(from: outgoing.$videoDisplaySize, storeIn: &cancellables) + outgoing.videoDisplaySize = CGSize(width: 1024, height: 576) + + let incoming = HostDouble(nil) + engine.mirrorSoftwareDisplaySize(from: incoming.$videoDisplaySize, storeIn: &cancellables) + + #expect(engine.softwareDisplaySize == nil) + } + + @MainActor + @Test("a session teardown clears the size with the session") + func teardownClearsTheSize() async throws { + let engine = try AetherEngine() + var cancellables = Set() + let host = HostDouble(nil) + engine.mirrorSoftwareDisplaySize(from: host.$videoDisplaySize, storeIn: &cancellables) + host.videoDisplaySize = CGSize(width: 1024, height: 576) + + engine.stopInternal() + + #expect(engine.softwareDisplaySize == nil, + "an overlay must not lay out the next source against this one's picture") + } +} From 92a44047e65d0ee7ca27cf003dfd1ea7df05fa86 Mon Sep 17 00:00:00 2001 From: Vincent Herbst Date: Sun, 9 Aug 2026 17:51:03 +0200 Subject: [PATCH 3/3] fix(swpath): attach the pixel aspect ratio on the VT-backed decoder too Anamorphic HEVC on the software host reached the layer at its coded dimensions: 720x576 declaring 64:45 presented as 720x576, a 16:9 picture squashed into 5:4. HardwareVideoDecoder attached no pixel aspect ratio, and the renderer builds its format description from the delivered CVPixelBuffer, so a ratio that is not an attachment on that buffer never reaches the layer at all. The libavcodec decoder on the same host has attached it since #177. The two are chosen per codec inside one host, so the defect was one decoder wide and invisible from every source that takes the other one. Resolved once at open() rather than per frame, because VT delivers pixel buffers and not AVFrames: the per-frame source SoftwareVideoDecoder prefers does not exist here, which also makes the #177 latch unnecessary since one resolution cannot oscillate. Both declared sources are read, since only one of them is the container's, and both existing gates run unchanged: the #177 component bound and the #290 display aspect. Square pixels attach nothing, and a stale attachment on a recycled pool buffer is removed, mirroring the libavcodec path. Measured with aetherctl play --sw --frame-times, one synthetic clip encoded four ways, reading the settled size published by #353: 720x576 SAR 64:45 hevc (VT) 720x576 -> 1024x576 fixed 720x576 SAR 64:45 h264 (libavcodec) 1024x576 unchanged 1280x720 square hevc (VT) 1280x720 unchanged 1280x720 square h264 (libavcodec) 1280x720 unchanged Reached in production by the interlaced detour (field order or an SPS that indicates interlacing) and by forward-only sources. Broadcast SD is where interlacing and a non-square SAR occur together. Re #354 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HX5zV3Fcf7Nzq4DYGdXvQE --- CHANGELOG.md | 10 +++ .../Decoder/HardwareVideoDecoder.swift | 60 ++++++++++++++++ ...ue354HardwareDecoderPixelAspectTests.swift | 69 +++++++++++++++++++ 3 files changed, 139 insertions(+) create mode 100644 Tests/AetherEngineTests/Issue354HardwareDecoderPixelAspectTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 88602f3b..44766c37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,16 @@ the public-API contract. ### Fixed +- Anamorphic HEVC on the software host rendered at its coded dimensions (#354). + The VT-backed decoder attached no pixel aspect ratio, and the renderer builds + its format description from the delivered pixel buffer, so nothing carried the + ratio to the layer: 720x576 declaring 64:45 presented as 720x576, a 16:9 + picture squashed into 5:4. The libavcodec decoder on the same host has + attached it since #177, so the gap was one decoder wide. Resolved once at open + from the bitstream ratio and the container's, through the same #177 and #290 + gates, and attached next to the colour metadata that is re-applied there for + the same reason. Reached in production by the interlaced-content detour and by + forward-only sources, which is where broadcast SD lands. - The software load path cancelled every Combine sink it had already wired. `softwareCancellables.removeAll()` stood between two groups of `.store(in:)` calls, so the SW-PiP cue mirror never delivered a cue after the frame diff --git a/Sources/AetherEngine/Decoder/HardwareVideoDecoder.swift b/Sources/AetherEngine/Decoder/HardwareVideoDecoder.swift index fa64f156..ae1bc4b2 100644 --- a/Sources/AetherEngine/Decoder/HardwareVideoDecoder.swift +++ b/Sources/AetherEngine/Decoder/HardwareVideoDecoder.swift @@ -60,6 +60,17 @@ final class HardwareVideoDecoder: VideoDecodingPipeline, @unchecked Sendable { private var colorTransfer: CFString? private var colorMatrix: CFString? + /// #354: the stream's pixel aspect ratio, re-applied to every CVPixelBuffer for the same reason + /// the colorimetry is: nothing else puts it there. The renderer builds its format description + /// from the delivered buffer, so a ratio that is not an attachment on that buffer never reaches + /// the layer, and anamorphic content is displayed at its coded dimensions. nil for square pixels + /// and for a ratio the policy rejects, which is the case where coded dimensions ARE correct. + /// + /// Resolved once at `open()`, not per frame: VT delivers pixel buffers rather than `AVFrame`s, so + /// the per-frame source `SoftwareVideoDecoder` prefers does not exist here. That also makes the + /// #177 latch unnecessary, since one resolution cannot oscillate. + private var pixelAspectRatio: AVRational? + /// Protects `session` across the demux thread (decode), main thread (close/flush), and VT callback (delivery). private let lock = NSLock() @@ -86,6 +97,26 @@ final class HardwareVideoDecoder: VideoDecodingPipeline, @unchecked Sendable { width = codecpar.pointee.width height = codecpar.pointee.height + // #354: both declared sources, because only one of them is the container's. The bitstream + // ratio reaches codecpar, while a container-declared one reaches AVStream alone (Matroska's + // DisplayWidth quotient, MP4's `pasp`), which is where every DVD remuxed to MKV carries it. + pixelAspectRatio = Self.resolvePixelAspectRatio( + bitstream: codecpar.pointee.sample_aspect_ratio, + container: stream.pointee.sample_aspect_ratio, + width: width, + height: height + ) + if let sar = pixelAspectRatio { + EngineLog.emit( + "[HWDecoder] SAR \(sar.num):\(sar.den) on \(width)x\(height) " + + "(bitstream=\(codecpar.pointee.sample_aspect_ratio.num):" + + "\(codecpar.pointee.sample_aspect_ratio.den) " + + "container=\(stream.pointee.sample_aspect_ratio.num):" + + "\(stream.pointee.sample_aspect_ratio.den))", + category: .swPlayback + ) + } + guard codecpar.pointee.codec_id == AV_CODEC_ID_HEVC else { throw VideoDecoderError.unsupportedCodec(id: codecpar.pointee.codec_id.rawValue) } @@ -319,6 +350,22 @@ final class HardwareVideoDecoder: VideoDecodingPipeline, @unchecked Sendable { close() } + // MARK: - Pixel aspect ratio (#354) + + /// The ratio to attach, or nil when there is nothing to correct. Bitstream first, container + /// second (`declaredStreamSAR`), then the same two gates the libavcodec path runs: the #177 + /// component bound and the #290 display aspect the ratio produces on this frame. Square pixels + /// return nil rather than 1:1, because attaching a correction of one is a correction a consumer + /// cannot tell from a real one. + static func resolvePixelAspectRatio( + bitstream: AVRational, container: AVRational, width: Int32, height: Int32 + ) -> AVRational? { + let declared = SoftwareVideoDecoder.declaredStreamSAR(bitstream: bitstream, container: container) + guard let sane = PixelAspectPolicy.saneSAR(declared, width: width, height: height), + sane.num != sane.den else { return nil } + return sane + } + // MARK: - Callback handling (called from VT's queue) /// Invoked by `hwDecoderOutputCallback`; delivers CVPixelBuffer+PTS, honouring `skipUntilPTS` for seek-pre-roll. @@ -345,6 +392,19 @@ final class HardwareVideoDecoder: VideoDecodingPipeline, @unchecked Sendable { CVBufferSetAttachment(imageBuffer, kCVImageBufferYCbCrMatrixKey, matrix, .shouldPropagate) } + // #354: without this the renderer's format description carries no pixel aspect ratio and + // anamorphic content is displayed at its coded dimensions. + if let sar = pixelAspectRatio { + let aspect: NSDictionary = [ + kCVImageBufferPixelAspectRatioHorizontalSpacingKey: Int(sar.num), + kCVImageBufferPixelAspectRatioVerticalSpacingKey: Int(sar.den), + ] + CVBufferSetAttachment(imageBuffer, kCVImageBufferPixelAspectRatioKey, aspect, .shouldPropagate) + } else { + // A recycled pool buffer can carry a stale attachment from an earlier stream. + CVBufferRemoveAttachment(imageBuffer, kCVImageBufferPixelAspectRatioKey) + } + onFrame?(imageBuffer, pts, nil) } } diff --git a/Tests/AetherEngineTests/Issue354HardwareDecoderPixelAspectTests.swift b/Tests/AetherEngineTests/Issue354HardwareDecoderPixelAspectTests.swift new file mode 100644 index 00000000..be8516b1 --- /dev/null +++ b/Tests/AetherEngineTests/Issue354HardwareDecoderPixelAspectTests.swift @@ -0,0 +1,69 @@ +import Foundation +import Libavutil +import Testing +@testable import AetherEngine + +/// AE#354: the software host's VT-backed decoder attached no pixel aspect ratio, so anamorphic HEVC +/// reached the layer at its coded dimensions (measured: 720x576 at 64:45 presented as 720x576, a +/// 16:9 picture squashed into 5:4). The libavcodec decoder on the same host has attached it since +/// #177; the gap was one decoder wide. +/// +/// The decision is the same one `SoftwareVideoDecoder` makes, minus the per-frame source: VT hands +/// over pixel buffers, not `AVFrame`s, so only the two declared ratios exist here. +@Suite("VT-backed decoder pixel aspect (#354)") +struct Issue354HardwareDecoderPixelAspectTests { + + private func rational(_ num: Int32, _ den: Int32) -> AVRational { + AVRational(num: num, den: den) + } + + @Test("the bitstream ratio wins over the container's") + func bitstreamWins() { + let resolved = HardwareVideoDecoder.resolvePixelAspectRatio( + bitstream: rational(64, 45), container: rational(1, 1), width: 720, height: 576) + + #expect(resolved?.num == 64) + #expect(resolved?.den == 45) + } + + /// The case that keeps the container fallback alive: Matroska writes its DisplayWidth quotient to + /// the stream and leaves codecpar at 0:1, which is every DVD remuxed to MKV. + @Test("an unset bitstream ratio falls back to the container's") + func containerIsTheFallback() { + let resolved = HardwareVideoDecoder.resolvePixelAspectRatio( + bitstream: rational(0, 1), container: rational(64, 45), width: 720, height: 576) + + #expect(resolved?.num == 64) + #expect(resolved?.den == 45) + } + + @Test("square pixels attach nothing") + func squarePixelsAttachNothing() { + #expect(HardwareVideoDecoder.resolvePixelAspectRatio( + bitstream: rational(1, 1), container: rational(1, 1), width: 1920, height: 1080) == nil) + } + + @Test("a garbage component ratio is rejected (#177)") + func garbageComponentsRejected() { + #expect(HardwareVideoDecoder.resolvePixelAspectRatio( + bitstream: rational(1088, 1), container: rational(0, 1), width: 1920, height: 1080) == nil) + } + + /// #290: plausible numbers, impossible picture. 1080p declaring 3:1 smears into a 5.33:1 band. + @Test("a ratio whose display aspect is impossible is rejected (#290)") + func impossibleDisplayAspectRejected() { + #expect(HardwareVideoDecoder.resolvePixelAspectRatio( + bitstream: rational(3, 1), container: rational(0, 1), width: 1920, height: 1080) == nil) + } + + /// The same 2:1 that is wrong on a full-width frame is right on a half-width broadcast one, which + /// is why the gate needs the frame and not just the numbers. + @Test("2:1 on a 960x1080 broadcast frame is kept") + func halfWidthBroadcastKept() { + let resolved = HardwareVideoDecoder.resolvePixelAspectRatio( + bitstream: rational(2, 1), container: rational(0, 1), width: 960, height: 1080) + + #expect(resolved?.num == 2) + #expect(resolved?.den == 1) + } +}