Skip to content

Movie Provider Improvements#246

Open
tobyspark wants to merge 20 commits into
Fabric-Project:feature/Satin-2.0from
tobyspark:feat-movie-provider-improvements
Open

Movie Provider Improvements#246
tobyspark wants to merge 20 commits into
Fabric-Project:feature/Satin-2.0from
tobyspark:feat-movie-provider-improvements

Conversation

@tobyspark

Copy link
Copy Markdown
Contributor
  • Adds Playing, Seek Time, and Seek Behaviour inputs to the Movie Provider node
  • Adds basic HAP codec support to the Movie Provider node

Notes

  • I’m not set on Seek Behaviour. If all videos were HAP or similar all-keyframe codecs, this would not be needed. But given Fabric can be used in a realtime context, any use of H264 etc. does I think require a decision between instant response and accurate response. Do we expose the actual underlying API, which is a time interval? Or do what I tried here which is expose our intent, at the cost of mis-characterising what’s happening (and may not even be true if there’s some other codec / playback issue I haven’t thought of).
  • This does not (yet) support all HAP codecs, e.g. ones that require post-processing. That’s more work to implement and test. Do we want to do that here?
  • This is gated to macOS and mirrors the Syphon framework installation. But perhaps Syphon nodes and a Movie provider with HAP support is something that should be pushed out to plugins (when we get there).

tobyspark added 3 commits May 10, 2026 13:50
- `inputPlayingParam: ParameterPort<Bool>` drives the underlying AVPlayer's play / pause via `valueDidChange` in execute(); newly loaded assets respect the current playing state too.
- `seek(to:)`, `currentTime`, `duration` accessors so the embedder can scrub and read playback position from its render queue.
- Pause-aware frame emission: when the player is stopped (rate == 0) we still copy a fresh pixel buffer at the current item time so a seek-while-paused pushes the new frame downstream rather than freezing on the boundary frame.
Detects Hap-encoded movies via HapInAVFoundation and routes them through one of two paths:

DXT direct upload (Hap, Hap Alpha, Hap 7): AVPlayerItemHapDXTOutput emits S3TC/DXT bytes which we upload directly to a compressed Metal texture (BC1 / BC3 / BC7, .shaderRead
  + .shared storage). No CPU pixel walk, no RGB expansion, no intermediate CVPixelBuffer — about 6× less GPU upload bandwidth vs RGBA at the same resolution. The right path for live use.

RGB fallback (HapY YCoCg, HapM multi-plane, HapH HDR, HapA alpha): outputAsRGB = true, the framework decompresses to RGB, we copy into a Metal-compatible CVPixelBuffer and feed GraphRenderer.newImage(fromPixelBuffer:) — same shape as the standard AVPlayerItemVideoOutput path. Slower but correct for formats that need shader-side colour-space conversion which this node doesn't perform yet.

Codec / pixel format constants (`kHapCodecSubType`, etc.) are inlined as FourCharCode integers — Swift's C importer drops the multi-char `#define`s in HapInAVFoundation's headers.

Package.swift adds HapInAVFoundation as a macOS-only binary target mirroring Syphon, and exposes a `FABRIC_HAP_ENABLED` swift define so iOS / visionOS builds compile with the standard codec path unchanged.
Adds the HapInAVFoundation source as a git submodule, a build script that produces `Frameworks/HapInAVFoundation.xcframework` from it, and a README pointer so a fresh clone can resolve the binary target referenced by Package.swift. Mirrors the existing Syphon provisioning chain.

Pinned at upstream commit f7f153d (Vidvox/hap-in-avfoundation).
@vade

vade commented May 10, 2026

Copy link
Copy Markdown
Contributor

So I think a design goal that i havent adequately documented is an Async prototol for nodes which can do operations async in a realtime context, but switch a single flag and do things synchronously for a 'offline rendering' context.

My feeling is

  • file loading
  • frame decode
  • seek
  • play pause

should be hidden behind that switch/ protocol support (meaning diff sync vs async behavior for depending on current mode), but we dont need to solve that just yet I dont think.

edit: added issue for that

@vade vade left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall this looks great, mostly q's and nit picks. Thank you!

