From a755d22c144176a6ce0f4659fd0776796a4d10ee Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Thu, 20 Aug 2026 20:49:27 +1000 Subject: [PATCH] test: verify frame counts and geometry against the pipeline, not the model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FrameMap was covered only as arithmetic — output_count, inverse, total_radius — which proves the model is self-consistent and says nothing about whether the plugins agree with it. The Rust suite cannot close that gap by design: run_job generates and inspects the .vpy and never runs vspipe | ffmpeg. integration_frame_mapping_test.dart encodes and counts for every pass that changes the count: double-rate QTGMC 31 -> 62, single-rate as the control 31 -> 31, IVTC cycle 5 90 -> 72, and FlowFPS 25 -> 23.976 75 -> 71. Expected values are derived by hand from the FrameMap definitions rather than read back from the model, so a change to the model cannot quietly redefine "correct". The retime case pins why FlowFPS was chosen over BlockFPS: n*num/den exactly, where BlockFPS gives floor((n-1)*r)+1 and every retimed job's progress total would be a frame out. It also checks the preview. --frame N is a SOURCE index, so under double-rate the preview of source frame S must be output frame 2S. Measured 0.00 mean abs diff at the right frame against 8-30 at its neighbours. The assertion is relative — the correct frame must be the closest of five — because an absolute tolerance loose enough to absorb the encoder and the colour conversion would also be loose enough to pass on a preview that is off by a frame. integration_upscale_resize_test.dart already asserted dimensions for upscaling and seven downscale kernels, but crop had no dimension assertion anywhere: the existing crop tests ran a crop and checked the job succeeded, which passes just as happily on a crop that did nothing. Adds asymmetric crop size, crop composed with resize, and crop pixels rather than crop size — a size assertion cannot tell a correct crop from one that swaps left with right, so the offset test compares against the pipeline's own uncropped output cropped in Dart, with the swapped offsets as negative controls (1.06 correct against 9.65 and 15.97). Plus preview/render agreement for crop, resize and both together, which also asserts the size, since a preview at the wrong resolution surfaces as a buffer-length mismatch in meanAbsDiff rather than as a pixel difference. Two traps found writing this, both recorded in CLAUDE.md. totalFrames is the POST-TRIM count. Trimming is decoder-side and pipe_source builds a fixed-length clip from that number, so declaring the full source length alongside a trim makes pipe_source pad by repeating the last real frame on EOF. This test was written that way first and reported 150 frames for a 31-frame trim, which reads exactly like FPSDivisor being ignored. The app and main.rs both already do it correctly. select=eq(n,N) on a source is not the pipeline's frame N. interlaced_test.avi declares 79 frames and decodes 75 — it carries null frames, and the decoder's default CFR mode expands them onto the 25 fps grid, so the first seven decoded frames come out 0,0,0,0,1,1,2. select counts decoded frames while the pipeline counts timeline positions. Correct behaviour, but it makes ffmpeg-side frame indexing useless as a reference, hence comparing pipeline output against pipeline output. WorkerHarness.frameRgb24 is the shared helper for that. It decodes from the start rather than seeking, because an input seek lands on the nearest keyframe and would silently compare the wrong frame. --- CLAUDE.md | 56 +++ app/test/integration_frame_mapping_test.dart | 381 ++++++++++++++++++ app/test/integration_upscale_resize_test.dart | 284 +++++++++++++ app/test/support/worker_harness.dart | 37 ++ 4 files changed, 758 insertions(+) create mode 100644 app/test/integration_frame_mapping_test.dart diff --git a/CLAUDE.md b/CLAUDE.md index b00333d..c79abd2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1390,6 +1390,62 @@ Subtitles' `burnInPath`. > either pass loses a real class of bug; both were verified against the actual > defects. +### Frame counts and geometry are verified against the pipeline, not the model + +`FrameMap` was covered only as arithmetic — `output_count`, `inverse`, +`total_radius` — which proves the model is self-consistent and says nothing +about whether the plugins agree with it. `run_job` in the Rust suite cannot +close that gap by design: it generates and inspects the `.vpy` and never runs +`vspipe | ffmpeg`. Two heavy Dart files now do (added 2026-08-20): + +- **`integration_frame_mapping_test.dart`** — encodes and *counts* for every + pass that changes the count: double-rate QTGMC (31 -> 62), single-rate as the + control (31 -> 31), IVTC cycle 5 (90 -> 72), and FlowFPS 25 -> 23.976 + (75 -> 71). Expected values are derived by hand from the FrameMap definitions + rather than read back from the model, so a change to the model cannot quietly + redefine "correct". The retime case is the one that pins why FlowFPS was + chosen over BlockFPS: `n*num/den` exactly, where BlockFPS gives + `floor((n-1)*r)+1` and every retimed job's progress total would be a frame out. +- **Preview/render correspondence** in the same file, and for geometry in + `integration_upscale_resize_test.dart`. `--frame N` is a **source** index (the + worker logs "Preview: source frame N") and the target's output index is + `output_count(local)`, so under double-rate the preview of source frame S must + be output frame 2S. Measured **0.00** mean abs diff at the right frame against + 8-30 at its neighbours, which is bit-exact; and 0.00 for crop, resize and + crop+resize. The geometry half also asserts the size, because a preview + rendered at a different resolution shows up as a buffer-length mismatch in + `meanAbsDiff` rather than as a pixel difference. +- **Crop pixels, not just crop size.** A size assertion cannot tell a correct + crop from one that swaps left with right - both give the requested + dimensions. The offset test crops asymmetrically and compares against the + pipeline's own uncropped output cropped in Dart, so the swapped offsets are + available as negative controls (measured 1.06 correct against 9.65 and 15.97). + +> **`totalFrames` is the POST-TRIM count, and getting that wrong looks like a +> filter bug.** Trimming is decoder-side (`-ss` + `-frames:v`) and +> `pipe_source` builds a fixed-length clip from `totalFrames`, so the number has +> to be what the decoder will actually pipe. The app sets it that way +> (`main_viewmodel.dart`: `effectiveEnd - effectiveStart + 1`) and `main.rs` +> computes the same thing when it is absent ("effective after trim"). Pass the +> **full** source length alongside a trim and the script declares a clip longer +> than the data: pipe_source hits EOF and repeats the last real frame to pad, so +> the output is full-length with a frozen tail. The frame-mapping test was +> written that way first and reported 150 frames for a 31-frame trim, which +> reads exactly like FPSDivisor being ignored. + +> **`select=eq(n,N)` on a source is not the pipeline's frame N.** +> `interlaced_test.avi` declares 79 frames and decodes 75: it carries null +> frames, and the decoder's default CFR mode expands them onto the 25 fps grid +> (measured: the first seven decoded frames come out 0,0,0,0,1,1,2). So `select` +> counts *decoded* frames while the pipeline counts *timeline* positions, and +> the two diverge wherever a null frame sits. That is correct behaviour — +> pipe_source needs a fixed-length CFR clip — but it makes ffmpeg-side frame +> indexing useless as a reference. Compare pipeline output against pipeline +> output, which uses one decoder and one indexing scheme. +> `WorkerHarness.frameRgb24` exists for that and decodes from the start rather +> than seeking, because an input seek lands on the nearest keyframe and would +> silently compare the wrong frame. + ### Suggestions and advice are hints, and must stay hints Two small pure-function models sit beside the pass list, and both are diff --git a/app/test/integration_frame_mapping_test.dart b/app/test/integration_frame_mapping_test.dart new file mode 100644 index 0000000..6216d7f --- /dev/null +++ b/app/test/integration_frame_mapping_test.dart @@ -0,0 +1,381 @@ +/// Frame mapping, verified against what actually comes out of the pipeline. +/// +/// `FrameMap` decides two things that are invisible until they are wrong: +/// `output_count` drives the progress total, and `inverse` decides which output +/// frame a preview shows for a given source frame. Both were covered only as +/// arithmetic — `FrameMap::Retime.output_count(97) == 116` and friends — which +/// proves the model is self-consistent and says nothing about whether the +/// plugins agree with it. +/// +/// That gap is exactly the failure this repo already documents for custom +/// VapourSynth: a real output length that differs from the declared one "makes +/// the progress bar lie *and* makes frame-accurate preview show a different +/// frame than its label, both silently". Custom code is guarded (the script +/// raises if `len(clip)` changed); the built-in passes that change the count on +/// purpose had no equivalent end-to-end check. +/// +/// The counts below are derived by hand from the FrameMap definitions rather +/// than read back from the model, so a change to the model cannot quietly +/// redefine what "correct" means: +/// +/// Fanout{factor:2} 75 -> 150, 31 -> 62 +/// Decimate{cycle:5,keep:4} 90 -> 72 +/// Retime 25 -> 24000/1001 ratio reduces to 960/1001, so 75 -> 71 +/// (75*960/1001 = 71.93, floored) +/// +/// Heavy (full encode + preview) — nightly, not the push gate. +@Tags(['heavy']) +library; + +// ignore_for_file: avoid_print — these tests print diagnostics to the test log. + +import 'dart:io'; +import 'dart:typed_data'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:path/path.dart' as p; +import 'package:uuid/uuid.dart'; + +import 'package:vapourbox/models/encoding_settings.dart'; +import 'package:vapourbox/models/frame_rate_parameters.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}/frame_mapping'; + +String get _interlaced => WorkerHarness.inputFile; // 720x576 yuv422p, 25 fps, 75 frames +String get _telecine => p.join( + WorkerHarness.repoRoot, 'Tests', 'TestResources', 'hard_telecine_test.avi'); + +/// The fixtures' real properties, asserted in `setUpAll` so a replaced fixture +/// fails naming itself rather than silently changing every expected count. +const _interlacedFrames = 75; +const _interlacedFps = 25.0; +const _telecineFrames = 90; + +VideoJob _job( + String name, { + required String input, + required ProcessingPipeline pipeline, + required int width, + required int height, + required String pixFmt, + required double fps, + required int totalFrames, + int? startFrame, + int? endFrame, + VideoCodec codec = VideoCodec.h264, +}) => + VideoJob( + id: const Uuid().v4(), + inputPath: input, + outputPath: '$_outDir/$name.mkv', + processingPipeline: pipeline, + encodingSettings: EncodingSettings( + codec: codec, + container: ContainerFormat.mkv, + audioMode: AudioMode.none, + ), + // Set explicitly rather than left to the worker's 720x480 / yuv420p + // fallbacks: the preview and the encode must read the source through the + // same geometry or the comparison below is meaningless. + // + // `totalFrames` is the count the decoder will actually pipe, i.e. the + // POST-TRIM count — trimming is decoder-side (`-ss` + `-frames:v`) and + // pipe_source builds a fixed-length clip from this number. The app sets + // it that way (`main_viewmodel.dart`: `effectiveEnd - effectiveStart + 1`) + // and the worker computes the same thing when it is absent + // (`main.rs`: "effective after trim"). + // + // Passing the full source length alongside a trim declares a clip longer + // than the data: pipe_source hits EOF and repeats the last real frame to + // pad, so the output is full-length with a frozen tail and every count + // here is wrong by the padding. That is not a hypothetical — this test + // was written that way first and reported 150 frames for a 31-frame trim. + inputWidth: width, + inputHeight: height, + inputPixelFormat: pixFmt, + inputFrameRate: fps, + totalFrames: totalFrames, + startFrame: startFrame, + endFrame: endFrame, + ); + +/// Frames actually present in a file, counted rather than read from a header. +Future _countFrames(String path) async { + final v = await WorkerHarness.firstStream(path, + selector: 'v:0', entries: ['nb_read_frames'], countFrames: true); + expect(v, isNotNull, reason: 'no video stream in $path'); + final n = int.tryParse(v!['nb_read_frames']?.toString() ?? ''); + expect(n, isNotNull, reason: 'ffprobe returned no frame count for $path'); + return n!; +} + +/// One frame of a video as packed rgb24, by absolute frame index. +Future _frameRgb(String path, int index) async { + final out = File(p.join(Directory.systemTemp.path, + 'vb_fm_${DateTime.now().microsecondsSinceEpoch}_$index.png')); + try { + final r = await Process.run( + WorkerHarness.ffmpegPath, + [ + '-y', '-v', 'error', + '-i', path, + // Decode from the start and pick by index — accurate, unlike an input + // seek, and these clips are short enough that it costs little. + '-vf', 'select=eq(n\\,$index)', + '-frames:v', '1', + out.path, + ], + environment: WorkerHarness.ffmpegEnv, + ); + expect(r.exitCode, 0, reason: 'extracting frame $index: ${r.stderr}'); + expect(await out.exists(), isTrue, reason: 'no frame $index in $path'); + return WorkerHarness.imageToRgb24(await out.readAsBytes(), + label: 'out$index'); + } finally { + if (await out.exists()) await out.delete().catchError((_) => out); + } +} + +void main() { + setUpAll(() async { + await WorkerHarness.ensureReady(); + await Directory(_outDir).create(recursive: true); + + // Every expected number below is a function of the fixtures' lengths, so + // pin those first — otherwise a re-encoded fixture turns these into + // confidently wrong assertions. + expect(await _countFrames(_interlaced), _interlacedFrames, + reason: 'interlaced_test.avi changed length; the expected output ' + 'counts in this file are derived from it'); + expect(await _countFrames(_telecine), _telecineFrames, + reason: 'hard_telecine_test.avi changed length'); + }); + + group('a pass that changes the frame count actually changes it', () { + test('double-rate QTGMC emits two frames per source frame', () async { + // Fanout{factor: 2}. Trimmed to 31 source frames, so 62 out. + const start = 10, end = 40; + const sourceFrames = end - start + 1; // 31 + final job = _job( + 'double_rate', + input: _interlaced, + pipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters( + preset: QTGMCPreset.superFast, tff: true, fpsDivisor: 1), + ), + width: 720, + height: 576, + pixFmt: 'yuv422p', + fps: _interlacedFps, + // Post-trim, per the contract above. + totalFrames: sourceFrames, + startFrame: start, + endFrame: end, + ); + + final r = await WorkerHarness.runJob(job.toJson(), label: 'double_rate'); + expect(r.success, isTrue, reason: r.error); + final n = await _countFrames(r.outputPath!); + print(' $sourceFrames source -> $n output (expected 62)'); + expect(n, sourceFrames * 2, + reason: 'double-rate QTGMC must emit one frame per field. A count ' + 'equal to the source length means FPSDivisor was ignored and ' + 'the progress total is now double the real work.'); + }); + + test('single-rate QTGMC leaves the count alone', () async { + // The control. Without it, a broken assertion that always sees the source + // count would still pass the test above on a pipeline that fanned out + // nothing. + const start = 10, end = 40; + const sourceFrames = end - start + 1; + final job = _job( + 'single_rate', + input: _interlaced, + pipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters( + preset: QTGMCPreset.superFast, tff: true, fpsDivisor: 2), + ), + width: 720, + height: 576, + pixFmt: 'yuv422p', + fps: _interlacedFps, + // Post-trim, per the contract above. + totalFrames: sourceFrames, + startFrame: start, + endFrame: end, + ); + + final r = await WorkerHarness.runJob(job.toJson(), label: 'single_rate'); + expect(r.success, isTrue, reason: r.error); + final n = await _countFrames(r.outputPath!); + print(' $sourceFrames source -> $n output (expected $sourceFrames)'); + expect(n, sourceFrames); + }); + + test('IVTC decimates 5 frames to 4', () async { + // Decimate{cycle: 5, keep: 4}: 90 telecined frames -> 72 film frames. + final job = _job( + 'ivtc', + input: _telecine, + pipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters( + enabled: true, + method: DeinterlaceMethod.ivtc, + tff: true, + ivtcCycle: 5, + ), + ), + width: 720, + height: 480, + pixFmt: 'yuv420p', + fps: 30000 / 1001, + totalFrames: _telecineFrames, + ); + + final r = await WorkerHarness.runJob(job.toJson(), label: 'ivtc'); + expect(r.success, isTrue, reason: r.error); + final n = await _countFrames(r.outputPath!); + print(' $_telecineFrames source -> $n output (expected 72)'); + expect(n, _telecineFrames * 4 ~/ 5, + reason: 'IVTC must remove the pulldown-duplicated frame from every ' + 'cycle of five. The source count means VDecimate did nothing.'); + }); + + test('frame rate conversion retimes by the reduced ratio', () async { + // Retime. 25 -> 24000/1001 reduces to 960/1001, so 75 -> 71. + // + // Deinterlacing is disabled *explicitly*: ProcessingPipeline()'s default + // has QTGMC enabled, which would fan the count out by two and make this + // assertion about the composition rather than about FlowFPS. + const expected = _interlacedFrames * 960 ~/ 1001; // 71 + final job = _job( + 'retime', + input: _interlaced, + pipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters(enabled: false), + frameRate: FrameRateParameters( + enabled: true, + target: FrameRateTarget.film23976, + sourceFpsNum: 25, + sourceFpsDen: 1, + ), + ), + width: 720, + height: 576, + pixFmt: 'yuv422p', + fps: _interlacedFps, + totalFrames: _interlacedFrames, + ); + + final r = await WorkerHarness.runJob(job.toJson(), label: 'retime'); + expect(r.success, isTrue, reason: r.error); + final n = await _countFrames(r.outputPath!); + print(' $_interlacedFrames source -> $n output (expected $expected)'); + expect(n, expected, + reason: 'FlowFPS was chosen over BlockFPS precisely because its ' + 'output count is n*num/den exactly, where BlockFPS is ' + 'floor((n-1)*r)+1. A count of ${expected + 1} means the ' + 'BlockFPS arithmetic is in play and every retimed job reports a ' + 'progress total one frame out.'); + }); + }); + + group('the preview shows the frame it claims to', () { + // `--frame S` is a SOURCE index (the worker logs "Preview: source frame + // S"), and the target's output index is output_count(local). So under + // double-rate the preview of source frame S must be output frame 2S. + // + // The assertion is deliberately RELATIVE — the correct output frame must be + // closer to the preview than its neighbours are. An absolute tolerance + // would have to absorb the encoder, the colour conversion and the window + // edges, and picking one loose enough to be safe would also be loose enough + // to pass on a preview that is off by a frame. The fixture moves ~12/255 + // per source frame, so neighbours are comfortably distinguishable. + late String encoded; + + setUpAll(() async { + // Untrimmed, so output frame index is exactly output_count(source index) + // with no start offset to reason about. Lossless, so the comparison is + // not measuring H.264. + final job = _job( + 'preview_sync', + input: _interlaced, + pipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters( + preset: QTGMCPreset.superFast, tff: true, fpsDivisor: 1), + ), + width: 720, + height: 576, + pixFmt: 'yuv422p', + fps: _interlacedFps, + totalFrames: _interlacedFrames, + codec: VideoCodec.ffv1, + ); + final r = await WorkerHarness.runJob(job.toJson(), label: 'preview_sync'); + expect(r.success, isTrue, reason: r.error); + encoded = r.outputPath!; + expect(await _countFrames(encoded), _interlacedFrames * 2); + }); + + /// The job used for the preview must match the encode exactly, or any + /// difference measured is the job's, not the mapping's. + VideoJob previewJob() => _job( + 'preview_sync', + input: _interlaced, + pipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters( + preset: QTGMCPreset.superFast, tff: true, fpsDivisor: 1), + ), + width: 720, + height: 576, + pixFmt: 'yuv422p', + fps: _interlacedFps, + totalFrames: _interlacedFrames, + codec: VideoCodec.ffv1, + ); + + for (final sourceFrame in [12, 24, 36]) { + test('source frame $sourceFrame previews as output frame ' + '${sourceFrame * 2}', () async { + final pv = await WorkerHarness.runPreview(previewJob().toJson(), + frame: sourceFrame, label: 'pv$sourceFrame'); + expect(pv.success, isTrue, reason: pv.toString()); + final preview = + await WorkerHarness.imageToRgb24(pv.png!, label: 'pv$sourceFrame'); + + final expectedIndex = sourceFrame * 2; + // ±4 output frames is ±2 source frames — far enough that motion is + // unambiguous, near enough to catch an off-by-one-source-frame error. + final candidates = {}; + for (final offset in [-4, -2, 0, 2, 4]) { + final idx = expectedIndex + offset; + candidates[idx] = WorkerHarness.meanAbsDiff( + preview, await _frameRgb(encoded, idx)); + } + candidates.forEach((idx, diff) => print( + ' output frame $idx: meanAbsDiff ${diff.toStringAsFixed(2)}' + '${idx == expectedIndex ? ' <- expected' : ''}')); + + final best = + candidates.entries.reduce((a, b) => a.value <= b.value ? a : b); + expect(best.key, expectedIndex, + reason: 'the preview of source frame $sourceFrame matched output ' + 'frame ${best.key} more closely than $expectedIndex, so the ' + 'preview and the render disagree about which frame this is'); + + // And it must actually be the same picture, not merely the closest of + // five wrong ones. + expect(candidates[expectedIndex]!, lessThan(6.0), + reason: 'the preview is nearest the right frame but still differs ' + 'from it substantially, which points at the pass rendering ' + 'differently in the two paths rather than at the mapping'); + }, timeout: const Timeout(Duration(minutes: 6))); + } + }); +} diff --git a/app/test/integration_upscale_resize_test.dart b/app/test/integration_upscale_resize_test.dart index 6368186..f9b6d41 100644 --- a/app/test/integration_upscale_resize_test.dart +++ b/app/test/integration_upscale_resize_test.dart @@ -12,6 +12,7 @@ library; // ignore_for_file: avoid_print — these tests print diagnostics to the test log. import 'dart:io'; +import 'dart:typed_data'; import 'package:flutter_test/flutter_test.dart'; import 'package:uuid/uuid.dart'; @@ -71,6 +72,70 @@ Future _expectSize(String name, CropResizeParameters cropResize, expect(int.parse(v['height'].toString()), height); } +/// A lossless job with the source's real pixel format, for comparisons where +/// the encoder must not be part of what is measured. +/// +/// The plain [_job] above encodes H.264 and leaves `inputPixelFormat` unset, so +/// the decoder normalises to yuv420p. Fine for "is the output 640x480", useless +/// for "is this the same picture" — the codec and the chroma round-trip would +/// both land in the difference. +VideoJob _losslessJob( + String name, + CropResizeParameters cropResize, { + int startFrame = 0, + int endFrame = 6, +}) => + VideoJob( + id: const Uuid().v4(), + inputPath: WorkerHarness.inputFile, + outputPath: '$_outDir/$name.mkv', + processingPipeline: ProcessingPipeline( + deinterlace: const QTGMCParameters(enabled: false), + cropResize: cropResize, + ), + encodingSettings: const EncodingSettings( + codec: VideoCodec.ffv1, + container: ContainerFormat.mkv, + audioMode: AudioMode.none, + ), + inputWidth: _srcWidth, + inputHeight: _srcHeight, + inputPixelFormat: 'yuv422p', + inputFrameRate: 25.0, + totalFrames: endFrame - startFrame + 1, + startFrame: startFrame, + endFrame: endFrame, + ); + +/// Crop a packed rgb24 frame, in Dart. +/// +/// The reference a crop is judged against is built by cropping the pipeline's +/// OWN uncropped output rather than by asking ffmpeg to crop the source, and +/// that is not incidental: `interlaced_test.avi` declares 79 frames but only 75 +/// decode — it carries null frames, which the decoder's default CFR mode expands +/// onto the 25 fps grid. So `select=eq(n,N)` on the source counts decoded frames +/// while the pipeline counts timeline positions, and the two diverge wherever a +/// null frame sits. Comparing pipeline output against pipeline output uses one +/// decoder, one indexing scheme, and isolates the crop itself. +Uint8List _cropRgb24( + Uint8List src, { + required int srcWidth, + required int x, + required int y, + required int width, + required int height, +}) { + final srcHeight = src.length ~/ (srcWidth * 3); + expect(x + width, lessThanOrEqualTo(srcWidth), reason: 'crop past right edge'); + expect(y + height, lessThanOrEqualTo(srcHeight), reason: 'crop past bottom'); + final out = Uint8List(width * height * 3); + for (var row = 0; row < height; row++) { + final from = ((y + row) * srcWidth + x) * 3; + out.setRange(row * width * 3, (row + 1) * width * 3, src, from); + } + return out; +} + void main() { setUpAll(() async { await WorkerHarness.ensureReady(); @@ -171,4 +236,223 @@ void main() { }, timeout: const Timeout(Duration(minutes: 5))); } }); + + group('cropping', () { + // Crop was exercised by the pipeline tests but nothing asserted the size it + // produced — they ran a crop and checked the job succeeded, which passes + // just as happily on a crop that did nothing. + test('an asymmetric crop removes exactly the requested edges', () async { + // Deliberately asymmetric on both axes. A symmetric crop cannot tell a + // correct implementation from one that swaps left/right or top/bottom, + // and a size-only assertion cannot tell either regardless. + const l = 16, r = 8, t = 12, b = 4; + const w = 720 - l - r; // 696 + const h = 576 - t - b; // 560 + + await _expectSize( + 'crop_asymmetric', + const CropResizeParameters( + enabled: true, + cropEnabled: true, + cropLeft: l, + cropRight: r, + cropTop: t, + cropBottom: b, + ), + width: w, + height: h, + ); + }); + + test('the crop is taken from the right offset, not just the right size', + () async { + // A size assertion cannot tell a correct crop from one that swaps left + // with right or top with bottom — both produce exactly the requested + // dimensions. This compares the pixels. + const l = 16, r = 8, t = 12, b = 4; + const w = 720 - l - r, h = 576 - t - b; + const frame = 3; + + final uncropped = await WorkerHarness.runJob( + _losslessJob('crop_none', const CropResizeParameters(enabled: false)) + .toJson(), + label: 'crop_none'); + expect(uncropped.success, isTrue, reason: uncropped.error); + + final cropped = await WorkerHarness.runJob( + _losslessJob( + 'crop_offset', + const CropResizeParameters( + enabled: true, + cropEnabled: true, + cropLeft: l, + cropRight: r, + cropTop: t, + cropBottom: b, + ), + ).toJson(), + label: 'crop_offset'); + expect(cropped.success, isTrue, reason: cropped.error); + + final full = await WorkerHarness.frameRgb24(uncropped.outputPath!, frame, + label: 'full'); + final actual = await WorkerHarness.frameRgb24(cropped.outputPath!, frame, + label: 'cropped'); + + double diffAt(int x, int y) => WorkerHarness.meanAbsDiff( + actual, + _cropRgb24(full, + srcWidth: 720, x: x, y: y, width: w, height: h), + ); + + // The requested offset, and the two a swap would produce. + final correct = diffAt(l, t); + final swappedH = diffAt(r, t); + final swappedV = diffAt(l, b); + print(' crop offset diffs — correct ($l,$t): ' + '${correct.toStringAsFixed(2)}, ' + 'left/right swapped ($r,$t): ${swappedH.toStringAsFixed(2)}, ' + 'top/bottom swapped ($l,$b): ${swappedV.toStringAsFixed(2)}'); + + // Not zero, and it cannot be: the two frames are converted to rgb24 at + // different widths, so chroma interpolation at the edges of a 4:2:2 clip + // differs slightly between them. Measured ~1.1 against ~10 and ~16 for + // the swaps, so the margin below carries the assertion and the absolute + // bound is only a backstop. + expect(correct, lessThan(3.0), + reason: 'the cropped output should be the region at ($l, $t) of the ' + 'uncropped output, to within the rgb conversion'); + expect(correct * 3, lessThan(swappedH), + reason: 'the crop matches the left/right-swapped offset almost as ' + 'well as the requested one, so the horizontal edges are suspect'); + expect(correct * 3, lessThan(swappedV), + reason: 'the crop matches the top/bottom-swapped offset almost as ' + 'well as the requested one, so the vertical edges are suspect'); + }); + + test('crop composes with resize, in that order', () async { + // Crop first, then scale the cropped picture to the target — so the + // target size is what comes out, whatever the crop was. If the order were + // reversed the crop would eat into the target and the output would be + // smaller than asked for. + await _expectSize( + 'crop_then_resize', + const CropResizeParameters( + enabled: true, + cropEnabled: true, + cropLeft: 16, + cropRight: 16, + cropTop: 8, + cropBottom: 8, + resizeEnabled: true, + targetWidth: 640, + targetHeight: 480, + maintainAspect: false, + kernel: ResizeKernel.spline36, + ), + width: 640, + height: 480, + ); + }); + }); + + group('the preview shows the same geometry as the render', () { + // The analogue of the frame-mapping preview test in + // integration_frame_mapping_test.dart: crop and resize are computed inside + // the .vpy, and the encode and the preview are separate scripts. A pass + // that reached one template and not the other renders a preview at a + // different size or framing than the file the user gets — silently, since + // both are individually valid pictures. The output-format block was in the + // encode template only for exactly this reason once already. + Future expectPreviewMatchesRender( + String name, + CropResizeParameters cropResize, { + required int width, + required int height, + int sourceFrame = 3, + }) async { + final job = _losslessJob(name, cropResize); + final result = await WorkerHarness.runJob(job.toJson(), label: name); + expect(result.success, isTrue, reason: result.error); + + final v = await WorkerHarness.firstStream(result.outputPath!, + selector: 'v:0', entries: ['width', 'height']); + expect(int.parse(v!['width'].toString()), width); + expect(int.parse(v['height'].toString()), height); + + final pv = await WorkerHarness.runPreview(job.toJson(), + frame: sourceFrame, label: '$name-pv'); + expect(pv.success, isTrue, reason: pv.toString()); + final preview = + await WorkerHarness.imageToRgb24(pv.png!, label: '$name-pv'); + + // No geometry pass changes the frame count, so the preview of source + // frame N is output frame N. meanAbsDiff throws on a size mismatch, which + // is the assertion that matters most here — a preview at a different + // resolution is the failure this test exists to catch, and it surfaces as + // a buffer-length difference rather than a pixel difference. + final rendered = await WorkerHarness.frameRgb24( + result.outputPath!, sourceFrame, + label: '$name-out'); + final diff = WorkerHarness.meanAbsDiff(preview, rendered); + print(' $name: preview vs render meanAbsDiff ${diff.toStringAsFixed(2)} ' + 'at ${width}x$height'); + expect(diff, lessThan(2.0), + reason: 'the preview and the render disagree about the picture at ' + 'the same frame, despite agreeing on the size'); + } + + test('a crop reaches the preview path', () async { + await expectPreviewMatchesRender( + 'pv_crop', + const CropResizeParameters( + enabled: true, + cropEnabled: true, + cropLeft: 16, + cropRight: 8, + cropTop: 12, + cropBottom: 4, + ), + width: 696, + height: 560, + ); + }); + + test('a resize reaches the preview path', () async { + await expectPreviewMatchesRender( + 'pv_resize', + const CropResizeParameters( + enabled: true, + resizeEnabled: true, + targetWidth: 512, + targetHeight: 384, + maintainAspect: false, + kernel: ResizeKernel.spline36, + ), + width: 512, + height: 384, + ); + }); + + test('crop and resize together reach the preview path', () async { + await expectPreviewMatchesRender( + 'pv_crop_resize', + const CropResizeParameters( + enabled: true, + cropEnabled: true, + cropLeft: 16, + cropRight: 16, + cropTop: 8, + cropBottom: 8, + resizeEnabled: true, + targetWidth: 640, + targetHeight: 480, + maintainAspect: false, + kernel: ResizeKernel.spline36, + ), + width: 640, + height: 480, + ); + }); + }); } diff --git a/app/test/support/worker_harness.dart b/app/test/support/worker_harness.dart index 2a765db..cda41bb 100644 --- a/app/test/support/worker_harness.dart +++ b/app/test/support/worker_harness.dart @@ -552,6 +552,43 @@ class WorkerHarness { return total / a.length; } + /// One frame of a video as packed rgb24, addressed by absolute frame index. + /// + /// Decodes from the start and picks by index rather than seeking: an input + /// seek lands on the nearest keyframe, which for these comparisons would + /// silently compare the wrong frame. The fixtures are short enough that the + /// full decode costs little. + /// + /// Pairs with [imageToRgb24] and [meanAbsDiff] to answer "is this the frame + /// the pipeline says it is" rather than merely "did something render". + static Future frameRgb24(String videoPath, int index, + {String label = 'frame'}) async { + final png = File(p.join(Directory.systemTemp.path, + 'vb_${label}_${DateTime.now().microsecondsSinceEpoch}_$index.png')); + try { + final r = await Process.run( + ffmpegPath, + [ + '-y', '-v', 'error', + '-i', videoPath, + '-vf', 'select=eq(n\\,$index)', + '-frames:v', '1', + png.path, + ], + environment: ffmpegEnv, + ); + if (r.exitCode != 0) { + throw Exception('extracting frame $index from $videoPath: ${r.stderr}'); + } + if (!await png.exists()) { + throw Exception('no frame $index in $videoPath (past the end?)'); + } + return imageToRgb24(await png.readAsBytes(), label: '$label$index'); + } finally { + if (await png.exists()) await png.delete().catchError((_) => png); + } + } + /// Extract one frame as PNG and return its md5 hash. static Future frameHash(String videoPath, {int frame = 5}) async { final framePath = p.join(Directory.systemTemp.path,