From dc8a81ac80cde7030bc985e0e380a37581da1855 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 12 Sep 2026 02:51:01 +1000 Subject: [PATCH 1/7] fix(ui): stop showing ProRes a CRF slider the worker ignores (#81) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- app/lib/models/encoding_settings.dart | 8 ++ app/lib/models/video_job.dart | 15 +++ app/lib/views/settings/settings_dialog.dart | 70 ++++++++----- app/test/codec_quality_control_test.dart | 103 ++++++++++++++++++++ 4 files changed, 169 insertions(+), 27 deletions(-) create mode 100644 app/test/codec_quality_control_test.dart diff --git a/app/lib/models/encoding_settings.dart b/app/lib/models/encoding_settings.dart index a151786..3812b8f 100644 --- a/app/lib/models/encoding_settings.dart +++ b/app/lib/models/encoding_settings.dart @@ -203,6 +203,14 @@ class EncodingSettings { /// Human-readable quality description. String get qualityDescription { + // Codecs whose quality the worker never reads must not be described in + // terms of a number that does nothing. ProRes is set by its profile; + // lossless has no quality at all. + if (!codec.hasQualityControl) { + return codec.isProRes + ? 'Fixed by the ${codec.displayName} profile' + : 'Lossless'; + } if (codec == VideoCodec.h264Videotoolbox || codec == VideoCodec.h265Videotoolbox) { // VideoToolbox: CRF is remapped to q:v (inverted scale) in the worker. // Show quality in user-friendly terms based on the CRF value. diff --git a/app/lib/models/video_job.dart b/app/lib/models/video_job.dart index 1a24813..56971ca 100644 --- a/app/lib/models/video_job.dart +++ b/app/lib/models/video_job.dart @@ -258,6 +258,21 @@ enum VideoCodec { bool get isLossless => this == VideoCodec.ffv1 || this == VideoCodec.huffyuv || this == VideoCodec.ffvhuff; + /// Whether a quality/bitrate control means anything for this codec. + /// + /// The worker decides quality per encoder family in + /// `build_encoder_quality_args`, and two families read nothing from + /// `EncodingSettings.quality`: the lossless codecs (there is no quality to + /// set) and ProRes (the profile fixes it — the ProRes branch emits + /// `-profile:v N` and returns). Showing a CRF slider for either is a control + /// that responds and changes nothing, which is worse than showing none: + /// ProRes used to present one labelled "High (CRF 18)". + /// + /// This lives on the model rather than inline in the settings dialog so it + /// can be asserted across `VideoCodec.values` — a codec added later cannot + /// quietly acquire a slider the worker ignores. + bool get hasQualityControl => !isLossless && !isProRes; + /// Whether this codec produces H.264 output (software or hardware). bool get isH264 => this == h264 || this == h264Nvenc || this == h264Qsv || this == h264Videotoolbox || this == h264Amf; diff --git a/app/lib/views/settings/settings_dialog.dart b/app/lib/views/settings/settings_dialog.dart index d5ef65d..0d1456c 100644 --- a/app/lib/views/settings/settings_dialog.dart +++ b/app/lib/views/settings/settings_dialog.dart @@ -810,10 +810,12 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { if (settings.codec.availablePresets != null) const SizedBox(height: 24), - // Quality (not applicable for lossless codecs). Intel VideoToolbox has - // no constant-quality mode, so it gets a native target-bitrate control + // Quality. Three cases, because two codec families read nothing + // from `quality` (see VideoCodec.hasQualityControl) and must not be + // shown a slider that does nothing. Intel VideoToolbox has no + // constant-quality mode, so it gets a native target-bitrate control // instead of the CRF slider. - if (!settings.codec.isLossless) + if (settings.codec.hasQualityControl) _buildSection( context, title: 'Quality', @@ -822,33 +824,18 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { : _buildCrfQuality(context, viewModel, settings), ), - // Note for lossless codec - if (settings.codec.isLossless) + // Note for codecs whose quality is not ours to set. + if (!settings.codec.hasQualityControl) _buildSection( context, title: 'Quality', - child: Container( - padding: const EdgeInsets.all(12), - decoration: BoxDecoration( - color: Theme.of(context).colorScheme.surfaceContainerHighest, - borderRadius: BorderRadius.circular(8), - ), - child: Row( - children: [ - Icon( - Icons.info_outline, - size: 20, - color: Theme.of(context).colorScheme.primary, - ), - const SizedBox(width: 12), - Expanded( - child: Text( - 'This is a lossless codec. No quality setting is needed.', - style: Theme.of(context).textTheme.bodyMedium, - ), - ), - ], - ), + child: _buildFixedQualityNote( + context, + settings.codec.isProRes + ? '${settings.codec.displayName} encodes at a fixed quality ' + 'set by the profile. There is no quality setting — pick a ' + 'different profile for a different data rate.' + : 'This is a lossless codec. No quality setting is needed.', ), ), @@ -1240,6 +1227,35 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { ); } + /// The panel shown in place of the quality slider for codecs whose quality + /// the worker does not set — lossless and ProRes. One helper for both so the + /// two cannot drift into looking like different kinds of message. + Widget _buildFixedQualityNote(BuildContext context, String message) { + return Container( + padding: const EdgeInsets.all(12), + decoration: BoxDecoration( + color: Theme.of(context).colorScheme.surfaceContainerHighest, + borderRadius: BorderRadius.circular(8), + ), + child: Row( + children: [ + Icon( + Icons.info_outline, + size: 20, + color: Theme.of(context).colorScheme.primary, + ), + const SizedBox(width: 12), + Expanded( + child: Text( + message, + style: Theme.of(context).textTheme.bodyMedium, + ), + ), + ], + ), + ); + } + /// The standard CRF/quality slider (software, NVENC, QSV, AMF, Apple-Silicon VT). Widget _buildCrfQuality( BuildContext context, MainViewModel viewModel, EncodingSettings settings) { diff --git a/app/test/codec_quality_control_test.dart b/app/test/codec_quality_control_test.dart new file mode 100644 index 0000000..40f2d66 --- /dev/null +++ b/app/test/codec_quality_control_test.dart @@ -0,0 +1,103 @@ +// Guards the pairing between `VideoCodec.hasQualityControl` and what the worker +// actually reads (issue #81). +// +// The bug being pinned: ProRes is not `isLossless`, so the settings dialog +// rendered the CRF slider for it, labelled "High (CRF 18)". The worker's +// `build_encoder_quality_args` takes the `prores_profile()` branch and returns +// without ever reading `EncodingSettings.quality` — so the slider moved and the +// output did not change. +// +// These tests are driven by `VideoCodec.values` rather than a hand-written +// list, because a hand-written list only covers the codecs someone thought to +// add, which is never the broken one. That is exactly how this shipped. +// +// Run with: flutter test test/codec_quality_control_test.dart + +import 'package:flutter_test/flutter_test.dart'; +import 'package:vapourbox/models/encoding_settings.dart'; +import 'package:vapourbox/models/video_job.dart'; + +void main() { + group('hasQualityControl', () { + test('is false for every ProRes profile', () { + final prores = VideoCodec.values.where((c) => c.isProRes).toList(); + expect(prores, isNotEmpty, reason: 'sanity: ProRes codecs exist'); + + for (final codec in prores) { + expect(codec.hasQualityControl, isFalse, + reason: '${codec.displayName} takes its quality from -profile:v; ' + 'the worker never reads EncodingSettings.quality for it'); + } + }); + + test('is false for every lossless codec', () { + final lossless = VideoCodec.values.where((c) => c.isLossless).toList(); + expect(lossless, isNotEmpty, reason: 'sanity: lossless codecs exist'); + + for (final codec in lossless) { + expect(codec.hasQualityControl, isFalse, + reason: '${codec.displayName} has no quality to set'); + } + }); + + test('is true for every other codec', () { + final tunable = VideoCodec.values + .where((c) => !c.isProRes && !c.isLossless) + .toList(); + expect(tunable, isNotEmpty, reason: 'sanity: tunable codecs exist'); + + for (final codec in tunable) { + expect(codec.hasQualityControl, isTrue, + reason: '${codec.displayName} is CRF/CQ/QP-controlled, so hiding ' + 'its quality control would remove a working setting'); + } + }); + + test('every codec is decided one way or the other', () { + // A codec that is somehow both ProRes and lossless, or whose getter + // disagrees with the two predicates, would render two Quality sections + // or none. + for (final codec in VideoCodec.values) { + expect(codec.isProRes && codec.isLossless, isFalse, + reason: '${codec.displayName} cannot be both'); + expect(codec.hasQualityControl, !(codec.isProRes || codec.isLossless), + reason: '${codec.displayName}: getter disagrees with its parts'); + } + }); + }); + + group('qualityDescription', () { + test('never quotes a CRF number for a codec that ignores it', () { + for (final codec in VideoCodec.values.where((c) => !c.hasQualityControl)) { + // Deliberately a value that would be conspicuous if it leaked through. + final settings = EncodingSettings(codec: codec, quality: 37); + final description = settings.qualityDescription; + + expect(description, isNot(contains('37')), + reason: '${codec.displayName} ignores quality, so naming the ' + 'number tells the user it matters'); + expect(description.toUpperCase(), isNot(contains('CRF')), + reason: '${codec.displayName} is not CRF-controlled'); + expect(description.toUpperCase(), isNot(contains('CQ ')), + reason: '${codec.displayName} is not CQ-controlled'); + } + }); + + test('names the profile for ProRes', () { + for (final codec in VideoCodec.values.where((c) => c.isProRes)) { + final settings = EncodingSettings(codec: codec); + expect(settings.qualityDescription, contains(codec.displayName), + reason: 'the user needs to know which profile fixed the quality'); + } + }); + + test('still describes the number for codecs that use it', () { + for (final codec in VideoCodec.values.where((c) => c.hasQualityControl)) { + final settings = EncodingSettings(codec: codec, quality: 37); + expect(settings.qualityDescription, contains('37'), + reason: '${codec.displayName} reads quality, so the slider label ' + 'must keep reporting it'); + } + }); + }); +} From b0135a54ed5984fa2431887a327b1618a6ef6571 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 12 Sep 2026 02:56:44 +1000 Subject: [PATCH 2/7] feat(output): add a 4:4:4 10-bit output colour format (#81) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- app/lib/models/encoding_settings.dart | 6 +- app/lib/utils/pixel_format.dart | 11 ++- app/lib/views/settings/settings_dialog.dart | 8 +- app/test/pixel_format_test.dart | 43 +++++++++-- worker/src/models/video_job.rs | 81 +++++++++++++++++++-- worker/tests/filter_integration_test.rs | 30 ++++++-- 6 files changed, 154 insertions(+), 25 deletions(-) diff --git a/app/lib/models/encoding_settings.dart b/app/lib/models/encoding_settings.dart index 3812b8f..fe411ba 100644 --- a/app/lib/models/encoding_settings.dart +++ b/app/lib/models/encoding_settings.dart @@ -74,7 +74,11 @@ enum ChromaSubsampling { yuv422('yuv422', '4:2:2 8-bit', 'more colour detail', 8), /// Convert to 10-bit YUV422: keeps a 10-bit source's precision while /// normalizing chroma, and gives an 8-bit source headroom for gradients. - yuv422p10('yuv422p10', '4:2:2 10-bit', 'keeps 10-bit precision', 10); + yuv422p10('yuv422p10', '4:2:2 10-bit', 'keeps 10-bit precision', 10), + /// Convert to 10-bit YUV444: full chroma resolution, no subsampling at all. + /// What ProRes 4444 stores, and what the software x264/x265 encoders can + /// take. No GPU encoder in this app can (see [hardwareEncoderChromaWarning]). + yuv444p10('yuv444p10', '4:4:4 10-bit', 'full chroma, no GPU encoders', 10); const ChromaSubsampling( this.value, this.label, this.blurb, this.outputBitDepth); diff --git a/app/lib/utils/pixel_format.dart b/app/lib/utils/pixel_format.dart index ea9a2ad..667a22f 100644 --- a/app/lib/utils/pixel_format.dart +++ b/app/lib/utils/pixel_format.dart @@ -199,10 +199,13 @@ String? hardwareEncoderChromaWarning({ depth = pixelFormatBitDepth(pixelFormat); describedAs = 'Your source is $pixelFormat, and "Match source" keeps it'; default: - layout = chromaSubsampling == ChromaSubsampling.yuv420 || - chromaSubsampling == ChromaSubsampling.yuv420p10 - ? ChromaLayout.c420 - : ChromaLayout.c422; + // Derived from the option's own format name rather than enumerated. + // The hand-written test this replaces returned c422 for anything that + // was not one of the two 4:2:0 variants, so the 4:4:4 option would have + // reported "cannot encode 4:2:2 on most GPUs" — the right advice under + // the wrong reason. `value` is exactly the shape the parser expects + // (`yuv444p10` -> c444), so a format added later cannot repeat this. + layout = pixelFormatChromaLayout(chromaSubsampling.value); depth = chromaSubsampling.outputBitDepth ?? 8; describedAs = '${chromaSubsampling.label} is selected'; } diff --git a/app/lib/views/settings/settings_dialog.dart b/app/lib/views/settings/settings_dialog.dart index 0d1456c..9cd7d10 100644 --- a/app/lib/views/settings/settings_dialog.dart +++ b/app/lib/views/settings/settings_dialog.dart @@ -63,7 +63,13 @@ const List<(String, String)> chromaFormatHelpSections = [ 'analogue-captured source, at 8-bit precision.\n\n' '4:2:2 10-bit — keeps the colour detail and the 10-bit grading. Best ' 'when the file is going on for more work; needs a player that handles ' - '10-bit.', + '10-bit.\n\n' + '4:4:4 10-bit — no chroma subsampling at all. ProRes 4444 and the ' + 'software H.264/H.265 encoders can store it; no GPU encoder here can. ' + 'Reach for it when the output is going into compositing or keying, ' + 'where subsampled chroma shows up on edges. It will not recover colour ' + 'a subsampled source never had — on an ordinary capture it just makes ' + 'a larger file.', ), ]; diff --git a/app/test/pixel_format_test.dart b/app/test/pixel_format_test.dart index e3f21e4..d7c0552 100644 --- a/app/test/pixel_format_test.dart +++ b/app/test/pixel_format_test.dart @@ -155,12 +155,20 @@ void main() { group('ChromaSubsampling output depth', () { test('every option declares the depth it converts to', () { // The warning above is driven entirely by this field, so a new option - // that forgets it would silently stop warning. - expect(ChromaSubsampling.original.outputBitDepth, isNull); - expect(ChromaSubsampling.yuv420.outputBitDepth, 8); - expect(ChromaSubsampling.yuv420p10.outputBitDepth, 10); - expect(ChromaSubsampling.yuv422.outputBitDepth, 8); - expect(ChromaSubsampling.yuv422p10.outputBitDepth, 10); + // that forgets it would silently stop warning. Driven by `values` rather + // than a hand-written list, which is how `yuv420p10` sat unchecked here. + for (final format in ChromaSubsampling.values) { + if (format == ChromaSubsampling.original) { + expect(format.outputBitDepth, isNull, + reason: '"Match source" converts nothing, so it has no depth'); + continue; + } + expect(format.outputBitDepth, isNotNull, + reason: '${format.value} must declare its depth or it stops ' + 'warning about reducing a deeper source'); + expect(format.label, contains('${format.outputBitDepth}-bit'), + reason: '${format.value}: the label and the field must agree'); + } }); test('values match the worker enum serde names', () { @@ -172,6 +180,29 @@ void main() { expect(ChromaSubsampling.yuv420p10.value, 'yuv420p10'); expect(ChromaSubsampling.yuv422.value, 'yuv422'); expect(ChromaSubsampling.yuv422p10.value, 'yuv422p10'); + expect(ChromaSubsampling.yuv444p10.value, 'yuv444p10'); + // The count is the half that catches an option added here but never + // mirrored into the Rust enum — the assertions above only cover the + // names someone thought to write down. + expect(ChromaSubsampling.values.length, 6, + reason: 'a new option must also exist in ChromaSubsampling in ' + 'worker/src/models/video_job.rs, with a matching serde name'); + }); + + test('the chroma layout of every option is read from its own name', () { + // Pins the fix to hardwareEncoderChromaWarning's `default:` arm, which + // used to return c422 for anything that was not 4:2:0 — so the 4:4:4 + // option would have been described as 4:2:2 on screen. + expect(pixelFormatChromaLayout(ChromaSubsampling.yuv420.value), + ChromaLayout.c420); + expect(pixelFormatChromaLayout(ChromaSubsampling.yuv420p10.value), + ChromaLayout.c420); + expect(pixelFormatChromaLayout(ChromaSubsampling.yuv422.value), + ChromaLayout.c422); + expect(pixelFormatChromaLayout(ChromaSubsampling.yuv422p10.value), + ChromaLayout.c422); + expect(pixelFormatChromaLayout(ChromaSubsampling.yuv444p10.value), + ChromaLayout.c444); }); }); } diff --git a/worker/src/models/video_job.rs b/worker/src/models/video_job.rs index 036c97a..d1265d2 100644 --- a/worker/src/models/video_job.rs +++ b/worker/src/models/video_job.rs @@ -445,9 +445,35 @@ pub enum ChromaSubsampling { /// Convert to 10-bit YUV422 — keeps a 10-bit source's precision while /// normalizing chroma, and gives an 8-bit source headroom for gradients. Yuv422P10, + /// Convert to 10-bit YUV444 — full chroma resolution. Needed by ProRes 4444 + /// and 4444 XQ, which store 4:4:4; no hardware encoder in this app takes it. + /// + /// Verified end to end before shipping, because a format the Y4M pipe + /// cannot name is a hard job failure rather than an error (the `Turn90` + /// 4:4:0 trap): vspipe emits the header `C444p10` and ffmpeg's demuxer + /// accepts it. Note ffmpeg's Y4M *muxer* calls both this and the + /// already-shipping `yuv422p10le` "not an official yuv4mpegpipe pixel + /// format" — irrelevant here, because vspipe is the muxer, not ffmpeg. + Yuv444P10, } impl ChromaSubsampling { + /// Every variant, so tests can sweep them instead of hand-listing. + /// + /// The hand-written list this replaces had gone stale without failing — + /// `Yuv420P10` was missing from it, so that variant was never checked at + /// all. `every_chroma_subsampling_is_listed` keeps this honest from both + /// ends: a match with no catch-all makes a new variant a compile error, and + /// the length assertion then fails until it is added here too. + pub const ALL: &'static [ChromaSubsampling] = &[ + ChromaSubsampling::Original, + ChromaSubsampling::Yuv420, + ChromaSubsampling::Yuv420P10, + ChromaSubsampling::Yuv422, + ChromaSubsampling::Yuv422P10, + ChromaSubsampling::Yuv444P10, + ]; + /// The VapourSynth format constant the pipeline converts to, or `None` for /// `Original` (no conversion at all). pub fn vapoursynth_format(&self) -> Option<&'static str> { @@ -457,6 +483,7 @@ impl ChromaSubsampling { ChromaSubsampling::Yuv420P10 => Some("vs.YUV420P10"), ChromaSubsampling::Yuv422 => Some("vs.YUV422P8"), ChromaSubsampling::Yuv422P10 => Some("vs.YUV422P10"), + ChromaSubsampling::Yuv444P10 => Some("vs.YUV444P10"), } } @@ -475,6 +502,7 @@ impl ChromaSubsampling { ChromaSubsampling::Yuv420P10 => Some("yuv420p10le"), ChromaSubsampling::Yuv422 => Some("yuv422p"), ChromaSubsampling::Yuv422P10 => Some("yuv422p10le"), + ChromaSubsampling::Yuv444P10 => Some("yuv444p10le"), } } } @@ -1051,13 +1079,7 @@ mod tests { /// decides whether a hardware encoder can take it. #[test] fn chroma_subsampling_names_agree() { - for cs in [ - ChromaSubsampling::Original, - ChromaSubsampling::Yuv420, - ChromaSubsampling::Yuv420P10, - ChromaSubsampling::Yuv422, - ChromaSubsampling::Yuv422P10, - ] { + for cs in ChromaSubsampling::ALL { assert_eq!( cs.vapoursynth_format().is_some(), cs.ffmpeg_pix_fmt().is_some(), @@ -1066,6 +1088,51 @@ mod tests { } } + /// Keeps `ChromaSubsampling::ALL` complete, so every table driven from it + /// really does cover the enum. The previous hand-written list had silently + /// lost `Yuv420P10`, which is the failure mode this closes: a skipped row + /// looks exactly like a passing one. + #[test] + fn every_chroma_subsampling_is_listed() { + // No catch-all arm: a new variant stops compiling here until it is + // handled, at which point the length assertion below demands it be + // added to ALL as well. + for cs in ChromaSubsampling::ALL { + match cs { + ChromaSubsampling::Original + | ChromaSubsampling::Yuv420 + | ChromaSubsampling::Yuv420P10 + | ChromaSubsampling::Yuv422 + | ChromaSubsampling::Yuv422P10 + | ChromaSubsampling::Yuv444P10 => {} + } + } + assert_eq!( + ChromaSubsampling::ALL.len(), + 6, + "a ChromaSubsampling variant was added without listing it in ALL" + ); + } + + /// The 4:4:4 option exists for ProRes 4444, and both of its names have to + /// be right or the conversion silently does nothing useful. + #[test] + fn yuv444p10_declares_both_names() { + assert_eq!( + ChromaSubsampling::Yuv444P10.vapoursynth_format(), + Some("vs.YUV444P10") + ); + assert_eq!( + ChromaSubsampling::Yuv444P10.ffmpeg_pix_fmt(), + Some("yuv444p10le") + ); + // The wire name the Dart enum's `value` string must match. + assert_eq!( + serde_json::to_string(&ChromaSubsampling::Yuv444P10).unwrap(), + "\"yuv444p10\"" + ); + } + #[test] fn test_container_format_serialization() { assert_eq!( diff --git a/worker/tests/filter_integration_test.rs b/worker/tests/filter_integration_test.rs index 0bb0ace..cec5d92 100644 --- a/worker/tests/filter_integration_test.rs +++ b/worker/tests/filter_integration_test.rs @@ -3343,12 +3343,13 @@ fn test_90_output_colour_format_reaches_the_preview_too() { ..ProcessingPipeline::default() }); - // Every convertible format, in both scripts. - for (subsampling, expected) in [ - (ChromaSubsampling::Yuv420, "vs.YUV420P8"), - (ChromaSubsampling::Yuv422, "vs.YUV422P8"), - (ChromaSubsampling::Yuv422P10, "vs.YUV422P10"), - ] { + // Every convertible format, in both scripts. Driven by ALL rather than a + // hand-written list: this table had silently lost Yuv420P10, so that + // option's substitution was never checked in either script. + for subsampling in ChromaSubsampling::ALL.iter().copied() { + let Some(expected) = subsampling.vapoursynth_format() else { + continue; // Original converts nothing; asserted separately below. + }; job.encoding_settings.chroma_subsampling = subsampling; let (encode, preview) = generate_both_scripts(&job); for (label, script) in [("encode", &encode), ("preview", &preview)] { @@ -3385,8 +3386,10 @@ fn test_91_chroma_subsampling_serde_names_match_the_app() { for (subsampling, expected) in [ (ChromaSubsampling::Original, "\"original\""), (ChromaSubsampling::Yuv420, "\"yuv420\""), + (ChromaSubsampling::Yuv420P10, "\"yuv420p10\""), (ChromaSubsampling::Yuv422, "\"yuv422\""), (ChromaSubsampling::Yuv422P10, "\"yuv422p10\""), + (ChromaSubsampling::Yuv444P10, "\"yuv444p10\""), ] { let json = serde_json::to_string(&subsampling).expect("serialize"); assert_eq!(json, expected, "{:?} serializes wrong", subsampling); @@ -3396,11 +3399,26 @@ fn test_91_chroma_subsampling_serde_names_match_the_app() { } // Only Original means "no conversion"; everything else names a format. + // Swept over ALL rather than spot-checked, so an option that gains a wire + // name but no format is caught here rather than at job time. assert_eq!(ChromaSubsampling::Original.vapoursynth_format(), None); + for cs in ChromaSubsampling::ALL { + if *cs == ChromaSubsampling::Original { + continue; + } + assert!( + cs.vapoursynth_format().is_some(), + "{cs:?} names no VapourSynth format, so selecting it converts nothing" + ); + } assert_eq!( ChromaSubsampling::Yuv422P10.vapoursynth_format(), Some("vs.YUV422P10") ); + assert_eq!( + ChromaSubsampling::Yuv444P10.vapoursynth_format(), + Some("vs.YUV444P10") + ); } /// Test 92: neither template may name an nnedi3 implementation directly. From c0dd9745c68acc6e8b0079902e11185765cd28de Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 12 Sep 2026 02:59:36 +1000 Subject: [PATCH 3/7] fix(encode): pin ProRes chroma to its profile instead of letting ffmpeg guess MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- worker/src/models/video_job.rs | 168 +++++++++++++++++++++++++++++++- worker/src/pipeline_executor.rs | 142 ++++++++++++++++++++++++--- worker/src/pixel_format.rs | 16 ++- 3 files changed, 307 insertions(+), 19 deletions(-) diff --git a/worker/src/models/video_job.rs b/worker/src/models/video_job.rs index d1265d2..70ce570 100644 --- a/worker/src/models/video_job.rs +++ b/worker/src/models/video_job.rs @@ -624,14 +624,55 @@ impl VideoCodec { } } + /// Every variant, so tests can sweep them instead of hand-listing. + /// Kept complete by `every_video_codec_is_listed`. + pub const ALL: &'static [VideoCodec] = &[ + VideoCodec::H264, + VideoCodec::H265, + VideoCodec::H264Nvenc, + VideoCodec::H265Nvenc, + VideoCodec::H264Qsv, + VideoCodec::H265Qsv, + VideoCodec::H264Videotoolbox, + VideoCodec::H265Videotoolbox, + VideoCodec::H264Amf, + VideoCodec::H265Amf, + VideoCodec::FFV1, + VideoCodec::Huffyuv, + VideoCodec::Ffvhuff, + VideoCodec::ProResProxy, + VideoCodec::ProResLT, + VideoCodec::ProRes422, + VideoCodec::ProResHQ, + ]; + /// Get the ProRes profile value, if applicable. + /// + /// Deliberately exhaustive — no catch-all arm. `build_encoder_quality_args` + /// dispatches on this while its fallthrough dispatches on + /// `encoder_family()`, so a new ProRes variant that reached the family but + /// not this table would emit no `-profile:v` at all and `prores_ks` would + /// silently default to profile 2. A compile error is the right way to find + /// that out. pub fn prores_profile(&self) -> Option { match self { VideoCodec::ProResProxy => Some(0), VideoCodec::ProResLT => Some(1), VideoCodec::ProRes422 => Some(2), VideoCodec::ProResHQ => Some(3), - _ => None, + VideoCodec::H264 + | VideoCodec::H265 + | VideoCodec::H264Nvenc + | VideoCodec::H265Nvenc + | VideoCodec::H264Qsv + | VideoCodec::H265Qsv + | VideoCodec::H264Videotoolbox + | VideoCodec::H265Videotoolbox + | VideoCodec::H264Amf + | VideoCodec::H265Amf + | VideoCodec::FFV1 + | VideoCodec::Huffyuv + | VideoCodec::Ffvhuff => None, } } @@ -674,6 +715,28 @@ impl VideoCodec { /// last, so a later `-pix_fmt` overrides this one. That is the escape hatch /// for someone whose card really does have the mode we refuse to assume. pub fn forced_pix_fmt(&self, encoder_input: &str) -> Option<&'static str> { + // ProRes: the profile decides the chroma, and ffmpeg's negotiation does + // not know that. Measured against the bundled build — `prores_ks` with + // `-profile:v 4` auto-selects `yuv422p10le` from a yuv420p input, + // exactly as `-profile:v 2` does, because format negotiation never + // looks at the profile. Left alone, ProRes 4444 therefore writes a file + // stamped 4444 that carries 4:2:2, which is the profile's whole point + // discarded silently. + // + // The 4:2:2 profiles are pinned for symmetry rather than necessity: + // `yuv422p10le` is the only 4:2:2 format the encoder has, so it is what + // negotiation already picks. Verified on pal-sd-25.mov at all four + // profiles — identical `framemd5` with and without the flag, so this + // changes nothing for anyone already encoding ProRes. + if let Some(profile) = self.prores_profile() { + return Some(if profile >= 4 { + "yuv444p10le" // 4444 and 4444 XQ. Alpha is never carried here, + // so the plain 444 format, not yuva444p10le. + } else { + "yuv422p10le" // Proxy, LT, 422, 422 HQ. + }); + } + match self { // Classic HuffYUV only supports yuv422p (not yuv420p); ffvhuff and // the others accept yuv420p. @@ -797,7 +860,13 @@ impl VideoCodec { VideoCodec::H264Videotoolbox | VideoCodec::H265Videotoolbox => EncoderFamily::Videotoolbox, VideoCodec::H264Amf | VideoCodec::H265Amf => EncoderFamily::Amf, VideoCodec::FFV1 | VideoCodec::Huffyuv | VideoCodec::Ffvhuff => EncoderFamily::Lossless, - _ => EncoderFamily::ProRes, + // Exhaustive, not a catch-all: a non-ProRes variant added later + // would otherwise be silently classified as ProRes and take the + // `-profile:v` branch in `build_encoder_quality_args`. + VideoCodec::ProResProxy + | VideoCodec::ProResLT + | VideoCodec::ProRes422 + | VideoCodec::ProResHQ => EncoderFamily::ProRes, } } @@ -1049,10 +1118,16 @@ mod tests { } } - /// Software, ProRes, lossless and VideoToolbox negotiate correctly on their - /// own — VideoToolbox never advertises a mode it lacks, which is the whole + /// Software, lossless and VideoToolbox negotiate correctly on their own — + /// VideoToolbox never advertises a mode it lacks, which is the whole /// difference from NVENC. Forcing a format on them would only throw away /// chroma they can keep. + /// + /// ProRes used to be in this list and is deliberately no longer. It does + /// negotiate a format successfully; it just negotiates one that contradicts + /// the profile number stamped into the container, because ffmpeg's format + /// selection never looks at `-profile:v`. See + /// `prores_profile_decides_the_chroma`. #[test] fn test_negotiating_encoders_are_left_alone() { for codec in [ @@ -1060,7 +1135,6 @@ mod tests { VideoCodec::H265, VideoCodec::H264Videotoolbox, VideoCodec::H265Videotoolbox, - VideoCodec::ProRes422, VideoCodec::FFV1, VideoCodec::Ffvhuff, ] { @@ -1074,6 +1148,90 @@ mod tests { } } + /// ProRes stores a chroma layout fixed by its profile, and ffmpeg's format + /// negotiation does not know that — measured against the bundled build, + /// `prores_ks -profile:v 4` auto-selects `yuv422p10le` from a yuv420p input + /// exactly as `-profile:v 2` does. Unpinned, ProRes 4444 therefore writes a + /// file stamped 4444 carrying 4:2:2. + /// + /// Like the HuffYUV and AMF pins, this holds whatever the pipeline produced + /// — the profile is the whole input to the decision. + #[test] + fn prores_profile_decides_the_chroma() { + for fmt in [ + "yuv420p", + "yuv422p", + "yuv422p10le", + "yuv444p16le", + "yuva444p10le", + "rgb24", + ] { + for codec in [ + VideoCodec::ProResProxy, + VideoCodec::ProResLT, + VideoCodec::ProRes422, + VideoCodec::ProResHQ, + ] { + assert_eq!( + codec.forced_pix_fmt(fmt), + Some("yuv422p10le"), + "{codec:?} stores 4:2:2 whatever {fmt} was" + ); + } + } + } + + /// Every ProRes codec must declare a profile, or `build_encoder_quality_args` + /// takes the wrong branch: it dispatches on `prores_profile()` while the + /// fallthrough dispatches on `encoder_family()`, which produces `ProRes` via + /// a catch-all. A variant that has the family but no profile emits no + /// `-profile:v` at all, and `prores_ks` then silently defaults to profile 2 + /// (standard) — a "ProRes 4444" file that is really 422. + #[test] + fn every_prores_codec_declares_a_profile() { + for codec in VideoCodec::ALL.iter().copied() { + assert_eq!( + codec.is_prores(), + codec.encoder_family() == EncoderFamily::ProRes, + "{codec:?}: is_prores() and encoder_family() disagree, so the \ + quality-args branch and the fallthrough branch disagree too" + ); + if codec.is_prores() { + assert!( + codec.prores_profile().is_some(), + "{codec:?} would emit no -profile:v and encode as standard" + ); + assert!( + codec.forced_pix_fmt("yuv420p").is_some(), + "{codec:?} would let ffmpeg negotiate a chroma layout that \ + contradicts its profile" + ); + } + } + } + + /// Keeps `VideoCodec::ALL` complete, so the sweeps driven from it really do + /// cover the enum rather than the subset someone remembered. + #[test] + fn every_video_codec_is_listed() { + // `ffmpeg_codec()` is already exhaustive with no catch-all, so a new + // variant cannot compile without an arm there; this pins that it also + // reaches ALL. Names are unique, so the count is enough. + let mut names: Vec<&str> = VideoCodec::ALL.iter().map(|c| c.display_name()).collect(); + names.sort_unstable(); + names.dedup(); + assert_eq!( + names.len(), + VideoCodec::ALL.len(), + "VideoCodec::ALL lists the same codec twice" + ); + assert_eq!( + VideoCodec::ALL.len(), + 17, + "a VideoCodec variant was added without listing it in ALL" + ); + } + /// The two names for the output conversion describe the same thing, so a /// variant gaining one and not the other is a bug — the ffmpeg name is what /// decides whether a hardware encoder can take it. diff --git a/worker/src/pipeline_executor.rs b/worker/src/pipeline_executor.rs index ffb54cd..d8b0850 100644 --- a/worker/src/pipeline_executor.rs +++ b/worker/src/pipeline_executor.rs @@ -871,19 +871,13 @@ impl PipelineExecutor { // NVENC/QSV cannot encode 4:2:2 on most hardware — issue #74). let encoder_input = job.encoder_input_pix_fmt(); if let Some(pix_fmt) = settings.codec.forced_pix_fmt(&encoder_input) { - if pix_fmt != encoder_input { - // Say so in the job log. A silent downconversion is the right - // behaviour — failing the whole encode helps nobody — but it - // changes the output, so it must not also be invisible. - self.reporter.send_log( - LogLevel::Info, - &format!( - "{} cannot encode {}; converting to {} for output", - settings.codec.display_name(), - encoder_input, - pix_fmt - ), - ); + // Say so in the job log. A silent conversion is the right behaviour + // — failing the whole encode helps nobody — but it changes the + // output, so it must not also be invisible. + if let Some(note) = + Self::pix_fmt_change_note(settings.codec, &encoder_input, pix_fmt) + { + self.reporter.send_log(LogLevel::Info, ¬e); } args.extend(["-pix_fmt".to_string(), pix_fmt.to_string()]); } @@ -1033,6 +1027,53 @@ impl PipelineExecutor { ((bps / 1000.0).round() as u32).max(500) } + /// What to tell the user when [`VideoCodec::forced_pix_fmt`] overrides the + /// format the pipeline produced, or `None` when there is nothing to say. + /// + /// Split out of `build_ffmpeg_args` so it can be tested: the arg-building + /// mirror used by the unit tests does not log, so any message written + /// inline there ships as unverified prose. + /// + /// The wording matters because the two reasons a format is overridden are + /// opposites. For a hardware encoder it is a genuine limitation and + /// something is lost. For ProRes it is the profile's definition — the + /// encoder could take the source format perfectly well, and a 4:2:0 source + /// into ProRes 4444 is *padded up*, losing nothing. The original single + /// sentence ("X cannot encode Y") was written for the first case and would + /// have told ProRes users their encoder was broken while it did exactly + /// what they asked. + fn pix_fmt_change_note(codec: VideoCodec, from: &str, to: &str) -> Option { + if from == to { + return None; + } + + let (from_chroma, from_depth) = pixel_format::chroma_and_depth(from); + let (to_chroma, to_depth) = pixel_format::chroma_and_depth(to); + + if codec.is_prores() { + // Nothing is lost when both chroma and depth are kept or widened, + // which is the common case for a ProRes job: say nothing rather + // than report a conversion as if it were a cost. + if to_chroma >= from_chroma && to_depth >= from_depth { + return None; + } + return Some(format!( + "{} stores {}; converting {} to {} for output", + codec.display_name(), + to_chroma.label(), + from, + to + )); + } + + Some(format!( + "{} cannot encode {}; converting to {} for output", + codec.display_name(), + from, + to + )) + } + /// Build encoder-family-specific quality and preset arguments. fn build_encoder_quality_args(args: &mut Vec, job: &VideoJob) { let settings = &job.encoding_settings; @@ -2084,6 +2125,81 @@ mod tests { assert_eq!(args[idx + 1], "p010le"); } + /// The ProRes pin has to reach the actual argument list, not just + /// `forced_pix_fmt` — that is the whole point of it. + #[test] + fn test_prores_pin_reaches_the_args() { + for (codec, expected) in [ + (VideoCodec::ProResProxy, "yuv422p10le"), + (VideoCodec::ProResHQ, "yuv422p10le"), + ] { + let mut job = create_test_job("output.mov"); + job.encoding_settings.codec = codec; + job.encoding_settings.container = ContainerFormat::Mov; + job.input_pixel_format = Some("yuv420p".to_string()); + + let args = build_ffmpeg_args_for_test(&job); + let idx = args + .iter() + .position(|a| a == "-pix_fmt") + .unwrap_or_else(|| panic!("{codec:?} emitted no -pix_fmt")); + assert_eq!(args[idx + 1], expected, "{codec:?}"); + + // And the profile still goes out alongside it. + let p = args.iter().position(|a| a == "-profile:v").unwrap(); + assert_eq!(args[p + 1], codec.prores_profile().unwrap().to_string()); + } + } + + /// The job log must not tell a ProRes user their encoder "cannot encode" + /// a format it simply stores differently — and must say nothing at all when + /// the pin costs them nothing, which is the common case. + #[test] + fn test_prores_pin_message_does_not_claim_a_loss() { + // Padding 4:2:0 up to 4:2:2 loses nothing: stay quiet. + assert_eq!( + PipelineExecutor::pix_fmt_change_note( + VideoCodec::ProResHQ, + "yuv420p", + "yuv422p10le" + ), + None + ); + // Same format in and out: nothing to report either. + assert_eq!( + PipelineExecutor::pix_fmt_change_note( + VideoCodec::ProRes422, + "yuv422p10le", + "yuv422p10le" + ), + None + ); + + // Dropping 4:4:4 to 4:2:2 is a real loss and must be reported — but as + // what ProRes stores, not as an encoder limitation. + let note = PipelineExecutor::pix_fmt_change_note( + VideoCodec::ProResHQ, + "yuv444p10le", + "yuv422p10le", + ) + .expect("a chroma reduction must be logged"); + assert!(note.contains("4:2:2"), "note should name the layout: {note}"); + assert!(note.contains("yuv444p10le") && note.contains("yuv422p10le")); + assert!( + !note.contains("cannot encode"), + "ProRes can encode it; the profile decides the layout: {note}" + ); + + // A hardware encoder keeps the original wording, which is accurate there. + let hw = PipelineExecutor::pix_fmt_change_note( + VideoCodec::H265Nvenc, + "yuv422p10le", + "p010le", + ) + .expect("a hardware substitution must be logged"); + assert!(hw.contains("cannot encode"), "{hw}"); + } + /// A 4:2:0 source into NVENC must emit no `-pix_fmt` at all, so the fix /// changes nothing for the jobs that already worked. #[test] diff --git a/worker/src/pixel_format.rs b/worker/src/pixel_format.rs index 694f860..49b473c 100644 --- a/worker/src/pixel_format.rs +++ b/worker/src/pixel_format.rs @@ -48,13 +48,27 @@ pub const DEFAULT_FORMAT: &str = "yuv420p"; /// Chroma resolution class of a source format. Ordered so that a source is /// always mapped to a class at least as detailed as its own. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// +/// `Ord` follows that declaration order, so `C420 < C422 < C444` and a +/// comparison reads as "carries at least as much chroma as". +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub enum ChromaClass { C420, C422, C444, } +impl ChromaClass { + /// How this reads in a message to the user. + pub fn label(&self) -> &'static str { + match self { + ChromaClass::C420 => "4:2:0", + ChromaClass::C422 => "4:2:2", + ChromaClass::C444 => "4:4:4", + } + } +} + /// The pipe format for a job, plus what it was converted from (if anything). #[derive(Debug, Clone, PartialEq, Eq)] pub struct PipeFormat { From 1fbf03d5235daea550f8600eef8f823a46d43edb Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 12 Sep 2026 03:09:00 +1000 Subject: [PATCH 4/7] feat(codec): add ProRes 4444 and 4444 XQ (#81) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- app/lib/models/video_job.dart | 10 +- app/lib/utils/pixel_format.dart | 70 ++++++++ app/lib/views/settings/settings_dialog.dart | 27 ++- .../integration_chroma_subsampling_test.dart | 111 +++++++++++- app/test/prores_chroma_pin_warning_test.dart | 170 ++++++++++++++++++ worker/src/models/video_job.rs | 58 +++++- 6 files changed, 434 insertions(+), 12 deletions(-) create mode 100644 app/test/prores_chroma_pin_warning_test.dart diff --git a/app/lib/models/video_job.dart b/app/lib/models/video_job.dart index 56971ca..51b8938 100644 --- a/app/lib/models/video_job.dart +++ b/app/lib/models/video_job.dart @@ -205,7 +205,9 @@ enum VideoCodec { proresProxy('prores_ks -profile:v 0', 'ProRes Proxy'), proresLT('prores_ks -profile:v 1', 'ProRes LT'), prores422('prores_ks -profile:v 2', 'ProRes 422'), - proresHQ('prores_ks -profile:v 3', 'ProRes 422 HQ'); + proresHQ('prores_ks -profile:v 3', 'ProRes 422 HQ'), + prores4444('prores_ks -profile:v 4', 'ProRes 4444'), + prores4444Xq('prores_ks -profile:v 5', 'ProRes 4444 XQ'); const VideoCodec(this.value, this.displayName); @@ -247,7 +249,11 @@ enum VideoCodec { case VideoCodec.prores422: return 'Broadcast quality'; case VideoCodec.proresHQ: - return 'Highest ProRes quality'; + return 'Highest 4:2:2 ProRes quality'; + case VideoCodec.prores4444: + return 'Full 4:4:4 colour, for compositing'; + case VideoCodec.prores4444Xq: + return 'Full 4:4:4 colour at the highest data rate'; } } diff --git a/app/lib/utils/pixel_format.dart b/app/lib/utils/pixel_format.dart index 667a22f..57df238 100644 --- a/app/lib/utils/pixel_format.dart +++ b/app/lib/utils/pixel_format.dart @@ -233,3 +233,73 @@ String? hardwareEncoderChromaWarning({ return '$describedAs, but $reason, so the output will be converted to ' '$substitute.$advice'; } + +/// Warning message when the selected ProRes profile stores a different chroma +/// layout than the chosen output colour format, or `null` when they agree. +/// +/// A deliberate sibling of [hardwareEncoderChromaWarning] rather than an +/// extension of it, because the two make opposite claims. That one says the +/// hardware *cannot* encode what you asked for and something will be lost. +/// This one says the profile *defines* what is stored — ProRes 4444 pads 4:2:0 +/// up rather than losing anything, and the cost is file size, not detail. +/// Folding them together would blur both messages and would 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, and that is the +/// risk.** If the two disagree the interface promises one thing and the encode +/// does another, which is worse than either being wrong alone. Both sides are +/// pinned to the same table of cases: `prores_profile_decides_the_chroma` in +/// `worker/src/models/video_job.rs` against +/// `app/test/prores_chroma_pin_warning_test.dart`. Change one, change both. +/// +/// Deliberately silent about bit depth on its own. ProRes is always 10-bit, so +/// an 8-bit selection into any profile is "converted" — warning about that +/// would fire on the majority of ProRes jobs, and a banner that is always +/// there is wallpaper. +String? proresChromaPinWarning({ + required VideoCodec codec, + required ChromaSubsampling chromaSubsampling, + String? pixelFormat, +}) { + if (!codec.isProRes) return null; + + // What the encoder will actually be handed: the output conversion when one + // is selected, the source's own format otherwise. + final ChromaLayout chosen; + final String describedAs; + if (chromaSubsampling == ChromaSubsampling.original) { + // Nothing to say until a file is loaded and we know its format. + if (pixelFormat == null) return null; + chosen = pixelFormatChromaLayout(pixelFormat); + describedAs = 'Your source is $pixelFormat, and "Match source" keeps it'; + } else { + chosen = pixelFormatChromaLayout(chromaSubsampling.value); + describedAs = '${chromaSubsampling.label} is selected'; + } + + // Mirrors VideoCodec::forced_pix_fmt: profiles 4 and 5 store 4:4:4, + // everything below them 4:2:2. + final storesC444 = + codec == VideoCodec.prores4444 || codec == VideoCodec.prores4444Xq; + final stored = storesC444 ? ChromaLayout.c444 : ChromaLayout.c422; + if (chosen == stored) return null; + + if (storesC444) { + // Padding up. Nothing is lost, so the cost to name is size. + return '$describedAs, but ${codec.displayName} always stores 4:4:4, so the ' + 'chroma will be padded up to it. Nothing is lost, but the file is ' + 'much larger than the colour detail in it warrants — ProRes 422 HQ is ' + 'the 4:2:2 equivalent.'; + } + + if (chosen == ChromaLayout.c444) { + // The only case where the user's choice is genuinely discarded. + return '$describedAs, but ${codec.displayName} stores 4:2:2, so the ' + 'output will be resampled back down to it. Choose ProRes 4444 to keep ' + 'the full chroma.'; + } + + // 4:2:0 into a 4:2:2 profile: padded up, same reasoning as above but far + // less costly, so this stays quiet. + return null; +} diff --git a/app/lib/views/settings/settings_dialog.dart b/app/lib/views/settings/settings_dialog.dart index 9cd7d10..4ce1a3a 100644 --- a/app/lib/views/settings/settings_dialog.dart +++ b/app/lib/views/settings/settings_dialog.dart @@ -1011,6 +1011,7 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { ), ..._buildChromaBitDepthWarning(viewModel, settings), ..._buildChromaEncoderWarning(viewModel, settings), + ..._buildProresChromaWarning(viewModel, settings), ], ), ), @@ -1130,6 +1131,22 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { return [const SizedBox(height: 12), WarningBanner(message: message)]; } + /// Issue #81: a ProRes profile stores a chroma layout of its own, and the + /// worker pins it — so say when that overrides the selected colour format, + /// in either direction. + List _buildProresChromaWarning( + MainViewModel viewModel, + EncodingSettings settings, + ) { + final message = proresChromaPinWarning( + codec: settings.codec, + chromaSubsampling: settings.chromaSubsampling, + pixelFormat: viewModel.videoInfo?.pixelFormat, + ); + if (message == null) return const []; + return [const SizedBox(height: 12), WarningBanner(message: message)]; + } + /// Whether a hardware encoder is relevant to the current platform's GPU APIs: /// VideoToolbox is macOS-only; QSV/NVENC/AMF apply to Windows and Linux. /// (Software/ProRes/lossless codecs are platform-agnostic.) @@ -1159,10 +1176,12 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { VideoCodec.h264Videotoolbox, VideoCodec.h265Videotoolbox, VideoCodec.h264Amf, VideoCodec.h265Amf, ]; - final proresCodecs = [ - VideoCodec.proresProxy, VideoCodec.proresLT, - VideoCodec.prores422, VideoCodec.proresHQ, - ]; + // Every ProRes codec, derived rather than hand-listed: this was the one + // place the group was enumerated, so a profile missing from it existed in + // the model and was unreachable in the UI, with no error. Order follows the + // enum, which runs Proxy -> 4444 XQ by ascending data rate. + final proresCodecs = + VideoCodec.values.where((c) => c.isProRes).toList(growable: false); final losslessCodecs = [ VideoCodec.ffv1, VideoCodec.huffyuv, VideoCodec.ffvhuff, ]; diff --git a/app/test/integration_chroma_subsampling_test.dart b/app/test/integration_chroma_subsampling_test.dart index d9a6263..699b608 100644 --- a/app/test/integration_chroma_subsampling_test.dart +++ b/app/test/integration_chroma_subsampling_test.dart @@ -45,12 +45,19 @@ class VideoFormatInfo { final String? colorSpace; final String? colorRange; + /// The codec profile ffprobe reports, e.g. "4444" or "Standard" for ProRes. + /// Needed because a ProRes file stamped 4444 can still carry 4:2:2 samples — + /// that is exactly the bug the chroma pin exists to prevent, and only the + /// profile and the pix_fmt together can tell the two apart. + final String? profile; + VideoFormatInfo({ this.pixelFormat, this.width, this.height, this.colorSpace, this.colorRange, + this.profile, }); /// Check if this is a 4:2:0 format @@ -86,7 +93,7 @@ Future getVideoFormatInfo(String videoPath) async { [ '-v', 'error', '-select_streams', 'v:0', - '-show_entries', 'stream=pix_fmt,width,height,color_space,color_range', + '-show_entries', 'stream=pix_fmt,width,height,color_space,color_range,profile', '-of', 'json', videoPath, ], @@ -113,6 +120,7 @@ Future getVideoFormatInfo(String videoPath) async { height: stream['height'] as int?, colorSpace: stream['color_space'] as String?, colorRange: stream['color_range'] as String?, + profile: stream['profile'] as String?, ); } @@ -694,12 +702,107 @@ void main() { expect(result.success, isTrue, reason: result.error); final outputInfo = await getVideoFormatInfo(result.outputPath!); - // ProRes is naturally 4:2:2 - expect(outputInfo.isYuv422, isTrue, - reason: 'ProRes should output 4:2:2 format'); + // ProRes is naturally 4:2:2, and the worker now pins that explicitly + // rather than leaving it to negotiation. yuv422p10le is the only 4:2:2 + // format prores_ks has, so this is what it produced before the pin too + // — verified identical framemd5 with and without the flag. + expect(outputInfo.pixelFormat, 'yuv422p10le', + reason: 'ProRes 422 must store exactly 4:2:2 10-bit'); print(' PASS: YUV422 works with ProRes codec'); }, timeout: const Timeout(Duration(minutes: 5))); + + /// The test that proves the whole premise of the ProRes chroma pin + /// (issue #81). ffmpeg's format negotiation never looks at `-profile:v`, + /// so without the pin this comes back stamped 4444 while carrying 4:2:2 + /// — a valid file that silently throws away the profile's entire point. + /// Only a real encode can catch that; a script or argument assertion + /// cannot. + /// + /// Note the decoder reports ProRes 4444 as 12-bit (`yuv444p12le`) + /// regardless of the 10-bit format we hand the encoder, so the assertion + /// is on the chroma part of the name, not the whole string. + test('ProRes 4444 really stores 4:4:4, not just the 4444 stamp', () async { + final job = VideoJob( + id: const Uuid().v4(), + outputPath: '${TestConfig.outputDir}/test_chroma_444_prores4444.mov', + inputPath: TestConfig.inputFile, + qtgmcParameters: const QTGMCParameters( + preset: QTGMCPreset.superFast, + tff: true, + fpsDivisor: 2, + ), + processingPipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters( + preset: QTGMCPreset.superFast, + tff: true, + fpsDivisor: 2, + ), + ), + encodingSettings: const EncodingSettings( + codec: VideoCodec.prores4444, + container: ContainerFormat.mov, + audioMode: AudioMode.none, + chromaSubsampling: ChromaSubsampling.yuv444p10, + ), + startFrame: 10, + endFrame: 40, + ); + + final result = await runFilterTest('YUV444 + ProRes 4444', job); + expect(result.success, isTrue, reason: result.error); + + final outputInfo = await getVideoFormatInfo(result.outputPath!); + expect(outputInfo.isYuv444, isTrue, + reason: 'unpinned, ffmpeg hands profile 4 yuv422p10le and this ' + 'comes back 4:2:2 — got ${outputInfo.pixelFormat}'); + expect(outputInfo.profile, '4444', + reason: 'the container must agree with the samples'); + + print(' PASS: ProRes 4444 stores ${outputInfo.pixelFormat}'); + }, timeout: const Timeout(Duration(minutes: 5))); + + /// The negative control for the test above: a 4:2:2 profile must hold + /// 4:2:2 even when 4:4:4 is explicitly selected, because the profile is + /// what the container declares. Without this, a pin that simply forced + /// 4:4:4 everywhere would pass the positive test. + test('ProRes 422 HQ stays 4:2:2 even when 4:4:4 is selected', () async { + final job = VideoJob( + id: const Uuid().v4(), + outputPath: '${TestConfig.outputDir}/test_chroma_444_prores422.mov', + inputPath: TestConfig.inputFile, + qtgmcParameters: const QTGMCParameters( + preset: QTGMCPreset.superFast, + tff: true, + fpsDivisor: 2, + ), + processingPipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters( + preset: QTGMCPreset.superFast, + tff: true, + fpsDivisor: 2, + ), + ), + encodingSettings: const EncodingSettings( + codec: VideoCodec.proresHQ, + container: ContainerFormat.mov, + audioMode: AudioMode.none, + chromaSubsampling: ChromaSubsampling.yuv444p10, + ), + startFrame: 10, + endFrame: 40, + ); + + final result = await runFilterTest('YUV444 + ProRes 422 HQ', job); + expect(result.success, isTrue, reason: result.error); + + final outputInfo = await getVideoFormatInfo(result.outputPath!); + expect(outputInfo.isYuv422, isTrue, + reason: 'ProRes 422 HQ stores 4:2:2 whatever was selected — ' + 'got ${outputInfo.pixelFormat}'); + + print(' PASS: ProRes 422 HQ held 4:2:2 (${outputInfo.pixelFormat})'); + }, timeout: const Timeout(Duration(minutes: 5))); }); }); } diff --git a/app/test/prores_chroma_pin_warning_test.dart b/app/test/prores_chroma_pin_warning_test.dart new file mode 100644 index 0000000..23baf96 --- /dev/null +++ b/app/test/prores_chroma_pin_warning_test.dart @@ -0,0 +1,170 @@ +// The UI half of the ProRes chroma pin (issue #81). +// +// A ProRes profile stores a fixed chroma layout, and ffmpeg's format +// negotiation does not know that — `prores_ks -profile:v 4` picks yuv422p10le +// from a 4:2:0 source exactly as `-profile:v 2` does. The worker therefore pins +// the format from the profile (`VideoCodec::forced_pix_fmt`), which means the +// user's output colour format choice can be overridden in either direction. +// This warning says so before the job runs. +// +// **It is a second implementation of the worker's decision.** If the two +// disagree the interface promises one thing and the encode does another, which +// is worse than either being wrong alone. The table below is pinned to +// `prores_profile_decides_the_chroma` and `prores_pin_matches_the_profile_it_claims` +// in worker/src/models/video_job.rs. Change one, change both. +// +// Run with: flutter test test/prores_chroma_pin_warning_test.dart + +import 'package:flutter_test/flutter_test.dart'; +import 'package:vapourbox/models/encoding_settings.dart'; +import 'package:vapourbox/models/video_job.dart'; +import 'package:vapourbox/utils/pixel_format.dart'; + +void main() { + String? warn({ + required VideoCodec codec, + ChromaSubsampling chroma = ChromaSubsampling.original, + String? pixelFormat, + }) => + proresChromaPinWarning( + codec: codec, + chromaSubsampling: chroma, + pixelFormat: pixelFormat, + ); + + /// The 4:2:2 profiles, and the 4:4:4 ones, exactly as the worker splits them + /// (profile >= 4 stores 4:4:4). + const c422Profiles = [ + VideoCodec.proresProxy, + VideoCodec.proresLT, + VideoCodec.prores422, + VideoCodec.proresHQ, + ]; + const c444Profiles = [ + VideoCodec.prores4444, + VideoCodec.prores4444Xq, + ]; + + group('which profiles store what', () { + test('matches the worker profile split, with no profile left out', () { + // Guards against a ProRes profile being added to the enum and silently + // treated as 4:2:2 by this file while the worker pins it to 4:4:4. + final all = VideoCodec.values.where((c) => c.isProRes).toList(); + expect( + all.toSet(), + {...c422Profiles, ...c444Profiles}, + reason: 'a ProRes profile exists that this test does not classify; ' + 'check it against forced_pix_fmt in the worker', + ); + }); + }); + + group('4:4:4 selected', () { + test('warns on every 4:2:2 profile, and says it is resampled down', () { + for (final codec in c422Profiles) { + final message = + warn(codec: codec, chroma: ChromaSubsampling.yuv444p10); + expect(message, isNotNull, reason: codec.displayName); + expect(message, contains('4:2:2')); + expect(message, contains('ProRes 4444'), + reason: 'the message must name the way out'); + } + }); + + test('is silent on the 4:4:4 profiles, which store exactly that', () { + for (final codec in c444Profiles) { + expect(warn(codec: codec, chroma: ChromaSubsampling.yuv444p10), isNull, + reason: '${codec.displayName} stores 4:4:4 already'); + } + }); + }); + + group('narrower than 4:4:4 selected with a 4:4:4 profile', () { + test('warns that the file grows without gaining detail', () { + for (final codec in c444Profiles) { + for (final chroma in [ + ChromaSubsampling.yuv420, + ChromaSubsampling.yuv420p10, + ChromaSubsampling.yuv422, + ChromaSubsampling.yuv422p10, + ]) { + final message = warn(codec: codec, chroma: chroma); + expect(message, isNotNull, + reason: '${codec.displayName} + ${chroma.label}'); + expect(message, contains('4:4:4')); + // Padding up costs size, not detail, and the message has to say so — + // otherwise it reads as a quality warning and pushes people off a + // profile that is doing exactly what they asked. + expect(message, contains('Nothing is lost')); + expect(message, contains('larger')); + } + } + }); + }); + + group('agreement is silent', () { + test('4:2:2 selected on a 4:2:2 profile says nothing', () { + for (final codec in c422Profiles) { + for (final chroma in [ + ChromaSubsampling.yuv422, + ChromaSubsampling.yuv422p10, + ]) { + expect(warn(codec: codec, chroma: chroma), isNull, + reason: '${codec.displayName} + ${chroma.label}'); + } + } + }); + + test('4:2:0 into a 4:2:2 profile stays quiet', () { + // True that it is padded up, but it is the ordinary case for every + // capture this app exists to process, and a banner that is always there + // is wallpaper. + for (final codec in c422Profiles) { + expect(warn(codec: codec, chroma: ChromaSubsampling.yuv420), isNull); + } + }); + + test('never fires for a non-ProRes codec', () { + for (final codec in VideoCodec.values.where((c) => !c.isProRes)) { + for (final chroma in ChromaSubsampling.values) { + expect( + warn(codec: codec, chroma: chroma, pixelFormat: 'yuv420p'), + isNull, + reason: '${codec.displayName} is not ProRes', + ); + } + } + }); + }); + + group('"Match source"', () { + test('says nothing until a file is loaded', () { + for (final codec in VideoCodec.values.where((c) => c.isProRes)) { + expect(warn(codec: codec), isNull, + reason: 'no source format is known yet'); + } + }); + + test('reads the layout from the source format', () { + // A 4:4:4 source into a 4:2:2 profile is the real loss case. + expect( + warn(codec: VideoCodec.proresHQ, pixelFormat: 'yuv444p10le'), + contains('4:2:2'), + ); + // A 4:2:0 source into 4444 is the padding case. + expect( + warn(codec: VideoCodec.prores4444, pixelFormat: 'yuv420p'), + contains('4:4:4'), + ); + // And a source that already matches is silent. + expect( + warn(codec: VideoCodec.proresHQ, pixelFormat: 'yuv422p10le'), + isNull, + ); + expect( + warn(codec: VideoCodec.prores4444, pixelFormat: 'yuv444p10le'), + isNull, + ); + }); + }); +} diff --git a/worker/src/models/video_job.rs b/worker/src/models/video_job.rs index 70ce570..0a1dedf 100644 --- a/worker/src/models/video_job.rs +++ b/worker/src/models/video_job.rs @@ -585,6 +585,12 @@ pub enum VideoCodec { #[serde(rename = "prores_ks -profile:v 3")] ProResHQ, + + #[serde(rename = "prores_ks -profile:v 4")] + ProRes4444, + + #[serde(rename = "prores_ks -profile:v 5")] + ProRes4444Xq, } /// Encoder family for grouping quality/preset logic. @@ -621,6 +627,8 @@ impl VideoCodec { VideoCodec::ProResLT => "prores_ks", VideoCodec::ProRes422 => "prores_ks", VideoCodec::ProResHQ => "prores_ks", + VideoCodec::ProRes4444 => "prores_ks", + VideoCodec::ProRes4444Xq => "prores_ks", } } @@ -644,6 +652,8 @@ impl VideoCodec { VideoCodec::ProResLT, VideoCodec::ProRes422, VideoCodec::ProResHQ, + VideoCodec::ProRes4444, + VideoCodec::ProRes4444Xq, ]; /// Get the ProRes profile value, if applicable. @@ -660,6 +670,8 @@ impl VideoCodec { VideoCodec::ProResLT => Some(1), VideoCodec::ProRes422 => Some(2), VideoCodec::ProResHQ => Some(3), + VideoCodec::ProRes4444 => Some(4), + VideoCodec::ProRes4444Xq => Some(5), VideoCodec::H264 | VideoCodec::H265 | VideoCodec::H264Nvenc @@ -866,7 +878,9 @@ impl VideoCodec { VideoCodec::ProResProxy | VideoCodec::ProResLT | VideoCodec::ProRes422 - | VideoCodec::ProResHQ => EncoderFamily::ProRes, + | VideoCodec::ProResHQ + | VideoCodec::ProRes4444 + | VideoCodec::ProRes4444Xq => EncoderFamily::ProRes, } } @@ -901,6 +915,8 @@ impl VideoCodec { VideoCodec::ProResLT => "ProRes LT", VideoCodec::ProRes422 => "ProRes 422", VideoCodec::ProResHQ => "ProRes 422 HQ", + VideoCodec::ProRes4444 => "ProRes 4444", + VideoCodec::ProRes4444Xq => "ProRes 4444 XQ", } } } @@ -989,6 +1005,14 @@ mod tests { serde_json::to_string(&VideoCodec::ProResHQ).unwrap(), "\"prores_ks -profile:v 3\"" ); + assert_eq!( + serde_json::to_string(&VideoCodec::ProRes4444).unwrap(), + "\"prores_ks -profile:v 4\"" + ); + assert_eq!( + serde_json::to_string(&VideoCodec::ProRes4444Xq).unwrap(), + "\"prores_ks -profile:v 5\"" + ); assert_eq!( serde_json::to_string(&VideoCodec::H264Nvenc).unwrap(), "\"h264_nvenc\"" @@ -1178,6 +1202,36 @@ mod tests { "{codec:?} stores 4:2:2 whatever {fmt} was" ); } + for codec in [VideoCodec::ProRes4444, VideoCodec::ProRes4444Xq] { + assert_eq!( + codec.forced_pix_fmt(fmt), + Some("yuv444p10le"), + "{codec:?} stores 4:4:4 whatever {fmt} was — unpinned, \ + ffmpeg hands it 4:2:2 and the 4444 stamp becomes a lie" + ); + } + } + } + + /// The pin and the profile have to agree, or the container says one thing + /// and the samples are another. Derived from the profile number rather than + /// listed per codec, so a profile added later cannot be missed. + #[test] + fn prores_pin_matches_the_profile_it_claims() { + for codec in VideoCodec::ALL.iter().copied() { + let Some(profile) = codec.prores_profile() else { + continue; + }; + let expected = if profile >= 4 { + "yuv444p10le" + } else { + "yuv422p10le" + }; + assert_eq!( + codec.forced_pix_fmt("yuv420p"), + Some(expected), + "{codec:?} is profile {profile}" + ); } } @@ -1227,7 +1281,7 @@ mod tests { ); assert_eq!( VideoCodec::ALL.len(), - 17, + 19, "a VideoCodec variant was added without listing it in ALL" ); } From d2417d7dfe53cba8ffc4a29179ea1d8f7654ff25 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 12 Sep 2026 03:16:27 +1000 Subject: [PATCH 5/7] feat(prores): expose vendor tag, bits-per-macroblock and quantisation matrix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- app/lib/models/encoding_settings.dart | 54 ++++++++ app/lib/views/settings/settings_dialog.dart | 141 ++++++++++++++++++++ app/test/parameter_copy_with_test.dart | 21 ++- app/test/prores_options_test.dart | 126 +++++++++++++++++ worker/src/models/video_job.rs | 66 +++++++++ worker/src/pipeline_executor.rs | 116 +++++++++++++++- 6 files changed, 516 insertions(+), 8 deletions(-) create mode 100644 app/test/prores_options_test.dart diff --git a/app/lib/models/encoding_settings.dart b/app/lib/models/encoding_settings.dart index fe411ba..605ef68 100644 --- a/app/lib/models/encoding_settings.dart +++ b/app/lib/models/encoding_settings.dart @@ -61,6 +61,27 @@ enum AudioQuality { /// Mirrors `ChromaSubsampling` in `worker/src/models/video_job.rs` — the [value] /// strings must match that enum's serde names. @JsonEnum(valueField: 'value') +/// The quantisation matrices `prores_ks` accepts by name. +/// +/// Mirrors `ProResQuantMat` in `worker/src/models/video_job.rs` — the `value` +/// strings here are the wire format. An enum rather than a free string because +/// ffmpeg rejects an unknown value outright and kills the encode, so a typo in +/// a saved preset would fail the whole job on an option nobody can see. +@JsonEnum(valueField: 'value') +enum ProResQuantMat { + /// Match the profile — what the encoder does with no option at all. + auto('auto', 'Auto (match profile)'), + proxy('proxy', 'Proxy'), + lt('lt', 'LT'), + standard('standard', 'Standard'), + hq('hq', 'HQ'); + + const ProResQuantMat(this.value, this.label); + + final String value; + final String label; +} + enum ChromaSubsampling { /// Keep original format (no conversion), bit depth included. original('original', 'Match source', null, null), @@ -125,6 +146,20 @@ class EncodingSettings { /// Output chroma subsampling format. final ChromaSubsampling chromaSubsampling; + /// Write `apl0` as the ProRes vendor tag instead of ffmpeg's `Lavc`. + /// A compatibility flag, not a quality one — some Avid and Apple tooling + /// reads this field, and the decoded frames are identical either way. + /// Ignored by every non-ProRes codec. + final bool proresVendorApl0; + + /// ProRes `-bits_per_mb`: the ceiling the encoder may spend per macroblock. + /// Null leaves it to the profile. Only affects Proxy and LT in practice. + final int? proresBitsPerMb; + + /// ProRes `-quant_mat`. Null leaves the encoder on `auto`, which picks the + /// matrix matching the profile. + final ProResQuantMat? proresQuantMat; + final String customFfmpegArgs; /// User-supplied VapourSynth, injected after every built-in pass. Same @@ -150,6 +185,9 @@ class EncodingSettings { this.audioCodec = AudioCodec.aac, this.audioQuality = AudioQuality.high, this.chromaSubsampling = ChromaSubsampling.original, + this.proresVendorApl0 = false, + this.proresBitsPerMb, + this.proresQuantMat, this.customFfmpegArgs = '', this.customVapoursynth = '', this.container = ContainerFormat.mkv, @@ -242,6 +280,16 @@ class EncodingSettings { AudioCodec? audioCodec, AudioQuality? audioQuality, ChromaSubsampling? chromaSubsampling, + bool? proresVendorApl0, + int? proresBitsPerMb, + // Nullable settings need an explicit clear, or unticking the override in + // the UI can never put them back to null and the value sticks forever, + // silently applied to every later ProRes encode. `outputDirectory` is the + // precedent; `videoBitrateKbps` is the counter-example that has to work + // around its own absence in _buildCodecRadio. + bool clearProresBitsPerMb = false, + ProResQuantMat? proresQuantMat, + bool clearProresQuantMat = false, String? customFfmpegArgs, String? customVapoursynth, ContainerFormat? container, @@ -266,6 +314,12 @@ class EncodingSettings { audioCodec: audioCodec ?? this.audioCodec, audioQuality: audioQuality ?? this.audioQuality, chromaSubsampling: chromaSubsampling ?? this.chromaSubsampling, + proresVendorApl0: proresVendorApl0 ?? this.proresVendorApl0, + proresBitsPerMb: clearProresBitsPerMb + ? null + : (proresBitsPerMb ?? this.proresBitsPerMb), + proresQuantMat: + clearProresQuantMat ? null : (proresQuantMat ?? this.proresQuantMat), customFfmpegArgs: customFfmpegArgs ?? this.customFfmpegArgs, customVapoursynth: customVapoursynth ?? this.customVapoursynth, container: container ?? this.container, diff --git a/app/lib/views/settings/settings_dialog.dart b/app/lib/views/settings/settings_dialog.dart index 4ce1a3a..292489e 100644 --- a/app/lib/views/settings/settings_dialog.dart +++ b/app/lib/views/settings/settings_dialog.dart @@ -508,6 +508,15 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { /// Default target bitrate (kbps) applied when an Intel-VT codec is selected. static const int _kDefaultVtBitrateKbps = 20000; + /// `prores_ks` declares `bits_per_mb` with this maximum; above it ffmpeg + /// rejects the option outright. Mirrors `PRORES_MAX_BITS_PER_MB` in + /// `worker/src/pipeline_executor.rs`, which clamps rather than trusting this. + static const int _kMaxProresBitsPerMb = 8192; + + /// What the override starts at when first ticked — the value the guide this + /// came from recommends, and the one the +3.6 dB measurement was taken at. + static const int _kDefaultProresBitsPerMb = 8000; + /// Bitrate preset shortcuts (label -> Mb/s) for Intel VideoToolbox. static const Map _kVtBitratePresetsMbps = { 'Low': 5, @@ -1016,6 +1025,20 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { ), ), + // ProRes-only encoder options, behind advanced mode. They are + // narrow enough that showing them to everyone would cost more in + // clutter than they return — two of the three do nothing at all on + // the profiles most people pick. + if (settings.codec.isProRes && + context.watch().enabled) ...[ + const SizedBox(height: 24), + _buildSection( + context, + title: 'ProRes Options', + child: _buildProresOptions(context, viewModel, settings), + ), + ], + const SizedBox(height: 24), // Custom FFmpeg Arguments @@ -1252,6 +1275,124 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { ); } + /// The three ProRes encoder options, with the measurements that justify + /// them. Two of the three do nothing on 422 and HQ, and the copy says so — + /// presenting them as general quality controls would repeat the overstatement + /// in the guide these came from (issue #81). + Widget _buildProresOptions( + BuildContext context, MainViewModel viewModel, EncodingSettings settings) { + final hint = Theme.of(context).textTheme.bodySmall?.copyWith( + color: Theme.of(context).colorScheme.onSurface.withValues(alpha: 0.6), + ); + + return Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + CheckboxListTile( + contentPadding: EdgeInsets.zero, + controlAffinity: ListTileControlAffinity.leading, + value: settings.proresVendorApl0, + title: const Text('Write the Apple vendor tag (apl0)'), + subtitle: Text( + 'Identifies the file as Apple-encoded rather than FFmpeg-encoded. ' + 'Some Avid and Apple tooling checks this field. It does not change ' + 'the picture — the frames are identical either way.', + style: hint, + ), + onChanged: (value) => viewModel.updateEncodingSettings( + settings.copyWith(proresVendorApl0: value ?? false), + ), + ), + const SizedBox(height: 16), + + // Bits per macroblock: an override, so it needs an explicit off state. + CheckboxListTile( + contentPadding: EdgeInsets.zero, + controlAffinity: ListTileControlAffinity.leading, + value: settings.proresBitsPerMb != null, + title: const Text('Raise the bits-per-macroblock ceiling'), + subtitle: Text( + 'Lets the encoder spend more on each macroblock. Worth about ' + '3.6 dB for 3% more size on Proxy and LT; on 422 and 422 HQ the ' + 'encoder is already below the ceiling and this changes nothing.', + style: hint, + ), + onChanged: (value) => viewModel.updateEncodingSettings( + (value ?? false) + ? settings.copyWith(proresBitsPerMb: _kDefaultProresBitsPerMb) + : settings.copyWith(clearProresBitsPerMb: true), + ), + ), + if (settings.proresBitsPerMb != null) + Padding( + padding: const EdgeInsets.only(left: 32, top: 4), + child: Row( + children: [ + Expanded( + child: Slider( + value: settings.proresBitsPerMb! + .clamp(1, _kMaxProresBitsPerMb) + .toDouble(), + min: 1, + max: _kMaxProresBitsPerMb.toDouble(), + divisions: 32, + label: '${settings.proresBitsPerMb}', + onChanged: (value) => viewModel.updateEncodingSettings( + settings.copyWith(proresBitsPerMb: value.round()), + ), + ), + ), + SizedBox( + width: 56, + child: Text( + '${settings.proresBitsPerMb}', + style: Theme.of(context).textTheme.bodyMedium, + textAlign: TextAlign.end, + ), + ), + ], + ), + ), + const SizedBox(height: 16), + + DropdownButtonFormField( + value: settings.proresQuantMat, + decoration: const InputDecoration( + labelText: 'Quantisation matrix', + border: OutlineInputBorder(), + ), + items: [ + const DropdownMenuItem( + value: null, + child: Text('Auto (match profile)'), + ), + // `auto` is offered explicitly as well as by omission, because the + // two are the same thing to ffmpeg and hiding one would make the + // dropdown disagree with a preset that saved it. + ...ProResQuantMat.values.map( + (m) => DropdownMenuItem( + value: m, + child: Text(m.label), + ), + ), + ], + onChanged: (value) => viewModel.updateEncodingSettings( + value == null + ? settings.copyWith(clearProresQuantMat: true) + : settings.copyWith(proresQuantMat: value), + ), + ), + const SizedBox(height: 8), + Text( + 'Auto picks the matrix that matches the profile. Choosing HQ on a ' + 'Proxy or LT encode spends about 19% more size for roughly 5.5 dB; ' + 'on 422 and 422 HQ it is already the matrix in use.', + style: hint, + ), + ], + ); + } + /// The panel shown in place of the quality slider for codecs whose quality /// the worker does not set — lossless and ProRes. One helper for both so the /// two cannot drift into looking like different kinds of message. diff --git a/app/test/parameter_copy_with_test.dart b/app/test/parameter_copy_with_test.dart index 69742b9..4d6539d 100644 --- a/app/test/parameter_copy_with_test.dart +++ b/app/test/parameter_copy_with_test.dart @@ -152,12 +152,19 @@ void main() { // shape, and Subtitles dropped a plain `String`. So the same rule is also // checked against the source, where the type does not matter. group('copyWith() names every field, whatever its type', () { - final files = Directory('lib/models') - .listSync() - .whereType() - .where((f) => f.path.endsWith('_parameters.dart')) - .toList() - ..sort((a, b) => a.path.compareTo(b.path)); + final files = [ + ...Directory('lib/models') + .listSync() + .whereType() + .where((f) => f.path.endsWith('_parameters.dart')), + // Not a *_parameters.dart file, and so invisible to this sweep until + // 2026-08-29 — but it has exactly the same failure mode and a worse + // blast radius. Every edit in the settings dialog goes through + // `updateEncodingSettings(settings.copyWith(...))`, so a field this + // method forgets is reset on the user's very next click, not merely on a + // pass toggle. `videoBitrateKbps` had already been added without one. + File('lib/models/encoding_settings.dart'), + ]..sort((a, b) => a.path.compareTo(b.path)); for (final file in files) { final name = file.uri.pathSegments.last; @@ -169,7 +176,7 @@ void main() { // `final` members of their own (`displayName`, `value`), and those are // no business of copyWith. final classBody = RegExp( - r'^class (\w*Parameters) \{$(.*?)^\}$', + r'^class (EncodingSettings|\w*Parameters) \{$(.*?)^\}$', multiLine: true, dotAll: true, ).firstMatch(source); diff --git a/app/test/prores_options_test.dart b/app/test/prores_options_test.dart new file mode 100644 index 0000000..b40e031 --- /dev/null +++ b/app/test/prores_options_test.dart @@ -0,0 +1,126 @@ +// The three advanced ProRes encoder options (issue #81), on the model side. +// +// `parameter_copy_with_test.dart` now scans this model's copyWith by field +// name, which catches a dropped field of any type. It cannot check the one +// thing these options additionally need: that they can be turned back *off*. +// +// Every edit in the settings dialog goes through +// `updateEncodingSettings(settings.copyWith(...))`, and `x ?? this.x` can only +// ever set a nullable field, never clear it. Without an explicit clear flag, +// unticking the override in the UI leaves the value in place and it is silently +// applied to every later ProRes encode. `videoBitrateKbps` has exactly that +// defect today and `_buildCodecRadio` works around it by recomputing the value +// on every codec change. +// +// Run with: flutter test test/prores_options_test.dart + +import 'package:flutter_test/flutter_test.dart'; +import 'package:vapourbox/models/encoding_settings.dart'; +import 'package:vapourbox/models/video_job.dart'; + +void main() { + group('defaults', () { + test('are off, so an existing job encodes exactly as it did', () { + const settings = EncodingSettings(); + expect(settings.proresVendorApl0, isFalse); + expect(settings.proresBitsPerMb, isNull); + expect(settings.proresQuantMat, isNull); + }); + }); + + group('copyWith', () { + const set = EncodingSettings( + codec: VideoCodec.proresHQ, + proresVendorApl0: true, + proresBitsPerMb: 8000, + proresQuantMat: ProResQuantMat.hq, + ); + + test('carries the three options through an unrelated edit', () { + // The realistic failure: the user sets these, then changes the audio + // mode, and the ProRes options quietly revert. + final after = set.copyWith(audioMode: AudioMode.none); + expect(after.proresVendorApl0, isTrue); + expect(after.proresBitsPerMb, 8000); + expect(after.proresQuantMat, ProResQuantMat.hq); + }); + + test('can clear the two nullable options', () { + expect(set.copyWith(clearProresBitsPerMb: true).proresBitsPerMb, isNull); + expect(set.copyWith(clearProresQuantMat: true).proresQuantMat, isNull); + }); + + test('clearing one leaves the others alone', () { + final after = set.copyWith(clearProresBitsPerMb: true); + expect(after.proresBitsPerMb, isNull); + expect(after.proresQuantMat, ProResQuantMat.hq, + reason: 'clearing one override must not disturb another'); + expect(after.proresVendorApl0, isTrue); + }); + + test('the vendor flag can be turned back off', () { + // A bool needs no clear flag, but `?? this.x` makes `false` indistinguish + // -able from "not supplied" if it is ever made nullable. Pinned so that + // change cannot pass silently. + expect(set.copyWith(proresVendorApl0: false).proresVendorApl0, isFalse); + }); + }); + + group('round trip', () { + test('survives JSON, so a saved preset keeps them', () { + const original = EncodingSettings( + codec: VideoCodec.prores4444, + proresVendorApl0: true, + proresBitsPerMb: 4096, + proresQuantMat: ProResQuantMat.proxy, + ); + + final restored = EncodingSettings.fromJson(original.toJson()); + expect(restored.proresVendorApl0, isTrue); + expect(restored.proresBitsPerMb, 4096); + expect(restored.proresQuantMat, ProResQuantMat.proxy); + expect(restored.codec, VideoCodec.prores4444); + }); + + test('an older preset without the fields still loads', () { + // Every one is #[serde(default)] on the worker side and nullable or + // defaulted here, so a preset saved before they existed must decode. + final json = const EncodingSettings(codec: VideoCodec.proresHQ).toJson() + ..remove('proresVendorApl0') + ..remove('proresBitsPerMb') + ..remove('proresQuantMat'); + + final restored = EncodingSettings.fromJson(json); + expect(restored.proresVendorApl0, isFalse); + expect(restored.proresBitsPerMb, isNull); + expect(restored.proresQuantMat, isNull); + }); + }); + + group('ProResQuantMat', () { + test('values match the worker enum serde names', () { + // ProResQuantMat in worker/src/models/video_job.rs uses + // rename_all = "lowercase", so these are the wire format. ffmpeg rejects + // an unknown value outright and kills the encode, so a mismatch here is a + // failed job on an option the user cannot see. + expect(ProResQuantMat.auto.value, 'auto'); + expect(ProResQuantMat.proxy.value, 'proxy'); + expect(ProResQuantMat.lt.value, 'lt'); + expect(ProResQuantMat.standard.value, 'standard'); + expect(ProResQuantMat.hq.value, 'hq'); + expect(ProResQuantMat.values.length, 5, + reason: 'a matrix added here must also exist in the Rust enum, and ' + 'must be a name prores_ks actually accepts'); + }); + + test('every value has a label distinct from its wire name', () { + for (final m in ProResQuantMat.values) { + expect(m.label, isNotEmpty); + } + final labels = ProResQuantMat.values.map((m) => m.label).toSet(); + expect(labels.length, ProResQuantMat.values.length, + reason: 'two matrices sharing a label are indistinguishable in the ' + 'dropdown'); + }); + }); +} diff --git a/worker/src/models/video_job.rs b/worker/src/models/video_job.rs index 0a1dedf..8357d87 100644 --- a/worker/src/models/video_job.rs +++ b/worker/src/models/video_job.rs @@ -289,6 +289,69 @@ pub struct EncodingSettings { /// estimate; ignored by all other encoder families. #[serde(default)] pub video_bitrate_kbps: Option, + + /// Write `apl0` as the ProRes vendor tag instead of ffmpeg's `Lavc`. + /// + /// A compatibility flag, not a quality one — some Avid and Apple tooling + /// reads this field. Measured: it changes four bytes per frame header and + /// nothing else, the decoded frames being bit-identical either way. + /// Defaults off so existing output is unchanged. + #[serde(default)] + pub prores_vendor_apl0: bool, + + /// ProRes `-bits_per_mb`: the ceiling the encoder may spend per macroblock. + /// + /// Only worth setting on the low profiles. Measured on pal-sd-25.mov at + /// profile 0, `8000` bought +3.6 dB for 2.8% more size; at profile 2 and + /// above the encoder is already below the cap and the output is + /// byte-identical. `None` (and `Some(0)`, which is the plugin's "use the + /// profile default") emit nothing. + #[serde(default)] + pub prores_bits_per_mb: Option, + + /// ProRes `-quant_mat`: which quantisation matrix to use. + /// + /// `None` means don't pass the option, which leaves the encoder on `auto` + /// — it then picks the matrix matching the profile. Choosing `Hq` on a + /// Proxy or LT encode measured +5.5 dB for 19% more size. + #[serde(default)] + pub prores_quant_mat: Option, +} + +/// The quantisation matrices `prores_ks` accepts by name. +/// +/// An enum rather than a free string on purpose: ffmpeg rejects an unknown +/// value outright ("Undefined constant") and kills the encode, so a preset or +/// imported job config carrying a typo would fail the whole job on an option +/// the user cannot see. Same reasoning as `ColorMetadata::from_raw` and +/// `parse_ratio` — validate, never forward. +/// +/// The plugin also offers `default` (the flat matrix), deliberately not exposed: +/// it is not what anyone reaching for this wants, and every option in a curated +/// dropdown has to earn its place. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum ProResQuantMat { + /// Match the profile — what the encoder does with no option at all. + Auto, + Proxy, + Lt, + Standard, + Hq, +} + +impl ProResQuantMat { + /// The name to pass to `-quant_mat`. All five verified accepted by the + /// bundled encoder. + pub fn ffmpeg_name(&self) -> &'static str { + match self { + ProResQuantMat::Auto => "auto", + ProResQuantMat::Proxy => "proxy", + ProResQuantMat::Lt => "lt", + ProResQuantMat::Standard => "standard", + ProResQuantMat::Hq => "hq", + } + } } fn default_encoder_preset() -> String { @@ -521,6 +584,9 @@ impl Default for EncodingSettings { custom_vapoursynth: String::new(), container: ContainerFormat::default(), video_bitrate_kbps: None, + prores_vendor_apl0: false, + prores_bits_per_mb: None, + prores_quant_mat: None, } } } diff --git a/worker/src/pipeline_executor.rs b/worker/src/pipeline_executor.rs index d8b0850..505b6f3 100644 --- a/worker/src/pipeline_executor.rs +++ b/worker/src/pipeline_executor.rs @@ -21,6 +21,11 @@ use anyhow::{bail, Context, Result}; /// succeeds. Kept as a stable substring so `main` can match it on the error. pub const PLUGIN_AUTOLOAD_MARKER: &str = "[plugin-autoload-failed]"; +/// `prores_ks` declares `bits_per_mb` with this maximum. Above it ffmpeg +/// rejects the option and the encode dies having written nothing, so a value +/// arriving from a preset or an imported config is clamped rather than passed. +pub const PRORES_MAX_BITS_PER_MB: u32 = 8192; + /// True if a vspipe stderr line is VapourSynth's plugin-autoload-skip signature /// (e.g. "There is no attribute or namespace named fmtc. Did you mistype a /// plugin namespace or forget to install a plugin?"). @@ -1083,6 +1088,32 @@ impl PipelineExecutor { if let Some(profile) = settings.codec.prores_profile() { args.push("-profile:v".to_string()); args.push(profile.to_string()); + + // The three advanced ProRes options, each omitted when unset so the + // default command line is exactly what it was before they existed. + if settings.prores_vendor_apl0 { + args.extend(["-vendor".to_string(), "apl0".to_string()]); + } + if let Some(bits) = settings.prores_bits_per_mb { + // 0 is the plugin's own "use the profile default", which is + // what omitting the option already does — and the encoder + // rejects anything above 8192 outright, killing the job on a + // value the user cannot see. Clamped here rather than only in + // the UI, because a saved preset or an imported job config can + // carry either. Same reasoning as `normalized_preset`. + if bits > 0 { + args.extend([ + "-bits_per_mb".to_string(), + bits.min(PRORES_MAX_BITS_PER_MB).to_string(), + ]); + } + } + if let Some(mat) = settings.prores_quant_mat { + args.extend([ + "-quant_mat".to_string(), + mat.ffmpeg_name().to_string(), + ]); + } } else { match settings.codec.encoder_family() { EncoderFamily::Software => { @@ -1422,7 +1453,7 @@ impl Drop for PipelineExecutor { #[cfg(test)] mod tests { use super::*; - use crate::models::{AudioCodec, AudioQuality, ChromaSubsampling, EncodingSettings, QTGMCParameters, VideoCodec}; + use crate::models::{AudioCodec, AudioQuality, ChromaSubsampling, EncodingSettings, ProResQuantMat, QTGMCParameters, VideoCodec}; use uuid::Uuid; /// The two codes that have actually cost debugging time must be named in @@ -2151,6 +2182,89 @@ mod tests { } } + /// The three advanced ProRes options must be absent unless asked for, so a + /// job that does not use them produces exactly the command line it always + /// did — and must be clamped when they are, because ffmpeg rejects an + /// out-of-range `bits_per_mb` and kills the encode. + #[test] + fn test_prores_advanced_flags_are_emitted_only_when_set() { + let base = || { + let mut job = create_test_job("output.mov"); + job.encoding_settings.codec = VideoCodec::ProResHQ; + job.encoding_settings.container = ContainerFormat::Mov; + job + }; + + // Default: the profile, and none of the three. + let args = build_ffmpeg_args_for_test(&base()); + assert!(args.contains(&"-profile:v".to_string())); + for flag in ["-vendor", "-bits_per_mb", "-quant_mat"] { + assert!( + !args.contains(&flag.to_string()), + "{flag} must not appear unless set" + ); + } + + // Vendor tag. + let mut job = base(); + job.encoding_settings.prores_vendor_apl0 = true; + let args = build_ffmpeg_args_for_test(&job); + let i = args.iter().position(|a| a == "-vendor").expect("-vendor"); + assert_eq!(args[i + 1], "apl0"); + + // Bits per macroblock, in range. + let mut job = base(); + job.encoding_settings.prores_bits_per_mb = Some(8000); + let args = build_ffmpeg_args_for_test(&job); + let i = args.iter().position(|a| a == "-bits_per_mb").unwrap(); + assert_eq!(args[i + 1], "8000"); + + // Zero is the plugin's "use the profile default", which is what + // omitting the option already does — so emit nothing rather than a + // value that reads as a deliberate choice. + let mut job = base(); + job.encoding_settings.prores_bits_per_mb = Some(0); + let args = build_ffmpeg_args_for_test(&job); + assert!(!args.contains(&"-bits_per_mb".to_string())); + + // Out of range is clamped, not forwarded: ffmpeg would reject it and + // the whole encode would fail on an option the user cannot see. + let mut job = base(); + job.encoding_settings.prores_bits_per_mb = Some(99_999); + let args = build_ffmpeg_args_for_test(&job); + let i = args.iter().position(|a| a == "-bits_per_mb").unwrap(); + assert_eq!(args[i + 1], PRORES_MAX_BITS_PER_MB.to_string()); + + // Quantisation matrix. + let mut job = base(); + job.encoding_settings.prores_quant_mat = Some(ProResQuantMat::Hq); + let args = build_ffmpeg_args_for_test(&job); + let i = args.iter().position(|a| a == "-quant_mat").unwrap(); + assert_eq!(args[i + 1], "hq"); + } + + /// The options are ProRes-only. Emitting them beside another encoder would + /// be an ffmpeg error at best and a silently ignored option at worst. + #[test] + fn test_prores_advanced_flags_never_reach_another_encoder() { + for codec in [VideoCodec::H264, VideoCodec::H265Nvenc, VideoCodec::FFV1] { + let mut job = create_test_job("output.mkv"); + job.encoding_settings.codec = codec; + job.encoding_settings.container = ContainerFormat::Mkv; + job.encoding_settings.prores_vendor_apl0 = true; + job.encoding_settings.prores_bits_per_mb = Some(8000); + job.encoding_settings.prores_quant_mat = Some(ProResQuantMat::Hq); + + let args = build_ffmpeg_args_for_test(&job); + for flag in ["-vendor", "-bits_per_mb", "-quant_mat"] { + assert!( + !args.contains(&flag.to_string()), + "{codec:?} must not be given {flag}" + ); + } + } + } + /// The job log must not tell a ProRes user their encoder "cannot encode" /// a format it simply stores differently — and must say nothing at all when /// the pin costs them nothing, which is the common case. From 9feb360d1ee29412273debb334d29873cd9e3279 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 12 Sep 2026 03:17:34 +1000 Subject: [PATCH 6/7] docs: record the ProRes chroma pin and the 4:4:4 output format (#81) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CLAUDE.md | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ README.md | 4 +-- 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2d314fa..85f8a78 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1894,8 +1894,26 @@ does, and it decides bit depth as well as chroma: |---|---|---| | `original` (default) | source format, depth included | archival; a 10-bit source stays 10-bit | | `yuv420` | `vs.YUV420P8` | compatibility — the only one every player opens | +| `yuv420p10` | `vs.YUV420P10` | the only 10-bit layout NVENC/QSV/AMF encode (issue #74) | | `yuv422` | `vs.YUV422P8` | more chroma detail, 8-bit | | `yuv422p10` | `vs.YUV422P10` | normalize chroma without dropping to 8-bit | +| `yuv444p10` | `vs.YUV444P10` | full chroma — ProRes 4444, x264/x265; no GPU encoder | + +> **This table had gone stale, and so had three tests driven from the same +> hand-written lists** — `yuv420p10` was missing from all of them, so that +> option's script substitution, its serde name and its declared depth were +> checked nowhere. A skipped row looks exactly like a passing one. Both sides +> now sweep the enum: `ChromaSubsampling::ALL` (kept complete by a +> catch-all-free match plus a count assertion) and `ChromaSubsampling.values`. +> Add a format to the enum, not to a list. + +> **Verify the Y4M pipe can name a new format before wiring anything.** vspipe +> writes the header, so the question is what *it* emits — measured for 4:4:4, +> `C444p10`, which ffmpeg's demuxer accepts. Note ffmpeg's own Y4M **muxer** +> calls `yuv444p10le` "not an official yuv4mpegpipe pixel format" and says the +> same of the long-shipping `yuv422p10le`, which is the proof that the muxer's +> opinion is irrelevant here. A format the pipe cannot name is a hard job +> failure with no error — the `Turn90` 4:4:0 trap. Three things to keep in mind: @@ -2072,6 +2090,84 @@ having the last word. A deliberately distinct icon (`schema_outlined`, not a second `info_outline`), asserted, because two identical adjacent buttons read as one control repeated. +### ProRes: the profile decides the chroma, not ffmpeg (issue #81) + +Reported as "please add ProRes", when Proxy/LT/422/HQ had shipped for months. +Most of what was wrong was that the app said otherwise, so this is mostly a +correctness change with two profiles added on the end. + +> **ffmpeg's pixel-format negotiation never looks at `-profile:v`.** Measured +> against the bundled build: `prores_ks -profile:v 4` and `-profile:v 5` +> auto-select `yuv422p10le` from a `yuv420p` input, exactly as `-profile:v 2` +> does. 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. `VideoCodec::forced_pix_fmt` now decides from the +> profile (4/5 → `yuv444p10le`, 0-3 → `yuv422p10le`), joining the HuffYUV and +> AMF pins. Issue #74's lesson generalises: an encoder's declared format list +> is not a statement about what the output should be. +> +> Pinning 0-3 is a measured no-op, not an assumed one — `pal-sd-25.mov` at all +> four profiles gives identical `framemd5` with and without the flag, because +> `yuv422p10le` is the only 4:2:2 format the encoder has. +> +> **The decoder reports ProRes 4444 as 12-bit** (`yuv444p12le`) whatever 10-bit +> format the encoder was handed, so assert on the chroma part of the name. And +> `prores_ks` offers no 12-bit pixel format at all: **4444 XQ is 10-bit here**, +> whatever Apple's spec says. Don't write "12-bit" in any UI copy. + +> **ProRes was showing a CRF slider wired to nothing.** It is not `isLossless`, +> so the dialog rendered one labelled "High (CRF 18)" while +> `build_encoder_quality_args` took the `prores_profile()` branch and never read +> `settings.quality`. The decision is now `VideoCodec.hasQualityControl`, a +> getter rather than a widget condition, so it can be asserted across +> `VideoCodec.values` — a hand-written list only covers the codecs someone +> thought of, which is never the broken one. + +> **`prores_profile()` and `encoder_family()` lost their catch-all arms.** They +> dispatch two halves of one decision — the quality-args branch uses the first, +> its fallthrough the second — so a ProRes variant reaching the family but not +> the profile table would emit no `-profile:v` and encode as profile 2 while +> claiming otherwise. Adding the two new variants then produced four compile +> errors naming exactly the sites that mattered, which is the point. + +> **`proresCodecs` in `settings_dialog.dart` was the only place the ProRes UI +> group was enumerated.** A profile missing from it exists in the model and is +> unreachable on screen, silently. Derived from `isProRes` now. + +The three advanced options (`-vendor apl0`, `-bits_per_mb`, `-quant_mat`) are +behind advanced mode, ProRes-only, default off. Their copy carries measurements +rather than the linked guide's framing, because **at profile 3 the guide's four +flags produce bit-identical frames** — `quant_mat auto` already resolves to the +HQ matrix and the bitrate is already under the cap. They only do anything on +Proxy/LT (+3.6 dB for 2.8% size, +5.5 dB for 19%), and `-vendor apl0` is a +compatibility flag: four bytes per frame header, identical pixels. + +`bits_per_mb` is clamped to 8192 **in the worker**, not just the UI — ffmpeg +rejects anything above it and the encode dies having written nothing, so a saved +preset can otherwise fail a job on an option nobody can see. `quant_mat` is an +enum on both sides for the same reason ("Undefined constant" kills the encode). + +> **`copyWith` needed explicit clear flags, and `parameter_copy_with_test` could +> not have caught it.** That test globs `lib/models/*_parameters.dart`, so it had +> never seen `encoding_settings.dart` — now added, and confirmed live by dropping +> a field and watching it fail by name. The blast radius here is worse than in a +> pass model: **every** edit in the settings dialog goes through +> `updateEncodingSettings(settings.copyWith(...))`, so a forgotten field resets on +> the user's next click, not on a pass toggle. `x ?? this.x` can only set a +> nullable field, never clear it, so an unticked override would stick forever — +> `videoBitrateKbps` still has that defect and works around it in +> `_buildCodecRadio`. + +`proresChromaPinWarning` is a **sibling** of `hardwareEncoderChromaWarning`, not +an extension. The two make opposite claims — one says the hardware cannot encode +what you asked and something is lost; the other says the profile defines what is +stored, and 4444 pads 4:2:0 *up*, costing size rather than detail. The message +says "Nothing is lost" explicitly, asserted, because a warning that reads as a +quality problem pushes people off a profile doing exactly what they asked. Both +are second implementations of the worker's decision and both are pinned +case-for-case to it. Depth is deliberately not warned about alone: ProRes is +always 10-bit, so that would fire on most ProRes jobs and become wallpaper. + ## QTGMC Parameters Reference The most important parameters: diff --git a/README.md b/README.md index 77320cc..e38cbd0 100644 --- a/README.md +++ b/README.md @@ -81,12 +81,12 @@ GPU-accelerated deinterlacing (NNEDI3CL) needs your GPU's OpenCL driver installe | | | |---|---| -| **Video** | H.264, H.265, ProRes, FFV1 (lossless), with hardware encoding via VideoToolbox, NVENC, Quick Sync or AMF where available | +| **Video** | H.264, H.265, ProRes (Proxy, LT, 422, 422 HQ, 4444, 4444 XQ), FFV1 (lossless), with hardware encoding via VideoToolbox, NVENC, Quick Sync or AMF where available | | **Audio** | Passthrough, or re-encode to AAC, Opus or FLAC, or strip | | **Containers** | MKV, MP4, MOV, AVI | | **Reads** | `.dv` · `.mts` `.m2ts` (AVCHD) · `.vob` `.m2v` `.mpg` `.mpeg` · `.mxf` · `.avi` `.mov` `.mp4` `.mkv` `.ts` `.wmv` `.webm` `.flv`, plus DVD discs and `VIDEO_TS` folders | | **Source formats** | 4:1:1 (NTSC DV), 4:1:0, 4:2:0/4:2:2/4:4:4 up to 16-bit, RGB, grayscale | -| **Colour format out** | Match the source, or convert to 4:2:0 8-bit (plays everywhere), 4:2:2 8-bit, or 4:2:2 10-bit. Worth setting for a 10-bit source: matching it produces a 10-bit file that some players and browsers refuse to open | +| **Colour format out** | Match the source, or convert to 4:2:0 8-bit (plays everywhere), 4:2:0 10-bit (the only 10-bit layout GPU encoders take), 4:2:2 8-bit, 4:2:2 10-bit, or 4:4:4 10-bit (ProRes 4444 and the software encoders). Worth setting for a 10-bit source: matching it produces a 10-bit file that some players and browsers refuse to open | | **Aspect ratio** | Non-square pixels preserved through the pipeline, including through a resize; or square up anamorphic pixels, force a display aspect, or letterbox to a target size | ## The filter pipeline From fce22ca3b4825d88b229a2f64bbd32bb645d6f35 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 12 Sep 2026 11:04:38 +1000 Subject: [PATCH 7/7] chore(worker): silence the dead-code warning on ChromaSubsampling::ALL 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) --- worker/src/models/video_job.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/worker/src/models/video_job.rs b/worker/src/models/video_job.rs index 8357d87..05763bc 100644 --- a/worker/src/models/video_job.rs +++ b/worker/src/models/video_job.rs @@ -528,6 +528,12 @@ impl ChromaSubsampling { /// all. `every_chroma_subsampling_is_listed` keeps this honest from both /// ends: a match with no catch-all makes a new variant a compile error, and /// the length assertion then fails until it is added here too. + /// + /// Only tests read it, and `#[cfg(test)]` would not do — the integration + /// tests in `worker/tests/` are a separate crate linking this library + /// normally, so it has to exist in an ordinary build. `impl VideoCodec` + /// carries the same attribute across its whole block for the same reason. + #[allow(dead_code)] pub const ALL: &'static [ChromaSubsampling] = &[ ChromaSubsampling::Original, ChromaSubsampling::Yuv420,