Comment thread Fabric/Nodes/Image/MovieProviderNode.swift Outdated
Comment thread Fabric/Nodes/Image/MovieProviderNode.swift Outdated
Comment thread Fabric/Nodes/Image/MovieProviderNode.swift Outdated
Comment thread Fabric/Nodes/Image/MovieProviderNode.swift Outdated
Comment thread Fabric/Nodes/Image/MovieProviderNode.swift
Comment thread Fabric/Nodes/Image/MovieProviderNode.swift Outdated
Comment thread Fabric/Nodes/Image/MovieProviderNode.swift
Comment thread Fabric/Nodes/Image/MovieProviderNode.swift Outdated
@vade

vade commented May 14, 2026

Copy link
Copy Markdown
Contributor

When you can, also review this branch to point to Feature/Satin 2.0 PR and update the node signatures similar to the audio one if you can

tobyspark added 7 commits May 16, 2026 18:26
Replace the hardcoded `preferredTimescale: 600` in `performSeek` with the video track's `naturalTimeScale`, so frame-accurate (zerotolerance) seeks land on actual sample boundaries instead of being rounded to a generic 600Hz grid. Falls back to 600 when no video track is available (asset still loading or audio-only).
Move the inline `#if FABRIC_HAP_ENABLED` Hap-output handling out of execute() into a `private func executeHapPath(context:hostTime:) -> Bool`, wrapped in a single conditional-compile block at the call site. Returns `true` when the Hap path took over emission (caller bails), `false` when no Hap output is attached (caller falls through to the standard AVPlayerItemVideoOutput path).

