ProRes: correct the controls, pin the chroma, add 4444 (#81) - #93
Merged
Merged
Conversation
ProRes is not `isLossless`, so the settings dialog rendered the quality slider for it, labelled "High (CRF 18)". The worker's `build_encoder_quality_args` takes the `prores_profile()` branch, emits `-profile:v N` and returns without ever reading `EncodingSettings.quality`. The slider moved; the output did not change. Replaced with an info panel naming the profile, reusing the shape the lossless codecs already get. Both now go through one `_buildFixedQualityNote` helper so the two cannot drift into looking like different kinds of message. The decision is a getter on the model, `VideoCodec.hasQualityControl`, rather than a condition inline in the widget. That is what makes it cheaply assertable across `VideoCodec.values`: the existing settings widget tests pump a single small widget, and reaching the Quality section would need a MainViewModel and its providers. A hand-written list of affected codecs would only ever cover the ones someone thought to add, which is never the broken one — that is exactly how this shipped. `qualityDescription` gains a matching arm. Note its only two callers are inside `_buildCrfQuality`, so for ProRes it is currently unreachable — it is here because the getter is public and would otherwise answer "High (CRF 18)" to any future caller, and the test pins it either way. Reported in issue #81, which asked for ProRes support without realising the codecs already ship; this is the first of the things that were actually wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Groundwork for ProRes 4444, which stores 4:4:4 and would otherwise be handed 4:2:2. Useful on its own: the software x264/x265 encoders both accept yuv444p10le, which matters for output going into compositing or keying where subsampled chroma shows on edges. Verified end to end before wiring any of it, because a format the Y4M pipe cannot name is a hard job failure rather than an error — the same shape as the Turn90 4:4:0 trap. vspipe emits the header `C444p10` and ffmpeg's demuxer accepts it; a full vspipe | ffmpeg | prores_ks run produces a genuine 4:4:4 file. Worth noting ffmpeg's Y4M *muxer* rejects yuv444p10le as "not an official yuv4mpegpipe pixel format" — and rejects the already-shipping yuv422p10le identically, which is the proof that the muxer's opinion is irrelevant here: vspipe is the muxer, not ffmpeg. Also fixes a latent bug this would otherwise have shipped. hardwareEncoderChromaWarning derived its chroma layout from a hand-written if/else that returned c422 for anything that was not one of the two 4:2:0 options, so selecting 4:4:4 would have produced "cannot encode 4:2:2 on most GPUs" — right advice, wrong reason, on screen. It now reads the layout from the option's own format name, which a format added later cannot get wrong. Three hand-maintained tables were driven from lists that had already gone stale — all three were missing Yuv420P10, so that option was unchecked in the serde-name test, in test_90's script substitutions, and in the Dart depth table. They now sweep the enum: `ChromaSubsampling::ALL` on the Rust side, kept complete by a catch-all-free match plus a count assertion, and `ChromaSubsampling.values` on the Dart side. A skipped row looks exactly like a passing one, which is why this went unnoticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eg guess ffmpeg's pixel-format negotiation never looks at `-profile:v`. Measured against the bundled build: `prores_ks -profile:v 4` auto-selects `yuv422p10le` from a yuv420p input, byte for byte the same choice it makes for `-profile:v 2`. So ProRes 4444 — added in the next commit — would have written a file stamped 4444 carrying 4:2:2, the profile's entire purpose discarded with no error anywhere. `forced_pix_fmt` now decides from the profile, joining the HuffYUV and AMF pins that already exist for the same reason: issue #74's lesson is that an encoder's declared format list is not a statement about what the output should be. The 4:2:2 profiles are pinned for symmetry rather than necessity, and that is measured rather than assumed — `yuv422p10le` is the only 4:2:2 format the encoder has, so it is what negotiation already picks. Encoding pal-sd-25.mov at all four existing profiles with and without the explicit flag gives identical framemd5 output. Nobody's ProRes file changes. Two consequences handled here rather than left for later: The job log claimed "X cannot encode Y", which was written for the hardware case and is false for ProRes — the encoder takes the source format fine, the profile just defines the layout, and a 4:2:0 source into 4444 is padded *up*, losing nothing. `pix_fmt_change_note` now says nothing when the pin costs nothing and names what the profile stores when it does. It is a standalone function because the arg-building mirror used by the unit tests does not log, so a message written inline there ships unverified. `prores_profile()` and `encoder_family()` lose their catch-all arms. They dispatched two halves of the same decision — `build_encoder_quality_args` branches on the first, its fallthrough on the second — so a ProRes variant reaching the family but not the profile table would emit no `-profile:v` at all and encode as profile 2 while claiming otherwise. That is now a compile error rather than a silent wrong file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two profiles the ProRes group was missing. They are only meaningful alongside the chroma pin from the previous commit — without it ffmpeg hands profile 4 the same yuv422p10le it hands profile 2, and the result is a file stamped 4444 carrying 4:2:2. Verified end to end through the worker binary: pinned it comes back yuv444p12le, unpinned yuv422p12le. (The decoder reports 4444 as 12-bit whatever 10-bit format the encoder was given, so tests assert the chroma part of the name, not the whole string.) The exhaustive matches added in the previous commit did their job: adding the variants produced four compile errors naming exactly the sites that mattered, including `prores_profile()`, which under its old catch-all would have silently returned None and emitted no `-profile:v` at all. `proresCodecs` in the settings dialog is now derived from `isProRes` rather than hand-listed. It was the only place the ProRes UI group was enumerated, so a profile missing from it existed in the model and was unreachable on screen with no error — a trap for the next person, not just for this change. Adds `proresChromaPinWarning`, a sibling of `hardwareEncoderChromaWarning` rather than an extension of it. The two make opposite claims: the existing one says the hardware cannot encode what you asked for and something is lost; this one says the profile defines what is stored, and 4444 pads 4:2:0 *up* — costing size, not detail. The message says so explicitly, because a warning that reads as a quality problem would push people off a profile doing exactly what they asked. Folding them into one function would blur both messages and drag ProRes into a test table pinned specifically to the NVENC/QSV arms of forced_pix_fmt. It is a second implementation of the worker's decision, which is the same hazard #74 documented, so it is pinned to the same cases from both sides and both files say so. Depth is deliberately not warned about on its own: ProRes is always 10-bit, so an 8-bit selection into any profile converts, and a banner that fires on the majority of ProRes jobs is wallpaper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… matrix The three options issue #81's linked guide is actually about, behind advanced mode and only for ProRes. Verified end to end through the worker: at profile 0 the same clip encodes to 30,469 bytes with them and 20,450 without, and `apl0` appears in the file only when asked for. The UI copy carries the measurements rather than the guide's framing, because the guide overstates them. At profile 3 its four recommended flags produce bit-identical frames — same framemd5, same byte count — since `quant_mat auto` already resolves to the HQ matrix and the bitrate is already under the 8000-bit cap. They earn their keep only on Proxy and LT (+3.6 dB for 2.8% more size, +5.5 dB for 19%), and the labels say exactly that. `-vendor apl0` is presented as a compatibility flag, which is what it is: four bytes per frame header, identical pixels. Defaults are off, so nothing anyone is already encoding changes. `bits_per_mb` is clamped in the worker, not only in the UI. prores_ks rejects anything above 8192 outright and the encode dies having written nothing, so a saved preset or an imported job config carrying a larger value would fail the whole job on an option the user cannot see — the same failure `normalized_preset` exists to prevent. Zero is the plugin's own "use the profile default", which is what omitting the option already does, so it emits nothing rather than a value that reads as a deliberate choice. `quant_mat` is an enum on both sides for the same reason: ffmpeg rejects an unknown name with "Undefined constant" and kills the encode. Validate, never forward — as with ColorMetadata::from_raw and parse_ratio. The two nullable fields get explicit clear flags on copyWith. `x ?? this.x` can only ever set a nullable field, never clear it, so without them unticking an override in the UI would leave the value in place and silently apply it to every later ProRes encode. That matters more here than in the pass models: every edit in the settings dialog goes through `updateEncodingSettings(settings.copyWith(...))`, so a forgotten field is reset on the user's very next click. parameter_copy_with_test now scans encoding_settings.dart, which it never did — it globs *_parameters.dart, and this model has the same failure mode with a wider blast radius. Confirmed the guard is live by dropping a field and watching it fail by name. It retro-covers videoBitrateKbps, which had already been added without a clear flag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per the standing rule that README.md and CLAUDE.md track behaviour changes. Two tables were already stale in both files before this work touched them: `yuv420p10` was missing from CLAUDE.md's Output Colour Format table and from the README's colour-format row, having shipped without either being updated. Fixed alongside the new 4:4:4 entry. The CLAUDE.md section leads with the finding that cost the most to establish — ffmpeg's format negotiation never reads `-profile:v`, so profiles 4 and 5 select the same 4:2:2 as profile 2 — because it is not discoverable from documentation and produces a valid, playable, wrong file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only tests read the constant, so a release build of the binary flagged it and added a third line to the crate's standing warning noise. `#[cfg(test)]` is not the fix: the integration tests in worker/tests/ are a separate crate that links this library normally, so the constant has to exist in an ordinary build. `impl VideoCodec` carries `#[allow(dead_code)]` across its whole block for exactly this reason, which is why VideoCodec::ALL never warned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
StuartCameronCode
added a commit
that referenced
this pull request
Sep 12, 2026
Verified green on all four platforms on both gates before merge: the push gate dispatched with deps_run_id (the PR's own check cannot pass while deps-version.json names a draft tag), and the nightly heavy suite. The two tests that prove the central claim pass on every platform: ProRes 4444 stores yuv444p12le - genuine 4:4:4 ProRes 422 HQ held 4:2:2 - negative control
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 the ProRes half of #81.
The request was "please add ProRes output", and Proxy/LT/422/HQ had shipped months ago — the requester could not tell. Most of what was wrong is that the app said otherwise, so this is mostly a correctness change with two profiles added on the end.
What measuring changed
Two findings reshaped the design, and neither is discoverable from the linked guide:
ffmpeg's pixel-format negotiation never reads
-profile:v. Measured against the bundled build:prores_ks -profile:v 4and-profile:v 5auto-selectyuv422p10lefrom ayuv420pinput, exactly as-profile:v 2does. So ProRes 4444 shipped without a pin writes a file stamped 4444 carrying 4:2:2 — valid, playable, and the profile's entire point discarded with no error anywhere.Verified end to end through the worker binary:
pix_fmtyuv422p12le— 4:2:2, the bugyuv444p12le— correctAt profile 3 the guide's four recommended flags produce bit-identical frames — same
framemd5, same byte count.quant_mat autoalready resolves to the HQ matrix and the bitrate is already under the 8000-bit cap. They earn their keep only on Proxy/LT (+3.6 dB for 2.8% size, +5.5 dB for 19%), and the UI copy says that rather than repeating the guide's framing.-vendor apl0is a compatibility flag: four bytes per frame header, identical pixels.Commits
isLossless, so the dialog rendered a CRF slider labelled "High (CRF 18)" while the worker took theprores_profile()branch and never readsettings.quality. Now an info panel, gated on a newVideoCodec.hasQualityControlgetter so it can be asserted acrossVideoCodec.values.C444p10and the demuxer accepts it. Also fixes a latent bug —hardwareEncoderChromaWarningderived layout from a hand-written if/else returningc422for anything non-4:2:0, so a 4:4:4 selection would have read "cannot encode 4:2:2 on most GPUs" on screen.framemd5at all four, with and without the flag.proresChromaPinWarning.Things found on the way
Three hand-written lists had already gone stale without failing —
yuv420p10was missing from CLAUDE.md's table, the README's row, and three test tables, so that option was checked nowhere. Both sides now sweep the enum.prores_profile()andencoder_family()lost their catch-all arms: they dispatch two halves of one decision, so a ProRes variant reaching the family but not the profile table would emit no-profile:vand encode as profile 2 while claiming otherwise. Adding the new variants then produced four compile errors naming exactly the right sites.parameter_copy_with_testglobs*_parameters.dartand had never seenencoding_settings.dart— where a forgottencopyWithfield resets on the user's next click, since every settings edit goes throughcopyWith. Now covered, and confirmed live by dropping a field and watching it fail by name. It retro-coversvideoBitrateKbps, which has the same defect.Verification
Rust 585 pass, Dart push gate 852 (was 832). Locally 3 tests fail for pre-existing environment reasons, confirmed against a clean tree: two need a complete
deps/, one needs the whisper add-on.Heavy tests added to
integration_chroma_subsampling_test.dart(ProRes 4444 stores real 4:4:4, plus a 4:2:2 negative control) run in nightly, dispatched separately against this branch.🤖 Generated with Claude Code