Repository navigation
Conversation
Keep authored subtitle history lossless while avoiding expanded RGBA storage for every retained cue. Document the drawing contract and encoded backing accounting, with pixel, provider lifetime and subtitle regressions.
superuser404notfound
left a comment
There was a problem hiding this comment.
Thanks for this, and for the careful write-up. The encode side holds up when I measure it, but I found one presentation path where the saving reverses, so I'd like to settle that before merging.
I measured on an M-series Mac using the PR's retainedBitmap as is, with 1400x140 cues filled with a deliberately noisy pattern (real PGS should compress better):
| Path | Raw RGBA | PNG backing (this PR) |
|---|---|---|
| Retained only, 1000 cues | +755 MB | +36 MB |
Drawn via CGContext.draw (the engine's SubtitleFrameCompositor) |
unchanged | unchanged, nothing gets cached |
Presented via Core Animation (CALayer.contents, rendered with CARenderer), 400 cues |
+193 MB, none of it tied to the images | +330 MB, of which 306 MB is freed only when the cue images are released |
| Draw cost per cue | 0.26 ms | 0.79 ms (plus 1.4 ms PNG encode on the decode worker) |
So:
- With CG drawing the PR does what it says: the provider is decoded on each draw and not kept.
- When a cue goes through Core Animation, which is what a SwiftUI
Image(decorative: cgImage)orUIImageViewdoes (Sodalite's overlay among them), CA decodes the lazy image and keeps the decoded pixels attached to thatCGImagefor as long as the image lives. Every cue that has been on screen is back at full RGBA size, plus the PNG bytes. The raw images don't show that growth. - The decode moves to the main thread at commit time, once per new cue. #628 moved PGS work off main for exactly that reason.
- The engine already prunes cues more than
subtitleCueRetentionSeconds(300 s) behind the playhead, and keeps packets in the store for backward seeks. Most retained images within that window are cues that have already been presented, which is the case where the saving disappears.
Caveat: this is CARenderer in-process on macOS, not a device. I expect the device commit path to decode the same way, but I haven't verified that on tvOS/iOS.
Two questions:
- Can you measure process footprint in your downstream app with the cues actually presented the way your UI draws them? The 289 MB figure is the expanded layout of all retained images, not footprint after presentation.
- If the retained history is the real cost, would a byte budget on the retained cue set (evicting the cues farthest from the playhead and re-decoding them from the packet store when a seek needs them) fit your case? That bound holds no matter how the consumer draws.
The benchmark scripts are small; happy to paste them if useful.
|
Thanks for measuring both presentation paths. I checked our current tvOS subtitle overlay: bitmap cues are presented with To clarify the original observation: the approximately 289 MB figure was the sum of expanded image layouts, not a measured process-footprint reduction after presentation. I do not yet have a before/after physical-device footprint run for this PR, and the now-green CI does not fill that gap. Please paste the benchmark scripts, including the footprint measurement and layer/image lifetime setup. That will let us compare the same retained-only, CGContext, and Core Animation cases before drawing a conclusion for the downstream UI. A byte budget with packet-store reconstruction is worth evaluating, but preserving subtitle offsets and backward seeks is a downstream requirement. I would need to verify reconstruction behavior and latency before agreeing to change retained-history policy. For now, the presentation-footprint question remains open; I am not claiming that the current PNG backing solves it. |
There was a problem hiding this comment.
Automated review
The PR stores each cropped bitmap subtitle as PNG bytes behind an ImageIO provider, so retained cue history no longer holds every image as raw RGBA. The direction is still open. The maintainer measured that on the Core Animation path (UIImageView / SwiftUI Image, which Sodalite uses) the decoded pixels stay cached on the CGImage and the decode runs on the main thread at commit. The author agreed that no footprint measurement after presentation exists yet.
Findings
- Blocking: The doc comment on
retainedBitmapsays drawing decodes "without caching them for the whole cue lifetime" and that the work stays off the presentation actor. Per the maintainer's measurement, both are only true forCGContext.draw. On the CA path the footprint is higher than today, and the PNG decode moves to the main thread once per new cue, which #628 removed. This needs a device footprint run with cues presented the way the host draws them, or a different bound such as the byte budget the maintainer proposed. (Sources/AetherEngine/Decoder/EmbeddedSubtitleDecoder.swift:740) - Should fix: Hosts that read the image's raw bytes lose a documented guarantee.
SubtitleImageused to promise a premultiplied RGBA provider. The ImageIO PNG provider can expose straight alpha in a different layout, and a host readingdataProviderbytes gets wrong pixels without any error. The CHANGELOG lists this under### Fixed. It belongs under### Changed, stated as a host-visible contract change. The new public property belongs under### Added. (CHANGELOG.md:115) - Should fix (question): Should
encodedBackingBytesbe public? Its own docs say it is not a memory measurement, and the maintainer's numbers show it can be far from the real cost once a cue has been presented. If a host has no decision to make from it, keeping itinternal(or innotHostAPIfor tests) avoids adding a public field that is easy to misread. (Sources/AetherEngine/PlayerState.swift:1639)
Checklist: CHANGELOG yes (wrong section), docs yes, tests yes, traits both ways n.a.
Needs a device run: Process footprint and main-thread time on tvOS/iOS with bitmap cues shown through UIImageView / SwiftUI, before and after this PR, over a long PGS title. Also the cost of the per-cue PNG encode on the decode worker on Apple TV hardware.
Thanks for the careful write-up and for being upfront about what the measurements do and do not show.
Automated review by Claude of 77e6deb. It reads the code, it does not build or run it; a maintainer reviews and merges.
| encodedBackingBytes: retained.encodedByteCount) | ||
| } | ||
|
|
||
| /// The subtitle history needs authored images for offsets and backward seeks, but does not |
There was a problem hiding this comment.
This comment holds for CGContext.draw, for example in SubtitleFrameCompositor, which also caches one overlay per cue set. It does not hold for the path Sodalite uses: UIImageView(image: UIImage(cgImage:)). In the maintainer's in-process CARenderer run, Core Animation decoded the lazy provider and kept the decoded pixels attached to the CGImage for as long as the image lived: +330 MB against +193 MB for raw RGBA over 400 presented cues. The decode also happens on the main thread at commit, once per new cue, which is the work #628 moved off main.
The cue store already prunes cues more than subtitleCueRetentionSeconds behind the playhead. That means most retained images have already been presented, and those are exactly the ones where the saving reverses.
Please settle this with a device footprint run on the real presentation path before relying on the PNG backing. The other option is to bound the retained set by bytes and re-decode evicted cues from the packet store. That bound holds however the host draws.
There was a problem hiding this comment.
The physical-device comparison is now posted at #879 (comment). It reproduces the direct-UIImageView cache growth and main-thread PNG decode, alongside a footprint benefit for the movie's much larger retained-but-unsubmitted set. The earlier claim that no physical footprint comparison existed was also corrected there after auditing the original artifacts.
qoli@d95d2d6 removes the unconditional no-cache/off-presentation-actor claims from this helper and makes the rendering-path limitation explicit in the public docs. This is a documentation correction, not an integration fix. The worker-rendered presentation copy remains a prototype; no byte-budget eviction or cue-retention change has been selected. I am leaving the presentation/integration question open.
| public let isForced: Bool | ||
| /// Known compressed provider storage, when the decoder retains a lossless encoded image. | ||
| /// This excludes ImageIO/CGImage overhead and temporary pixels used while drawing. | ||
| public let encodedBackingBytes: Int? |
There was a problem hiding this comment.
Question: what would a host do with this value? The doc says it excludes ImageIO/CGImage overhead and decoded pixels. The maintainer measured that after Core Animation presentation the real cost is the full RGBA plane plus these bytes, so the number can be off by more than an order of magnitude. Once a field like this is public, hosts will sum it as a memory figure. If only the tests need it, consider keeping it internal; retainedBitmap already returns the count for the tests.
There was a problem hiding this comment.
There is a downstream use beyond the tests: the host's DEBUG memory observation reports known encoded backing bytes and the count of images with that accounting, alongside expanded-layout counters and separately sampled process footprint. It is diagnostic accounting, not an eviction decision or a memory estimate. In the posted movie run the PNG count was 43.98 MB while process footprint was hundreds of MB, so substituting one for the other would be incorrect.
qoli@d95d2d6 now explicitly excludes renderer-retained decoded pixels as well as framework overhead and temporary draw pixels, and calls out that this is neither a process-footprint measurement nor a memory bound. I have kept visibility unchanged in this documentation follow-up and am leaving the public-surface decision open for review.
…it out actions/checkout v7 refuses a fork's head under pull_request_target (first live run on superuser404notfound#879). Rather than opting out of that guard, the head is now only fetched as refs/remotes/pr/head and read with git show / git ls-tree, so no fork file ever lands in the working tree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJcXkGQrMiBPixVEFTSBJS
The first live review (superuser404notfound#879) ended after ten turns with four denied commands and no comment: pipes and $(cat <<EOF) are outside the allowlist on purpose (cat would bypass the /proc read denial). The rules now name each allowed command and the heredoc form for the summary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJcXkGQrMiBPixVEFTSBJS
… or GitHub token The action-based setup gave Claude gh and git on fork content with secrets present, and every protective layer (read denials, the action's sandbox, a network allowlist, nested isolation) broke the next command: five live runs, no review posted. Now each job is split in three: - plain shell steps with the GitHub token fetch the PR or issue into files (fetch-pr.sh reads the head from git objects, never checks it out, and renames instruction files a PR adds so they cannot act as instructions); - the pinned Claude Code CLI runs with Read, Glob and Grep only, reading the base checkout plus that data directory, with credential paths denied, and returns structured JSON (run-claude.sh); a denied read inside the data directory fails the run instead of posting an empty review; - plain shell steps post the result: one PR review with inline comments that falls back to listing them in the body when GitHub refuses a line (post-review.sh), or allowlisted labels plus one comment (post-triage.sh). claude-code-action, the helper scripts and the @claude workflow are gone. Both paths were run end to end locally against superuser404notfound#879 and superuser404notfound#886 with posting in dry-run mode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJcXkGQrMiBPixVEFTSBJS
|
New physical-device results: I reproduced the Core Animation concern on tvOS, while the real playback workload still shows a footprint benefit from PNG history. First, a correction to my earlier comment: re-auditing the original session artifacts found that physical raw/0010 footprint runs did exist. That adjacent ten-minute pair had sampled peaks of 827.8 MB and 618.2 MB. The approximately 289 MB number itself was expanded layout, as previously clarified. Those older runs lacked a complete frozen-binary manifest, so the new comparison below has stronger provenance. Real playback, ten minutes per modeApple TV 4K Both modes used the same installed binary, with an explicit DEBUG-only switch selecting the original raw return versus PNG storage. Fresh process per run, PNG first then raw; same REMUX, playback starting point, audio/subtitle tracks, PGS codec, and buffer preference. Both advanced approximately 600 seconds without a reported memory-pressure event. Measurements are
That is 27.7% lower sampled peak and 26.3% lower last-120-second median for this workload. This is one pair, not a general performance guarantee: 428/425 samples, approximately 1.075-second median sampling intervals, with gaps up to 11.1 seconds. The peaks are sampled peaks. I also added weak, non-owning tracking of images submitted to the actual subtitle overlay. At the end of the PNG run, only 560 of the 2,207 retained images had been submitted, accounting for 77.34 MB of the 290.01 MB logical layout. Submission does not prove scanout, and this observation does not distinguish future cues from skipped updates. It does show that the retained set in this workload is much larger than the set submitted to the UI. Controlled presentation through the real downstream UIImageView overlayInside the fully initialized app, I retained 400 distinct generated 1400×140 images, presented every image, removed the views, then released history. No history eviction. Transparent-text inputs were run twice in reversed mode order, each in a fresh process. Footprints below are deltas from each process's baseline.
A separate deterministic 16-color noise control, once per mode, gave post-presentation deltas of 336.77 / 394.85 / 79.46 MB respectively. These generated inputs are independent controls, not your unpublished benchmark script. The movie and generated tests used separate instrumented builds; each comparison kept its own binary fixed. Time Profiler on the physical TV captured this on the main thread: PNG encoding remained on the worker. In the temporary-presentation-image prototype, the PNG draw/decode was sampled on its presentation worker instead. Separate profiling runs were excluded from the footprint/timing comparisons. For the transparent-text control, median main flush time per cue was raw 0.61–0.92 ms, direct PNG 5.86–8.49 ms, and worker-copy approximately 0.35 ms. PNG encoding cost approximately 3.4 ms on the worker; the worker presentation copy approximately 8.4 ms. These are Debug synthetic diagnostics with varying realized display cadence, not Release FPS or real-movie per-cue latency claims. The device results therefore support both observations: PNG storage benefits this movie's large retained-but-unsubmitted set, and handing the retained lazy images directly to CA can accumulate decoded pixels and move work onto main. The helper comment should not claim that all drawing paths avoid lifetime caching or presentation-actor work. The worker-copy result is a feasibility prototype preserving encoded history, not a completed player integration. Subtitle offsets, backward-seek reconstruction, cancellation, and rapid track changes still need device acceptance for that integration. I have not changed cue retention or introduced a byte budget/eviction policy on the basis of these measurements. |
There was a problem hiding this comment.
Automated review
The head is still 77e6deb, so the code is the same as in the last review and all three earlier findings are still open. The author's tvOS device run (AppleTV6,2) since then confirms the blocking finding. Fed directly to UIImageView, the PNG-backed images keep their decoded pixels attached: about +329 MB against about +112 MB raw for 400 presented cues. A Time Profiler capture also shows PNGReadPlugin::decodeImageImp running on the main thread under CA::Transaction::commit. The same run shows a real footprint gain on a ten-minute PGS title (sampled peak 815.84 MB raw against 589.55 MB PNG), because most retained cues were never handed to the overlay.
Findings
- Blocking (unchanged): The doc comment on
retainedBitmapis still unchanged, and the direction is still undecided. The device data shows that PNG backing only pays off for cues that are never presented through Core Animation. For cues that are presented, it moves the decode back onto main, which is the work #628 removed. The two remaining options are the worker-rendered presentation copy the author prototyped and the byte budget the maintainer proposed. Choosing between them is the maintainer's call, and neither is in this diff. (Sources/AetherEngine/Decoder/EmbeddedSubtitleDecoder.swift:740) - Should fix (unchanged): The CHANGELOG entry is still filed under
### Fixed, but it describes a contract change for hosts. It also does not listencodedBackingBytesunder### Added. (CHANGELOG.md:115) - Should fix, question (unchanged): It is still open whether
encodedBackingBytesneeds to be public. (Sources/AetherEngine/PlayerState.swift:1639)
Checklist: CHANGELOG yes (wrong section), docs yes, tests yes, traits both ways n.a.
Needs a device run: The author's run settles the measurement question for the current code. Whichever integration is chosen (a worker presentation copy or a byte budget) needs its own device run. That run should cover footprint, main-thread time, subtitle offsets and backward seeks.
Thanks for the careful device measurements, and for being clear about what they show and what they don't.
Automated review by Quint (Claude) of 77e6deb. It reads the code, it does not build or run it; a maintainer reviews and merges.
Document renderer-retained pixels and main-thread decode risks. Separate the host-visible provider contract change from the added diagnostic count.
There was a problem hiding this comment.
Automated review
New head d95d2d6. The CHANGELOG and the retainedBitmap doc comment now state the Core Animation behaviour honestly, but the code is unchanged: hosts still get the lazy PNG-backed CGImage. On the path Sodalite uses, that decodes on main at commit and grows footprint for every presented cue. The direction is still open.
Findings
- Fixed: The CHANGELOG entry now sits under
### Changedand leads with the contract change.encodedBackingBytesis listed under### Added. - Blocking (still open, reframed): The doc comment on
retainedBitmapis now accurate, but documenting the regression does not resolve it. The author's tvOS run measured about +329 MB against about +112 MB raw for 400 cues fed straight toUIImageView, withPNGReadPlugin::decodeImageImprunning on main underCA::Transaction::commit. #628 removed exactly that main-thread work. The new docs leave hosts on the CA path with that cost, and the only way out is a host-side worker copy. That is host compensation for engine behaviour. The engine needs to own the bound, either through the worker-rendered presentation copy the author prototyped or through the byte budget the maintainer proposed. Choosing between them is the maintainer's call. (CHANGELOG.md:89; earlier comment atSources/AetherEngine/Decoder/EmbeddedSubtitleDecoder.swift:740) - Should fix, question (unchanged, no answer yet): Does
encodedBackingBytesneed to be public? Its own docs say it is neither a footprint figure nor a bound. If only the tests use it,retainedBitmapalready returns the count for them. (Sources/AetherEngine/PlayerState.swift:1640)
Checklist: CHANGELOG yes, docs yes, tests yes, traits both ways n.a.
Needs a device run: Whichever integration is chosen (a worker presentation copy or a byte budget) needs its own tvOS/iOS run. It should cover footprint, main-thread commit time, subtitle offsets and backward seeks over a long PGS title.
Thanks for the quick follow-up and for correcting the docs to match the measurements.
Automated review by Quint (Claude) of d95d2d6. It reads the code, it does not build or run it; a maintainer reviews and merges.
| must draw into an explicitly configured bitmap context instead of interpreting them as raw | ||
| RGBA. Authored pixels, alpha, crop, placement, forced flags and cue lifetime are preserved. | ||
| PNG encoding runs on the subtitle worker; pixel decoding follows the consumer's rendering | ||
| path. Core Animation may decode during a main-thread commit and retain decoded pixels for |
There was a problem hiding this comment.
This sentence documents a regression instead of fixing it. A host that shows bitmap cues through UIImageView or SwiftUI Image, which is what Sodalite does, now gets a PNG decode on the main thread at every new cue's commit. It also keeps the full decoded RGBA alive for the life of the image, on top of the PNG bytes. The author's tvOS run measured about +329 MB against about +112 MB raw for 400 presented cues. The only way for a host to avoid this is to render its own copy on a worker, and that is host-side compensation for engine behaviour (CLAUDE.md: playback bugs are fixed in the engine). Before this lands, the engine should hand hosts an image that is safe to present: for example a worker-rendered raw copy for the cues being shown, as in the author's prototype, or a byte budget on retained history. Then this entry would not need to warn about the CA path.
Preserve the pending subtitle contract entries under Unreleased and the upstream 7.48.0 notes. Resolve the changelog conflict without changing the subtitle implementation or host checkout.
There was a problem hiding this comment.
Automated review
I checked the new head 6cbf2a9. It is a rebase onto main after the 7.48.0 release, and the PR's code and docs have not changed since d95d2d6, so the findings from the last review still stand: the blocker on the Core Animation path and the open question about encodedBackingBytes being public.
Automated review by Quint (Claude) of 6cbf2a9. It reads the code, it does not build or run it; a maintainer reviews and merges.
Summary
Retained bitmap subtitle history keeps cropped cues expanded to RGBA. This change retains lossless PNG-backed
CGImageproviders while preserving rendered pixels, alpha, crop, placement, canvas, forced flags, interpolation and cue history.This changes the host-visible provider contract:
SubtitleImage.cgImageno longer guarantees premultiplied RGBA provider bytes. Hosts needing that layout must draw into an explicitly configured bitmap context. PNG encoding runs on the existing subtitle worker; pixel decoding and caching depend on the consumer's rendering path.Changes
SubtitleImage.encodedBackingBytesfor known compressed provider storage. It excludes framework overhead, temporary drawing pixels and renderer-retained decoded pixels; it is neither a process-footprint measurement nor a memory bound.Device evidence and remaining integration
Physical-device results and measurement limitations use Apple TV 4K
AppleTV6,2(A10X), tvOS 26.6, Xcode 27.1 and signed Debug builds. The movie comparison used the same installed binary with an explicit raw/PNG switch, fresh processes, the same ten-minute PGS workload, and a downstream 7.47.0 snapshot with the same bitmap helper implementation.UIImageViewoverlay reproduced the concern: approximately +112 MB raw versus +329 MB PNG, with decoded pixels surviving view removal until history was released.CA::Transaction::commit. A separate worker-rendered presentation-image prototype avoided that direct-lazy-image path, but it is not an integrated engine solution.These observations support a benefit for the movie's retained-but-unsubmitted set and a regression for directly presented lazy images. Encoded backing alone is not a general post-presentation footprint fix. The engine-owned presentation/retention design remains open; this PR does not implement the worker-copy prototype or a byte-budget eviction policy. The chosen integration still needs device acceptance for offsets, backward seeks, cancellation and rapid track changes.
The approximately 289 MB original observation was expanded image layout, not itself measured footprint saving. The original session audit also recovered an older physical raw/PNG pair; the newer fixed-binary comparison above has stronger provenance.
Validation
77e6deb:swift build; focused 6 XCTest cases and 28 Swift Testing tests; strict-cooperative-pool 28 Swift Testing tests; documentation links and whitespace checks passed.b40b269eto resolve the release-changelog conflict. The net PR still changes the same six subtitle/test/documentation files. Documentation links and whitespace passed on the resolved preview.6cbf2a96: CI run 40424788615 passed macOS build/tests (default and all traits, including strict cooperative-pool runs), iOS Simulator/release aetherctl, tvOS/visionOS Simulator and docs links. The automated review job also completed successfully; its substantive integration and public-surface questions remain open.Checklist
CHANGELOG.mdand public API documentation updatedencodedBackingBytes