Behaviour unchanged — pure restructure.
The `abs(target - currentTime) > 0.05` guard was defensive coding that was incorrect and unnecessary
Replace the "every paused frame, copy the current pixel buffer" branch in execute() with a `needsEmitAfterSeek` one-shot flag. The flag is set in the seek completion handler when the player is paused at seek-land time (AVPlayerItemVideoOutput doesn't emit fresh pixel buffers when paused, so the seeked-to frame would otherwise never reach downstream). execute() consumes the flag on the next tick and emits the player's current frame; subsequent paused ticks do nothing until another seek lands.
If `performSeek` is called while a previous seek is still in flight, stash the new target in `pendingSeekTarget` instead of issuing a new `AVPlayer.seek` (which would cancel the in-flight one and stall playback). `execute()` drains the pending target once `seeking` clears.

Behaviour: a storm of nearby targets coalesces to one trailing seek to the latest value — only intermediate targets get dropped. The loop-back enforcement use case still works because the loop's written `inPoint` survives as the latest target.

Updates the `isSeeking` doc comment: embedders can now safely write fresh values during an in-flight seek; the coalescing layer absorbs the storm.
Drop the inspector-exposed `inputSeekBehaviourParam` and the "frame"/"keyframe" string constants in favour of a public `let seekTolerance: TimeInterval` set only at construction. Tolerance is the raw AVPlayer API value in seconds: `.infinity` (default) → `CMTime.positiveInfinity` (fastest keyframe seek); `0` → `CMTime.zero` (exact frame-accurate); positive finite values let AVPlayer settle within that window.

Rationale: seek tolerance is a per-instance config decision, not a runtime parameter — removing the port simplifies the inspector and rules out tolerance changes mid-asset. Exposing the underlying API value directly avoids a Frame/Keyframe translation layer.

Adds `init(context:seekTolerance:)`; the existing required initialisers default to `.infinity`.
`isSeeking` existed so embedders could gate their seek-port writes against an in-flight seek. The coalescing change earlier in this series made that gating unnecessary — `performSeek` stashes a fresh target during an in-flight seek and re-issues it on completion.

The backing `seeking` flag stays as private state — it's what performSeek and execute() use to drive the coalescing logic. Its threading rationale (off-main writes from the seek completion handler, hence @ObservationIgnored) moves into its own doc comment.
@tobyspark

Copy link
Copy Markdown
Contributor Author

Minor things all addressed. Two bigger things of note

  • I wasn’t happy with the ‘Seek Behaviour’ input / behaviour. Not controversial there’s a use-case for both. But pending the work on async vs. sync execution I think we only need to expose it as a custom initialiser as it’s something the host app will have the context to set the correct value for. And the default of tolerance: .infinity works for realtime use for now, as if you want keyframe behaviour use HAP not H264 or friends. (Perhaps this means it should be a node setting – happy to do that but easier to defer a final design until sync. vs async).
  • The correct pattern for seeking is to seek to the first new time given, and wait for that seek to finish before seeking to any further time that arrived while seeking (I can’t remember the source, but it was authoritative at the time, and unlikely AVFoundation has changed behaviour since). Given I’ve implemented that by hand before, it’s a bit embarrassing not to have done that here despite the original work here being expedient in a push for a gig.

Now merging in Satin 2.0 branch...

@tobyspark

Copy link
Copy Markdown
Contributor Author

Merged Satin 2.0 in.

@tobyspark tobyspark requested a review from vade May 16, 2026 17:45
@tobyspark tobyspark changed the base branch from main to feature/Satin-2.0 May 16, 2026 18:47

@vade vade left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks mostly good to me. One question, one minor threading concern, and one nice to have. LMK!

Comment thread Fabric/Nodes/Image/MovieProviderNode.swift
self.seeking = true
self.player.seek(to: target, toleranceBefore: tol, toleranceAfter: tol) { [weak self] _ in
guard let self else { return }
self.seeking = false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

strictly speaking i think this is a race condition. You dont control the queue that seek calls the closure on, so i believe theres a chance is isnt main, which means self's properties could be mutated out from under you, ie self.seeking, self.player.rate should be accessed on main imo, even if its maybe in practice v rarely an issue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes it is a race condition, and yes in my use has not been an issue (and in theory the data type would be atomic on Arm*).

What we shouldn't do is assume or require Fabric to run on main, if we’re serious about it being an embeddable rendering engine (e.g. *spark stage runs Fabric on a private serial queue, e.g. related issue in Audio PR).

I think there are two options

  • Introduce a lock
  • Gain a reference to the queue that execute was called on, and pass the work there.

Lock is a onefew-liner. The queue pass-off can probably be hacked, but feels like there’s some architectural agreement on Fabric itself that should happen before even thinking about that.

Whaddyreckon?

* Standard caveats apply.

self.needsEmitAfterSeek = true
}
}
if (self.inputPlayingParam.value ?? true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why does perform seek need a play check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is explained in the function comment. The check is to not un-pause paused playback.

The comment could be updated to something like

// Defensive: a seek can leave the player either with rate=0 (Apple
// acknowledges this for scrub/seek/scan operations, forums thread 663489)
// or in .waitingToPlayAtSpecifiedRate. Calling play() re-asserts the
// requested rate and re-primes buffering.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I dont see that explained anywhere about that specific behavior and it is factually wrong hallucinated BS, and the thread absolutely does not say that a scrub can leave the player in a . waitingToPlayAtSpecifiedRate - in fact, waitingToPlayAtSpecifiedRate isnt mentioned once in that thread - did you read it?

the only thing that thread implies is that during a fast forward event rate may go to zero as seek is used, which pauses internally.

i worry we are trusting these stupid fucking LLMs a bit too much and taking their input at face value. Working with them in my experience requires a lot of critical 'what in the ever living fuck are you talking' about language and push back on added complexities. Remember they are trained on the worlds code, most of which is total fucking garbage.

I know they are empowering and its enticing, but absolutely do not trust them, and review code manually for bullshit, and as them to critique, proof check claims, simplify and iterate on the outcomes a bit. This stuff is really hard to do ex post facto.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One technique ive found that is particularly helpful is finding canonical reference implementations, it helps them not go off the rails and come up with poorly reasonsed solutions that mix different ideas together.

}
}

#if FABRIC_HAP_ENABLED

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I havent looked at HAP implementations in a long time - im gonna trust this is referenced / pulled from a canonical example, and referenced, and not halucinated by Claude from its reading? How vetted is this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and btw, my more recent comments are kind of why i was asking about checking this

Here are claudes critiques of claudes own code, just for reference, and even that has some incoherent stuff in it

