fix(menus,titles): fall back to source image for menu backgrounds, fix audio track compatibility#74
Merged
ScottMorris merged 9 commits intoJun 24, 2026
Conversation
ScottMorris
added
frontend
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: e187a4e3ce
ℹ️ 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".
Comment on lines
+555
to
+556
| const compatible = audioCompatByStream.get(as_.index) ?? false; | ||
| const outputTarget = compatible ? (AUDIO_CODEC_TARGETS[as_.codec] ?? 'AC3') : 'AC3'; |
There was a problem hiding this comment.
When an asset was saved before compatibilityDetail existed (the model defaults the field to null on parse), audioCompatByStream is empty even though audioStreams still has codec data. In that context selecting the asset marks every stream incompatible, so even an existing AC3 track is remapped to AC3 re-encode instead of copy, regressing existing projects; please fall back to the codec table or re-inspect before defaulting to false.
Useful? React with 👍 / 👎.
Comment on lines
+1128
to
+1131
| mp2: 'MP2', | ||
| pcm_s16le: 'LPCM', | ||
| pcm_s16be: 'LPCM', | ||
| }; |
There was a problem hiding this comment.
For assets whose inspector reports codec value lpcm, the backend marks that value DVD-compatible, but this table has no lpcm entry. Because the compatible-codec fallback is AC3, those LPCM streams are created as copy-mode tracks with an AC3 output target/bitrate estimate, and toggling re-encode will silently transcode them to AC3 rather than preserving LPCM; add lpcm: 'LPCM' here.
Useful? React with 👍 / 👎.
ScottMorris
force-pushed
the
fix/menu-background-fallback-and-audio-track-compat
branch
from
e187a4e to
7237ad1
Compare
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ScottMorris
force-pushed
the
fix/menu-background-fallback-and-audio-track-compat
branch
from
b6f6953 to
fdb13b9
Compare
…x audio track compatibility Menu canvas backgrounds rendered nothing when no thumbnail cache existed yet or the cached thumbnail failed to load. `BackgroundImage` now falls back to the original asset file via the `asset://` protocol (`convertFileSrc`), with a placeholder shown only if that also fails. Requires enabling the asset protocol and `tauri-plugin-persisted-scope` so arbitrary asset source paths can be read through it. Audio track mapping in the Titles editor always defaulted every source stream's output target to AC3 regardless of actual codec compatibility. It now reads `asset.compatibilityDetail` to pick a 1:1 output target for already-compatible codecs (ac3/dts/mp2/pcm) and only forces a re-encode to AC3 for incompatible ones. Also adds a per-track remove button, an 8-track DVD limit warning, and a compact source-stream summary line. Closes #57, Closes #71
… output DVD-Video menus encode at a 720x480 (NTSC) raster but display anamorphically at 16:9 via a non-square sample-aspect-ratio (32:27). `build_ffmpeg_menu_command`'s background-image/video scaling used ffmpeg's `force_original_aspect_ratio=decrease`, which compares the source's aspect against the *raw pixel* box (720x480 = 1.5:1) instead of the *true display* aspect (16:9 ≈ 1.78:1). A genuinely 16:9 background image got needlessly letterboxed as a result — confirmed against a real rendered DVD via `ffprobe`/`cropdetect`, which showed ~62px bars baked into the encoded menu VOB despite the background being a 3840x2160 16:9 source. - Extracted the display-aspect-aware fit math already used correctly for title-video transcoding (`build_dvd_scale_filter`) into a shared `dvd_active_dimensions()` helper in `ffmpeg.rs`. - `menu.rs`'s background scaling (both still-image and motion-video paths) now computes `source_dar` from the background asset's probed `video_streams` and uses `dvd_active_dimensions` against the menu's true target display aspect, falling back to "no padding" when the asset has no probed dimensions. - Added two regression tests: a 16:9 background on a 16:9 menu now fills the frame with zero padding (the actual bug), and a 4:3 background on a 16:9 menu still pillarboxes correctly (proving the fix didn't just remove padding outright). Separately investigated a title-video letterboxing report from the same session: confirmed via a temporary diagnostic test that current `build_ffmpeg_transcode_command` already produces zero padding for a 16:9-source/16:9-target title (`scale=720:480,pad=720:480:0:0`), so the bars observed in that on-disk VOB were from a stale prior build, not a live bug — no code change needed there. Verified via: - `cargo test -p tauri-plugin-spindle-project` (202 passed, up from 199) - `cargo clippy -p tauri-plugin-spindle-project --all-targets` (clean) - `cargo fmt -p tauri-plugin-spindle-project -- --check` (clean) - Independent code review (no @codex review — Codex usage limits exhausted)
ScottMorris
force-pushed
the
fix/menu-background-fallback-and-audio-track-compat
branch
from
fdb13b9 to
6283de2
Compare
Adds an optional `bitrate_bps: Option<u32>` field to
`AudioTrackMapping` so each audio track in the Titles editor can have
its output bitrate overridden independently of codec defaults.
**Model / backend (`tauri-plugin-spindle-project`)**
- `models/title.rs`: new `bitrate_bps: Option<u32>` field with
`#[serde(default)]` for backward compat with existing project files.
- `build/capacity.rs`: `estimate_title_audio_bitrate_bps` uses the
override when present; LPCM always ignores it (its rate is derived
from channel count/sample depth, not requestable).
- `build/ffmpeg.rs`: AC3, MP2, and DTS arms pass the override as the
`-b:a` flag; LPCM gets no `-b:a` at all.
- All 5 `AudioTrackMapping` construction sites updated with
`bitrate_bps: None`.
**Frontend (`apps/spindle`)**
- `TitlesPage.tsx`: new bitrate `<select>` after the channel-layout
control — options from 192 kbps to 768 kbps plus "Auto (codec
default)". Selecting a bitrate forces `copyMode` to `'re-encode'`
(you can't change bitrate on a copied stream). Disabled for LPCM
with an explanatory tooltip. Switching output target to LPCM or
copy mode to `'copy'` clears the override.
- Default builder initialises `bitrateBps: null` on new mappings.
- 3 test-fixture construction sites updated with `bitrateBps: null`.
**Tests**
- `capacity.rs`: `reencode_audio_bitrate_override_replaces_the_codec_default`,
`lpcm_ignores_a_bitrate_override_since_its_rate_is_derived_not_requestable`
- `ffmpeg.rs`: `ffmpeg_uses_bitrate_override_when_set`,
`ffmpeg_falls_back_to_codec_default_bitrate_when_no_override_is_set`,
`ffmpeg_ignores_bitrate_override_for_lpcm`
- `titles.test.tsx`: 5-test `describe('audio track bitrate control')`
block covering defaults, select→re-encode promotion, copy-clears,
LPCM-disabled, and LPCM-switch-clears-override.
Verified end-to-end via Tauri MCP driver: bitrate select renders
with "Auto (codec default)" selected; choosing 448 kbps sets
`bitrateBps: 448000` and flips `copyMode` to `'re-encode'`; switching
to LPCM clears `bitrateBps` and disables the select.
Closes #71.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L3tSFK3P9vGwbNNtb4AYQG
Replace the single `flex-wrap` controls row (which split 4+1 at the panel width) with two explicit CSS grid rows: - Row 1 (`1fr 1fr`): output target + copy mode - Row 2 (`1fr 1fr 1fr`): channel layout + bitrate + language All cells are equal-width within their row, so the column positions are consistent across every track. With a fixed row count per track, the × remove button always lands at the same vertical position regardless of how many tracks are open. Also gives `titles__track-lang` proper select-matching styles (border, bg, padding) so it fits flush in the grid alongside the selects. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3tSFK3P9vGwbNNtb4AYQG
…y for incompatible streams Source bitrate from file metadata: - `audioSourceSummary` now includes the source stream's bitrate when known (`bitrateBps` from `AudioStreamInfo`), formatted as e.g. `448kbps`. Gives users a clear reference point when choosing an output bitrate override. Incompatible-stream guard: - New `isCopyCompatible` helper checks `asset.compatibilityDetail .audioStreams[].codec.compatible` for the mapped stream index. Defaults to `true` when no compatibility detail is available. - The "Copy" option in the copy-mode select is `disabled` when the source stream's codec is not DVD-compatible (e.g. AAC, Opus, FLAC), preventing users from accidentally stream-copying a format DVD players won't understand. A tooltip explains why. - Compatible streams (AC3, DTS, MP2, LPCM) are unaffected — both Copy and Re-encode remain available. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3tSFK3P9vGwbNNtb4AYQG
Replace the flat hardcoded bitrate list with a per-codec table (`AUDIO_BITRATE_OPTIONS`) so the select only shows rates that are actually valid for the chosen output target: - AC3: 192 / 256 / 320 / 384 / 448 / 640 kbps (DVD-Video max 640) - MP2: 192 / 256 / 320 / 384 kbps (MPEG-1 Layer II max 384) - DTS: 768 kbps / 1.5 Mbps — DVD-Video allows both standard (~768) and high-bitrate (~1536) DTS; 1.5 Mbps was previously missing - LPCM: no options (rate is derived from channel count × depth) Switching output target now also clears `bitrateBps` when the current override isn't in the new codec's valid list, preventing a stale e.g. 768 kbps value from silently carrying over to AC3. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3tSFK3P9vGwbNNtb4AYQG
Replace generic "Auto (source)" / "Auto (codec default)" with labels that surface the actual values at a glance: - Channel layout: "Auto Channels (5.1)" / "Auto Channels (stereo)" — shows the source channel count when asset metadata is known, or plain "Auto Channels" when no source is selected yet. - Bitrate: "Auto Bitrate (448 kbps)" for AC3, "Auto Bitrate (768 kbps)" for DTS, "Auto Bitrate (384 kbps)" for MP2 — driven by a new `CODEC_DEFAULT_BPS` map that mirrors the hard-coded defaults in `ffmpeg.rs`. LPCM shows plain "Auto Bitrate" since its rate is derived from channel count × sample depth, not a fixed default. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3tSFK3P9vGwbNNtb4AYQG
Contributor
Author
|
@codex review |
All four `titles__row-remove` buttons (titleset, title, audio track,
subtitle track) are opacity-0 until hover — invisible to sighted users
not hovering, but previously reachable by Tab. Tab-Tab-Enter from a
track label would silently delete the track.
`tabIndex={-1}` keeps them mouse-accessible while removing them from
the sequential focus order. Tab flow per audio track is now:
label → codec → copy mode → channels → bitrate → lang
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L3tSFK3P9vGwbNNtb4AYQG
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3tSFK3P9vGwbNNtb4AYQG
ScottMorris
deleted the
fix/menu-background-fallback-and-audio-track-compat
branch
2 tasks
ScottMorris
added a commit
that referenced
this pull request
Jun 24, 2026
## Summary `.titles__track-lang` was defined twice in `TitlesPage.css`: a dead full-width/`text-sm` block (overridden immediately by the block below it) followed by the intended 48px/mono/`text-xs` block. Consolidate into one block and move the `:focus` rule to follow it. No visual change — the second block was already winning the cascade. Caught during review of #74. ## Test plan - [x] `pnpm format:check` passes - [x] Visual spot-check: lang input still renders as a narrow mono code field
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.
Summary
Closes #57. Closes #71.
Test plan