diff --git a/CLAUDE.md b/CLAUDE.md index ad22d58..e938056 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -702,9 +702,21 @@ as a broken filter argument. Same shape as the SAR handling above, same fix site. Colour tags (`color_space`, `color_range`, etc.) must be read from ffprobe, carried on `VideoJob`, and -re-stamped as **encoder output-stream flags** (`-colorspace`/`-color_primaries`/ -`-color_trc`/`-color_range`) — `SetFrameProps` in the script is inert, because -the Y4M pipe strips frame properties the same way it strips SAR. `ColorMetadata::from_raw` +re-stamped with `-colorspace`/`-color_primaries`/`-color_trc`/`-color_range` — +`SetFrameProps` in the script is inert, because the Y4M pipe strips frame +properties the same way it strips SAR. + +**Declare them on BOTH sides: on the Y4M input (`to_ffmpeg_input_args()`, +between `-f yuv4mpegpipe` and `-i -`) and on the output (`to_ffmpeg_output_args()`), +always identical.** Output-only tags are not a label to FFmpeg: it sees an +untagged input, auto-inserts a scaler that reads "unknown" as BT.601, and +**re-matrixes every pixel** to the output tag — and output-only primaries/trc +never reach the file at all. Input-only lets a full-range source be negotiated +down to limited. Don't move this into `setparams` in the `-vf` chain: a Custom +FFmpeg Argument `-vf` replaces the chain and would bring the shift back. Test a +change here with **pixels**, not just ffprobe tags +(`integration_colour_tag_pixels_test.dart`, heavy) — the tag-only test passed +throughout the shift. See docs/ENGINEERING_NOTES.md (2026-09-26). `ColorMetadata::from_raw` validates against FFmpeg's own accepted values rather than forwarding ffprobe's `"unknown"` verbatim. `build_ffmpeg_args_for_test` duplicates `build_ffmpeg_args` rather than calling it, so anything added to one must be added to the other. diff --git a/app/test/integration_colour_tag_pixels_test.dart b/app/test/integration_colour_tag_pixels_test.dart new file mode 100644 index 0000000..d394e24 --- /dev/null +++ b/app/test/integration_colour_tag_pixels_test.dart @@ -0,0 +1,205 @@ +/// Colour tags must label the output without changing a single pixel. +/// +/// The worker re-declares the source's colour tags on the encode because the +/// Y4M pipe from vspipe strips them. Until 2026-09-26 it declared them on the +/// *output only*, which FFmpeg does not treat as a label: it saw an untagged +/// input feeding a bt709-tagged output, auto-inserted a scaler, read "unknown" +/// as BT.601, and re-matrixed every pixel from 601 to 709. Every encode from a +/// tagged source came out colour-shifted, while the tag assertions in +/// `integration_chroma_subsampling_test.dart` passed happily — they check the +/// label, not the picture. This file checks the picture. +/// +/// The reference is the same job with the tags left off (an untagged job has +/// nothing to convert), plus the source's own decoded frames: with no passes +/// enabled and a lossless codec, the output's samples must be bit-identical to +/// both. +/// +/// Heavy (full-encode) — runs in the nightly workflow, not the push gate. +@Tags(['heavy']) +library; + +// ignore_for_file: avoid_print — these tests print diagnostics to the test log. + +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:uuid/uuid.dart'; + +import 'package:vapourbox/models/encoding_settings.dart'; +import 'package:vapourbox/models/processing_pipeline.dart'; +import 'package:vapourbox/models/qtgmc_parameters.dart'; +import 'package:vapourbox/models/video_job.dart'; + +import 'support/worker_harness.dart'; + +String get _outDir => '${WorkerHarness.outputDir}/colour_tag_pixels'; + +/// 10-bit 4:2:2 ProRes tagged bt709/bt709/bt709/tv — a real tagged source. +String get _fixture => '${WorkerHarness.repoRoot}/Tests/TestResources/pal-sd-25.mov'; + +/// Frames encoded per job. Enough to be a real encode, short enough to be quick. +const _frames = 10; + +/// md5 of the decoded video samples, in the stream's own pixel format. +/// +/// Decoding to rawvideo in the stream's native format involves no scaler, so +/// this hashes exactly what the file stores rather than a conversion of it. +/// +/// [size] reproduces the worker's own decoder for the source reference: this +/// fixture has a clean aperture (720x576 coded, 702x576 decoded) and the +/// worker's decoder forces the probed 720x576 back with `-s`. +Future _sampleMd5(String path, {int? frames, String? size, String? pixFmt}) async { + final r = await Process.run( + WorkerHarness.ffmpegPath, + [ + '-v', 'error', + '-i', path, + '-map', '0:v:0', + if (frames != null) ...['-frames:v', '$frames'], + if (size != null) ...['-s', size], + if (pixFmt != null) ...['-pix_fmt', pixFmt], + '-f', 'md5', '-', + ], + environment: WorkerHarness.ffmpegEnv, + ); + if (r.exitCode != 0) throw Exception('md5 of $path failed: ${r.stderr}'); + return (r.stdout as String).trim(); +} + +void main() { + late Map src; + + setUpAll(() async { + await WorkerHarness.ensureReady(); + await Directory(_outDir).create(recursive: true); + src = (await WorkerHarness.firstStream(_fixture, selector: 'v:0', entries: [ + 'pix_fmt', 'width', 'height', + 'color_space', 'color_primaries', 'color_transfer', 'color_range', + ]))!; + }); + + /// A job that does nothing to the picture: no deinterlace, no passes, + /// source colour format. [tagged] decides whether the source's colour tags + /// are passed to the worker, as the app does after probing. + VideoJob job(String name, VideoCodec codec, ContainerFormat container, + {required bool tagged}) { + return VideoJob( + id: const Uuid().v4(), + inputPath: _fixture, + outputPath: '$_outDir/$name', + processingPipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters(enabled: false), + ), + encodingSettings: EncodingSettings( + codec: codec, + container: container, + audioMode: AudioMode.none, + ), + // The post-trim count, which is what pipe_source must be told. + totalFrames: _frames, + inputFrameRate: 25, + startFrame: 0, + endFrame: _frames - 1, + inputWidth: src['width'] as int, + inputHeight: src['height'] as int, + inputPixelFormat: src['pix_fmt'] as String, + inputColorMatrix: tagged ? src['color_space'] as String? : null, + inputColorPrimaries: tagged ? src['color_primaries'] as String? : null, + inputColorTransfer: tagged ? src['color_transfer'] as String? : null, + inputColorRange: tagged ? src['color_range'] as String? : null, + ); + } + + Future encode(VideoJob j) async { + final result = await WorkerHarness.runJob(j.toJson(), label: j.outputPath); + expect(result.success, isTrue, reason: '${result.error}\n${result.logs.join('\n')}'); + return result.outputPath ?? j.outputPath; + } + + test('fixture is a tagged source (otherwise nothing here is exercised)', () { + expect(src['color_space'], 'bt709'); + expect(src['color_range'], 'tv'); + }); + + test('FFV1: a tagged encode stores the same samples as the source', () async { + final tagged = await encode( + job('ffv1_tagged.mkv', VideoCodec.ffv1, ContainerFormat.mkv, tagged: true)); + final untagged = await encode( + job('ffv1_untagged.mkv', VideoCodec.ffv1, ContainerFormat.mkv, tagged: false)); + + final tags = await WorkerHarness.firstStream(tagged, selector: 'v:0', entries: [ + 'pix_fmt', 'color_space', 'color_primaries', 'color_transfer', 'color_range', + ]); + print(' tagged output: $tags'); + + final t = await WorkerHarness.frameAverages(tagged); + final u = await WorkerHarness.frameAverages(untagged); + print(' frame averages tagged: $t\n untagged: $u'); + + // The picture: identical to the untagged encode and to the source itself. + final sourceMd5 = await _sampleMd5(_fixture, + frames: _frames, + size: '${src['width']}x${src['height']}', + pixFmt: src['pix_fmt'] as String); + expect(await _sampleMd5(untagged), sourceMd5, + reason: 'control: a no-op pipeline into FFV1 must be lossless'); + expect(await _sampleMd5(tagged), sourceMd5, + reason: 'declaring the source\'s colour tags changed the pixels — ' + 'ffmpeg is converting untagged input to the output tag ' + '(tagged $t vs untagged $u)'); + + // The label: all four tags written, not just the two ffmpeg negotiates. + expect(tags!['pix_fmt'], src['pix_fmt']); + expect(tags['color_space'], 'bt709'); + expect(tags['color_range'], 'tv'); + expect(tags['color_primaries'], 'bt709', + reason: 'output-only -color_primaries never reached the file'); + expect(tags['color_transfer'], 'bt709', + reason: 'output-only -color_trc never reached the file'); + }, timeout: const Timeout(Duration(minutes: 5))); + + // The preview converts the (untagged) Y4M frame to RGB with the source's + // matrix fed to swscale explicitly (`swscale_input_opts`). The encode, read + // back through its tags, must show the same picture — before the fix it was + // re-matrixed and then labelled bt709, so it could not. + test('the tagged encode looks the same as the preview of that frame', () async { + final j = job('ffv1_preview_match.mkv', VideoCodec.ffv1, ContainerFormat.mkv, + tagged: true); + final out = await encode(j); + const frame = 2; + final preview = await WorkerHarness.runPreview(j.toJson(), frame: frame); + expect(preview.success, isTrue, reason: '$preview'); + + final previewRgb = await WorkerHarness.imageToRgb24(preview.png!, label: 'pv'); + final encodedRgb = await WorkerHarness.frameRgb24(out, frame, label: 'enc'); + final diff = WorkerHarness.meanAbsDiff(previewRgb, encodedRgb); + print(' preview vs encode mean abs diff: ${diff.toStringAsFixed(3)}/255'); + expect(diff, lessThan(1.0), + reason: 'the encode shows different colours from the preview'); + }, timeout: const Timeout(Duration(minutes: 5))); + + // The forced-pix_fmt path: ProRes 4444 pins yuv444p10le, so a scaler *is* + // inserted for the 4:2:2 -> 4:4:4 step. It must only resample chroma, never + // re-matrix, so the tagged and untagged encodes must still match exactly. + test('ProRes 4444 (pinned pix_fmt): tags do not change the pixels', () async { + final tagged = await encode(job( + 'prores4444_tagged.mov', VideoCodec.prores4444, ContainerFormat.mov, + tagged: true)); + final untagged = await encode(job( + 'prores4444_untagged.mov', VideoCodec.prores4444, ContainerFormat.mov, + tagged: false)); + + final t = await WorkerHarness.frameAverages(tagged); + final u = await WorkerHarness.frameAverages(untagged); + print(' frame averages tagged: $t\n untagged: $u'); + + expect(await _sampleMd5(tagged), await _sampleMd5(untagged), + reason: 'tagged $t vs untagged $u'); + + final tags = await WorkerHarness.firstStream(tagged, + selector: 'v:0', entries: ['pix_fmt', 'color_space', 'color_range']); + expect(tags!['pix_fmt'], contains('444')); + expect(tags['color_space'], 'bt709'); + expect(tags['color_range'], 'tv'); + }, timeout: const Timeout(Duration(minutes: 5))); +} diff --git a/docs/ENGINEERING_NOTES.md b/docs/ENGINEERING_NOTES.md index f11740c..b937024 100644 --- a/docs/ENGINEERING_NOTES.md +++ b/docs/ENGINEERING_NOTES.md @@ -384,6 +384,110 @@ Note the version parser accepts `n9.0.1` and `9.0.1` and deliberately **rejects a `master` build** (`N-125978-...`), so reverting any platform to an unpinned master URL is a red build rather than a silent regression. +### Colour tags on the output only re-matrixed every pixel (2026-09-26) + +The colour-metadata fix (read the tags with ffprobe, carry them on `VideoJob`, +re-stamp them on the encode) put them on the encoder as **output-stream flags** +only, with a comment calling it "a *metadata* fix, not a pixel one". With the +bundled FFmpeg 9.0.1 it was a pixel change on every encode from a tagged +source. + +**Reproduction** (flat 64x64 `yuv422p10le` frame, Y/U/V = 565/236/756, into +FFV1 and decoded back): + +| encoder args | decoded Y/U/V | written tags | +|---|---|---| +| none | 565/236/756 | range tv only | +| output `-colorspace bt709 -color_range tv` | **546/259/741** | bt709/tv | +| output `-colorspace bt709` alone | **546/259/741** | bt709 | +| output `-color_range tv` alone | 565/236/756 | tv | +| output, all four flags | **546/259/741** | bt709/tv; primaries/trc **unknown** | +| same four flags on the **input** (before `-i`) *and* the output | 565/236/756 | all four | +| `-vf setparams=…` (all four) | 565/236/756 | all four | + +`rawvideo` and `yuv4mpegpipe` inputs behave identically. The output-only shift +reproduced with ProRes 4444 (with its `-pix_fmt` pin) as well as FFV1, and the +input-side declaration matched the untagged encode exactly with x264, ProRes +4444, HuffYUV (`-pix_fmt yuv422p`, 8-bit) and hevc_videotoolbox. End to end +through the worker on `Tests/TestResources/pal-sd-25.mov` (bt709/tv ProRes) +into FFV1, the tagged encode's frame averages moved from (Y 382.7, U 529.9, +V 509.4) to (382.0, 530.1, 510.7) in 10-bit units, and the encode read back +through its own tags differed from the preview of the same frame by a mean +6.1/255 per RGB sample. + +**Root cause.** FFmpeg negotiates the colour matrix and range through the +filter graph the same way it negotiates the pixel format. The Y4M pipe carries +no colour information, so the input is `csp:unknown range:unknown`; the +output flags make the encoder demand `csp:bt709 range:tv`; so the graph +auto-inserts a scaler to get from one to the other (`-v debug`: +`auto_scale_0 … fmt:yuv422p10le csp:unknown range:unknown -> … csp:bt709 +range:tv`, followed by swscale's `YUV color matrix differs for YUV->YUV, using +intermediate RGB to convert`). swscale treats "unknown" as BT.601, so it +converts 601→709 — a genuine re-matrix of samples that were already 709. An +explicit range alone caused nothing because unknown range defaults to limited, +which matched. + +A second, quieter defect fell out of the same measurements: output-only +`-color_primaries` and `-color_trc` **never reached the file at all**. Those two +are not negotiated; the encoder copies them from the frames, which were +untagged, overriding the option. So the "re-stamp all four tags" fix had only +ever re-stamped two. + +**The fix** declares the tags on the pipe's input (`ColorMetadata::to_ffmpeg_input_args`, +between `-f yuv4mpegpipe` and `-i -`) as well as on the output +(`to_ffmpeg_output_args`). Both are the same list by construction. With the +input declared as what it already is, input and output agree, there is nothing +for the scaler to convert, and the frames carry primaries/transfer to the +encoder. Every value `ColorMetadata` accepts (13 matrices, 12 primaries, 16 +transfers, the six range spellings) was checked on both sides against the +bundled binary: all pixel-identical. + +Why input options rather than `setparams` in the `-vf` chain, which also works: + +- **A Custom FFmpeg Argument containing `-vf` replaces the whole chain** (the + last `-vf` wins), which would silently drop a `setparams` and bring the shift + back for exactly the users who customise. Input options are untouched by it. +- The tags are true of the frames from the moment they are decoded, so every + filter in the chain (burnt-in subtitles, anything a user adds) sees the right + matrix, not just the encoder. +- The `-vf` chain stays empty for a job with no aspect/subtitle work, rather + than every tagged job gaining a filter. + +The output flags are **kept**, not replaced: declared only on the input, a +full-range source came out converted to limited (548/271/726 for the test +frame) — nothing pinned the output range, and ffmpeg negotiated `tv`. Both +sides together are what is pixel-exact. + +Consequences worth knowing: + +- A Custom FFmpeg Argument like `-colorspace smpte170m` still works and still + converts — but now from the source's *real* matrix, which is what such an + argument means. Before, it converted from an assumed 601. +- A forced `-pix_fmt` (HuffYUV, AMF `nv12`, the #74 hardware pins, ProRes + profile pins) still inserts a scaler, which now only resamples chroma/depth + and does not re-matrix. The ProRes 4444 case is asserted bit-identical + tagged-vs-untagged. +- The preview path needed no change: it already feeds the source's matrix and + range to swscale explicitly (`swscale_input_opts`), which is why the preview + showed the correct colours while the encode did not. + +**Tests.** Rust: `input_and_output_declarations_always_agree` and +`input_declaration_carries_every_tag` (color_metadata.rs); +`test_color_tags_are_declared_on_the_y4m_input_too` (position-checked against +the `-f`/`-i` pair) and `test_real_ffmpeg_builder_declares_colour_on_the_pipe_input`, +which reads `build_ffmpeg_args`' own source — the args-level tests otherwise +only see `build_ffmpeg_args_for_test`'s copy. Flutter (heavy): +`integration_colour_tag_pixels_test.dart` encodes the tagged fixture with and +without tags and asserts the samples are bit-identical to each other and to +the source (FFV1), bit-identical tagged-vs-untagged for ProRes 4444, all four +tags present, and the encode matches the preview (mean diff 0.000/255). With +the input declaration disabled it fails all three, as the Rust tests do. + +The existing "tags survive" test in `integration_chroma_subsampling_test.dart` +passed throughout — it checked the label, not the picture. **A metadata change +needs a pixel assertion too**, because FFmpeg decides for itself whether a +label is a label. + ### zsmooth ships once per CPU baseline, and is loaded by path (issue #82, 2026-08-28) A plugin can also have **no** dispatch at all. zsmooth is compiled for a whole diff --git a/worker/src/models/color_metadata.rs b/worker/src/models/color_metadata.rs index a111ce9..07176e6 100644 --- a/worker/src/models/color_metadata.rs +++ b/worker/src/models/color_metadata.rs @@ -8,10 +8,25 @@ //! BT.601 limited by every player — which silently shifts the colours of any //! BT.709 or full-range source. //! -//! Note this is a *metadata* fix, not a pixel one. Nothing here converts -//! anything: the samples coming off the pipe already carry whatever matrix the -//! source used, and none of the passes re-matrix them. The bug was only ever -//! that we failed to say so on the way out. +//! The tags have to be declared on **both** sides of the encoder ffmpeg: on the +//! Y4M *input* (options before `-i -`) as well as on the output stream. This is +//! not a formality. FFmpeg negotiates the colour matrix and range through the +//! filter graph the way it negotiates the pixel format, so an untagged input +//! (`csp:unknown`) feeding an output tagged `bt709` makes ffmpeg auto-insert a +//! scaler — and swscale treats "unknown" as BT.601, so it *re-matrixes every +//! pixel* from 601 to 709 on the way through. Tagging only the output shifted +//! the colours of every encode from a tagged source (measured on FFmpeg 9.0.1: +//! a flat 10-bit 4:2:2 frame at Y/U/V 565/236/756 came back 546/259/741). +//! Declare the input as the same thing and there is nothing for the scaler to +//! convert, so the samples reach the encoder untouched. Output-only +//! `-color_primaries`/`-color_trc` also never reached the file at all (the +//! encoder takes those from the frames, which were untagged); declared on the +//! input they ride on the frames and are written. +//! +//! So this is a *metadata* fix, and the input-side declaration is what keeps it +//! one. Nothing here should convert anything: the samples coming off the pipe +//! already carry whatever matrix the source used, and none of the passes +//! re-matrix them. See docs/ENGINEERING_NOTES.md (2026-09-26). //! //! Values are validated against what FFmpeg actually accepts rather than passed //! through, on the same principle as `parse_ratio`: a value we do not recognise @@ -88,10 +103,30 @@ impl ColorMetadata { && self.range.is_none() } - /// Output-stream flags for the encoder. Each tag is independent: a source + /// Input options for the Y4M pipe, placed between `-f yuv4mpegpipe` and + /// `-i -`. They state what the samples already *are*, which the Y4M header + /// cannot say. Without them ffmpeg sees an untagged input and, because the + /// output is tagged, converts the pixels to match (see the module docs). + pub fn to_ffmpeg_input_args(&self) -> Vec { + self.tag_args() + } + + /// Output-stream flags for the encoder. Must always equal the input + /// declaration: any difference between the two is carried out by ffmpeg as + /// a conversion of the pixels, not a relabelling. Kept alongside the input + /// side (rather than relying on the frames' tags alone) because without an + /// explicit output range ffmpeg can negotiate a different one — a + /// full-range source declared only on the input came out converted to + /// limited. + pub fn to_ffmpeg_output_args(&self) -> Vec { + self.tag_args() + } + + /// The four tag flags, spelled the same way whether they are given as input + /// (decoder) or output (encoder) options. Each tag is independent: a source /// that declares only a matrix gets only `-colorspace`, rather than having /// the other three guessed for it. - pub fn to_ffmpeg_args(&self) -> Vec { + fn tag_args(&self) -> Vec { let mut args = Vec::new(); for (flag, value) in [ ("-colorspace", &self.matrix), @@ -142,7 +177,7 @@ mod tests { fn recognised_values_survive() { let c = ColorMetadata::from_raw(Some("bt709"), Some("bt709"), Some("bt709"), Some("tv")); assert_eq!( - c.to_ffmpeg_args(), + c.to_ffmpeg_output_args(), vec![ "-colorspace", "bt709", "-color_primaries", "bt709", @@ -158,7 +193,7 @@ mod tests { for junk in ["unknown", "N/A", "", " ", "reserved"] { let c = ColorMetadata::from_raw(Some(junk), Some(junk), Some(junk), Some(junk)); assert!(c.is_empty(), "{junk:?} should not be stamped"); - assert!(c.to_ffmpeg_args().is_empty()); + assert!(c.to_ffmpeg_output_args().is_empty()); } assert!(ColorMetadata::from_raw(None, None, None, None).is_empty()); } @@ -176,7 +211,7 @@ mod tests { // A source that declares only a matrix gets only -colorspace; the other // three are not guessed on its behalf. let c = ColorMetadata::from_raw(Some("smpte170m"), None, None, None); - assert_eq!(c.to_ffmpeg_args(), vec!["-colorspace", "smpte170m"]); + assert_eq!(c.to_ffmpeg_output_args(), vec!["-colorspace", "smpte170m"]); } #[test] @@ -208,4 +243,37 @@ mod tests { let c = ColorMetadata::from_raw(Some("rgb"), None, None, None); assert_eq!(c.swscale_input_opts(), vec!["in_range=tv"]); } + + /// Any difference between what the Y4M input is declared as and what the + /// output is tagged as, ffmpeg carries out as a pixel conversion. The two + /// lists must be identical, tag for tag, whatever subset the source has. + #[test] + fn input_and_output_declarations_always_agree() { + let cases = [ + ColorMetadata::default(), + ColorMetadata::from_raw(Some("bt709"), Some("bt709"), Some("bt709"), Some("tv")), + ColorMetadata::from_raw(Some("bt709"), None, None, Some("pc")), + ColorMetadata::from_raw(Some("smpte170m"), None, None, None), + ColorMetadata::from_raw(None, Some("bt2020"), Some("smpte2084"), None), + ColorMetadata::from_raw(Some("rgb"), None, None, Some("jpeg")), + ]; + for c in cases { + assert_eq!(c.to_ffmpeg_input_args(), c.to_ffmpeg_output_args(), "{c:?}"); + } + } + + #[test] + fn input_declaration_carries_every_tag() { + let c = ColorMetadata::from_raw(Some("bt709"), Some("bt709"), Some("bt709"), Some("tv")); + assert_eq!( + c.to_ffmpeg_input_args(), + vec![ + "-colorspace", "bt709", + "-color_primaries", "bt709", + "-color_trc", "bt709", + "-color_range", "tv", + ] + ); + assert!(ColorMetadata::default().to_ffmpeg_input_args().is_empty()); + } } diff --git a/worker/src/pipeline_executor.rs b/worker/src/pipeline_executor.rs index 505b6f3..d790874 100644 --- a/worker/src/pipeline_executor.rs +++ b/worker/src/pipeline_executor.rs @@ -808,6 +808,13 @@ impl PipelineExecutor { // Input 0: Processed video from vspipe (Y4M pipe) args.extend(["-f".to_string(), "yuv4mpegpipe".to_string()]); + // Declare what the piped samples already are. The Y4M header cannot + // carry colour tags, and an untagged input feeding the tagged output + // below is not a relabelling: ffmpeg auto-inserts a scaler that reads + // "unknown" as BT.601 and re-matrixes every pixel to the output's tag. + // Declaring the same tags here leaves it nothing to convert. Must match + // the output flags exactly — see ColorMetadata. + args.extend(job.color_metadata().to_ffmpeg_input_args()); args.extend(["-i".to_string(), "-".to_string()]); // Input 1: Original file for audio stream @@ -952,7 +959,11 @@ impl PipelineExecutor { // SAR above: the Y4M pipe strips them, so an untagged output results and // every player then reads it as BT.601 limited. Nothing in the pipeline // re-matrixes the samples, so the source's tags still describe them. - args.extend(job.color_metadata().to_ffmpeg_args()); + // These equal the input-side declaration on the pipe, which is what + // stops ffmpeg treating them as a conversion target. A Custom FFmpeg + // Argument such as `-colorspace smpte170m` still wins (it comes later) + // and is then a genuine conversion from the source's real matrix. + args.extend(job.color_metadata().to_ffmpeg_output_args()); // Audio handling match settings.audio_mode { @@ -1657,8 +1668,10 @@ mod tests { let mut args = Vec::new(); let settings = &job.encoding_settings; - // Input 0: Processed video from vspipe (Y4M pipe) + // Input 0: Processed video from vspipe (Y4M pipe), with the source's + // colour tags declared on it (mirrors build_ffmpeg_args). args.extend(["-f".to_string(), "yuv4mpegpipe".to_string()]); + args.extend(job.color_metadata().to_ffmpeg_input_args()); args.extend(["-i".to_string(), "-".to_string()]); // Input 1: Original file for audio stream @@ -1697,7 +1710,7 @@ mod tests { // Colour tags. This helper duplicates build_ffmpeg_args rather than // calling it, so anything added there has to be added here too — the // SAR block was missed that way and is still absent below. - args.extend(job.color_metadata().to_ffmpeg_args()); + args.extend(job.color_metadata().to_ffmpeg_output_args()); // Audio handling match settings.audio_mode { @@ -1780,6 +1793,77 @@ mod tests { assert_eq!(pair("-color_range").as_deref(), Some("tv")); } + /// The tags must also be declared on the Y4M *input*, identically. + /// + /// Output-only tags are not a relabelling: ffmpeg sees an untagged input, + /// auto-inserts a scaler, reads "unknown" as BT.601 and re-matrixes every + /// pixel to the output's tag. Measured on FFmpeg 9.0.1 (2026-09-26), a flat + /// 10-bit frame at Y/U/V 565/236/756 came back 546/259/741 from a bt709 + /// source. Declaring the input as the same thing leaves nothing to convert. + #[test] + fn test_color_tags_are_declared_on_the_y4m_input_too() { + let mut job = create_test_job("out.mkv"); + job.input_color_matrix = Some("bt709".to_string()); + job.input_color_primaries = Some("bt709".to_string()); + job.input_color_transfer = Some("bt709".to_string()); + job.input_color_range = Some("tv".to_string()); + + let args = build_ffmpeg_args_for_test(&job); + let pipe_input = args + .windows(2) + .position(|w| w[0] == "-i" && w[1] == "-") + .expect("the Y4M pipe input"); + let y4m_format = args + .windows(2) + .position(|w| w[0] == "-f" && w[1] == "yuv4mpegpipe") + .expect("-f yuv4mpegpipe"); + let second_input = pipe_input + + 2 + + args[pipe_input + 2..].iter().position(|a| a == "-i").expect("audio input"); + + // Input options apply to the next -i only, so they must sit between the + // pipe's -f and its -i, not before the format or after the input. + let input_side = &args[y4m_format + 2..pipe_input]; + let output_side: Vec = { + let tail = &args[second_input + 2..]; + let flags = ["-colorspace", "-color_primaries", "-color_trc", "-color_range"]; + tail.windows(2) + .filter(|w| flags.contains(&w[0].as_str())) + .flat_map(|w| [w[0].clone(), w[1].clone()]) + .collect() + }; + let expected = job.color_metadata().to_ffmpeg_output_args(); + assert_eq!(input_side, expected.as_slice(), "input declaration"); + assert_eq!(output_side, expected, "output tags"); + } + + /// `build_ffmpeg_args_for_test` duplicates `build_ffmpeg_args` rather than + /// calling it (the real one needs a located deps bundle), so the test above + /// only proves the copy. Pin the real function to the same shape by its + /// source: the input declaration must come before the pipe's `-i`. + #[test] + fn test_real_ffmpeg_builder_declares_colour_on_the_pipe_input() { + let src = include_str!("pipeline_executor.rs"); + let start = src.find("fn build_ffmpeg_args(&self").expect("real builder"); + let body = &src[start..]; + let body = &body[..body.find("\n }\n").expect("end of builder")]; + let declare = body.find("to_ffmpeg_input_args()").expect( + "build_ffmpeg_args must declare the colour tags on the Y4M input", + ); + let pipe_input = body + .find(r#"args.extend(["-i".to_string(), "-".to_string()]);"#) + .expect("pipe input"); + let y4m = body.find(r#""yuv4mpegpipe""#).expect("y4m format"); + assert!( + y4m < declare && declare < pipe_input, + "the input declaration must sit between -f yuv4mpegpipe and -i -" + ); + assert!( + body.contains("to_ffmpeg_output_args()"), + "build_ffmpeg_args must still tag the output" + ); + } + /// An untagged source must stay untagged rather than being guessed at. #[test] fn test_untagged_source_declares_nothing() {