Repository navigation
Conversation
…n the base-layer buffer Everything FEL-specific is behind the DualLayerDolbyVision trait. Deadline release. A base frame whose enhancement layer has not arrived is released as its base layer once its presentation time is within a margin of maxFramesInFlight frame intervals (at least 100 ms), after a short grace for a layer that is only a moment late. Before, it waited for three newer base frames, so a damaged or missing enhancement-layer stretch held the picture and dropped frames in bursts. A paused or prerolling clock holds the frames (bounded at 2 s) and a count of 7 newer frames stays as a backstop. Compose in place. When the decoded base-layer buffer is IOSurface-backed, 10-bit biplanar video range, even-sized and the luma curve is not MMR, the chroma and luma passes write into that buffer, so the output pool is not allocated for those frames. A buffer is never composed twice at once and a failed command buffer drops the frame. AETHER_FEL_INPLACE=0 turns it off. Bypassed frames keep their mapping. A frame shown as its base layer carries the Profile 8.1 RPU with the source's mapping, built next to the composed form; the composed form has its mapping removed whatever the el_type. The raw-base-layer RPU half follows TheFireKahuna/AetherEngine@d3a1531c and @c8910a0e. Composer fixes. Chroma planes round up for odd sizes, a zero NLQ ceiling drops the residual, the prediction is clamped in float, a passed enhancement slot pairs by access unit unless the base layer reorders, and the el_type probe rewinds with a bounded seek. The arrival-order path counts and logs the frames its seek threshold discards. Logging. The E-AC-3 decoded-to-PCM route is logged once per session and the dual-layer decoder logs session totals of its releases at info level; a flush folds the second in progress into those totals first. The heavy dual-layer test suites run one at a time under a serialized parent.
fe1a9f3 to
0b75e15
Compare
They are small synthetic suites with an injected clock, so they stay out of the heavy serialized group, but a loaded runner can pass 60 s.
The composed Dolby Vision frame is defined on the 4:2:0 chroma sites. AETHER_FEL_CHROMA=422 or 444 writes it as 4:2:2 or 4:4:4 instead: a site keeps its 4:2:0 composition, and a sample between two sites is the mean of the clamped 4:2:0 samples around it, rounded once. 4:2:0 stays the default and the output playback uses; any other value keeps it. 4:4:4 has no reconstruction in the spec and is marked experimental. Both formats take the pool path, since in-place composition is 4:2:0 only. The 4:2:0 composition moves into shared helpers with its arithmetic unchanged.
There was a problem hiding this comment.
Automated review
This PR releases a FEL base frame by its display deadline instead of waiting for three newer frames, composes into the decoded base-layer buffer, and attaches a Profile 8.1 RPU that keeps the mapping to frames shown as their raw base layer. It also adds an opt-in chroma sampling switch and several composer fixes. The direction is right: everything is fixed in the engine and kept behind the DualLayerDolbyVision trait. The PR gives a reason for each part that is not gated, and the reasons hold up (the renderer skip counter only runs on the arrival-order path, the el_type probe change cannot be reached without the trait, and the E-AC-3 note returns nil off the dual-layer route).
I checked the decoder's locking and epoch handling, the sweep timer's retirement on flush and close, OrderedDelivery.skip, the exclusive in-place claim, how the measurement run is excluded, the removeMapping scope (keepRpuBytes is false on the remux route), and how the render clock is read outside the decoder lock. I found nothing wrong there.
Findings
- Should fix:
docs/dolby-vision-fel.mddescribesskip=as frames the clock had already passed. The code counts frames below the seek threshold, which is also what the CHANGELOG says. (docs/dolby-vision-fel.md:640) - Should fix: the docs now quote several new figures: the 100 ms margin floor, the 15 to 50 ms grace, the 2 s held-clock bound, the backstop of 7, the 2 to 250 ms sweep and the 10 s totals interval. None of them is pinned by
DocumentedConstantsTests, and the one existing pin was loosened to drop the "3". (Tests/AetherEngineTests/DocumentedConstantsTests.swift:92) - Question: in-place composition binds the same texture as
access::readand asaccess::writein one dispatch. Metal's defined way to read and write one texture in one kernel isaccess::read_write. This is bit-identical on the Mac, but the GPU write into decoder surfaces has not been checked on the A12. (Sources/AetherEngine/Video/Metal/DoviFelComposer.swift:835) - Nit:
summaryInPlaceDroppedis not cleared inflush()orclose(), unlike the other per-second counters. A drop counted just before a seek or a close shows up in the next summary.
Checklist: CHANGELOG yes, docs yes, tests yes, traits both ways yes (CI confirms both configurations)
Needs a device run:
- In-place composition on the A12: drop medians against the pool path, a picture A/B with
AETHER_FEL_INPLACE=0and1, and the GPU write into VideoToolbox surfaces. - The deadline release on a title with a damaged enhancement layer. Cover a pause longer than 2 s, a seek while paused, 60 fps, a rate other than 1, and peak memory with up to 7 waiting 4K frames.
- 4:2:2 and 4:4:4 picture output and their chroma-location label.
The author lists all of these already.
Thanks for a large, carefully measured and well-documented change.
Automated review by Quint (Claude) of 101d864. It reads the code, it does not build or run it; a maintainer reviews and merges.
| count of composed frames and of the base frames released by the deadline, the held-wall bound and the | ||
| backstop. It is sent at most every 10 s and only when one of the counts moved since the last one, so a | ||
| device run is judged from it. The software host's `[SWDiag]` line carries `skip=`, the number of frames | ||
| the renderer skipped on arrival because the clock had already passed them, next to `lost=` and `layerDrop=`. |
There was a problem hiding this comment.
This sentence says skip= counts frames "the renderer skipped on arrival because the clock had already passed them". That does not match the code. _arrivalSkippedFramesDropped is incremented only for frames below the seek threshold: arrivalSkipBelow in handOverOnArrival, and skipUntilPTS when arrivalFloor != nil. The CHANGELOG and the Cadence.arrivalSkipped doc comment both describe it that way. A reader of a device log would take a non-zero skip= during steady playback as late frames, which it cannot be. Suggest: "the number of frames the arrival-order path discarded below a seek's target (the start of the GOP the seek landed in), apart from lost=".
|
|
||
| assertDocumented("once K = 3 newer base-layer", fel) | ||
| assertDocumented("is the maximum layer skew, 3;", fel) | ||
| assertDocumented("(`maxLayerSkewFrames`) is the maximum layer", fel) |
There was a problem hiding this comment.
This pin used to fix K = 3 in the prose ("is the maximum layer skew, 3;"). It now matches only the symbol name, so the number can drift. The PR also adds several numbers to docs/dolby-vision-fel.md §5.5, §5.10 and §5.11 that are not pinned:
- the 100 ms margin floor (
deadlineMarginFloor) - the 15 to 50 ms grace (
enhancementGraceFloor/Cap) - the 2 s held-clock bound (
heldClockWallBound) maxDeadlineHoldFrames= 7 and the stall threshold of 8- the 2 to 250 ms sweep bounds
- the 10 s totals interval (
totalsLogIntervalSeconds) - the 175 MB figure
Per CLAUDE.md, numbers quoted in docs are pinned here. Please add assertDocumented lines plus XCTAssertEqual on the constants, the same way the K = 3 and capacity-32 entries do. The sweep bounds are private static, so they need to become internal or be dropped from the doc.
| let outYTex: MTLTexture | ||
| let outUvTex: MTLTexture | ||
| if inPlace { | ||
| outYTex = blYTex |
There was a problem hiding this comment.
Question: in place, one MTLTexture is bound to texture(1) as texture2d<ushort, access::read> and to texture(3) as access::write in the chroma pass. The luma pass does the same with texture(0) and texture(3). Metal's documented way to read and write one texture inside a kernel is a single access::read_write argument. Aliasing two bindings with different access is not covered by that contract, even though each thread reads its own sample before writing it. It is bit-identical on the Mac. On the A12 only the CPU-write probe ran, so the GPU path into decoder surfaces is unverified there.
Two options:
- Add
read_writevariants of the two kernels for the in-place dispatch.r16Uint/rg16Uintread-write needs tier 2 texture support, so checkdevice.readWriteTextureSupportininPlaceDecision. - Keep this approach and make the A12 picture A/B with
AETHER_FEL_INPLACE=0/1a hard gate before merge.
If you have a reference that says aliased read and write bindings are defined behaviour, a comment citing it would settle this.
Everything FEL-specific is behind the opt-in
DualLayerDolbyVisiontrait (off by default). With the trait off the build changes only inCHANGELOG.md,docs/dolby-vision-fel.md, a renderer counter and its log field that nothing reaches (see below), an unreachableel_typeprobe change, and a log helper that only fires on the dual-layer route. Open as a draft: the Mac side is tested, the Apple TV checks are listed at the end.What changes
maxFramesInFlightframe intervals (at least 100 ms), after a short grace (one frame interval, 15 to 50 ms, none past the presentation time) for a layer that is only a moment late. Before, it waited for three newer base frames, so a damaged or missing enhancement-layer stretch held the picture and dropped frames in bursts. The host hands the decoder its render clock; a paused or prerolling clock holds the frames (bounded at 2 s, or the margin for a preroll), and a count of 7 newer frames stays as a backstop below the stall guard's 8. An access unit with no enhancement-layer NAL releases its base frame at once. The sweep re-arms on every arrival. The 1 Hz summary line splits the bypasses into six counters by the rule that released them.AETHER_FEL_INPLACE=0turns it off for an A/B run.AETHER_FEL_CHROMApicks the chroma sampling of the composed frame: 4:2:0 (the default, and the composition itself), 4:2:2 or 4:4:4. The composition is defined on the 4:2:0 sites; 4:2:2 and 4:4:4 step it up by co-sited means (a copy on a site, the mean of the sites around a sample between them). 4:4:4 has no reconstruction in the spec and is marked experimental. The two experimental formats take the pool path, since in-place composition is 4:2:0 only, and their step-up kernels recompute the 4:2:0 sites (about 4 times and 16 times the chroma work; a later change can compose the sites once). Nothing changes unless the variable is set.el_typeis FEL, and that comes from the NLQ block, which is absent whendisable_residual_flagis set, so the composed form also has its mapping removed after mode 2 (dovi_rpu_remove_mapping, dual-layer route only; the remux route is unchanged). Cost: a second libdovi parse and conversion per RPU on the dual-layer route and one more NAL per side-data entry. When the mapping-keeping form cannot be built, the frame carries the composed form, which on the dual-layer route now always has its mapping removed, so that practically unreachable fallback gives an unreshaped frame (the second parse reads the same bytes through the same guard).vdr_in_maxthat shifts down to 0 drops the residual instead of clamping it, and the prediction is clamped in float before the integer conversion (an MMR result far outside the range overflowed int). An enhancement slot with no base time pairs by access unit when the base layer has never reordered; a reordering base layer keeps extrapolating.el_typeprobe. It rewinds with the same 5 s bounded seek as the interlace probe; a stalled rewind leaves the type unknown, so the route is not taken.skip=in the cadence line, apart fromlost=) and logs the first once per seek.FrameSideDataRingdocuments its 90 kHz keys (with a test).composed, deadline, held-wall and backstop releases) at info level, at most every 10 s and only when a count moved, so a device run can be judged from the host's log without a verbose trace.Not trait-gated, and why it is safe: the renderer's skip counter and the
skip=log field sit in the generic arrival-order code; they are reached only whensetArrivalOrderHandovergets a frame rate, which the host passes only for the dual-layer decoder, so without the trait the counter stays 0. Theel_typeprobe change is unreachable without the trait (DualLayerRouteDecisionanswers.notBuiltInbefore the probe). The E-AC-3 log helper returns nil unless the dual-layer route is active.Credit: the mapping-keeping RPU for a raw base layer follows TheFireKahuna/AetherEngine@d3a1531c ("compose base-only when the enhancement layer is missing; a raw base layer keeps its mapping") and TheFireKahuna/AetherEngine@c8910a0e ("strip the mapping from every composed frame's RPU, not only those libdovi reads as FEL"). From the first only the RPU half is taken (a raw base layer carries the mapping-keeping RPU; the frame is still shown as its base layer, not composed base-only); the second is the same fix as the mapping removal after mode 2 here. Thanks @TheFireKahuna.
Overlap with open PRs: #902 and #867 touch
SoftwarePlaybackHost.swiftand merge cleanly with this branch; #703 touchesSoftwarePlaybackHost.swiftandSampleBufferRenderer.swiftbut already conflicts with main in 34 files. #810 will need aCHANGELOG.mdmerge only.Tested on Mac
On the branch tip, in build slots, every build and test line behind the soak guard:
swift build --build-tests, trait off and trait on (--traits DualLayerDolbyVision): both build clean.DualLayerDeadlineBypassandDualLayerBypassRpu(31 tests in 2 suites) passed three times with the trait on, and the build is clean with the trait on and off; the two suites now carry a two-minute time limit.--no-parallel --filter DoviFelRealFileTests,DV_FEL_TESTS_DIRset): 12 of 12 passed (411 s on a loaded Mac).swift build --enable-all-traits --build-tests: clean. 14 covering suites with all traits: 161 tests passed. TheProbes/AllTraitstvOS devicexcodebuild: BUILD SUCCEEDED.flush()folds the interval in progress into the session totals before the per-second counters are zeroed (it passed with the DualLayerVideoDecoder, deadline-bypass, in-place compose and related suites: 130 tests in 7 suites, trait on).swift test, trait off: Swift Testing 5074 tests in 698 suites passed, XCTest 798 (1 skipped), 0 failures.swift test --traits DualLayerDolbyVision: Swift Testing 5378 tests in 727 suites passed, XCTest 800 (1 skipped), 0 failures; no time-limit issues in parallel mode. The count includes the chroma sampling switch (theDoviChromaFormatTestssuite, 10 tests, with its 4:2:0 bit-exactness, 4:2:2 and 4:4:4 mean checks, odd sizes and in-place refusal).LIBDISPATCH_COOPERATIVE_POOL_STRICT=1 swift test --disable-xctest), trait off 5074 passed, trait on 5378 passed. One earlier trait-on strict run had failures in three replay and TLS suites that pass when run alone; the full run passed on the rerun.build-for-testing(trait off): TEST BUILD SUCCEEDED, both. With the trait off the objects of the composer, decoder, side-data ring and picture-alignment files are empty.Still needs Apple TV
AETHER_FEL_INPLACE=0and1(colour and HDR mode switch, picture match), plusinPlaceDroppedandcomposeFailuresin the summary line.AETHER_FEL_CHROMA): the picture on the TV against the 4:2:0 default, and the chroma-location label the output carries (copied from the base layer; dump one 4:2:2 frame's attachments on the Mac first). Not needed for the 4:2:0 default.totals:lines of the host log give the numbers (delta of the deadline, held-wall and backstop fields).skip=in the cadence line and no change in drops.