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 diff --git a/app/lib/models/encoding_settings.dart b/app/lib/models/encoding_settings.dart index a151786..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), @@ -74,7 +95,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); @@ -121,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 @@ -146,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, @@ -203,6 +245,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. @@ -230,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, @@ -254,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/models/video_job.dart b/app/lib/models/video_job.dart index 1a24813..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'; } } @@ -258,6 +264,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/utils/pixel_format.dart b/app/lib/utils/pixel_format.dart index ea9a2ad..57df238 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'; } @@ -230,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 d5ef65d..292489e 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.', ), ]; @@ -502,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, @@ -810,10 +825,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 +839,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.', ), ), @@ -1018,10 +1020,25 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { ), ..._buildChromaBitDepthWarning(viewModel, settings), ..._buildChromaEncoderWarning(viewModel, settings), + ..._buildProresChromaWarning(viewModel, settings), ], ), ), + // 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 @@ -1137,6 +1154,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.) @@ -1166,10 +1199,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, ]; @@ -1240,6 +1275,153 @@ 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. + 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'); + } + }); + }); +} 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/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/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/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/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 036c97a..05763bc 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 { @@ -445,9 +508,41 @@ 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. + /// + /// 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, + 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 +552,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 +571,7 @@ impl ChromaSubsampling { ChromaSubsampling::Yuv420P10 => Some("yuv420p10le"), ChromaSubsampling::Yuv422 => Some("yuv422p"), ChromaSubsampling::Yuv422P10 => Some("yuv422p10le"), + ChromaSubsampling::Yuv444P10 => Some("yuv444p10le"), } } } @@ -493,6 +590,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, } } } @@ -557,6 +657,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. @@ -593,17 +699,64 @@ impl VideoCodec { VideoCodec::ProResLT => "prores_ks", VideoCodec::ProRes422 => "prores_ks", VideoCodec::ProResHQ => "prores_ks", + VideoCodec::ProRes4444 => "prores_ks", + VideoCodec::ProRes4444Xq => "prores_ks", } } + /// 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, + VideoCodec::ProRes4444, + VideoCodec::ProRes4444Xq, + ]; + /// 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::ProRes4444 => Some(4), + VideoCodec::ProRes4444Xq => Some(5), + 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, } } @@ -646,6 +799,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. @@ -769,7 +944,15 @@ 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 + | VideoCodec::ProRes4444 + | VideoCodec::ProRes4444Xq => EncoderFamily::ProRes, } } @@ -804,6 +987,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", } } } @@ -892,6 +1077,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\"" @@ -1021,10 +1214,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 [ @@ -1032,7 +1231,6 @@ mod tests { VideoCodec::H265, VideoCodec::H264Videotoolbox, VideoCodec::H265Videotoolbox, - VideoCodec::ProRes422, VideoCodec::FFV1, VideoCodec::Ffvhuff, ] { @@ -1046,18 +1244,126 @@ 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" + ); + } + 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}" + ); + } + } + + /// 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(), + 19, + "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. #[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 +1372,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/src/pipeline_executor.rs b/worker/src/pipeline_executor.rs index ffb54cd..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?"). @@ -871,19 +876,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 +1032,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; @@ -1042,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 => { @@ -1381,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 @@ -2084,6 +2156,164 @@ 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 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. + #[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 { 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.