Repository navigation
Fix DVD audio compatibility gaps: bitrate, DTS, and track order - #137
Conversation
ScottMorris
added
bug
|
@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: e6f0018f35
ℹ️ 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".
| let mapping = titleset.titles.iter().find_map(|t| t.audio_mappings.get(i)); | ||
| if let Some(am) = mapping { | ||
| let format = dvdauthor_audio_format(am.output_target); |
There was a problem hiding this comment.
When a mapping uses copy mode, output_target does not determine the emitted codec: build_ffmpeg_transcode_command passes the source stream through unchanged. The UI nevertheless allows, for example, selecting DTS or MP2 while copying an AC3 source, and this new code then declares that physical AC3 stream to dvdauthor using the selected target format. That mismatch can cause dvdauthor to reject or incorrectly author the audio stream; resolve the actual source codec for copied mappings or reject mismatched copy targets before generating the declaration.
Useful? React with 👍 / 👎.
| let compatible = a.bitrate_bps.is_some_and(|bps| bps <= ceiling); | ||
| PropertyCheck { | ||
| value: a | ||
| .bitrate_bps | ||
| .map_or("unknown".to_string(), |bps| format!("{}kbps", bps / 1000)), | ||
| dvd_requires: format!("at most {}kbps for {}", ceiling / 1000, a.codec), | ||
| action: if compatible { | ||
| "none".to_string() | ||
| } else { | ||
| "re-encode".to_string() | ||
| }, | ||
| compatible, |
There was a problem hiding this comment.
For an otherwise DVD-compatible MPEG-2 asset containing 640 kbps AC3, this new check correctly marks the bitrate incompatible, but assess_dvd_compatibility still classifies the asset as RemuxCompatible because it only checks the audio codec name. The Assets page uses that unchanged overall value for its badge and only renders each audio codec check, so users are told the asset is remux-compatible even though selecting it now forces an audio re-encode. Fold this bitrate result into the overall assessment and expose it in the compatibility details.
Useful? React with 👍 / 👎.
| <option | ||
| value="DTS" | ||
| disabled={am.copyMode !== 'copy'} | ||
| title="ffmpeg has no reliable DTS encoder — DTS is only supported as a passthrough of an already-DTS source, not as a re-encode target." | ||
| > | ||
| DTS{am.copyMode !== 'copy' ? ' (passthrough only)' : ''} |
There was a problem hiding this comment.
This changes user-facing behaviour by making DTS passthrough-only and automatically switching incompatible audio and video streams to re-encode, but the commit updates neither README.md nor SPEC.md. Document the new restrictions and automatic defaults in both required files so the supported authoring workflow matches the application.
AGENTS.md reference: AGENTS.md:L89-L92
Useful? React with 👍 / 👎.
|
All three findings were real, verified, and addressed in the latest commits:
Full |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dca4a8ae7
ℹ️ 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".
| let mapping = titleset | ||
| .titles | ||
| .iter() | ||
| .find_map(|t| t.audio_mappings.get(i).map(|am| (t, am))); |
There was a problem hiding this comment.
When titles in the same titleset produce different codecs at the same audio index—for example, title 1 emits AC3 at index 0 while title 2 emits MP2—this find_map uses only title 1 to create the titleset-level <audio> declaration, even though ffmpeg independently encodes title 2 as MP2. The declaration therefore disagrees with title 2's physical stream, which can make dvdauthor reject or mis-author that title; validate/normalise the codec for each slot across every title before emitting a single declaration.
Useful? React with 👍 / 👎.
| a.codec.as_str(), | ||
| "ac3" | "dts" | "pcm_s16le" | "pcm_s16be" | "mp2" | "lpcm" | ||
| ) | ||
| ) && audio_bitrate_is_dvd_legal(&a.codec, a.bitrate_bps) |
There was a problem hiding this comment.
For a multi-audio asset containing one compliant stream and one over-ceiling stream, the surrounding .any(...) makes this new bitrate predicate succeed on the compliant stream and still reports RemuxCompatible, although TitleEditor includes every stream and re-encodes the incompatible one. Fresh evidence after the prior review is that the current fix added the bitrate predicate inside .any rather than requiring every included audio stream to pass, so the Assets badge remains inconsistent for mixed assets.
Useful? React with 👍 / 👎.
| }, | ||
| compatible: is_dvd_audio, | ||
| }, | ||
| bitrate: bitrate_check, |
There was a problem hiding this comment.
This stores the new incompatibility reason, but CompatibilityDetailView in apps/spindle/src/pages/AssetsPage.tsx lines 450-452 still adds only a.codec to its rows, so users who expand an over-ceiling AC3 asset cannot see why it requires re-encoding. Fresh evidence after the prior review is that the model and inspection result now contain bitrate, while the current Assets view still never consumes it.
Useful? React with 👍 / 👎.
Without explicit `<audio>` declarations, dvdauthor auto-detects a title's audio streams from the vob itself, and its inferred physical- track order does not reliably match the order the streams actually appear in. Confirmed in practice with a mixed AC3+MP2 title: dvdauthor placed the second (MP2) stream at physical audio 0, silently ignoring which track was authored as the default — so players default to the wrong (non-default) audio track regardless of the disc's own intent. Fix: declare every title's audio streams explicitly, in `audio_mappings` order, so dvdauthor's physical-track assignment matches what was authored instead of whatever it infers from the vob. `dvdauthor_subpicture_language` is renamed to `dvdauthor_language_code` since it's now shared between subpicture and audio stream declarations rather than subpicture-only.
A source stream can be a DVD-legal codec by name and still be illegal to stream-copy because its bitrate exceeds what DVD-Video allows for that codec — e.g. 640 kbps AC-3 is a legal AC-3 bitstream (produced by plenty of non-DVD encoders) but exceeds the DVD-Video spec's 448 kbps AC-3 ceiling. The codec-name check alone was greenlighting these for a stream copy, which would have shipped an out-of-spec disc. Adds a `bitrate` compatibility check to `AudioStreamCompatibility`, separate from `codec`, covering AC3 (448 kbps), MP2 (384 kbps), and DTS (1536 kbps, rounded up slightly from the DVD spec's 1509.75 kbps for encoder/muxer bitrate-field rounding). An unknown bitrate on a ceiling-bearing codec defaults to incompatible rather than optimistically assuming it's fine. Codecs without a simple bitrate ceiling (LPCM, whose legality depends on channel count/sample rate/depth) are unaffected. The Titles UI now requires both `codec.compatible` and `bitrate.compatible` before defaulting a track to copy mode, in both the initial auto-map and the existing-mapping compatibility check.
ffmpeg's only DTS encoder path (`-c:a dts`, reachable only behind `-strict -2` since it's marked experimental) is confirmed broken, not merely unsupported in theory: it produces a stream that decodes fine in isolation but corrupts once actually muxed through the `-f dvd` program-stream muxer. Real dvdauthor output on a DTS-mapped title showed hundreds of "audio sector out of range" warnings, and playback silently fell back to a different audio stream. DTS is DVD-legal and works fine as a stream copy of an already-DTS source; re-encoding *to* DTS is not something this toolchain can do reliably, so it's refused outright rather than shipped broken: - `build_ffmpeg_transcode_command` now returns `crate::Result<...>` and errors out on a DTS re-encode mapping, instead of emitting a command that produces a corrupt disc. - Validation flags a DTS re-encode mapping as an error at project-edit time too, so the problem surfaces before a build is even attempted. - The Titles UI disables DTS as a re-encode target in the output dropdown (offering it only for copy-mode tracks, with an explanatory tooltip), and falls back the output target to AC3 if a track already set to DTS is switched to re-encode mode.
…o to copy mode Picking a video source stream in the Titles editor defaulted it to copy mode unconditionally, even when its resolution or frame rate weren't actually DVD-legal — only the audio side had a compatibility gate. Since `CompatibilityDetail.video` always assesses whichever stream inspection treated as primary (in practice `videoStreams[0]`), picking any other stream also has no compatibility signal to check against and must default to re-encode rather than assume copy. Adds `isVideoCopyCompatible()`, checking codec, resolution, and frame rate together (any one mismatch means re-encode), and uses it both in the initial auto-map and when the user changes the video source stream.
Follow-up to the audio-stream-declaration-order fix, from Codex review: `output_target` only reflects what a *re-encoded* stream becomes — `build_ffmpeg_transcode_command` emits `-c:a copy` for a `CopyMode::Copy` mapping, ignoring `output_target` entirely and passing the source stream's own bytes through unchanged. The Titles UI lets `copyMode` and `outputTarget` be set independently, though, so a track can end up in copy mode with a stale `output_target` left over from before the user switched modes (e.g. was re-encoding AC3 -> MP2, then switched to copy). Declaring that stale target's format to dvdauthor would tell it a physical AC3 stream is MP2, which can make dvdauthor reject or mis-author the stream. `declared_audio_format` now resolves the real source codec (via the title's `source_asset_id` and the mapping's `source_stream_index`) for copy-mode mappings, falling back to `output_target` only when the source asset/stream can't be resolved.
…ility result Follow-up to the audio bitrate ceiling check, from Codex review: the new per-stream `bitrate` check landed in `build_compatibility_detail`, but `assess_dvd_compatibility` (which sets the asset's overall `RemuxCompatible`/`TransformCompatible`/`ReEncodeRequired` label) still only checked audio codec *name*. An otherwise DVD-compatible MPEG-2 asset with 640 kbps AC3 was still reported `RemuxCompatible` overall even though selecting it forces an audio re-encode — the Assets page badge told users the file was fine to remux when it wasn't. Extracts the ceiling table and the compatibility check into shared `audio_bitrate_ceiling_bps`/`audio_bitrate_is_dvd_legal` helpers, used by both `assess_dvd_compatibility`'s overall `has_dvd_audio` check and `build_compatibility_detail`'s per-stream `bitrate` check, so the two can no longer drift apart.
Per AGENTS.md's docs-sync convention, from Codex review: this PR's DTS re-encode refusal and audio bitrate ceiling check are both user-facing behaviour changes that weren't reflected in README.md or SPEC.md. - README: notes that compatibility explanations now cover per-codec DVD-Video audio bitrate ceilings, not just codec-name matching. - SPEC §12.2.4: documents that DTS output is confirmed unavailable as a re-encode target regardless of environment (not merely "not yet probed"), and that copy eligibility requires both codec name and bitrate to be DVD-legal. - SPEC §21.2: updates the DTS error example to the actual message the app now emits. - SPEC §26 open question 9: narrowed now that DTS re-encoding isn't offered at all — the remaining question is only about copy-mode DTS passthrough, which was never gated by the broken encoder.
7dca4a8 to
fcae297
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcae297f0f
ℹ️ 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".
| <option | ||
| value="DTS" | ||
| disabled={am.copyMode !== 'copy'} | ||
| title="ffmpeg has no reliable DTS encoder — DTS is only supported as a passthrough of an already-DTS source, not as a re-encode target." |
There was a problem hiding this comment.
When an already-DTS track is in copy mode, selecting a channel layout or bitrate still changes copyMode to re-encode without changing outputTarget. Disabling DTS only in this codec picker does not cover those implicit transitions, so the new backend refusal makes the resulting project fail validation and build. Apply the same DTS-to-AC3 fallback to those controls, or disable them while DTS passthrough is selected.
Useful? React with 👍 / 👎.
| if let Some((title, am)) = mapping { | ||
| let format = declared_audio_format(am, title, assets); | ||
| let lang = dvdauthor_language_code(&am.language); |
There was a problem hiding this comment.
When titles in one titleset assign different languages to the same audio slot—for example, English on title 1 and French on title 2—this titleset-level declaration takes only the first title's language and applies it to the slot for every title. The second title is therefore authored with incorrect language metadata despite the project model allowing per-title values; require a consistent language per slot or normalise the mappings before emitting the declaration.
Useful? React with 👍 / 👎.
…gaps (#138) ## Summary - Two follow-up fixes from a post-merge Codex review on #137, both real regressions introduced in that PR: - Titles that share a titleset but declare different audio codec or language at the same slot index now fail validation with a clear error, instead of the authored dvdauthor XML silently matching only the first title (DVD-Video's VTSI audio attribute table is shared across an entire titleset, not per-title). - Picking a channel layout or bitrate on a DTS copy-mode track now falls back `outputTarget` to AC3, matching the fallback the copy-mode selector already had — previously this combination flipped to re-encode+DTS, which fails validation and the build. ## Test plan - [x] `pnpm validate` (format:check, vitest, tsc build, cargo fmt/clippy -D warnings, cargo nextest) — all green - [x] New Rust tests: `validate_titles_flags_inconsistent_codec_across_titles_in_a_titleset`, `validate_titles_flags_inconsistent_language_across_titles_in_a_titleset`, `validate_titles_does_not_flag_consistent_titleset_audio` - [x] New JS tests: DTS→AC3 fallback via the bitrate control and the channel-layout control
## Summary - Prepare the v0.4.0 release: bump all release-facing manifest versions (root and desktop `package.json`, `tauri.conf.json`, both Cargo manifests, `Cargo.lock`) from 0.3.0 to 0.4.0. - Since 0.3.0, this brings in the full motion-menu authoring stack (per-format constraint profiles, semantic menu roles, colour-management rules, motion menus with animated button highlights, a timeline editor, and supporting docs — PRs #116–#128) and the audio/DVD-compatibility fixes (bitrate ceilings, DTS refusal, authored audio track order, titleset audio consistency validation — PRs #137–#138). ## Test plan - [x] `pnpm release:version:check` — all manifests synchronised at 0.4.0 - [x] `pnpm validate` (format:check, vitest, tsc build, cargo fmt/clippy -D warnings, cargo nextest) — all green
Summary
Four independent fixes to DVD-Video audio compatibility handling, each a self-contained commit:
bitratecompatibility check, consulted alongsidecodecbefore defaulting a track to copy mode.Test plan
pnpm validatepasses clean (format, JS tests, TS build,cargo fmt/clippy -D warnings,cargo nextest) — 230/230 Rust tests passing, including 12 new tests covering all four fixeslsdvdconfirms AC3 (declared default) lands at physical stream 0/0x80and stays there across rebuilds, matching authored order