Skip to content

perf(subtitles): retain bitmap images with deferred PNG backing - #879

Open
qoli wants to merge 3 commits into
superuser404notfound:mainfrom
qoli:fix/subtitle-bitmap-storage
Open

qoli wants to merge 3 commits into
superuser404notfound:mainfrom
qoli:fix/subtitle-bitmap-storage

Conversation

@qoli

@qoli qoli commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Retained bitmap subtitle history keeps cropped cues expanded to RGBA. This change retains lossless PNG-backed CGImage providers while preserving rendered pixels, alpha, crop, placement, canvas, forced flags, interpolation and cue history.

This changes the host-visible provider contract: SubtitleImage.cgImage no 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

  • Retain PGS, DVB and DVD bitmap history through a lossless ImageIO provider.
  • Add optional SubtitleImage.encodedBackingBytes for 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.
  • Compare rendered premultiplied RGBA in the palette regression and cover all alpha values, provider lifetime, repeated drawing and compressed backing.
  • Document the provider contract under Changed, the diagnostic count under Added, and Core Animation's presentation costs in the source comments and API reference.

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.

  • Movie workload: sampled peak process footprint was 815.84 MB raw versus 589.55 MB PNG; last-120-second medians were 756.73 MB versus 557.61 MB. Both retained 2,207 images. Only 560 retained images were observed being submitted to the PNG overlay; submission is not proof of scanout.
  • Controlled presentation of all 400 generated cues through the actual UIImageView overlay reproduced the concern: approximately +112 MB raw versus +329 MB PNG, with decoded pixels surviving view removal until history was released.
  • Time Profiler captured PNG decoding on main under 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

  • Initial implementation 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.
  • Initial implementation upstream CI: macOS tests, iOS Simulator/release aetherctl, tvOS/visionOS Simulator and docs links passed in run 40214404523.
  • Documentation follow-up: link/anchor and whitespace checks passed; removing documentation comments leaves both changed Swift files' executable source identical.
  • The fork branch now includes current upstream b40b269e to 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.
  • Current head 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.
  • The physical measurements above are separate downstream Debug experiments, not final acceptance of an engine-owned presentation integration or a release-performance guarantee.

Checklist

  • CHANGELOG.md and public API documentation updated
  • Conventional Commits
  • Provider-layout change is explicit
  • Focused pixel/lifetime regressions included
  • Resolve the engine-owned Core Animation presentation/retention design
  • Settle the public visibility of encodedBackingBytes

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.
@qoli
qoli marked this pull request as ready for review October 25, 2026 18:24

@superuser404notfound superuser404notfound left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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) or UIImageView does (Sodalite's overlay among them), CA decodes the lazy image and keeps the decoded pixels attached to that CGImage for 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:

  1. 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.
  2. 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.

@qoli

qoli commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for measuring both presentation paths. I checked our current tvOS subtitle overlay: bitmap cues are presented with UIImageView(image: UIImage(cgImage: image.cgImage)), so the Core Animation case is directly relevant to our integration. The rendered-pixel regressions do not establish the post-presentation footprint.

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.

@superuser404notfound superuser404notfound added the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@github-actions github-actions Bot removed the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@superuser404notfound superuser404notfound added the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@github-actions github-actions Bot removed the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@superuser404notfound superuser404notfound added the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@github-actions github-actions Bot removed the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@superuser404notfound superuser404notfound added the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@github-actions github-actions Bot removed the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@superuser404notfound superuser404notfound added the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@github-actions github-actions Bot removed the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@superuser404notfound superuser404notfound added the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@github-actions github-actions Bot removed the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
@superuser404notfound superuser404notfound added the claude-review Maintainer: request an automated Claude review label Oct 26, 2026

@github-actions github-actions Bot 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.

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 retainedBitmap says 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 for CGContext.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. SubtitleImage used to promise a premultiplied RGBA provider. The ImageIO PNG provider can expose straight alpha in a different layout, and a host reading dataProvider bytes 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 encodedBackingBytes be 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 it internal (or in notHostAPI for 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

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.

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.

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.

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.

Comment thread CHANGELOG.md Outdated
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?

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.

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.

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.

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.

@github-actions github-actions Bot removed the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
ijuniorfu pushed a commit to ijuniorfu/AetherEngine that referenced this pull request Oct 26, 2026
…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
ijuniorfu pushed a commit to ijuniorfu/AetherEngine that referenced this pull request Oct 26, 2026
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
ijuniorfu pushed a commit to ijuniorfu/AetherEngine that referenced this pull request Oct 26, 2026
… 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
@qoli

qoli commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

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 mode

Apple TV 4K AppleTV6,2 (A10X), tvOS 26.6 (23L773), Xcode 27.1, signed Debug build. This is a downstream AetherEngine 7.47.0 snapshot (a1a70cd63c1fef89cffcf0e8110456807d0fab05) with the same retainedBitmap helper as this PR's 77e6deb.

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 TASK_VM_INFO.phys_footprint, in decimal MB.

Measurement Raw PNG
Sampled process-footprint peak 815.84 MB 589.55 MB
Last 120 seconds, median footprint 756.73 MB 557.61 MB
Final unique retained images 2,207 2,207
Final expanded layout 290,007,028 bytes 290,007,028 bytes
Final encoded backing 0 43,979,585 bytes
Median source-ready buffer ahead 53.40 s 53.37 s
Median AVPlayer loaded buffer ahead 13.74 s 13.79 s

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 overlay

Inside 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.

Mode Retained only After all 400 presented Views removed, history retained History released
Raw 111.87–111.92 MB 111.51 MB 111.51 MB approximately baseline
PNG directly to UIImageView 14.61–14.78 MB 329.22–329.27 MB 329.22–329.25 MB +0.10–0.16 MB
PNG history + separate worker-rendered raw presentation image 14.68–14.88 MB 14.35–14.55 MB 14.32–14.52 MB +0.20–0.21 MB

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:

CA::Transaction::commit
  -> CA::Render::prepare_image
    -> PNGReadPlugin::decodeImageImp

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.

@superuser404notfound superuser404notfound added the claude-review Maintainer: request an automated Claude review label Oct 26, 2026

@quint-aether quint-aether Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 retainedBitmap is 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 list encodedBackingBytes under ### Added. (CHANGELOG.md:115)
  • Should fix, question (unchanged): It is still open whether encodedBackingBytes needs 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.

@github-actions github-actions Bot removed the claude-review Maintainer: request an automated Claude review label Oct 26, 2026
Document renderer-retained pixels and main-thread decode risks. Separate the host-visible provider contract change from the added diagnostic count.

@quint-aether quint-aether Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 ### Changed and leads with the contract change. encodedBackingBytes is listed under ### Added.
  • Blocking (still open, reframed): The doc comment on retainedBitmap is 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 to UIImageView, with PNGReadPlugin::decodeImageImp running on main under CA::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 at Sources/AetherEngine/Decoder/EmbeddedSubtitleDecoder.swift:740)
  • Should fix, question (unchanged, no answer yet): Does encodedBackingBytes need to be public? Its own docs say it is neither a footprint figure nor a bound. If only the tests use it, retainedBitmap already 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.

Comment thread CHANGELOG.md Outdated
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@quint-aether quint-aether Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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