diff --git a/CLAUDE.md b/CLAUDE.md index 400ca92..ed5a833 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1657,8 +1657,89 @@ value here shows up as "this GPU doesn't work" rather than as an error: (a later `-pix_fmt` wins), which is the escape hatch for 10-bit HEVC on hardware that supports it. +- **A hardware encoder's declared pix_fmt list is not a statement about the + machine (issue #74).** The list ffmpeg negotiates against is compiled in; + NVENC's real capabilities are queried from the driver at `avcodec_open2`. A + recent ffmpeg built against NVENC SDK 13 advertises `yuv422p` on + `h264_nvenc` for Blackwell's 4:2:2 support, so negotiation picks it for any + 4:2:2 source — and every pre-Blackwell card then fails the job outright with + *"YUV422P not supported / No capable devices found"*, zero frames written. + Reported on an RTX 4070 Super with a CineForm `yuv422p10le` capture at the + default "Match source" colour format. **ffmpeg cannot negotiate its way out + of this**, so `forced_pix_fmt` picks the format instead. + +`VideoCodec::forced_pix_fmt` therefore takes the format the pipeline will +actually hand the encoder — `VideoJob::encoder_input_pix_fmt`, which is the +output conversion when one is selected and the pipe format otherwise. Three +things about it are load-bearing: + +- **The NVENC/QSV arm is conditional and the other two are not.** HuffYUV and + AMF take one format whatever the source was; NVENC does not. Pinning it + unconditionally the way AMF is pinned would flatten a 10-bit 4:2:0 source to + 8-bit for everyone it already serves correctly, so the guard fires only on + 4:2:2, 4:4:4, or >8-bit into an H.264 encoder (neither family has a 10-bit + H.264 mode). A 4:2:0 job emits no `-pix_fmt` at all, exactly as before. +- **HEVC keeps the depth, H.264 cannot.** 4:2:2 10-bit into `hevc_nvenc` + becomes `p010le`, not `yuv420p` — only the chroma has to go. NVENC gets the + planar `yuv420p` and QSV the semi-planar `nv12`, each family's native name. +- **VideoToolbox is deliberately excluded.** It never advertises a mode it + lacks, so its negotiation is trustworthy and forcing a format would only + throw away chroma it could have kept. QSV *is* included, preventively rather + than on a report: `hevc_qsv` advertises `y210le` on builds whose hardware may + not have it, which is the same trap. + None of the AMF behaviour can be verified in CI or on macOS — there is no AMD -hardware in the matrix — so changes here rest on reporter confirmation. +hardware in the matrix — and the same is true of NVENC and QSV, so these rest +on unit tests over the emitted arguments plus reporter confirmation. Note the +functional probe in `HardwareEncoderDetector` cannot catch the #74 class at +all: it encodes one `yuv420p` frame from lavfi, so it correctly reports NVENC +as *available* — the device works, only the format doesn't. Don't try to fix a +format problem in the device probe; the format isn't known until a file loads. + +### The user-facing half of #74 + +The guard above keeps the job running, but silently changing someone's output +is only acceptable if they can see it coming and choose otherwise. Two things +shipped with it: + +**A 4:2:0 10-bit output format.** `ChromaSubsampling::Yuv420P10` / +`ChromaSubsampling.yuv420p10` — the only 10-bit layout NVENC, QSV and AMF can +encode, so it is how a 10-bit source keeps its grading through a GPU encoder by +the user's own choice rather than by the guard's fallback. Verified end to end +(`integration_high_bit_depth_filters_test`): a 10-bit 4:2:2 source comes out +`yuv420p10le`, profile **High 10**. Adding an option touches four places — +`vapoursynth_format`, `ffmpeg_pix_fmt`, the Dart enum with its `outputBitDepth`, +and `chromaFormatHelpSections`, which `settings_chroma_help_test` fails on if +the new label goes unmentioned. + +**A warning under the dropdown.** `hardwareEncoderChromaWarning` in +`app/lib/utils/pixel_format.dart` says which format will be substituted and +why, before the job runs. It covers both routes into #74 — a 4:2:2 *source* at +"Match source", and an explicitly chosen 4:2:2 *output* — and it stays silent +for everything encodable, for VideoToolbox and AMF, and until a file is loaded. + +> **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 +> therefore pinned to **the same table of cases** — `substitutions match the +> worker, case for case` in `hardware_encoder_chroma_warning_test.dart` against +> `test_nvenc_cannot_be_handed_422` and its neighbours in `video_job.rs`. Change +> one and change both. `pixelFormatChromaLayout` is likewise a coarse Dart twin +> of `ChromaClass`; the Dart side already reimplements this kind of pix_fmt +> parsing in `pixelFormatBitDepth`, so it follows that precedent rather than +> inventing a new one. + +**A second help dialog**, `ColourPipelineHelpIcon` / +`colourPipelineHelpSections`, beside the existing `ChromaFormatHelpIcon`. The +two answer different questions and both are worth having: the first is *what +are these formats and which do I pick*, the second is *what does the app do to +my colour* — the pipe source normalising upward on the way in, filters +converting down and back per pass, every UI threshold being in 8-bit units and +rescaled to the clip depth, the output conversion dithering, the Y4M pipe +stripping SAR and colour tags so they must be re-stamped, and the encoder +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. ## QTGMC Parameters Reference diff --git a/app/lib/models/encoding_settings.dart b/app/lib/models/encoding_settings.dart index 6895bb3..a151786 100644 --- a/app/lib/models/encoding_settings.dart +++ b/app/lib/models/encoding_settings.dart @@ -66,6 +66,10 @@ enum ChromaSubsampling { original('original', 'Match source', null, null), /// Convert to 8-bit YUV420 for maximum compatibility (smaller files). yuv420('yuv420', '4:2:0 8-bit', 'most compatible', 8), + /// Convert to 10-bit YUV420. The only 10-bit layout NVENC, QSV and AMF can + /// encode, so it is the way to keep a 10-bit source's grading on a GPU + /// encoder — 4:2:2 fails outright on most of them (issue #74). + yuv420p10('yuv420p10', '4:2:0 10-bit', 'best 10-bit for GPU encoders', 10), /// Convert to 8-bit YUV422 for higher chroma quality. yuv422('yuv422', '4:2:2 8-bit', 'more colour detail', 8), /// Convert to 10-bit YUV422: keeps a 10-bit source's precision while diff --git a/app/lib/utils/pixel_format.dart b/app/lib/utils/pixel_format.dart index 1f1eeb5..ea9a2ad 100644 --- a/app/lib/utils/pixel_format.dart +++ b/app/lib/utils/pixel_format.dart @@ -1,6 +1,9 @@ // Utilities for interpreting FFmpeg pixel-format strings (e.g. the `pix_fmt` // reported by ffprobe: "yuv420p", "yuv422p10le", "yuv420p16le"). +import '../models/encoding_settings.dart'; +import '../models/video_job.dart'; + /// Best-effort per-component bit depth for an FFmpeg pixel-format string. /// /// Returns 8 for the common 8-bit formats (yuv420p, yuv422p, nv12, rgb24, …), @@ -71,3 +74,159 @@ String? chromaConversionBitDepthWarning({ 'colour format. Choose "Match source" to keep the source\'s bit depth' '${targetBitDepth < 10 ? ', or 4:2:2 10-bit to keep more of it' : ''}.'; } + +/// Chroma layout of an FFmpeg pixel-format string, coarse enough for the one +/// question the UI asks of it: can a hardware encoder take this? +/// +/// Mirrors `ChromaClass` in `worker/src/pixel_format.rs`, which is the authority +/// — it decides what the worker actually does. This copy only decides what the +/// warning *says*, and `hardware_encoder_chroma_warning_test.dart` pins the two +/// to the same table so they cannot drift into disagreeing on screen. +enum ChromaLayout { + /// 4:2:0 — one colour sample per 2x2 block. + c420, + + /// 4:2:2 and anything hardware treats like it (4:4:0, 4:1:1, 4:1:0). + c422, + + /// 4:4:4, and RGB, which carries full colour resolution by construction. + c444, +} + +/// Best-effort chroma layout for an FFmpeg pixel-format string. +/// +/// Unknown and null formats fall back to [ChromaLayout.c420] — the conservative +/// choice here, because 4:2:0 is what every encoder accepts, so an unrecognized +/// format produces no spurious warning. (`pixelFormatBitDepth` falls back the +/// same way and for the same reason.) +ChromaLayout pixelFormatChromaLayout(String? pixFmt) { + if (pixFmt == null || pixFmt.trim().isEmpty) return ChromaLayout.c420; + final f = pixFmt.trim().toLowerCase(); + + // Planar YUV: the three digits after the family prefix are the subsampling. + final planar = RegExp(r'^yuv[aj]?(\d{3})').firstMatch(f); + if (planar != null) { + switch (planar.group(1)!) { + case '420': + return ChromaLayout.c420; + // 4:1:0 and 4:1:1 subsample more coarsely than 4:2:2 horizontally and + // 4:4:0 more coarsely vertically, but none of them is 4:2:0, and no + // hardware encoder takes any of them — so they warn alongside 4:2:2. + case '410': + case '411': + case '422': + case '440': + return ChromaLayout.c422; + default: + return ChromaLayout.c444; + } + } + + // Semi-planar: nv12/nv21 are 4:2:0, nv16 4:2:2, nv24/nv42 4:4:4; p010/p016 + // are 4:2:0, p210/p216 4:2:2, p410/p416 4:4:4 (the middle digit is the + // subsampling, as in `pixelFormatBitDepth`). + const semiPlanar = { + 'nv12': ChromaLayout.c420, + 'nv21': ChromaLayout.c420, + 'nv16': ChromaLayout.c422, + 'nv24': ChromaLayout.c444, + 'nv42': ChromaLayout.c444, + }; + final stem = f.replaceFirst(RegExp(r'(le|be)$'), ''); + final named = semiPlanar[stem]; + if (named != null) return named; + final p = RegExp(r'^p(\d)(?:10|12|16)$').firstMatch(stem); + if (p != null) { + switch (p.group(1)!) { + case '0': + return ChromaLayout.c420; + case '2': + return ChromaLayout.c422; + default: + return ChromaLayout.c444; + } + } + + // Gray has no chroma at all, so every encoder can hold it. + if (stem.startsWith('gray') || stem.startsWith('ya')) return ChromaLayout.c420; + + // RGB and planar GBR carry full colour resolution. + if (RegExp(r'^(a?rgb|a?bgr|gbra?p)').hasMatch(stem)) return ChromaLayout.c444; + + return ChromaLayout.c420; +} + +/// Warning message when the chosen output colour format is one the selected +/// hardware encoder cannot take, and the worker will therefore substitute +/// another — or null when there is nothing to say. +/// +/// This is the UI half of issue #74. An encoder's declared format list is +/// compiled into ffmpeg, but NVENC's real capabilities are queried from the +/// driver at open time: a recent ffmpeg advertises `yuv422p` on `h264_nvenc` +/// for Blackwell's 4:2:2 support, and every earlier card then failed the whole +/// job. The worker now substitutes an encodable format instead of failing, so +/// this exists to say so *before* the job runs rather than only in its log. +/// +/// **Keep in step with `VideoCodec::forced_pix_fmt` in +/// `worker/src/models/video_job.rs`**, which is what actually happens. The two +/// share a table of cases in their tests. +/// +/// [chromaSubsampling] decides the format outright unless it is +/// [ChromaSubsampling.original], in which case the source's [pixelFormat] does. +String? hardwareEncoderChromaWarning({ + required VideoCodec codec, + required ChromaSubsampling chromaSubsampling, + String? pixelFormat, +}) { + // Only NVENC and QSV advertise formats their hardware may not have. + // VideoToolbox never does, so its negotiation is trustworthy; AMF is pinned + // to nv12 unconditionally and has been since issue #51, so it converts + // whatever it is given and there is no surprise to warn about. + final isNvenc = codec.isNvenc; + final isQsv = codec == VideoCodec.h264Qsv || codec == VideoCodec.h265Qsv; + if (!isNvenc && !isQsv) return null; + + // What the encoder will actually be handed: the output conversion when one is + // selected, the source's own format otherwise. + final ChromaLayout layout; + final int depth; + final String describedAs; + switch (chromaSubsampling) { + case ChromaSubsampling.original: + // Nothing to warn about until a file is loaded and we know its format. + if (pixelFormat == null) return null; + layout = pixelFormatChromaLayout(pixelFormat); + 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; + depth = chromaSubsampling.outputBitDepth ?? 8; + describedAs = '${chromaSubsampling.label} is selected'; + } + + final chromaUnsupported = layout != ChromaLayout.c420; + // Neither family has a 10-bit H.264 mode at all. + final depthUnsupported = depth > 8 && codec.isH264; + if (!chromaUnsupported && !depthUnsupported) return null; + + // Mirrors forced_pix_fmt: H.264 has only 8-bit 4:2:0; HEVC keeps the depth. + final substitute = codec.isH264 + ? '4:2:0 8-bit' + : (depth > 8 ? '4:2:0 10-bit' : '4:2:0 8-bit'); + + final reason = chromaUnsupported + ? '${codec.encoderFamily} cannot encode ${layout == ChromaLayout.c444 ? "4:4:4" : "4:2:2"} ' + 'on most GPUs' + : '${codec.encoderFamily} has no 10-bit H.264 mode'; + + final advice = codec.isH264 && depth > 8 + ? ' Choose an H.265 encoder to keep the 10-bit grading, or a software ' + 'encoder to keep the colour detail as well.' + : ' Choose a software encoder to keep it as it is.'; + + return '$describedAs, but $reason, so the output will be converted to ' + '$substitute.$advice'; +} diff --git a/app/lib/views/settings/settings_dialog.dart b/app/lib/views/settings/settings_dialog.dart index 28fd377..d5ef65d 100644 --- a/app/lib/views/settings/settings_dialog.dart +++ b/app/lib/views/settings/settings_dialog.dart @@ -55,6 +55,10 @@ const List<(String, String)> chromaFormatHelpSections = [ 'refuse to open.\n\n' '4:2:0 8-bit — plays on everything. Use it for anything going to ' "someone else's device, the web, or a TV.\n\n" + '4:2:0 10-bit — the same universal colour layout with the finer ' + 'grading. This is the one to pick for a 10-bit source on a GPU ' + 'encoder: NVENC, QSV and AMF can all encode it, and none of them can ' + 'encode 4:2:2 on most cards.\n\n' '4:2:2 8-bit — keeps the extra colour detail of a 4:2:2 or ' 'analogue-captured source, at 8-bit precision.\n\n' '4:2:2 10-bit — keeps the colour detail and the 10-bit grading. Best ' @@ -144,6 +148,148 @@ Future showChromaFormatHelp(BuildContext context) { ); } +/// How the colour of a frame is carried from the source file to the output — +/// the mechanics behind the dropdown above, for anyone who wants to know why +/// their file came out the way it did. +/// +/// Everything here is a real property of the pipeline, not a simplification. +/// If any of it changes, this text is wrong and +/// `settings_colour_pipeline_help_test.dart` cannot tell — it only checks the +/// sections are present and well-formed. +const List<(String, String)> colourPipelineHelpSections = [ + ( + 'Reading the source', + 'VapourBox does not open your file with a VapourSynth source filter. FFmpeg ' + 'decodes it and pipes raw planar frames in, which is what makes seeking ' + 'frame-accurate.\n\n' + 'The pipe can carry most planar YUV formats directly, so a 10-bit 4:2:2 ' + 'ProRes or CineForm arrives at full precision. A format it cannot carry ' + '(RGB, semi-planar, alpha, 4:1:1) is converted on the way in — always ' + 'upward, never to less chroma resolution or fewer bits, so reading a ' + 'file can never be the thing that degrades it.', + ), + ( + 'Processing', + 'Filters run in whatever format the frame arrived in. Where a plugin cannot ' + 'handle the depth — a few are 8-bit only, one caps at 10 — the pipeline ' + 'converts down for that pass alone and restores the format immediately ' + 'after, rather than dropping the whole clip to 8-bit.\n\n' + 'Every threshold, level and offset in the interface is expressed in ' + '8-bit terms (0–255), because that is the vocabulary the filters were ' + 'documented in. Those values are rescaled to the actual depth inside the ' + 'script, so a brightness or levels adjustment does the same thing to a ' + '10-bit source as to an 8-bit one.', + ), + ( + 'Converting for output', + 'The colour format you choose above is applied last, after every filter, so ' + 'the whole graph runs at the source precision even when the output is ' + '8-bit.\n\n' + 'Reducing depth dithers rather than rounds. Plain rounding turns a ' + 'shallow gradient — a sky, a fade, a VHS luma ramp — into visible bands; ' + 'error diffusion keeps it smooth.', + ), + ( + 'What the pipe loses, and what puts it back', + 'The Y4M pipe between VapourSynth and the encoder carries pixels and ' + 'nothing else. It strips the pixel aspect ratio and every colour tag — ' + 'matrix, primaries, transfer, range — so anything VapourSynth knew about ' + 'them is gone by the time the encoder sees the frames.\n\n' + 'They are therefore re-declared on the encoder from the values read out ' + 'of your source. Without that the output would be untagged, and an ' + 'untagged file is read as BT.601 limited by every player: a BT.709 or ' + 'full-range source would come out subtly wrong in colour with nothing to ' + 'indicate why. Tags are carried through, never invented — a source that ' + 'declares nothing stays undeclared.', + ), + ( + 'The encoder has the last word', + 'A hardware encoder advertises the formats its driver was built to support, ' + 'not the ones the card in your machine actually has. NVENC lists 4:2:2 ' + 'because the newest NVIDIA cards can do it; every earlier card rejects ' + 'it when the encode starts.\n\n' + 'So the format is chosen for hardware encoders rather than negotiated: ' + 'anything they cannot take is converted to 4:2:0 first, keeping 10-bit ' + 'precision where the codec allows it (H.265 does, H.264 has no 10-bit ' + 'mode at all). The job log names the substitution whenever one happens. ' + 'A later -pix_fmt in Custom FFmpeg Arguments overrides it, which is the ' + 'way to use a mode your card really does have.', + ), +]; + +/// The second help affordance beside the colour format dropdown: the same +/// click-to-open dialog pattern as [ChromaFormatHelpIcon], but answering a +/// different question. That one explains what the formats *are* and which to +/// choose; this one explains what the application *does* with them. +/// +/// A distinct icon rather than a second `info_outline`, so two adjacent buttons +/// don't look like the same thing twice. +class ColourPipelineHelpIcon extends StatelessWidget { + const ColourPipelineHelpIcon({super.key}); + + @override + Widget build(BuildContext context) { + return IconButton( + icon: const Icon( + Icons.schema_outlined, + size: 20, + semanticLabel: 'How VapourBox handles colour', + ), + color: Theme.of(context).colorScheme.primary, + visualDensity: VisualDensity.compact, + padding: const EdgeInsets.only(left: 4), + constraints: const BoxConstraints(), + onPressed: () => showColourPipelineHelp(context), + ); + } +} + +/// Open the colour pipeline explanation. Same shape as [showChromaFormatHelp] — +/// fixed width, scrollable, height-capped by the dialog itself. +Future showColourPipelineHelp(BuildContext context) { + return showDialog( + context: context, + builder: (ctx) { + final theme = Theme.of(ctx); + return AlertDialog( + title: const Text('How VapourBox handles colour'), + content: SizedBox( + width: 520, + child: SingleChildScrollView( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text( + 'What happens to a frame between your source file and the ' + 'finished output.', + style: theme.textTheme.bodyMedium, + ), + for (final (heading, body) in colourPipelineHelpSections) ...[ + const SizedBox(height: 20), + Text( + heading, + style: theme.textTheme.titleSmall + ?.copyWith(fontWeight: FontWeight.bold), + ), + const SizedBox(height: 6), + Text(body, + style: theme.textTheme.bodyMedium?.copyWith(height: 1.4)), + ], + ], + ), + ), + ), + actions: [ + TextButton( + onPressed: () => Navigator.of(ctx).pop(), + child: const Text('Close'), + ), + ], + ); + }, + ); +} + class SettingsDialog extends StatefulWidget { const SettingsDialog({super.key}); @@ -850,6 +996,10 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { // and which option to reach for. The short line below the // dropdown is the summary; this is the explanation. const ChromaFormatHelpIcon(), + // A second dialog answering the other question people + // ask here: not "which do I pick" but "what is the app + // actually doing to my colour". + const ColourPipelineHelpIcon(), ], ), const SizedBox(height: 8), @@ -867,6 +1017,7 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { ), ), ..._buildChromaBitDepthWarning(viewModel, settings), + ..._buildChromaEncoderWarning(viewModel, settings), ], ), ), @@ -970,6 +1121,22 @@ class _OutputSettingsTabState extends State<_OutputSettingsTab> { return [const SizedBox(height: 12), WarningBanner(message: message)]; } + /// Issue #74: warn when the selected colour format is one the selected + /// hardware encoder cannot take, so the substitution the worker performs is + /// visible before the job runs rather than only in its log. + List _buildChromaEncoderWarning( + MainViewModel viewModel, + EncodingSettings settings, + ) { + final message = hardwareEncoderChromaWarning( + 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.) diff --git a/app/test/hardware_encoder_chroma_warning_test.dart b/app/test/hardware_encoder_chroma_warning_test.dart new file mode 100644 index 0000000..b109774 --- /dev/null +++ b/app/test/hardware_encoder_chroma_warning_test.dart @@ -0,0 +1,189 @@ +// The UI half of issue #74. +// +// A hardware encoder advertises the formats its driver was *built* against, not +// the ones the card actually has: a recent ffmpeg lists `yuv422p` on +// `h264_nvenc` for Blackwell, and every earlier NVIDIA card then failed the +// whole job at encoder open. The worker substitutes an encodable format instead +// of failing; this warning is what tells the user before the job runs. +// +// The substitution table below is the same one asserted in Rust by +// `test_nvenc_cannot_be_handed_422` and its neighbours in +// `worker/src/models/video_job.rs`. If the two disagree, the interface promises +// one thing and the encode does another — which is worse than either being +// wrong on its own, so both sides are pinned to the same cases. + +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() { + group('pixelFormatChromaLayout', () { + test('reads planar YUV subsampling', () { + expect(pixelFormatChromaLayout('yuv420p'), ChromaLayout.c420); + expect(pixelFormatChromaLayout('yuv420p10le'), ChromaLayout.c420); + expect(pixelFormatChromaLayout('yuv422p'), ChromaLayout.c422); + expect(pixelFormatChromaLayout('yuv422p10le'), ChromaLayout.c422); + expect(pixelFormatChromaLayout('yuv444p12le'), ChromaLayout.c444); + expect(pixelFormatChromaLayout('yuvj420p'), ChromaLayout.c420); + expect(pixelFormatChromaLayout('yuva444p10le'), ChromaLayout.c444); + }); + + test('the coarse layouts warn alongside 4:2:2', () { + // No hardware encoder takes any of these either, and calling them 4:2:2 + // in a warning is close enough to be useful and never wrong about the + // conclusion. + expect(pixelFormatChromaLayout('yuv411p'), ChromaLayout.c422); + expect(pixelFormatChromaLayout('yuv410p'), ChromaLayout.c422); + expect(pixelFormatChromaLayout('yuv440p'), ChromaLayout.c422); + }); + + test('reads semi-planar and packed formats', () { + expect(pixelFormatChromaLayout('nv12'), ChromaLayout.c420); + expect(pixelFormatChromaLayout('nv16'), ChromaLayout.c422); + expect(pixelFormatChromaLayout('nv24'), ChromaLayout.c444); + expect(pixelFormatChromaLayout('p010le'), ChromaLayout.c420); + expect(pixelFormatChromaLayout('p210le'), ChromaLayout.c422); + expect(pixelFormatChromaLayout('p410le'), ChromaLayout.c444); + expect(pixelFormatChromaLayout('rgb24'), ChromaLayout.c444); + expect(pixelFormatChromaLayout('gbrp'), ChromaLayout.c444); + }); + + test('unknown and gray formats fall back to the encodable layout', () { + // The fallback must never invent a warning: 4:2:0 is what every encoder + // takes, so an unrecognised name stays silent. + expect(pixelFormatChromaLayout(null), ChromaLayout.c420); + expect(pixelFormatChromaLayout(''), ChromaLayout.c420); + expect(pixelFormatChromaLayout('something_new'), ChromaLayout.c420); + expect(pixelFormatChromaLayout('gray10le'), ChromaLayout.c420); + }); + }); + + group('hardwareEncoderChromaWarning', () { + String? warn(VideoCodec codec, String? pixFmt, + [ChromaSubsampling cs = ChromaSubsampling.original]) => + hardwareEncoderChromaWarning( + codec: codec, + chromaSubsampling: cs, + pixelFormat: pixFmt, + ); + + test('the reported case warns and names the substitute', () { + // CineForm yuv422p10le -> h264_nvenc at "Match source", on an RTX 4070 + // Super. This produced zero frames and "No capable devices found". + final msg = warn(VideoCodec.h264Nvenc, 'yuv422p10le'); + expect(msg, isNotNull); + expect(msg, contains('yuv422p10le')); + expect(msg, contains('4:2:0 8-bit')); + expect(msg, contains('NVIDIA NVENC')); + }); + + test('substitutions match the worker, case for case', () { + // MIRRORS the Rust table in worker/src/models/video_job.rs. The right + // column is the format `forced_pix_fmt` returns, spelled the way this + // warning says it. + const cases = <(VideoCodec, String, String?)>[ + // 4:2:2 in — H.264 loses the depth too, H.265 keeps it. + (VideoCodec.h264Nvenc, 'yuv422p10le', '4:2:0 8-bit'), // -> yuv420p + (VideoCodec.h265Nvenc, 'yuv422p10le', '4:2:0 10-bit'), // -> p010le + (VideoCodec.h265Nvenc, 'yuv422p', '4:2:0 8-bit'), // -> yuv420p + (VideoCodec.h264Nvenc, 'yuv444p', '4:2:0 8-bit'), // -> yuv420p + // 10-bit 4:2:0 in — only H.264 has a problem with it. + (VideoCodec.h264Nvenc, 'yuv420p10le', '4:2:0 8-bit'), // -> yuv420p + (VideoCodec.h265Nvenc, 'yuv420p10le', null), // -> None + // 8-bit 4:2:0 in — nothing to do on either. + (VideoCodec.h264Nvenc, 'yuv420p', null), // -> None + (VideoCodec.h265Nvenc, 'yuv420p', null), // -> None + // QSV takes the same decisions (nv12/p010le on the worker side). + (VideoCodec.h264Qsv, 'yuv422p', '4:2:0 8-bit'), // -> nv12 + (VideoCodec.h265Qsv, 'yuv422p10le', '4:2:0 10-bit'), // -> p010le + (VideoCodec.h264Qsv, 'yuv420p', null), // -> None + ]; + + for (final (codec, pixFmt, expected) in cases) { + final msg = warn(codec, pixFmt); + if (expected == null) { + expect(msg, isNull, + reason: '$pixFmt into ${codec.value} is encodable as-is'); + } else { + expect(msg, isNotNull, + reason: '$pixFmt into ${codec.value} must warn'); + expect(msg, contains(expected), + reason: '$pixFmt into ${codec.value} converts to $expected'); + } + } + }); + + test('encoders that negotiate correctly are never warned about', () { + // VideoToolbox never advertises a mode it lacks. AMF has been pinned to + // nv12 unconditionally since issue #51, so it always converts and there + // is no surprise. Software and ProRes take 4:2:2 natively. + for (final codec in [ + VideoCodec.h264, + VideoCodec.h265, + VideoCodec.h264Videotoolbox, + VideoCodec.h265Videotoolbox, + VideoCodec.h264Amf, + VideoCodec.h265Amf, + VideoCodec.prores422, + VideoCodec.ffv1, + ]) { + expect(warn(codec, 'yuv422p10le'), isNull, + reason: '${codec.value} should not warn'); + } + }); + + test('an explicit 4:2:2 choice warns whatever the source was', () { + // The conversion is done by the pipeline, so a 4:2:0 source reaches the + // encoder as 4:2:2 all the same — this is the second route into #74. + final msg = + warn(VideoCodec.h265Nvenc, 'yuv420p', ChromaSubsampling.yuv422p10); + expect(msg, isNotNull); + expect(msg, contains('4:2:2 10-bit')); + expect(msg, contains('4:2:0 10-bit')); + }); + + test('the new 4:2:0 10-bit option is the way out, and is silent', () { + // The whole point of adding it: a 10-bit source keeping its grading on a + // GPU encoder, with nothing to warn about. + expect( + warn(VideoCodec.h265Nvenc, 'yuv422p10le', + ChromaSubsampling.yuv420p10), + isNull); + // H.264 still cannot do 10-bit, so it still warns. + expect( + warn(VideoCodec.h264Nvenc, 'yuv422p10le', + ChromaSubsampling.yuv420p10), + contains('4:2:0 8-bit')); + }); + + test('4:2:0 8-bit is always safe', () { + for (final codec in [ + VideoCodec.h264Nvenc, + VideoCodec.h265Nvenc, + VideoCodec.h264Qsv, + VideoCodec.h265Qsv, + ]) { + expect(warn(codec, 'yuv422p10le', ChromaSubsampling.yuv420), isNull); + } + }); + + test('silent before a file is loaded', () { + // "Match source" cannot be judged without knowing the source. + expect(warn(VideoCodec.h264Nvenc, null), isNull); + // But an explicit choice can be, and still warns. + expect( + warn(VideoCodec.h264Nvenc, null, ChromaSubsampling.yuv422), isNotNull); + }); + + test('H.264 is told which way out actually helps', () { + // Switching to H.265 keeps the depth; only software keeps the chroma. + final msg = warn(VideoCodec.h264Nvenc, 'yuv422p10le'); + expect(msg, contains('H.265')); + final chromaOnly = warn(VideoCodec.h265Nvenc, 'yuv422p'); + expect(chromaOnly, contains('software')); + expect(chromaOnly, isNot(contains('H.265'))); + }); + }); +} diff --git a/app/test/integration_high_bit_depth_filters_test.dart b/app/test/integration_high_bit_depth_filters_test.dart index 53da129..fab2fc5 100644 --- a/app/test/integration_high_bit_depth_filters_test.dart +++ b/app/test/integration_high_bit_depth_filters_test.dart @@ -902,6 +902,17 @@ void main() { expect(out['profile'], 'High'); }, timeout: const Timeout(Duration(minutes: 5))); + test('4:2:0 10-bit keeps the precision and drops only the chroma', () async { + // The option added for issue #74: NVENC, QSV and AMF can all encode + // 10-bit 4:2:0 and none of them can encode 4:2:2 on most hardware, so + // this is how a 10-bit source keeps its grading through a GPU encoder. + // High 10 rather than High, and 4:2:0 rather than 4:2:2 — getting either + // half wrong would defeat the purpose. + final out = await encodeWith(ChromaSubsampling.yuv420p10, 'yuv420p10'); + expect(out['pix_fmt'], 'yuv420p10le'); + expect(out['profile'], 'High 10'); + }, timeout: const Timeout(Duration(minutes: 5))); + test('4:2:2 gives an 8-bit 4:2:2 file', () async { final out = await encodeWith(ChromaSubsampling.yuv422, 'yuv422'); expect(out['pix_fmt'], 'yuv422p'); diff --git a/app/test/pixel_format_test.dart b/app/test/pixel_format_test.dart index 23f9481..e3f21e4 100644 --- a/app/test/pixel_format_test.dart +++ b/app/test/pixel_format_test.dart @@ -158,6 +158,7 @@ void main() { // 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); }); @@ -168,6 +169,7 @@ void main() { // the wire format. A mismatch makes the worker reject the job config. expect(ChromaSubsampling.original.value, 'original'); expect(ChromaSubsampling.yuv420.value, 'yuv420'); + expect(ChromaSubsampling.yuv420p10.value, 'yuv420p10'); expect(ChromaSubsampling.yuv422.value, 'yuv422'); expect(ChromaSubsampling.yuv422p10.value, 'yuv422p10'); }); diff --git a/app/test/settings_colour_pipeline_help_test.dart b/app/test/settings_colour_pipeline_help_test.dart new file mode 100644 index 0000000..09aff23 --- /dev/null +++ b/app/test/settings_colour_pipeline_help_test.dart @@ -0,0 +1,124 @@ +// The second colour help dialog: not "which format do I pick" (that is +// settings_chroma_help_test.dart) but "what does VapourBox actually do to my +// colour". Added for people who want to reason about the output rather than +// just choose from a list. +// +// The same two failures the first dialog already hit are pinned here, because +// nothing about them was specific to that one: prose in a const drifts away +// from the code it describes, and a multi-paragraph explanation that cannot +// scroll runs off the bottom of a small window with no way to read the rest. + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import 'package:vapourbox/views/settings/settings_dialog.dart'; + +String get _helpText => + colourPipelineHelpSections.map((s) => '${s.$1}\n${s.$2}').join('\n\n'); + +Widget _harness(Brightness brightness) => MaterialApp( + theme: ThemeData( + colorScheme: + ColorScheme.fromSeed(seedColor: Colors.blue, brightness: brightness), + useMaterial3: true, + ), + home: const Scaffold(body: Center(child: ColourPipelineHelpIcon())), + ); + +void main() { + group('the pipeline explanation', () { + test('covers every stage a frame passes through', () { + // Each of these is a real, separately-observable stage. Dropping one + // leaves a gap exactly where someone would be trying to reason. + expect(_helpText.toLowerCase(), contains('ffmpeg')); + expect(_helpText.toLowerCase(), contains('pipe')); + expect(_helpText.toLowerCase(), contains('dither')); + expect(_helpText.toLowerCase(), contains('encoder')); + }); + + test('explains the two things that surprise people', () { + // The Y4M pipe silently dropping aspect and colour tags is why the output + // has to be re-stamped, and is invisible from outside. + expect(_helpText, contains('BT.601')); + expect(_helpText.toLowerCase(), contains('untagged')); + // And a hardware encoder advertising a mode its card lacks is issue #74. + expect(_helpText, contains('4:2:2')); + expect(_helpText, contains('NVENC')); + }); + + test('says values are rescaled rather than taken literally', () { + // The 8-bit-vocabulary rule is the single most useful thing here for + // anyone tuning a filter on a 10-bit source. + expect(_helpText, contains('0–255')); + expect(_helpText.toLowerCase(), contains('rescal')); + }); + + test('every section has a heading and a real body', () { + expect(colourPipelineHelpSections, isNotEmpty); + for (final (heading, body) in colourPipelineHelpSections) { + expect(heading.trim(), isNotEmpty); + expect(body.trim().length, greaterThan(80), + reason: '"$heading" has no real explanation under it'); + } + }); + }); + + group('the icon', () { + testWidgets('opens the dialog on click', (tester) async { + await tester.pumpWidget(_harness(Brightness.dark)); + await tester.tap(find.byType(IconButton)); + await tester.pumpAndSettle(); + + expect(find.text('How VapourBox handles colour'), findsOneWidget); + for (final (heading, _) in colourPipelineHelpSections) { + expect(find.text(heading), findsOneWidget); + } + }); + + testWidgets('is distinguishable from the format help beside it', ( + tester, + ) async { + // Two adjacent buttons drawn the same way read as one control repeated. + await tester.pumpWidget(_harness(Brightness.light)); + final icon = tester.widget(find.byType(Icon)); + expect(icon.icon, isNot(Icons.info_outline), + reason: 'ChromaFormatHelpIcon already uses info_outline'); + expect(icon.semanticLabel, isNotNull, + reason: 'a screen reader must be able to tell them apart too'); + }); + }); + + group('the dialog', () { + testWidgets('scrolls, and fits a small window', (tester) async { + tester.view.physicalSize = const Size(800, 600); + tester.view.devicePixelRatio = 1.0; + addTearDown(tester.view.reset); + + await tester.pumpWidget(_harness(Brightness.dark)); + await tester.tap(find.byType(IconButton)); + await tester.pumpAndSettle(); + expect(tester.takeException(), isNull); + + final scrollable = find.descendant( + of: find.byType(AlertDialog), + matching: find.byType(Scrollable), + ); + expect(scrollable, findsWidgets); + + final last = colourPipelineHelpSections.last.$1; + await tester.drag(scrollable.first, const Offset(0, -4000)); + await tester.pumpAndSettle(); + expect(find.text(last), findsOneWidget); + expect(tester.takeException(), isNull); + }); + + testWidgets('closes again', (tester) async { + await tester.pumpWidget(_harness(Brightness.light)); + await tester.tap(find.byType(IconButton)); + await tester.pumpAndSettle(); + await tester.tap(find.widgetWithText(TextButton, 'Close')); + await tester.pumpAndSettle(); + expect(find.byType(AlertDialog), findsNothing); + }); + }); +} diff --git a/worker/src/models/video_job.rs b/worker/src/models/video_job.rs index 668950d..036c97a 100644 --- a/worker/src/models/video_job.rs +++ b/worker/src/models/video_job.rs @@ -103,6 +103,28 @@ impl VideoJob { ) } + /// The pixel format the encoder ffmpeg is handed on stdin. + /// + /// The output conversion is the last thing the `.vpy` does, so when one is + /// selected it decides the format outright. With `Original` nothing + /// converts and the pipe carries the format the decoder was asked for — + /// which is the *pipe* format, not necessarily the source's own (see + /// `pixel_format`, where an unreadable source is normalised on the way in). + /// + /// A pass that changes format mid-graph always restores it (Turn90, + /// DeScratch, LUTDeCrawl all convert back), so the graph's output format is + /// its input format. Custom VapourSynth could break that assumption, and + /// like everything else about custom code, it is the user's to get right. + pub fn encoder_input_pix_fmt(&self) -> String { + self.encoding_settings + .chroma_subsampling + .ffmpeg_pix_fmt() + .map(str::to_string) + .unwrap_or_else(|| { + crate::pixel_format::decode_pixel_format(self.input_pixel_format.as_deref()).name + }) + } + /// Get the effective processing pipeline. /// Uses processing_pipeline if set, otherwise creates one from legacy qtgmc_parameters. pub fn effective_pipeline(&self) -> ProcessingPipeline { @@ -414,6 +436,10 @@ pub enum ChromaSubsampling { Original, /// Convert to 8-bit YUV420 for maximum compatibility. Yuv420, + /// Convert to 10-bit YUV420 — the only 10-bit layout NVENC, QSV and AMF can + /// encode, so it keeps a 10-bit source's grading where 4:2:2 fails outright + /// on most GPUs (issue #74). + Yuv420P10, /// Convert to 8-bit YUV422 for higher chroma quality. Yuv422, /// Convert to 10-bit YUV422 — keeps a 10-bit source's precision while @@ -428,10 +454,29 @@ impl ChromaSubsampling { match self { ChromaSubsampling::Original => None, ChromaSubsampling::Yuv420 => Some("vs.YUV420P8"), + ChromaSubsampling::Yuv420P10 => Some("vs.YUV420P10"), ChromaSubsampling::Yuv422 => Some("vs.YUV422P8"), ChromaSubsampling::Yuv422P10 => Some("vs.YUV422P10"), } } + + /// The same format as an FFmpeg pixel-format name — what actually comes out + /// of the Y4M pipe once the conversion above has run. `None` for `Original`, + /// where the format is the source's and only the pipe knows it. + /// + /// Keep in step with [`vapoursynth_format`](Self::vapoursynth_format): the + /// two describe the same conversion, and + /// `chroma_subsampling_names_agree` fails if a variant gains one and not + /// the other. + pub fn ffmpeg_pix_fmt(&self) -> Option<&'static str> { + match self { + ChromaSubsampling::Original => None, + ChromaSubsampling::Yuv420 => Some("yuv420p"), + ChromaSubsampling::Yuv420P10 => Some("yuv420p10le"), + ChromaSubsampling::Yuv422 => Some("yuv422p"), + ChromaSubsampling::Yuv422P10 => Some("yuv422p10le"), + } + } } impl Default for EncodingSettings { @@ -577,23 +622,82 @@ impl VideoCodec { matches!(self.encoder_family(), EncoderFamily::Lossless) } - /// FFmpeg output pixel format to force for this codec, if it cannot accept - /// the pipeline's native format. Classic HuffYUV only supports yuv422p (not - /// yuv420p), so force conversion; ffvhuff and the others accept yuv420p. - pub fn forced_pix_fmt(&self) -> Option<&'static str> { + /// FFmpeg output pixel format to force for this codec, given the format the + /// pipeline will actually hand it (`encoder_input`, from + /// [`VideoJob::encoder_input_pix_fmt`]). `None` leaves the choice to + /// ffmpeg's own negotiation. + /// + /// Two of these are unconditional, because the encoder takes one format + /// whatever the source was. The NVENC/QSV arm is not: it must only fire + /// when the pipeline's format is genuinely unencodable, or it would drag a + /// perfectly good 10-bit 4:2:0 source down to 8-bit for no reason. + /// + /// **Do not assume ffmpeg's negotiation handles this.** An encoder's + /// declared pix_fmt list is static, but NVENC's real capabilities are + /// queried from the driver at `avcodec_open2`. A recent ffmpeg built + /// against NVENC SDK 13 advertises `yuv422p` on `h264_nvenc` for + /// Blackwell's 4:2:2 support, so negotiation happily picks it for a 4:2:2 + /// source — and every pre-Blackwell card then fails the job outright with + /// "YUV422P not supported / No capable devices found" (issue #74, on an + /// RTX 4070 Super). Negotiation cannot avoid this, because the list it + /// negotiates against is wrong for the hardware in the machine. + /// + /// Custom FFmpeg Arguments still wins in every case: they are appended + /// 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> { match self { - VideoCodec::Huffyuv => Some("yuv422p"), + // Classic HuffYUV only supports yuv422p (not yuv420p); ffvhuff and + // the others accept yuv420p. + VideoCodec::Huffyuv => return Some("yuv422p"), // AMF takes nv12 natively. Left to negotiate, ffmpeg will hand a // >8-bit source to the encoder as p010, and 10-bit HEVC encode is // only supported on some AMD ASICs — where it isn't, the AMF // runtime faults (0xC0000005) instead of failing cleanly, which is // one candidate for the crashes in issue #51. h264_amf has no // 10-bit mode at all. Pinning nv12 makes the conversion explicit - // and identical on every card. Custom FFmpeg Arguments still wins: - // a later -pix_fmt overrides this one. - VideoCodec::H264Amf | VideoCodec::H265Amf => Some("nv12"), - _ => None, + // and identical on every card. + VideoCodec::H264Amf | VideoCodec::H265Amf => return Some("nv12"), + _ => {} + } + + let family = self.encoder_family(); + if !matches!(family, EncoderFamily::Nvenc | EncoderFamily::Qsv) { + return None; } + + let (class, depth) = crate::pixel_format::chroma_and_depth(encoder_input); + + // 4:2:0 is the only chroma layout every NVENC and QSV part encodes. + // 4:2:2 is Blackwell-only on NVENC and needs a recent VDENC on QSV; + // 4:4:4 needs a profile this pipeline never selects (`build_encoder_ + // quality_args` pins h264_nvenc to `-profile:v high`, which is 4:2:0). + let chroma_unsupported = class != crate::pixel_format::ChromaClass::C420; + // Neither family has a 10-bit H.264 mode at all. + let depth_unsupported = depth > 8 && self.is_h264(); + + if !chroma_unsupported && !depth_unsupported { + return None; + } + + // NVENC names planar 4:2:0 `yuv420p`; QSV's native format is the + // semi-planar `nv12`. Both are 8-bit 4:2:0 and either encoder accepts + // either, but each family's own name is the one that avoids a + // needless swscale hop. + let planar = matches!(family, EncoderFamily::Nvenc); + Some(if self.is_h264() { + // H.264: 8-bit 4:2:0 is the only option on either family. + if planar { "yuv420p" } else { "nv12" } + } else if depth > 8 { + // HEVC: drop the chroma, but keep the source's precision. Every + // NVENC from Pascal on, and every QSV with an HEVC VDENC, takes + // 10-bit 4:2:0. + "p010le" + } else if planar { + "yuv420p" + } else { + "nv12" + }) } /// Encoder presets this codec accepts. Mirrors `availablePresets` in @@ -836,6 +940,132 @@ mod tests { assert!(!VideoCodec::ProResHQ.is_hardware()); } + /// Issue #74: a 4:2:2 source killed every NVENC job on pre-Blackwell + /// hardware, because a recent ffmpeg advertises `yuv422p` on `h264_nvenc` + /// and only the driver knows the card can't do it. The pipeline must pick + /// the format itself rather than leaving it to negotiation. + #[test] + fn test_nvenc_cannot_be_handed_422() { + // The exact case reported: CineForm yuv422p10le -> h264_nvenc. + assert_eq!( + VideoCodec::H264Nvenc.forced_pix_fmt("yuv422p10le"), + Some("yuv420p") + ); + // HEVC drops the chroma but keeps the 10 bits. + assert_eq!( + VideoCodec::H265Nvenc.forced_pix_fmt("yuv422p10le"), + Some("p010le") + ); + // 8-bit 4:2:2 needs no depth change, only the chroma. + assert_eq!( + VideoCodec::H265Nvenc.forced_pix_fmt("yuv422p"), + Some("yuv420p") + ); + // 4:4:4 is equally unencodable: `build_encoder_quality_args` pins + // h264_nvenc to `-profile:v high`, which is a 4:2:0 profile. + assert_eq!( + VideoCodec::H264Nvenc.forced_pix_fmt("yuv444p"), + Some("yuv420p") + ); + } + + /// The guard must stay off for what already worked. Pinning NVENC + /// unconditionally (as AMF is pinned) would silently flatten a 10-bit + /// 4:2:0 source to 8-bit for everyone it currently serves correctly. + #[test] + fn test_nvenc_leaves_encodable_formats_alone() { + assert_eq!(VideoCodec::H264Nvenc.forced_pix_fmt("yuv420p"), None); + assert_eq!(VideoCodec::H265Nvenc.forced_pix_fmt("yuv420p"), None); + // 10-bit 4:2:0 into HEVC is exactly what NVENC is good at — untouched. + assert_eq!(VideoCodec::H265Nvenc.forced_pix_fmt("yuv420p10le"), None); + assert_eq!(VideoCodec::H265Nvenc.forced_pix_fmt("p010le"), None); + } + + /// H.264 has no 10-bit mode on either hardware family, so depth alone is + /// enough to force a conversion even when the chroma is already fine. + #[test] + fn test_h264_hardware_has_no_10_bit_mode() { + assert_eq!( + VideoCodec::H264Nvenc.forced_pix_fmt("yuv420p10le"), + Some("yuv420p") + ); + assert_eq!( + VideoCodec::H264Qsv.forced_pix_fmt("yuv420p10le"), + Some("nv12") + ); + } + + /// QSV is the same class of trap — `hevc_qsv` advertises 4:2:2 (`y210le`) + /// on builds where the hardware may not have it. Not reported, guarded on + /// the same reasoning. QSV's native layout is semi-planar, so it gets + /// nv12 where NVENC gets yuv420p. + #[test] + fn test_qsv_takes_its_own_native_formats() { + assert_eq!(VideoCodec::H264Qsv.forced_pix_fmt("yuv422p"), Some("nv12")); + assert_eq!( + VideoCodec::H265Qsv.forced_pix_fmt("yuv422p10le"), + Some("p010le") + ); + assert_eq!(VideoCodec::H265Qsv.forced_pix_fmt("yuv422p"), Some("nv12")); + assert_eq!(VideoCodec::H264Qsv.forced_pix_fmt("yuv420p"), None); + } + + /// The two unconditional pins predate this and must not become conditional: + /// they hold whatever the pipeline's format is. + #[test] + fn test_unconditional_pins_ignore_the_input_format() { + for fmt in ["yuv420p", "yuv422p10le", "yuv444p16le", "nv12"] { + assert_eq!(VideoCodec::Huffyuv.forced_pix_fmt(fmt), Some("yuv422p")); + assert_eq!(VideoCodec::H264Amf.forced_pix_fmt(fmt), Some("nv12")); + assert_eq!(VideoCodec::H265Amf.forced_pix_fmt(fmt), Some("nv12")); + } + } + + /// Software, ProRes, 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. + #[test] + fn test_negotiating_encoders_are_left_alone() { + for codec in [ + VideoCodec::H264, + VideoCodec::H265, + VideoCodec::H264Videotoolbox, + VideoCodec::H265Videotoolbox, + VideoCodec::ProRes422, + VideoCodec::FFV1, + VideoCodec::Ffvhuff, + ] { + for fmt in ["yuv420p", "yuv422p10le", "yuv444p16le"] { + assert_eq!( + codec.forced_pix_fmt(fmt), + None, + "{codec:?} should negotiate {fmt} itself" + ); + } + } + } + + /// 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, + ] { + assert_eq!( + cs.vapoursynth_format().is_some(), + cs.ffmpeg_pix_fmt().is_some(), + "{cs:?} declares one output format name but not the other" + ); + } + } + #[test] fn test_container_format_serialization() { assert_eq!( diff --git a/worker/src/pipeline_executor.rs b/worker/src/pipeline_executor.rs index 878fb10..cc9e7c0 100644 --- a/worker/src/pipeline_executor.rs +++ b/worker/src/pipeline_executor.rs @@ -835,8 +835,24 @@ impl PipelineExecutor { Self::build_encoder_quality_args(&mut args, job); // Force a compatible output pixel format for codecs that can't accept the - // pipeline's native format (e.g. classic HuffYUV requires yuv422p). - if let Some(pix_fmt) = settings.codec.forced_pix_fmt() { + // pipeline's native format (e.g. classic HuffYUV requires yuv422p, and + // 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 + ), + ); + } args.extend(["-pix_fmt".to_string(), pix_fmt.to_string()]); } @@ -1332,7 +1348,7 @@ impl Drop for PipelineExecutor { #[cfg(test)] mod tests { use super::*; - use crate::models::{AudioCodec, AudioQuality, EncodingSettings, QTGMCParameters, VideoCodec}; + use crate::models::{AudioCodec, AudioQuality, ChromaSubsampling, EncodingSettings, QTGMCParameters, VideoCodec}; use uuid::Uuid; /// A leftover `progress=end` must not end a run that has produced no frames. @@ -1525,8 +1541,9 @@ mod tests { // Encoder-family-specific quality and preset args PipelineExecutor::build_encoder_quality_args(&mut args, job); - // Force a compatible output pixel format (e.g. HuffYUV requires yuv422p) - if let Some(pix_fmt) = settings.codec.forced_pix_fmt() { + // Force a compatible output pixel format (e.g. HuffYUV requires yuv422p, + // NVENC/QSV cannot encode 4:2:2 on most hardware) + if let Some(pix_fmt) = settings.codec.forced_pix_fmt(&job.encoder_input_pix_fmt()) { args.extend(["-pix_fmt".to_string(), pix_fmt.to_string()]); } @@ -1948,6 +1965,106 @@ mod tests { assert!(!args.contains(&"-preset".to_string()), "HuffYUV should not have -preset"); } + /// Issue #74, end to end: the reported job was a CineForm `yuv422p10le` + /// source at the default "Match source" colour format, encoded with + /// h264_nvenc. With nothing forced, ffmpeg negotiated `yuv422p` (which the + /// encoder advertises) and the RTX 4070 Super rejected it at open, so the + /// job produced zero frames. + #[test] + fn test_nvenc_422_source_gets_an_encodable_pix_fmt() { + let mut job = create_test_job("output.mp4"); + job.encoding_settings.codec = VideoCodec::H264Nvenc; + job.input_pixel_format = Some("yuv422p10le".to_string()); + assert_eq!( + job.encoding_settings.chroma_subsampling, + ChromaSubsampling::Original, + "the reported job used the default colour format" + ); + + let args = build_ffmpeg_args_for_test(&job); + let idx = args + .iter() + .position(|a| a == "-pix_fmt") + .expect("NVENC must not be left to negotiate a 4:2:2 source"); + assert_eq!(args[idx + 1], "yuv420p"); + } + + /// The same source into HEVC keeps its 10 bits — only the chroma has to go. + #[test] + fn test_hevc_nvenc_422_source_keeps_10_bits() { + let mut job = create_test_job("output.mp4"); + job.encoding_settings.codec = VideoCodec::H265Nvenc; + job.input_pixel_format = Some("yuv422p10le".to_string()); + + let args = build_ffmpeg_args_for_test(&job); + let idx = args.iter().position(|a| a == "-pix_fmt").unwrap(); + assert_eq!(args[idx + 1], "p010le"); + } + + /// Choosing a 4:2:2 output format explicitly is the other route to the same + /// failure: the conversion is done by the `.vpy`, so the source format never + /// enters into it. + #[test] + fn test_nvenc_422_output_format_is_also_guarded() { + let mut job = create_test_job("output.mp4"); + job.encoding_settings.codec = VideoCodec::H265Nvenc; + job.encoding_settings.chroma_subsampling = ChromaSubsampling::Yuv422P10; + // An 8-bit 4:2:0 source — the pipeline is what makes it 4:2:2. + job.input_pixel_format = Some("yuv420p".to_string()); + + assert_eq!(job.encoder_input_pix_fmt(), "yuv422p10le"); + let args = build_ffmpeg_args_for_test(&job); + let idx = args.iter().position(|a| a == "-pix_fmt").unwrap(); + assert_eq!(args[idx + 1], "p010le"); + } + + /// 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] + fn test_nvenc_420_source_is_untouched() { + for fmt in ["yuv420p", "yuv420p10le"] { + let mut job = create_test_job("output.mp4"); + job.encoding_settings.codec = VideoCodec::H265Nvenc; + job.input_pixel_format = Some(fmt.to_string()); + + let args = build_ffmpeg_args_for_test(&job); + assert!( + !args.contains(&"-pix_fmt".to_string()), + "{fmt} is encodable; NVENC should negotiate it" + ); + } + } + + /// x264 handles 4:2:2 natively. Forcing a format here would throw away + /// chroma the encoder was perfectly able to keep. + #[test] + fn test_software_encoder_keeps_422() { + let mut job = create_test_job("output.mkv"); + job.encoding_settings.codec = VideoCodec::H264; + job.input_pixel_format = Some("yuv422p10le".to_string()); + + let args = build_ffmpeg_args_for_test(&job); + assert!(!args.contains(&"-pix_fmt".to_string())); + } + + /// Custom FFmpeg Arguments are appended last, so a user who knows their + /// card really does have 4:2:2 can still ask for it — ffmpeg takes the + /// later `-pix_fmt`. That is the escape hatch this guard relies on. + #[test] + fn test_custom_args_can_override_the_forced_format() { + let mut job = create_test_job("output.mp4"); + job.encoding_settings.codec = VideoCodec::H264Nvenc; + job.input_pixel_format = Some("yuv422p10le".to_string()); + job.encoding_settings.custom_ffmpeg_args = "-pix_fmt yuv422p".to_string(); + + let args = build_ffmpeg_args_for_test(&job); + let last = args + .iter() + .rposition(|a| a == "-pix_fmt") + .expect("both formats should be present"); + assert_eq!(args[last + 1], "yuv422p", "the user's choice must win"); + } + #[test] fn test_ffmpeg_args_video_codec_ffvhuff_no_forced_pix_fmt() { let mut job = create_test_job("output.mkv"); diff --git a/worker/src/pixel_format.rs b/worker/src/pixel_format.rs index 40536f1..694f860 100644 --- a/worker/src/pixel_format.rs +++ b/worker/src/pixel_format.rs @@ -49,7 +49,7 @@ 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)] -enum ChromaClass { +pub enum ChromaClass { C420, C422, C444, @@ -78,6 +78,16 @@ impl PipeFormat { } } +/// Chroma class and bit depth of an FFmpeg pixel format name. +/// +/// Public counterpart of [`classify`]. `VideoCodec::forced_pix_fmt` needs it to +/// decide whether the format the pipeline will hand a hardware encoder is one +/// that encoder can actually take — a question about the *output* of the graph, +/// where the rest of this module is about its input. +pub fn chroma_and_depth(name: &str) -> (ChromaClass, u32) { + classify(&name.to_ascii_lowercase()) +} + /// The raw pixel format to pipe for a source whose probed format is `probed`. pub fn decode_pixel_format(probed: Option<&str>) -> PipeFormat { let probed = probed.map(str::trim).filter(|s| !s.is_empty());