Skip to content

fix(subtitles): preserve secondary sidecar ASS metadata - #826

Merged
superuser404notfound merged 1 commit into
superuser404notfound:mainfrom
orut34iop:codex/feat-secondary-ass-metadata
Oct 21, 2026
Merged

superuser404notfound merged 1 commit into
superuser404notfound:mainfrom
orut34iop:codex/feat-secondary-ass-metadata

Conversation

@orut34iop

Copy link
Copy Markdown
Contributor

Summary

preserveASSMarkup already preserves primary sidecar ASS events and their script header, but secondary sidecars silently use the default text decoder and discard the header. Honor the same opt-in option on the secondary channel and publish secondarySidecarASSHeader independently.

What changed

  • Secondary decoding passes the existing option to SubtitleDecoder; the existing ASS/SSA codec gate keeps SRT/VTT output unchanged.
  • Reset the secondary header on a new sidecar, embedded selection, clear and stop. Publication stays behind the existing cancellation/active-channel checks.
  • Document which header belongs to which cue list. No renderer, forced default styling, channel-exclusivity rule or new embedded decoding path.

Builds on the existing primary API from #48 and per-stream ASS decoding from #267. All changes are based on main 151322d1b843b4414a0fcffa9ed44e8b3a9d087e; #824 and #825 touch adjacent secondary state/comments but are separate contributions, so merge order may require textual reconciliation.

Test plan

  • Host: Mac17,3, macOS 26.7.1 (25G241), Swift 6.4 / Xcode toolchain; default traits, native SwiftPM.
  • Media: existing generated Matroska fixture with English ASS at 640×480 and Spanish ASS at 1920×1080, identical raw placement tags; generated SubRip sidecar. No private media or device playback claim.
  • Regression coverage: two independent headers/raw events, default headerless output, ASS-to-SubRip/embedded selection, independent clear, cancelled publication, stop, carryover restoration and WebVTT staying text.
  • Strict cooperative-pool focused run: 34 Swift Testing tests passed. Full serial run passed: 795 XCTest (one existing skip) and 4620 Swift Testing tests / 634 suites.
  • The initial full parallel run failed only in the existing SoftwarePacketMemoryFIFOTests history-walk assertion with invalidCursor; reclaimable-history test assumptions are also addressed by open test(cache): respect kernel purges in history eviction checks #819. No unrelated tests were edited; failed evidence was retained.
  • Documentation links and git diff --check passed.
  • Valid Apple Development signing and verification for local test executables; no ad-hoc signing.

Checklist

  • CHANGELOG.md updated
  • Conventional commit
  • Fix lives in the engine
  • Public API documented

@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 @orut34iop, this is a clean, well-scoped change.

Review notes:

  • The embedded secondary path already builds its drain decoder with preserveASSMarkup, so secondary ASS reached hosts as raw event lines from that path but as cleaned text from a sidecar. This PR makes the channel consistent, and the separate secondarySidecarASSHeader is the right shape since PlayRes and styles differ per script (the 640/1920 fixture shows it well).
  • Reset points look complete: new sidecar, embedded selection, clearSecondarySubtitle(), stopInternal(). Publication stays behind the cancellation and active-channel guards, and after the merge also behind #822's load-generation guard.
  • Conflict with #822 in startSecondarySidecarDecode was additive on both sides; resolved by keeping the origin-aware header resolution plus the generation capture and adding the preserveASS capture.
  • Changelog entry moved into [Unreleased] (the auto-merge had placed it under 7.40.0) and rewritten in house style.

Verified on the merged tree: 780 XCTest (1 skipped, 0 failures) and 4711 Swift Testing tests in 650 suites, all passing.

Merging.

@superuser404notfound
superuser404notfound merged commit 867b172 into superuser404notfound:main Oct 21, 2026
4 checks passed
superuser404notfound pushed a commit that referenced this pull request Oct 21, 2026
The auto-merge had placed it under 7.40.0's Added section.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WJcXkGQrMiBPixVEFTSBJS
@superuser404notfound

Copy link
Copy Markdown
Owner

Released in 7.45.0 (52802af).

Quick104 added a commit to Silo-Server/AetherEngine that referenced this pull request Oct 24, 2026
…o fork

Upstream superuser404notfound#826 decodes a secondary sidecar with preserveASSMarkup and
publishes its raw events and secondarySidecarASSHeader. The fork keeps
raw ASS on the primary overlay only, as it already does for an embedded
secondary track, because Silo draws secondary cues as text: with the
upstream change an ASS secondary showed its event lines. The superuser404notfound#826 tests
now pin the fork's behaviour, and the fork notes are in CHANGELOG.md
and docs/api.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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