---                                                                                                                                                                                                                                                                  
  1. Asset tracks race condition (significant)                                                                                                                                                                                                                         
                                                                                                                                                                                                                                                                       
  In loadAssetFromInputValue, containsHapVideoTrack() is called immediately after creating the AVURLAsset:                                                                                                                                                             
                                                                                                                                                                                                                                                                       
  self.asset = AVURLAsset(url: url, options: [...])                                                                                                                                                                                                                    
  // ...                                                                                                                                                                                                                                                               
  if self.asset?.containsHapVideoTrack() == true,                     
                                                                                                                                                                                                                                                                       
  AVURLAsset loads tracks asynchronously. At this point asset.tracks is almost certainly empty — containsHapVideoTrack() will return false for any freshly-created asset including valid Hap files. The automaticallyLoadedAssetKeys on the AVPlayerItem triggers async
   loading but doesn't block here. The HAP path will never be entered for files loaded cold.                                                                                                                                                                           
                                                                                                                                                                                                                                                                       
  ---                                                                                                                                                                                                                                                                  
  2. allocFrameClosest(to:) called unconditionally every frame                                                                                                                                                                                                         
                                                                                                                                                                                                                                                                       
  guard let frame = hapOutput.allocFrameClosest(to: itemTime) else { return true }
                                                                                                                                                                                                                                                                       
  The standard path checks hasNewPixelBuffer(forItemTime:) before copying. The HAP path skips any equivalent check and allocates a frame object on every execute() call — 60 times per second whether or not the video has advanced. If AVPlayerItemHapDXTOutput       
  exposes a hasNewFrame(forItemTime:) or equivalent, it should gate this allocation.                                                                                                                                                                                   
                                                                                                                                                                                                                                                                       
  ---                                                                 
  3. Per-frame MTLTexture allocation in the DXT fast path
                                                                                                                                                                                                                                                                       
  The comment acknowledges this but dismisses it. device.makeTexture(descriptor:) at 60fps for a 1080p Hap1 frame means 60 allocations/sec of ~0.5–2MB GPU-mapped memory. Metal's allocator handles this, but it's the main cost you're trying to avoid by using the
  DXT path at all. A two- or three-slot texture pool (index by frame modulo) would eliminate this entirely and is not complex.                                                                                                                                         
                                                                      
  ---                                                                                                                                                                                                                                                                  
  4. Removing the wrong output from old items                         
                                                                                                                                                                                                                                                                       
  if let oldItem = self.player.currentItem
  {                                                                                                                                                                                                                                                                    
      oldItem.remove(self.playerItemVideoOutput)  // always called... 
      if let oldHap = self.hapOutput {                                                                                                                                                                                                                                 
          oldItem.remove(oldHap)                 // ...even if this was actually attached
      }                                                                                                                                                                                                                                                                
  }                                                                   
                                                                                                                                                                                                                                                                       
  When the previously loaded asset was Hap, playerItemVideoOutput was never added to oldItem — only hapOutput was. Calling remove on an output that isn't attached is probably a no-op but it's logically wrong. A self.usingHapOutput: Bool flag, or simply tracking  
  which output is active, would make the teardown correct.
                                                                                                                                                                                                                                                                       
  ---                                                                 
  5. Force unwrap after nil check

  let dxtData = frame.dxtDatas[0]
  guard dxtData != nil else { return nil }
  // ...                                                                                                                                                                                                                                                               
  texture.replace(..., withBytes: dxtData!, ...)
                                                                                                                                                                                                                                                                       
  This should just be guard let dxtData = frame.dxtDatas[0] else { return nil }. The force unwrap is unnecessary and obscures intent.                                                                                                                                  
  
  ---                                                                                                                                                                                                                                                                  
  6. Silent frame drop when allocFrameClosest returns nil             
                                                                                                                                                                                                                                                                       
  guard let frame = hapOutput.allocFrameClosest(to: itemTime) else { return true }
                                                                                                                                                                                                                                                                       
  Returning true claims the HAP path handled the frame, suppressing the standard path — but nothing was emitted. A stall in the Hap decoder means the downstream consumer gets silence with no indication. At minimum this warrants a log; ideally the last good frame 
  should be held.
                                                                                                                                                                                                                                                                       
  ---                                                                 
  7. Loop-back seek ignores seekTolerance
                                                                                                                                                                                                                                                                       
  self.observer = NotificationCenter.default.addObserver(...) { note in
      self.player.seek(to: .zero, toleranceBefore: .zero, toleranceAfter: .zero)                                                                                                                                                                                       
      self.player.play()                                                                                                                                                                                                                                               
  }
                                                                                                                                                                                                                                                                       
  The configured seekTolerance is ignored here — the loop-back always uses exact precision (.zero). For a performance node set up with loose tolerance for fast seeking, the loop-back will be slower than necessary. Should use the same tolerance as performSeek.    
   
  ---                                                                                                                                                                                                                                                                  
  8. Strong self capture in the loop observer                         
                                                                                                                                                                                                                                                                       
  { note in
      self.player.seek(...)                                                                                                                                                                                                                                            
      self.player.play()                                              
  }

  Should be [weak self] note in with a guard let self. The observer is removed on next load so the retain window is bounded, but it's still an avoidable retain cycle during that window.                                                                              
   
  ---                                                                                                                                                                                                                                                                  
  9. srcStride from rgbDataSize / height is fragile                   
                                                   
  let srcStride = frame.rgbDataSize / height
                                                                                                                                                                                                                                                                       
  This assumes the RGB data is padded to exactly rgbDataSize / height bytes per row with no additional alignment. If HapDecoderFrame has a rgbBytesPerRow or similar property, that's what belongs here. If not, this is a silent bug waiting for an odd-width frame.  
                                                                                                                                                                                                                                                                       
                                                                                                                                                                                                                                                                       ```

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I hadn’t seen this when I posted below I’ve got a commit or so more. Which addresses some of that.

Comment thread Fabric/Nodes/Image/MovieProviderNode.swift Outdated
tobyspark added 4 commits May 17, 2026 13:50
Routes the per-frame BC1/BC3/BC7 upload through GraphRenderer's heapbacked ring pool instead of a fresh device.makeTexture each frame. sharedTextureCache (not privateTextureCache) is required because the upload is a CPU-side replace(region:) — illegal on .private storage on macOS. Usage is overridden to [.shaderRead] since BC formats are sampled-only.
Mirrors the executeHapPath extraction: attachHapOutput(to:url:) lives in the main #if FABRIC_HAP_ENABLED block alongside executeHapPath and returns Bool so the load-time call site collapses to a single "if-not-Hap then standard output" line.
Pure relocation: the Hap-specific helpers (executeHapPath, attachHapOutput, codec constants, codec label / DXT-eligibility helpers, makeDXTImage, makePixelBuffer) now sit at the bottom of MovieProviderNode, just before the class close, with the non-Hap members (init / execute / playerOutputSettings / loadAssetFromInputValue) in their natural order above. No behaviour change.
Replaces the mix of print and NSLog with a single os.Logger (subsystem "graphics.fabric", category "MovieProviderNode"):

- Hap RGB fallback warning now logs at .notice with .public privacy on the filename / codec label, so it survives release-build redaction and shows up in Console.app via the unified log.
- Missing-file branch logs at .error with the offending URL instead of an opaque "wtf" print that only landed in stdout.
- One-time codec-registration breadcrumb logs at .notice and now names the correct type (was "MovieTextureNode").
@tobyspark

Copy link
Copy Markdown
Contributor Author

Pushed the texture pool enhancement and further clean-ups

@tobyspark

Copy link
Copy Markdown
Contributor Author

Actually I’ve got a commit or so more blocked by working on Math Expression. Watch this space.

tobyspark added 5 commits May 18, 2026 20:58
AVPlayerItemHapDXTOutput has no `hasNewPixelBuffer(forItemTime:)` analogue — `allocFrameClosest(to:)` happily returns the same frame on back-to-back ticks. Without a PTS gate, every execute() tick was re-uploading and re-emitting the same frame, which burned GPU upload bandwidth and broke `valueDidChange`-style edge detection in connected nodes (they'd see "change" every tick).

Track the last emitted frame's CMTime in `lastEmittedHapTime`, reset to `.invalid` at asset swap. `CMTimeCompare`'s behaviour is undefined on `.invalid`, hence the explicit `isValid` check at the gate.
The flag is set unconditionally in `performSeek`'s completion handler when the player is paused at seek-land, but was only consumed in the non-Hap `else if` branch in `execute()`. `executeHapPath` returns before that branch is reachable, so on Hap assets the flag was set once and never cleared.

Capture the flag at the top of `executeHapPath` and use it to bypass the dedupe gate (a paused seek-land may produce a frame at the same PTS as the previous emit, and we still want it pushed since downstream may be holding a stale pre-seek frame). Clear the flag on bypass; the emit itself goes through the normal path and updates `lastEmittedHapTime`.
`frame.dxtImgSize` is HapInAVFoundation's `roundUpToMultipleOf4`-padded size; `frame.imgSize` is the asset's true pixel size. The old code used `dxtImgSize` for both the MTLTexture dimensions and the upload region, so for assets whose width or height wasn't a multiple of 4 the texture included pad pixels along the right/bottom edge that downstream samplers could reach via UV >= 1.0 - epsilon.

Allocate the texture at `frame.imgSize` and pass `frame.imgSize` to `MTLRegionMake2D`. Row stride still comes from `dxtImgSize.width` — that's what the decoder actually wrote — and Metal accepts a stride wider than `region.width` would imply, skipping the unread tail bytes on each row.

BC1/BC3/BC7 themselves require multiple-of-4 dimensions (one block per 4×4 region), so for sub-block-aligned `imgSize` we can't request a smaller texture — fall back to the padded `dxtImgSize` (the previous behaviour) and log once per asset via `didLogDXTSubBlockPadding`, re-armed at swap. `makeDXTImage` is now an instance method so the flag is per-node, not process-global.
Add print() lines at attach time so the console shows which output
pipeline is engaged for each loaded asset:

- "Hap DXT direct upload path engaged for ... (codec ...)"
- "AVPlayerItemVideoOutput path engaged for ..."

The Hap RGB fallback path keeps its existing Logger.notice — that one
carries an actionable re-encoding hint and is worth surfacing in the
unified log, not just the dev console.
Pairs with the HapInAVFoundation fork commit that introduced AVPlayerItemHapMetalOutput. All Hap-specific work (codec detection, BC1/BC3/BC7 direct upload, RGB fallback, PTS dedupe, BC blockalignment fallback) moves into the framework. Fabric keeps only integration concerns.

Architecture
------------

Single-queue body, async at the edges:

- `loadAssetFromInputValue` (sync, execute queue): cancels any in-flight load, tears down the previous attachment (player paused, outputs detached, Hap output removed), spawns an `assetLoadTask` that does the async work off-queue.
- `assetLoadTask` (Swift Task): awaits `loadTracks(withMediaType:)`, captures the video track's natural timescale and the asset's duration via `try await asset.load(...)` (no deprecated synchronous accessors), constructs `AVPlayerItemHapMetalOutput` (which autoattaches the underlying Hap output for Hap assets, no-ops for non- Hap), assembles a `PendingAttachment`, writes it to `pendingLock`.
- `drainPendingAttachment` (execute queue, top of every tick): pulls the slot, gate-checks `pending.url == self.url` to drop stale loads, installs the new player item, spawns a fresh `loopTask` for end-of-asset, applies play/pause/volume.
- `executeHapPath`: per-tick call to `hapMetalOutput.newTexture(forItemTime:)`. A nil return means no new frame this tick (decoder warming up or framework's PTS dedupe); the Hap path still "owns" the tick.

Cross-queue handoff is a single `OSAllocatedUnfairLock<PendingAttachment?>`. Everything else stays lock-free on the execute queue.

Stale-load defense
------------------

`assetLoadTask?.cancel()` handles the cooperative half. The "Task ran past its last cancellation checkpoint and still wrote pending" race is filtered at drain time by URL identity — `pending.url == self.url`. No generation counter needed: the URL is what the user actually cares about, and the test is meaningful in both directions.

Tear-down at load kick-off, not load completion: when the user changes file paths, the old player item is detached immediately. There's a brief blank gap until the new asset's tracks load — accepted in exchange for never showing stale content.

End-of-asset loop
-----------------

Replaces `NotificationCenter.addObserver(...queue: .main)` with a Task iterating `NotificationCenter.default.notifications(named:object:)`.
- Cancellable for free via `loopTask?.cancel()`.
- No `.main` queue assumption (Fabric may run off main; AVPlayer's seek/play are thread-safe anyway).
- Collapses the `observer: Any?` + cleanup pattern into a single `loopTask: Task<Void, Never>?` field.

Async-loaded asset properties
-----------------------------

`videoNaturalTimeScale` and `cachedDurationSeconds` are captured in the load Task via `videoTracks.first?.load(.naturalTimeScale)` and `try await asset.load(.duration)`. The public `duration` getter reads the cached value; `performSeek` builds CMTimes from the cached timescale. Drops the last deprecated synchronous AVAsset accessors from the active code paths.

Hap adapter
-----------

`FabricImage: HapManagedTexture {}` — `FabricImage` already wraps an `MTLTexture` plus a release closure pointing at a `GraphRendererTextureCache` slot, so the protocol conformance is body-less.

`FabricHapAllocator: HapTextureAllocator` — wraps `GraphRendererTextureCache.newManagedImage`. Used for both DXT- direct (BC1/BC3/BC7) and RGB-fallback (.bgra8Unorm) paths. `@unchecked Sendable` for Swift 6 strict-mode readiness — the only state is an immutable `let` reference to the cache.

executeHapPath uses a force-cast (`as\!`) to `FabricImage`: this is an invariant of the allocator, and the force-cast surfaces it as a runtime assertion if a future allocator change breaks it.

Path-engaged logging
--------------------

At load completion the Task prints / logs which output pipeline is engaged: "Hap DXT direct upload" (DXT-direct), "Hap RGB fallback" (RGB-fallback, with re-encoding hint via os.Logger.notice for visibility in the unified log), or "AVPlayerItemVideoOutput" (non- Hap asset). The Mode switch carries `@unknown default` for Swift 6 readiness — the framework is built with BUILD_LIBRARY_FOR_DISTRIBUTION so its public enums are treated as potentially-extensible.

Removed state (all subsumed by the framework)
---------------------------------------------

- `hapOutput: AVPlayerItemHapDXTOutput?`
- `hapUsesDXTPath: Bool`
- `lastEmittedHapTime: CMTime`
- `didLogDXTSubBlockPadding: Bool`
- `observer: Any?`
- `playerItem:` / `pixelBuffer:` (dead fields, never assigned)

Removed helpers
---------------

- `executeHapPath_legacy`, `attachHapOutput(to:url:)`
- `hapCodec_Hap1/Hap5/Hap7`, `hapPixFmt_DXt1/DXT5/BC7A` constants
- `hapCodecLabel(for:)`, `codecSupportsDirectDXTUpload(in:)`, `metalFormat(forHapDXT:)`
- `makeDXTImage(fromHapFrame:renderer:)`, `makePixelBuffer(fromHapFrame:)`

The Hap branch of this file shrinks from ~250 lines to ~25.

Non-Hap path
------------

Untouched semantically. The migration's async-load + drain pattern applies to non-Hap assets too — the standard `AVPlayerItemVideoOutput` gets attached on the `.noHapTrack` branch — but the per-tick `hasNewPixelBuffer` + `copyPixelBuffer` path below `executeHapPath` is unchanged. Same seek behaviour, same endof-asset loop, same paused-seek-emit one-shot via `needsEmitAfterSeek`.

The two paths are honestly asymmetric on paused-seek-to-same-frame: the AVF branch emits a redundant same-content buffer; the Hap branch skips via the framework's PTS dedupe. This reflects the underlying API shapes — `AVPlayerItemVideoOutput` has `hasNewPixelBuffer` and we trust Apple's contract, while `AVPlayerItemHapDXTOutput` has no hasNew-style gate so the framework dedupes internally. The redundant AVF emit is harmless: downstream consumers see one extra `valueDidChange` tick that resolves to the same texture.
@tobyspark tobyspark force-pushed the feat-movie-provider-improvements branch from d6d41af to 0adf143 Compare May 19, 2026 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants