From 70b8883ec3214e2eadba3f8c849f6b3b763c7d52 Mon Sep 17 00:00:00 2001 From: JonLuca DeCaro Date: Sun, 25 Oct 2026 09:26:02 -0700 Subject: [PATCH] fix(stills): preserve request size and snapshot time in cache keys --- CHANGELOG.md | 4 ++ .../FrameExtractor/FrameCache.swift | 51 ++++++++++--------- .../FrameExtractor/FrameExtractor.swift | 6 +-- Tests/AetherEngineTests/FrameCacheTests.swift | 51 ++++++++++++++++--- docs/api.md | 2 +- 5 files changed, 81 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cadfbc99f..d3051d296 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -126,6 +126,10 @@ the public-API contract. after its response head reports the hop that answered and its status. The observer does not alter request policy and requires hosts to redact source URLs. +- **Still extraction respects requested size and precise snapshot position on cache hits.** + Reusing one extractor for different thumbnail widths or snapshot bounds no longer returns the + first size rendered. Snapshot requests no longer share a coarse time bucket across frame boundaries. + - **A remote disc image on an extensionless URL is recognized by its `Content-Type` (AE#860, thanks @qoli).** The HTTP disc adapter was gated on the `.iso` / `.img` / `.udf` extension alone, so an ISO behind an opaque proxy path reached libavformat as raw filesystem bytes and failed with `Invalid data found diff --git a/Sources/AetherEngine/FrameExtractor/FrameCache.swift b/Sources/AetherEngine/FrameExtractor/FrameCache.swift index 8a44738b7..fdb01d427 100644 --- a/Sources/AetherEngine/FrameExtractor/FrameCache.swift +++ b/Sources/AetherEngine/FrameExtractor/FrameCache.swift @@ -1,25 +1,27 @@ import Foundation import CoreGraphics -/// Size-bounded LRU cache of rendered frames keyed by `(mode, position bucket)`. +/// Count-bounded LRU cache keyed by mode, requested position, and requested dimensions. /// Per-mode count limit caps the cache at `thumbnailLimit + snapshotLimit` CGImages. -/// Key omits target size: one extractor serves one fixed-size surface, so a repeat -/// position at a different maxWidth/maxSize returns the first-rendered image. /// Not thread-safe; FrameExtractor owns the only instance and touches it actor-isolated. final class FrameCache { + private struct Key: Hashable { + let position: Double + let width: CGFloat + let height: CGFloat + } + private let thumbnailLimit: Int private let snapshotLimit: Int private let thumbnailBucketSeconds: Double - // Snapshots are frame-accurate, so a 0.1 s grid (~3 frames at 30 fps) suffices. - private static let snapshotBucketSeconds: Double = 0.1 // Each FrameMode case needs a matching store + order pair below, plus // mirroring in get/set/clear. Add both when introducing a new case. /// Use order per mode, front = MRU; holds bucket keys, `store` holds payloads. - private var thumbnailOrder: [Int] = [] - private var snapshotOrder: [Int] = [] - private var thumbnailStore: [Int: CGImage] = [:] - private var snapshotStore: [Int: CGImage] = [:] + private var thumbnailOrder: [Key] = [] + private var snapshotOrder: [Key] = [] + private var thumbnailStore: [Key: CGImage] = [:] + private var snapshotStore: [Key: CGImage] = [:] init(thumbnailLimit: Int, snapshotLimit: Int, thumbnailBucketSeconds: Double) { precondition(thumbnailBucketSeconds > 0, "thumbnailBucketSeconds must be positive") @@ -28,18 +30,20 @@ final class FrameCache { self.thumbnailBucketSeconds = thumbnailBucketSeconds } - private func bucket(_ seconds: Double, mode: FrameMode) -> Int { - let grid = mode == .thumbnail ? thumbnailBucketSeconds : Self.snapshotBucketSeconds - let scaled = max(0, seconds) / grid - // Thumbnails floor to grid; snapshots round to nearest so a value within - // half a bucket of a stored position resolves to the same entry. - let rounded = mode == .thumbnail ? scaled.rounded(.down) : scaled.rounded() - // Audit BIT-105: `Int(_:)` traps past `Int.max`; every such position shares the last bucket. - return rounded < 0x1p62 ? Int(rounded) : 1 << 62 + private func key(seconds: Double, mode: FrameMode, targetWidth: Int, maxSize: CGSize?) -> Key { + if mode == .thumbnail { + let bucket = (max(0, seconds) / thumbnailBucketSeconds).rounded(.down) + // Preserve the saturating thumbnail bucket for enormous finite requests. + return Key(position: min(bucket, 0x1p62), width: CGFloat(targetWidth), height: 0) + } + // Any two nearby requests can straddle a frame boundary. Accurate extraction must + // reuse only the same request, rather than guess a frame rate with a time bucket. + let size = maxSize.flatMap { $0.width > 0 && $0.height > 0 ? $0 : nil } + return Key(position: max(0, seconds), width: size?.width ?? 0, height: size?.height ?? 0) } - func get(mode: FrameMode, seconds: Double) -> CGImage? { - let key = bucket(seconds, mode: mode) + func get(mode: FrameMode, seconds: Double, targetWidth: Int = 0, maxSize: CGSize? = nil) -> CGImage? { + let key = key(seconds: seconds, mode: mode, targetWidth: targetWidth, maxSize: maxSize) switch mode { case .thumbnail: guard let img = thumbnailStore[key] else { return nil } @@ -52,8 +56,9 @@ final class FrameCache { } } - func set(_ image: CGImage, mode: FrameMode, seconds: Double) { - let key = bucket(seconds, mode: mode) + func set(_ image: CGImage, mode: FrameMode, seconds: Double, + targetWidth: Int = 0, maxSize: CGSize? = nil) { + let key = key(seconds: seconds, mode: mode, targetWidth: targetWidth, maxSize: maxSize) switch mode { case .thumbnail: thumbnailStore[key] = image @@ -73,14 +78,14 @@ final class FrameCache { snapshotStore.removeAll() } - private func touch(_ order: inout [Int], _ key: Int) { + private func touch(_ order: inout [Key], _ key: Key) { if let idx = order.firstIndex(of: key) { order.remove(at: idx) } order.insert(key, at: 0) } - private func evict(_ order: inout [Int], _ store: inout [Int: CGImage], limit: Int) { + private func evict(_ order: inout [Key], _ store: inout [Key: CGImage], limit: Int) { while store.count > limit, let lru = order.popLast() { store[lru] = nil } diff --git a/Sources/AetherEngine/FrameExtractor/FrameExtractor.swift b/Sources/AetherEngine/FrameExtractor/FrameExtractor.swift index 29b3ca755..3261486e8 100644 --- a/Sources/AetherEngine/FrameExtractor/FrameExtractor.swift +++ b/Sources/AetherEngine/FrameExtractor/FrameExtractor.swift @@ -54,7 +54,7 @@ public actor FrameExtractor { } /// Only used with a one-segment DataIOReader. No URL or provider fallback. - /// Refined results bypass the old 0.1 s snapshot bucket entirely. + /// Refined results bypass the rendered-frame cache so they carry the actual decoded PTS. func residentPreview(rawTarget: Double, refined: Bool, maxWidth: Int, isCancelled: @escaping @Sendable () -> Bool) async -> ScrubFrame? { guard !isShutDown, rawTarget.isFinite, @@ -284,7 +284,7 @@ public actor FrameExtractor { private func produce(at seconds: Double, mode: FrameMode, targetWidth: Int, maxSize: CGSize?) async -> CGImage? { // Audit BIT-105: a 0/0 scrub fraction or `CMTime.indefinite.seconds` is a host bug, not a position. guard !isShutDown, seconds.isFinite, seconds >= 0 else { return nil } - if let hit = cache.get(mode: mode, seconds: seconds) { + if let hit = cache.get(mode: mode, seconds: seconds, targetWidth: targetWidth, maxSize: maxSize) { scheduleIdleClose() return hit } @@ -331,7 +331,7 @@ public actor FrameExtractor { return FrameResult(image: image) } if let image = result.image, !token.isCancelled { - cache.set(image, mode: mode, seconds: seconds) + cache.set(image, mode: mode, seconds: seconds, targetWidth: targetWidth, maxSize: maxSize) } scheduleIdleClose() return result.image diff --git a/Tests/AetherEngineTests/FrameCacheTests.swift b/Tests/AetherEngineTests/FrameCacheTests.swift index 263f795b6..9ff4dacb7 100644 --- a/Tests/AetherEngineTests/FrameCacheTests.swift +++ b/Tests/AetherEngineTests/FrameCacheTests.swift @@ -48,14 +48,53 @@ struct FrameCacheTests { #expect(cache.get(mode: .thumbnail, seconds: 3.0) === c) } - @Test("Snapshot uses a frame-accurate (sub-second) bucket") - func snapshotBucket() { + @Test("Snapshots preserve distinct requests across a frame boundary") + func snapshotPosition() { let cache = FrameCache(thumbnailLimit: 4, snapshotLimit: 4, thumbnailBucketSeconds: 1.0) let a = dummyImage(), b = dummyImage() - cache.set(a, mode: .snapshot, seconds: 10.10) - cache.set(b, mode: .snapshot, seconds: 10.90) - #expect(cache.get(mode: .snapshot, seconds: 10.12) === a) - #expect(cache.get(mode: .snapshot, seconds: 10.88) === b) + // Even sub-millisecond requests can lie on opposite sides of a frame boundary. + cache.set(a, mode: .snapshot, seconds: 1.0) + cache.set(b, mode: .snapshot, seconds: 1.0001) + #expect(cache.get(mode: .snapshot, seconds: 1.0) === a) + #expect(cache.get(mode: .snapshot, seconds: 1.0001) === b) + #expect(cache.get(mode: .snapshot, seconds: 1.0002) == nil) + } + + @Test("Thumbnail cache distinguishes requested widths") + func thumbnailDimensions() { + let cache = FrameCache(thumbnailLimit: 4, snapshotLimit: 2, thumbnailBucketSeconds: 1.0) + let small = dummyImage(), large = dummyImage() + cache.set(small, mode: .thumbnail, seconds: 2, targetWidth: 160) + cache.set(large, mode: .thumbnail, seconds: 2, targetWidth: 640) + #expect(cache.get(mode: .thumbnail, seconds: 2.2, targetWidth: 160) === small) + #expect(cache.get(mode: .thumbnail, seconds: 2.2, targetWidth: 640) === large) + #expect(cache.get(mode: .thumbnail, seconds: 2.2, targetWidth: 320) == nil) + } + + @Test("Snapshot cache distinguishes native and both requested dimensions") + func snapshotDimensions() { + let cache = FrameCache(thumbnailLimit: 2, snapshotLimit: 4, thumbnailBucketSeconds: 1.0) + let native = dummyImage(), wide = dummyImage(), short = dummyImage() + cache.set(native, mode: .snapshot, seconds: 2) + cache.set(wide, mode: .snapshot, seconds: 2, maxSize: CGSize(width: 320, height: 240)) + cache.set(short, mode: .snapshot, seconds: 2, maxSize: CGSize(width: 320, height: 120)) + #expect(cache.get(mode: .snapshot, seconds: 2) === native) + #expect(cache.get(mode: .snapshot, seconds: 2, maxSize: CGSize(width: 320, height: 240)) === wide) + #expect(cache.get(mode: .snapshot, seconds: 2, maxSize: CGSize(width: 320, height: 120)) === short) + #expect(cache.get(mode: .snapshot, seconds: 2, maxSize: CGSize(width: 640, height: 240)) == nil) + } + + @Test("Size variants share the same per-mode count limit") + func sizeVariantEviction() { + let cache = FrameCache(thumbnailLimit: 2, snapshotLimit: 2, thumbnailBucketSeconds: 1.0) + let a = dummyImage(), b = dummyImage(), c = dummyImage() + cache.set(a, mode: .thumbnail, seconds: 1, targetWidth: 160) + cache.set(b, mode: .thumbnail, seconds: 1, targetWidth: 320) + _ = cache.get(mode: .thumbnail, seconds: 1, targetWidth: 160) + cache.set(c, mode: .thumbnail, seconds: 1, targetWidth: 640) + #expect(cache.get(mode: .thumbnail, seconds: 1, targetWidth: 160) === a) + #expect(cache.get(mode: .thumbnail, seconds: 1, targetWidth: 320) == nil) + #expect(cache.get(mode: .thumbnail, seconds: 1, targetWidth: 640) === c) } @Test("clear empties the cache") diff --git a/docs/api.md b/docs/api.md index 2a2a4da77..dfb18930a 100644 --- a/docs/api.md +++ b/docs/api.md @@ -1193,7 +1193,7 @@ say so on the tracker rather than working around it. | `presentationAxisMap` | `PresentationAxisMap`: `sourceSeconds(forItemSeconds:)`, `itemSeconds(forSourceSeconds:)`, `shiftSeconds(atItemSeconds:)`, `seams` (each a `PresentationAxisMap.Seam` of `itemSeconds` and `shiftSeconds`), `isEmpty`. Readable off the main actor. Cue times and `sourceTime` live on the source axis; `AVPlayerItem.currentTime()` lives on the item axis, and they differ by the producer shift. Returns nil rather than a guess where no axis is established. | | `setNativeVideoFrameTimeObserver(_:)` | Takes a `NativeVideoFrameTimeObserver`, called per muxed frame on both axes (`NativeVideoFrameTime`: `source`, `item`, `segmentIndex`, `isKeyframe`, `epoch`). Called on the producer pump thread in **decode** order, so `source` is not monotonic under B-frames. `epoch` rises process-wide. | | `setSoftwareVideoFrameTimeObserver(_:)`, `softwarePresentationTimebase` | The software-path equivalent, a `SoftwareVideoFrameTimeObserver` over `SoftwareVideoFrameTime` (`presentation`, `generation`), in ascending presentation order, on the same axis as the cues. `generation` moves on every renderer flush. | -| `FrameExtractor` | Off-playback stills: `thumbnail(at:maxWidth:)`, `snapshot(at:maxSize:)`, `prewarm()`, `shutdown()`, over a URL or an `IOReader`. Opens its own demuxer, so it needs a source that tolerates a second connection. `boundedSnapshot(at:maxSize:limits:cancellation:)` is the disposable form: one caller-supplied `ProbeLimits` / `ProbeCancellation` budget spans open, stream analysis, seek and decode, a stalled HTTP header included, and the result is a `BoundedStillFrame`. Use a fresh extractor, since an already-open context is refused rather than charged to the budget; the context closes on every outcome. | +| `FrameExtractor` | Off-playback stills: `thumbnail(at:maxWidth:)`, `snapshot(at:maxSize:)`, `prewarm()`, `shutdown()`, over a URL or an `IOReader`. Cache entries include the requested dimensions, so one extractor may serve multiple sizes. Thumbnails reuse their position bucket; snapshots reuse only an identical requested time so adjacent frame requests remain distinct. Opens its own demuxer, so it needs a source that tolerates a second connection. `boundedSnapshot(at:maxSize:limits:cancellation:)` is the disposable form: one caller-supplied `ProbeLimits` / `ProbeCancellation` budget spans open, stream analysis, seek and decode, a stalled HTTP header included, and the result is a `BoundedStillFrame`. Use a fresh extractor, since an already-open context is refused rather than charged to the budget; the context closes on every outcome. | ## Now Playing and the audio session