Add DVD text subtitle rendering to the authoring pipeline#19
Merged
ScottMorris merged 8 commits intomainfrom Apr 4, 2026
Merged
Add DVD text subtitle rendering to the authoring pipeline#19ScottMorris merged 8 commits intomainfrom
ScottMorris merged 8 commits intomainfrom
Conversation
**What changed** - add explicit `RenderTextSubtitles` build jobs and subtitle render-mode support in the Rust and TypeScript models - prepare text subtitle streams through FFmpeg, compose them onto authored title MPEGs with `spumux`, and surface the work on the Build page - preserve mixed bitmap and text subtitle mapping order in the planner and add regression coverage for the ordering and text-render planning paths - improve render-step diagnostics with a host-font hint when subtitle composition fails **Why** - issue `#17` needs real DVD text subtitle authoring instead of warning-only handling - explicit render jobs and order-aware planning make the build output and progress reporting match the authored subtitle intent more closely - stronger tests and clearer diagnostics reduce the risk of silent subtitle regressions during authoring
**What changed** - update the product summary and spec to describe first-pass text subtitle rendering as shipped DVD capability - revise the DVD authoring notes and subtitle plan so they describe the hybrid FFmpeg plus `spumux` seam that the build now uses - remove the outdated mixed-order limitation from the docs now that the planner keeps subtitle mapping order intact **Why** - issue `#17` changes the shipped subtitle authoring surface, so the repository docs need to describe the real behaviour and its remaining first-pass limits - keeping the docs aligned with the implementation avoids misleading review, QA, and release notes work
ScottMorris
added
enhancement
Spindle Rust tests94 tests +7 94 ✅ +7 1s ⏱️ -1s Results for commit 5f65ac6. ± Comparison against base commit 2eb52d6. This pull request removes 1 and adds 8 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
**What changed** - restore the `skip unsupported streams` escape hatch so builds can strip text subtitle mappings instead of scheduling render jobs - resolve text subtitle fonts through Fontconfig from a small sans-serif shortlist rather than hard-coding a single alias - add validation for missing host subtitle fonts and a regression test that covers the skip-text fallback path - apply the formatting cleanup needed for the updated subtitle authoring docs and Rust sources **Why** - the review found that the existing fallback no longer let users author a subtitle-free disc when text subtitle rendering was unavailable - relying on one host-specific font alias made the new authoring path too machine-dependent and could fail late during build execution - early validation and a tested fallback make the text subtitle path safer without hiding the first-pass host-font constraint
**What changed** - treat zero-cue extracted text subtitle files as a handled runtime outcome during authoring - carry the current authored MPEG forward when a subtitle pass is skipped so later passes still chain correctly - add executor coverage for empty and whitespace-only prepared subtitle files **Why** - trimmed sample clips can legitimately include subtitle streams that have no cues in the authored range - builds should continue when subtitle extraction succeeds structurally but produces no usable subtitle text for the selected portion
**What changed** - normalise recognised three-letter subtitle languages to the two-letter codes that `dvdauthor` accepts in `<subpicture>` declarations - omit the `lang` attribute when a subtitle language cannot be made DVD-safe - add authoring coverage for normalised and invalid subtitle language values **Why** - real projects can carry ISO 639-2 style values such as `eng` and `fre` in subtitle mappings - `dvdauthor` rejects those values during final authoring, even when the rest of the build has succeeded
**What changed** - add `isolang` to normalise subtitle language codes before writing `dvdauthor` subpicture declarations - accept both ISO 639-1 and ISO 639-3 inputs, and trim simple region suffixes such as `en-US` - keep the existing fallback of omitting `lang` when a subtitle language cannot be mapped safely **Why** - a library-backed mapping is easier to trust and maintain than a local hand-written conversion table - this keeps the `dvdauthor` fix focused on behaviour while moving the code-to-code mapping burden onto a dedicated crate
**What changed** - canonicalise common ISO 639-2/B bibliographic subtitle codes such as `fre` before passing them through `isolang` - document why the alias shim exists at the `dvdauthor` normalisation boundary - add regression coverage for the French bibliographic code path **Why** - source media metadata can legitimately report bibliographic language codes even when `dvdauthor` needs a two-letter output tag - `isolang` handles canonical 639-3 values well, but the bibliographic alias layer still needs a small compatibility shim
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 254ae48e4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
**What changed** - stop failing build planning when Fontconfig cannot resolve a host subtitle font - fall back to a generic `sans-serif` font-family hint for `spumux` when `fc-match` is unavailable - update the validation warning so it reflects the fallback behaviour more accurately **Why** - `spumux` can still attempt generic family or path-based font resolution even when Fontconfig is not present - blocking planning on `fc-match` availability was stricter than the authoring toolchain itself and could reject otherwise valid text subtitle builds
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #17
Summary
User-facing changes
spumux.renderTextSubtitleswork in the build model and Build page so subtitle rendering appears as its own visible stage.Workflow runtime
RenderTextSubtitlesjobs, keep text subtitle work outside the base title transcode, and preserve mixed bitmap/text subtitle mapping order in the authored output plan.Documentation
README.md,SPEC.md, and the DVD subtitle docs to describe first-pass text subtitle rendering as shipped behaviour.Test plan
docker run --rm -v "$PWD":/workspace -w /workspace ghcr.io/liminal-hq/tauri-dev-desktop:latest bash -lc 'cargo fmt --all --check && cargo test -p tauri-plugin-spindle-project --no-run'docker run --rm -v "$PWD":/workspace -w /workspace ghcr.io/liminal-hq/tauri-dev-desktop:latest bash -lc 'cargo test -p tauri-plugin-spindle-project build_plan_renders_text_subtitles_after_base_transcode -- --nocapture'docker run --rm -v "$PWD":/workspace -w /workspace ghcr.io/liminal-hq/tauri-dev-desktop:latest bash -lc 'cargo test -p tauri-plugin-spindle-project build_plan_preserves_mixed_subtitle_stream_order -- --nocapture'docker run --rm -v "$PWD":/workspace -w /workspace ghcr.io/liminal-hq/tauri-dev-desktop:latest bash -lc 'pnpm -C apps/spindle exec tsc --noEmit'docker run --rm -v "$PWD":/workspace -w /workspace ghcr.io/liminal-hq/tauri-dev-desktop:latest bash -lc 'cargo test -p tauri-plugin-spindle-project execute_build_plan_smoke_authors_text_subtitle_stream -- --ignored --nocapture'The ignored smoke test is checked because the test target exists and was invoked successfully, but it skipped in the container because
ffmpegwas not available onPATHin that image.