diff --git a/CLAUDE.md b/CLAUDE.md index efb1d02..aeb410b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -319,6 +319,9 @@ Adding a filter touches many files. Missing any step causes silent failures (fil `pass_list_stages_test.dart` fails if one is missed. Stages are labels over the **existing pipeline order**, so a pass goes in the stage its position already falls in; never reorder rows to suit a grouping. + The README's "The filter pipeline" table is the user-facing statement of the + same order — add the pass's row at its position there too; + `readme_pipeline_order_test.dart` fails if the table and `stages` disagree. - `app/lib/views/pass_list/pass_list_item.dart` — add icon in `_getIconForPass()` - `app/lib/views/pass_settings/pass_settings_inline.dart` — add case in `_getFilterId()` - `app/lib/viewmodels/main_viewmodel.dart` — add case in BOTH `_convertToParams()` AND `_updatePipelineFromDynamic()` diff --git a/README.md b/README.md index 27fab7f..5de2655 100644 --- a/README.md +++ b/README.md @@ -93,37 +93,44 @@ GPU-accelerated deinterlacing (NNEDI3CL) needs your GPU's OpenCL driver installe ## The filter pipeline -Twenty-one filters, each switchable independently, applied in a fixed order. Most sources need none or a few. - -| Filter | What it addresses | -|--------|-------------------| -| **Deinterlace** | Comb-like jagged edges on moving objects. QTGMC for interlaced video, IVTC to recover the original film frames from telecined DVD, or Bwdif when you want it done in a fraction of the time. | -| **Edge Repair** | The dirty rows and columns at the very edge of a tape capture — rebuilt from the picture just inside, instead of cropped away. | -| **Ghost Removal** | A faint second copy of the picture shifted sideways, left behind by an aerial or a long cable run. | -| **Deflicker** | Brightness pulsing between frames, which is what scanned cine film almost always has. | -| **DeScratch** | Vertical scratch lines on scanned film. | -| **SpotLess** | Dust, dirt and single-frame specks. RemoveDirt is the faster choice — around six times the speed for about 60% of the removal. | -| **Noise Reduction** | Grain and video noise across the whole frame. Motion-compensated by default, with mClean as a gentler alternative that keeps more detail; DFTTest, FFT3DFilter, TTempSmooth, FluxSmooth, STPresso, TemporalDegrain2 and a large-window median are available under advanced options for noise the default handles badly. | -| **Chroma Denoise** | Blotchy, smeared color — common on VHS captures and old camcorder footage. Leaves luma detail untouched. | -| **Dehalo** | Bright outlines around edges, ringing, and residual ghosting left by a deinterlacer. HQDeringmod targets ringing specifically. | -| **Deblock** | Square blocking from heavy compression, and the ringing around edges that comes with it. | -| **Deband** | Visible steps in gradients and skies. | -| **Anti-Aliasing** | Stair-stepping on diagonal edges, left by deinterlacing or upscaling. Runs before sharpening, which would otherwise make the steps more visible. | -| **Stabilize** | Shake and weave — telecine wobble, jittery film scans, handheld footage. Runs last before cropping, so a small crop removes the edges it exposes. | -| **Film Grain** | Grain added back after denoising, so the picture is not left plastic — and to hide banding in skies and fades. | -| **Rotate / Flip** | Footage shot sideways, mirrored captures, scans that came off the scanner the wrong way round. | -| **Sharpen** | Soft sources needing edge and fine detail recovery. aWarpSharp2 sharpens by warping edges instead of raising contrast, so it adds no halos. | -| **Chroma Fixes** | Colour that sits sideways from the picture (corrected automatically or by hand), bleeding past edges, rainbowing and dot crawl — including the shimmering kind that only shows when the picture moves — and residual combing. Each repair has its own switch, and its settings appear only once it is on. | -| **Color Correction** | Brightness, contrast, saturation, hue, levels, white balance (warm/cool, green/magenta), and lifting detail out of the shadows of underexposed footage. Levels and white balance can each be measured automatically or set by hand. | -| **Crop & Resize** | Trimming overscan, scaling, and edge-directed upscaling — plus bars in a colour of your choice to bring a cropped picture back to an exact frame size (720×576 for PAL DVD, 720×480 for NTSC) without rescaling it. | -| **Frame Rate** | Converting between PAL and NTSC rates, for a tape that was already converted once and now plays at the wrong speed. | -| **Subtitles** | Whisper AI speech-to-text — written alongside the video as `.srt`, embedded as a selectable track, burnt into the picture, or a combination. | +Twenty-one filters, each switchable independently. Most sources need none or a few. They always run in the order below, top to bottom, whichever ones are switched on — the same order the list in the app shows them in. The order cannot be rearranged. + +| # | Filter | What it addresses | +|---|--------|-------------------| +| 1 | **Deinterlace** | Comb-like jagged edges on moving objects. QTGMC for interlaced video, IVTC to recover the original film frames from telecined DVD, or Bwdif when you want it done in a fraction of the time. | +| 2 | **Edge Repair** | The dirty rows and columns at the very edge of a tape capture — rebuilt from the picture just inside, instead of cropped away. | +| 3 | **Ghost Removal** | A faint second copy of the picture shifted sideways, left behind by an aerial or a long cable run. | +| 4 | **Deflicker** | Brightness pulsing between frames, which is what scanned cine film almost always has. | +| 5 | **DeScratch** | Vertical scratch lines on scanned film. | +| 6 | **SpotLess** | Dust, dirt and single-frame specks. RemoveDirt is the faster choice — around six times the speed for about 60% of the removal. | +| 7 | **Noise Reduction** | Grain and video noise across the whole frame. Motion-compensated by default, with mClean as a gentler alternative that keeps more detail; DFTTest, FFT3DFilter, TTempSmooth, FluxSmooth, STPresso, TemporalDegrain2 and a large-window median are available under advanced options for noise the default handles badly. | +| 8 | **Chroma Denoise** | Blotchy, smeared color — common on VHS captures and old camcorder footage. Leaves luma detail untouched. | +| 9 | **Dehalo** | Bright outlines around edges, ringing, and residual ghosting left by a deinterlacer. HQDeringmod targets ringing specifically. | +| 10 | **Deblock** | Square blocking from heavy compression, and the ringing around edges that comes with it. | +| 11 | **Deband** | Visible steps in gradients and skies. | +| 12 | **Anti-Aliasing** | Stair-stepping on diagonal edges, left by deinterlacing or upscaling. Runs before sharpening, which would otherwise make the steps more visible. | +| 13 | **Chroma Fixes** | Colour that sits sideways from the picture (corrected automatically or by hand), bleeding past edges, rainbowing and dot crawl — including the shimmering kind that only shows when the picture moves — and residual combing. Each repair has its own switch, and its settings appear only once it is on. | +| 14 | **Color Correction** | Brightness, contrast, saturation, hue, levels, white balance (warm/cool, green/magenta), and lifting detail out of the shadows of underexposed footage. Levels and white balance can each be measured automatically or set by hand. | +| 15 | **Stabilize** | Shake and weave — telecine wobble, jittery film scans, handheld footage. Runs last before cropping, so a small crop removes the edges it exposes. | +| 16 | **Rotate / Flip** | Footage shot sideways, mirrored captures, scans that came off the scanner the wrong way round. | +| 17 | **Crop & Resize** | Trimming overscan, scaling, and edge-directed upscaling — plus bars in a colour of your choice to bring a cropped picture back to an exact frame size (720×576 for PAL DVD, 720×480 for NTSC) without rescaling it. | +| 18 | **Sharpen** | Soft sources needing edge and fine detail recovery. aWarpSharp2 sharpens by warping edges instead of raising contrast, so it adds no halos. Runs after the resize, on the picture that is actually delivered, and before any added grain. | +| 19 | **Film Grain** | Grain added back after denoising, so the picture is not left plastic — and to hide banding in skies and fades. | +| 20 | **Frame Rate** | Converting between PAL and NTSC rates, for a tape that was already converted once and now plays at the wrong speed. | +| 21 | **Subtitles** | Whisper AI speech-to-text — written alongside the video as `.srt`, embedded as a selectable track, burnt into the picture, or a combination. | + +A few things about the order are worth knowing: + +- **Crop & Resize is one step, cropping first.** It runs after Stabilize and Rotate / Flip, so a small crop removes the thin edges stabilising exposes, and the four crop sides are the sides of the picture after any rotation. Everything before it works on the uncropped frame. +- **Clean up, then frame, then finish.** Damage and noise are removed first, colour is corrected next, the picture is stabilised, turned and resized, and only then sharpened. Sharpening earlier would have its work softened again by the resize. +- **Grain goes on after sharpening**, so it is neither sharpened into grit nor resampled away, and the frame rate is converted last of all, so every other filter works on real frames rather than invented ones. +- **After the filters:** any Custom VapourSynth code runs next, then the conversion to the output colour format, then added borders — last, so that nothing touches the bars. Subtitles are transcribed from the source and added once the video has been encoded. Each filter leads with a plain-language summary and a **More** expander describing what it does and when it's the right choice, so the settings can be understood in place rather than looked up elsewhere. The list also reacts to the file you dropped in. Filters that match what was detected in your source are marked **Suggested** with the reason — "source is hard telecine (3:2 pulldown)", "anamorphic source (10:11) — check pixel aspect" — and ones that can't apply say so, such as deinterlacing a progressive file. Nothing is switched on or off for you; detection is sometimes wrong, so it stays a hint. Filters whose problems can't be spotted from the file alone — dirt, scratches, grain, halos — say nothing either way. -Where two filters work against each other, the one that loses out says so when you open it: sharpening ahead of a denoiser that will undo it, for instance. +Where two filters work against each other, the one that loses out says so when you open it: sharpening that would put back the halos Dehalo has just removed, for instance. ## Details diff --git a/app/lib/models/pass_advice.dart b/app/lib/models/pass_advice.dart index 5c083fe..bb94c4e 100644 --- a/app/lib/models/pass_advice.dart +++ b/app/lib/models/pass_advice.dart @@ -16,8 +16,8 @@ class PassAdvice { /// /// At a dozen-odd passes the complexity that actually costs users is not the /// length of the list, it is *interaction*: two denoisers stacked until the -/// picture is plastic, sharpening applied before the denoiser eats it again, -/// grain re-added before the deband that will smooth it away. None of that is +/// picture is plastic, sharpening that puts back the halos Dehalo just removed, +/// sharpening that exaggerates the grain Deband added. None of that is /// an error — every combination here produces a valid render — so none of it can /// be caught by validation. It has to be said out loud at the point the user is /// looking. @@ -29,12 +29,16 @@ class PassAdvice { /// app's job is to make sure they meant it. /// - **Only fires on enabled passes.** Advice about a pass nobody has turned on /// is noise. -/// - **Says what to do, not just what is wrong.** "Sharpen runs before Noise -/// Reduction" is a fact; the useful half is that the denoiser will undo it. +/// - **Says what to do, not just what is wrong.** "Dehalo runs before Sharpen" +/// is a fact; the useful half is that sharpening can put the halos back. +/// - **Never states an order the pipeline does not have.** Every "runs +/// before/after" below is pinned against `enabledPasses` in +/// `pass_advice_test.dart`, because a wrong one reads as a design flaw in the +/// app rather than as a typo (issue #109). /// /// Pass order is fixed (see `PassListPanel.stages` and `script_generator.rs`), /// which is what makes the ordering advice statable at all: Sharpen genuinely -/// always runs before Color Correction, so there is no case to qualify. +/// always runs after Noise Reduction, so there is no case to qualify. List adviseOn(ProcessingPipeline pipeline) { final advice = []; @@ -49,18 +53,13 @@ List adviseOn(ProcessingPipeline pipeline) { // combination. Deliberately silent. } - // --- Sharpening fights the denoiser, and loses --- - // Sharpen runs *before* Noise Reduction in the pipeline, so a denoiser set - // strongly enough to matter will remove most of what was just sharpened, and - // amplified noise is what survives. - if (on(PassType.sharpen) && on(PassType.noiseReduction)) { - advice.add(const PassAdvice( - PassType.sharpen, - 'Sharpen runs before Noise Reduction, so the denoiser will soften much ' - 'of this again — and sharpened noise is what it has to work on. ' - 'Consider denoising alone first and judging the result.', - )); - } + // --- Sharpening after the denoiser --- + // Deliberately silent, and asserted so in pass_advice_test.dart. Sharpen runs + // *after* Noise Reduction — after every clean-up pass and the resize, in + // fact, with only Grain and Frame Rate behind it — which is the order a + // restoration chain wants: there is nothing to warn about. This used to claim + // the reverse and told users the denoiser would undo their sharpening + // (issue #109) — it never did. // --- Deband after grain --- // Deband runs after the denoiser but the f3kdb grain it adds back is applied diff --git a/app/lib/models/processing_pipeline.dart b/app/lib/models/processing_pipeline.dart index f15e23a..66f3bcd 100644 --- a/app/lib/models/processing_pipeline.dart +++ b/app/lib/models/processing_pipeline.dart @@ -254,10 +254,9 @@ class ProcessingPipeline { /// Get the ordered list of enabled passes. List get enabledPasses { final passes = []; - // Order: Crop first (pre-processing), then deinterlace, noise, dehalo, deblock, deband, sharpen, chroma, color, resize last - if (cropResize.enabled && cropResize.cropEnabled) { - passes.add(PassType.cropResize); // Pre-crop - } + // Order: deinterlace, damage, noise, dehalo, deblock, deband, anti-alias, + // chroma, color, stabilize, rotate, crop + resize, then sharpen, grain, + // frame rate. if (deinterlace.enabled) { passes.add(PassType.deinterlace); } @@ -304,9 +303,6 @@ class ProcessingPipeline { if (antiAlias.enabled) { passes.add(PassType.antiAlias); } - if (sharpen.enabled) { - passes.add(PassType.sharpen); - } if (chromaFixes.enabled) { passes.add(PassType.chromaFixes); } @@ -324,11 +320,19 @@ class ProcessingPipeline { if (geometry.hasEffect) { passes.add(PassType.geometry); } - if (cropResize.enabled && cropResize.resizeEnabled) { - // Resize (post-processing) - if not already added for crop - if (!passes.contains(PassType.cropResize)) { - passes.add(PassType.cropResize); - } + // Crop and resize are one pass and run together, crop first. The crop used + // to run ahead of everything else, which meant it could not remove the + // edges Stabilize exposes (issue #109). + if (cropResize.enabled && + (cropResize.cropEnabled || cropResize.resizeEnabled)) { + passes.add(PassType.cropResize); + } + // Sharpening follows the resize (issue #109), so it works on the delivered + // pixels: sharpened earlier, a downscale softens the edges again and an + // upscale enlarges the halos. It still follows anti-aliasing, and precedes + // the grain it would otherwise exaggerate. + if (sharpen.enabled) { + passes.add(PassType.sharpen); } // Grain goes last of the video passes: added before the resize it is // resampled away, before the deband it is smoothed away. diff --git a/app/lib/views/pass_list/pass_list_panel.dart b/app/lib/views/pass_list/pass_list_panel.dart index dc4c843..cb7b290 100644 --- a/app/lib/views/pass_list/pass_list_panel.dart +++ b/app/lib/views/pass_list/pass_list_panel.dart @@ -58,7 +58,6 @@ class PassListPanel extends StatelessWidget { title: 'Detail & Color', passes: [ PassType.antiAlias, - PassType.sharpen, PassType.chromaFixes, PassType.colorCorrection, ], @@ -69,7 +68,7 @@ class PassListPanel extends StatelessWidget { ), ( title: 'Finishing', - passes: [PassType.grain, PassType.frameRate], + passes: [PassType.sharpen, PassType.grain, PassType.frameRate], ), ( title: 'Post-Processing', diff --git a/app/test/integration_new_passes_test.dart b/app/test/integration_new_passes_test.dart index ace9d2f..576ec97 100644 --- a/app/test/integration_new_passes_test.dart +++ b/app/test/integration_new_passes_test.dart @@ -23,6 +23,7 @@ import 'package:vapourbox/models/chroma_denoise_parameters.dart'; import 'package:vapourbox/models/dehalo_parameters.dart'; import 'package:vapourbox/models/chroma_fix_parameters.dart'; import 'package:vapourbox/models/color_correction_parameters.dart'; +import 'package:vapourbox/models/crop_resize_parameters.dart'; import 'package:vapourbox/models/descratch_parameters.dart'; import 'package:vapourbox/models/deblock_parameters.dart'; import 'package:vapourbox/models/grain_parameters.dart'; @@ -235,6 +236,66 @@ void main() { await _expectValidVideo(result); }, timeout: const Timeout(Duration(minutes: 6))); + // Issue #109: Sharpen moved to after the resize. Every method has to run + // on a clip the resize has already changed the size of, in the encode — the + // script-only tests prove the order, not that the plugins accept it. + for (final method in SharpenMethod.values) { + test('sharpen: ${method.name} runs after a resize', () async { + final label = 'sharpen_after_resize_${method.name}'; + final job = _baseJob( + label, + pipeline: ProcessingPipeline( + deinterlace: const QTGMCParameters(enabled: false), + cropResize: const CropResizeParameters( + enabled: true, + resizeEnabled: true, + targetWidth: 640, + targetHeight: 480, + ), + sharpen: SharpenParameters(enabled: true, method: method), + ), + ); + final result = await WorkerHarness.runJob(job.toJson(), label: label); + await _expectValidVideo(result); + final v = await WorkerHarness.firstStream(result.outputPath!, + selector: 'v:0', entries: ['width', 'height']); + // 720x576 fitted inside 640x480 with the stored aspect kept: 600x480. + expect(v?['width'], 600, reason: 'sharpening must not undo the resize'); + expect(v?['height'], 480); + }, timeout: const Timeout(Duration(minutes: 6))); + } + + // Issue #109: the crop used to run before everything else. It now runs + // with the resize, after Stabilize and Rotate / Flip, so its four sides + // are the sides of the turned picture. The frame size tells the two orders + // apart: 720x576 turned is 576x720, and cropping 16 off each side and 8 off + // top and bottom leaves 544x704. Cropped before the turn it was 560x688. + test('crop: runs after stabilize and rotation', () async { + const label = 'crop_after_rotation'; + final job = _baseJob( + label, + pipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters(enabled: false), + stabilize: StabilizeParameters(enabled: true), + geometry: GeometryParameters(enabled: true, rotation: Rotation.cw90), + cropResize: CropResizeParameters( + enabled: true, + cropEnabled: true, + cropLeft: 16, + cropRight: 16, + cropTop: 8, + cropBottom: 8, + ), + ), + ); + final result = await WorkerHarness.runJob(job.toJson(), label: label); + await _expectValidVideo(result); + final v = await WorkerHarness.firstStream(result.outputPath!, + selector: 'v:0', entries: ['width', 'height']); + expect(v?['width'], 544); + expect(v?['height'], 704); + }, timeout: const Timeout(Duration(minutes: 6))); + test('dehalo: HQDeringmod runs end-to-end', () async { final job = _baseJob( 'dehalo_hqderingmod', diff --git a/app/test/pass_advice_test.dart b/app/test/pass_advice_test.dart index e7cd3fc..b1df7cb 100644 --- a/app/test/pass_advice_test.dart +++ b/app/test/pass_advice_test.dart @@ -50,22 +50,24 @@ void main() { }); }); - group('sharpening against the denoiser', () { - test('warns on the Sharpen pass, naming the consequence', () { - final advice = adviceFor( - PassType.sharpen, - ProcessingPipeline( + group('sharpening with the denoiser', () { + // Issue #109: this pair used to warn that "Sharpen runs before Noise + // Reduction, so the denoiser will soften much of this again". It does not + // — Sharpen runs after it — so the warning described a flaw the pipeline + // does not have. + test('is silent, since Sharpen already runs after Noise Reduction', () { + expect( + adviseOn(ProcessingPipeline( deinterlace: const QTGMCParameters(enabled: false), noiseReduction: NoiseReductionParameters.fromPreset(NoiseReductionPreset.heavy), sharpen: const SharpenParameters(enabled: true), - ), + )), + isEmpty, ); - expect(advice, isNotNull); - expect(advice, contains('denoiser')); }); - test('silent when only one of the two is on', () { + test('silent when only Sharpen is on', () { expect( adviceFor( PassType.sharpen, @@ -79,6 +81,35 @@ void main() { }); }); + group('the order the advice talks about is the order the pipeline has', () { + // The advice states pass order as fact. These pin each statement to + // enabledPasses, so the text and the pipeline cannot drift apart again. + final order = ProcessingPipeline( + deinterlace: const QTGMCParameters(enabled: false), + noiseReduction: + NoiseReductionParameters.fromPreset(NoiseReductionPreset.moderate), + chromaDenoise: const ChromaDenoiseParameters(enabled: true), + dehalo: const DehaloParameters(enabled: true), + deband: const DebandParameters(enabled: true), + sharpen: const SharpenParameters(enabled: true), + ).enabledPasses; + + test('Sharpen runs after every clean-up pass', () { + final sharpen = order.indexOf(PassType.sharpen); + expect(sharpen, isNonNegative); + for (final earlier in const [ + PassType.noiseReduction, + PassType.chromaDenoise, + PassType.dehalo, + PassType.deband, + ]) { + expect(order.indexOf(earlier), isNonNegative, reason: '$earlier'); + expect(order.indexOf(earlier), lessThan(sharpen), + reason: '$earlier must run before Sharpen'); + } + }); + }); + group('dehalo against sharpening', () { test('warns on Sharpen, since Dehalo runs first', () { final advice = adviceFor( diff --git a/app/test/pass_list_stages_test.dart b/app/test/pass_list_stages_test.dart index 247c35c..88c8b93 100644 --- a/app/test/pass_list_stages_test.dart +++ b/app/test/pass_list_stages_test.dart @@ -48,12 +48,12 @@ void main() { PassType.deblock, PassType.deband, PassType.antiAlias, - PassType.sharpen, PassType.chromaFixes, PassType.colorCorrection, PassType.stabilize, PassType.geometry, PassType.cropResize, + PassType.sharpen, PassType.grain, PassType.frameRate, PassType.subtitles, @@ -72,6 +72,15 @@ void main() { lessThan(indexOf(PassType.sharpen))); }); + test('sharpening runs after the resize and before the grain', () { + // Issue #109. Sharpened before the resize, a downscale softens the edges + // again and an upscale enlarges the halos; sharpened after the grain, the + // grain is exaggerated. + expect(indexOf(PassType.sharpen), + greaterThan(indexOf(PassType.cropResize))); + expect(indexOf(PassType.sharpen), lessThan(indexOf(PassType.grain))); + }); + test('rotation settles before any framing decision', () { // A quarter turn swaps width and height, so crop, resize and the aspect // declaration must all see the final shape. diff --git a/app/test/readme_pipeline_order_test.dart b/app/test/readme_pipeline_order_test.dart new file mode 100644 index 0000000..9e15f6e --- /dev/null +++ b/app/test/readme_pipeline_order_test.dart @@ -0,0 +1,52 @@ +// The README's "The filter pipeline" table is where users are told what order +// the filters run in. It is prose, so nothing stops it drifting from the +// pipeline — and a documented order that is wrong is worse than none: issue +// #109 was a user reasoning from an in-app message that misstated the order. +// +// This pins the table, row for row, to PassListPanel.stages, which +// pass_list_stages_test.dart in turn pins to the pipeline. +// +// Run with: flutter test test/readme_pipeline_order_test.dart + +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:vapourbox/models/processing_pipeline.dart'; +import 'package:vapourbox/views/pass_list/pass_list_panel.dart'; + +/// The README writes a couple of names differently from the pass list. +const _readmeNames = { + PassType.cropResize: 'Crop & Resize', +}; + +void main() { + final readme = File('../README.md').readAsLinesSync(); + final row = RegExp(r'^\| (\d+) \| \*\*(.+?)\*\* \|'); + + final start = readme.indexOf('## The filter pipeline'); + final rows = [ + for (final line in readme.skip(start + 1).takeWhile((l) => !l.startsWith('## '))) + if (row.firstMatch(line) case final m?) (int.parse(m.group(1)!), m.group(2)!), + ]; + + test('the README has a filter pipeline table', () { + expect(start, isNonNegative); + expect(rows, isNotEmpty); + }); + + test('the table lists every filter in the order it runs', () { + final expected = [ + for (final stage in PassListPanel.stages) + for (final pass in stage.passes) + _readmeNames[pass] ?? pass.displayName, + ]; + expect(rows.map((r) => r.$2).toList(), expected, + reason: 'README.md "The filter pipeline" must list the filters in ' + 'pipeline order — see PassListPanel.stages'); + }); + + test('the rows are numbered 1..n without gaps', () { + expect(rows.map((r) => r.$1).toList(), + [for (var i = 1; i <= rows.length; i++) i]); + }); +} diff --git a/docs/ENGINEERING_NOTES.md b/docs/ENGINEERING_NOTES.md index b980c3b..b4ebe6e 100644 --- a/docs/ENGINEERING_NOTES.md +++ b/docs/ENGINEERING_NOTES.md @@ -884,8 +884,32 @@ Two orderings are load-bearing and asserted from both sides - **Anti-aliasing runs before Sharpen.** Sharpening a stair-stepped edge makes the stepping more visible, not less. +- **Sharpen runs after Crop/Resize and before Grain** (added 2026-10-05, issue + #109 — it used to sit straight after Anti-Aliasing, ahead of Chroma Fixes, + Colour Correction, Stabilize and the resize). Resampling a sharpened picture + softens it again on a downscale and enlarges the sharpening halos on an + upscale, and Stabilize's sub-pixel shifts soften it too. Consequences worth + knowing: Sharpen now works at the **output** resolution, so it costs more + when upscaling and less when downscaling, and existing presets and saved + jobs that combine Sharpen with a resize render slightly differently than + before. The same issue also removed a `pass_advice.dart` warning that claimed + Sharpen ran *before* Noise Reduction — it never did. - **Stabilize runs last before Crop/Resize.** It shifts the picture within the frame and exposes thin empty edges, so a crop afterwards removes them. + **This was not true until 2026-10-05 (issue #109).** The *resize* ran after + Stabilize, but the crop was a separate `PRE_CROP` block at the top of both + templates, ahead of Deinterlace — so the reason given for the placement + described something the pipeline did not do, in this file, the README, the + model comments and `pass_list_stages_test.dart` alike. The crop is now a + `CROP` block directly before `RESIZE`, after Stabilize and Rotate / Flip. + What that costs, deliberately: every earlier pass works on the uncropped + frame (slower, by the cropped area), Edge Repair rebuilds the edge of the + *uncropped* frame, and the automatic measurements (levels, white balance, + chroma alignment) see whatever the crop was going to remove — head-switching + noise along the bottom of a VHS capture included. Crop values now refer to + the picture *after* a rotation, where they used to refer to the source. + `test_110` pins the script order; `integration_new_passes_test.dart` tells + the two orders apart by frame size on a rotated, cropped encode. > **`santiag`'s `type` is pinned to `nnedi3`.** havsfunc also accepts `eedi2` > and `sangnom`; **neither is in the deps bundle**, and naming an absent one diff --git a/worker/src/models/processing_pipeline.rs b/worker/src/models/processing_pipeline.rs index 827caf5..6de91b5 100644 --- a/worker/src/models/processing_pipeline.rs +++ b/worker/src/models/processing_pipeline.rs @@ -337,10 +337,9 @@ impl ProcessingPipeline { pub fn enabled_passes(&self) -> Vec { let mut passes = Vec::new(); - // Order: Crop first (pre-processing), then deinterlace, noise, dehalo, deblock, deband, sharpen, chroma, color, resize last - if self.crop_resize.enabled && self.crop_resize.crop_enabled { - passes.push(PassType::CropResize); // Pre-crop - } + // Order: deinterlace, damage, noise, dehalo, deblock, deband, anti-alias, + // chroma, color, stabilize, rotate, crop + resize, then sharpen, grain, + // frame rate. if self.deinterlace_enabled() { passes.push(PassType::Deinterlace); } @@ -390,9 +389,6 @@ impl ProcessingPipeline { if self.anti_alias.enabled { passes.push(PassType::AntiAlias); } - if self.sharpen.enabled { - passes.push(PassType::Sharpen); - } if self.chroma_fixes.enabled { passes.push(PassType::ChromaFixes); } @@ -410,11 +406,21 @@ impl ProcessingPipeline { if self.geometry.has_effect() { passes.push(PassType::Geometry); } - if self.crop_resize.enabled && self.crop_resize.resize_enabled { - // Resize (post-processing) - if not already added for crop - if !passes.contains(&PassType::CropResize) { - passes.push(PassType::CropResize); - } + // Crop and resize are one pass and run together, crop first. The crop + // used to run ahead of everything else, which meant it could not + // remove the edges Stabilize exposes (issue #109). + if self.crop_resize.enabled + && (self.crop_resize.crop_enabled || self.crop_resize.resize_enabled) + { + passes.push(PassType::CropResize); + } + + // Sharpening follows the resize (issue #109), so it works on the + // delivered pixels: sharpened earlier, a downscale softens the edges + // again and an upscale enlarges the halos. It still follows + // anti-aliasing, and precedes the grain it would otherwise exaggerate. + if self.sharpen.enabled { + passes.push(PassType::Sharpen); } // Grain goes last of the video passes. Added before the resize it is diff --git a/worker/src/script_generator.rs b/worker/src/script_generator.rs index 411107b..99a3540 100644 --- a/worker/src/script_generator.rs +++ b/worker/src/script_generator.rs @@ -362,19 +362,20 @@ impl ScriptGenerator { let params = &pipeline.deinterlace; // ==================================================================== - // PRE-CROP PASS + // CROP PASS (its block sits after Stabilize and Rotate / Flip, directly + // before the resize — the template's position is what orders it) // ==================================================================== let crop = &pipeline.crop_resize; if crop.enabled && crop.crop_enabled && (crop.crop_left > 0 || crop.crop_right > 0 || crop.crop_top > 0 || crop.crop_bottom > 0) { - script = script.replace("{{#PRE_CROP}}", ""); - script = script.replace("{{/PRE_CROP}}", ""); + script = script.replace("{{#CROP}}", ""); + script = script.replace("{{/CROP}}", ""); script = script.replace("{{CROP_LEFT}}", &crop.crop_left.to_string()); script = script.replace("{{CROP_RIGHT}}", &crop.crop_right.to_string()); script = script.replace("{{CROP_TOP}}", &crop.crop_top.to_string()); script = script.replace("{{CROP_BOTTOM}}", &crop.crop_bottom.to_string()); } else { - script = remove_block("{{#PRE_CROP}}", "{{/PRE_CROP}}", script); + script = remove_block("{{#CROP}}", "{{/CROP}}", script); } // ==================================================================== diff --git a/worker/templates/pipeline_template.vpy b/worker/templates/pipeline_template.vpy index d7b4ec0..06c084a 100644 --- a/worker/templates/pipeline_template.vpy +++ b/worker/templates/pipeline_template.vpy @@ -84,13 +84,6 @@ def _expr(clips, expr, **kwargs): return _akarin_expr(clips, expr, **kwargs) return core.std.Expr(clips, expr, **kwargs) -# ============================================================================ -# PASS 1: PRE-CROP (before deinterlacing to reduce processing area) -# ============================================================================ -{{#PRE_CROP}} -clip = core.std.Crop(clip, left={{CROP_LEFT}}, right={{CROP_RIGHT}}, top={{CROP_TOP}}, bottom={{CROP_BOTTOM}}) -{{/PRE_CROP}} - # ============================================================================ # PASS 2: DEINTERLACING # ============================================================================ @@ -1247,59 +1240,6 @@ clip = haf.santiag( {{/AA_SANTIAG}} {{/ANTI_ALIAS}} -# ============================================================================ -# PASS 7: SHARPEN -# ============================================================================ -{{#SHARPEN}} - -{{#SHARPEN_LSFMOD}} -# LSFmod - Limited Sharpening with overshoot control -clip = haf.LSFmod( - clip, -{{#SHARPEN_STRENGTH}} - strength={{SHARPEN_STRENGTH}}, -{{/SHARPEN_STRENGTH}} -{{#SHARPEN_OVERSHOOT}} - overshoot={{SHARPEN_OVERSHOOT}}, -{{/SHARPEN_OVERSHOOT}} -{{#SHARPEN_UNDERSHOOT}} - undershoot={{SHARPEN_UNDERSHOOT}}, -{{/SHARPEN_UNDERSHOOT}} -{{#SHARPEN_SOFT_EDGE}} - soft={{SHARPEN_SOFT_EDGE}}, -{{/SHARPEN_SOFT_EDGE}} -) -{{/SHARPEN_LSFMOD}} - -{{#SHARPEN_CAS}} -# CAS - Contrast Adaptive Sharpening -clip = core.cas.CAS( - clip, -{{#SHARPEN_CAS_SHARPNESS}} - sharpness={{SHARPEN_CAS_SHARPNESS}}, -{{/SHARPEN_CAS_SHARPNESS}} -) -{{/SHARPEN_CAS}} -{{#SHARPEN_AWARPSHARP2}} -# aWarpSharp2 - sharpens by warping pixels toward edges rather than raising -# local contrast, so it produces no halos. -# -# `chroma` and `planes` are deliberately not passed. This VapourSynth port takes -# chroma as 0 or 1 (NOT Avisynth's 0-6, where 4 means "warp chroma with the luma -# mask") and rejects anything else outright, and by default it processes luma -# only — the conventional use as a sharpener. Passing a guessed value here is how -# this block first shipped broken; expose it deliberately, with a test, or leave -# the plugin's own default alone. -clip = core.warp.AWarpSharp2( - clip, - thresh={{SHARPEN_WARP_THRESH}}, - blur={{SHARPEN_WARP_BLUR}}, - type={{SHARPEN_WARP_TYPE}}, - depth={{SHARPEN_WARP_DEPTH}}, -) -{{/SHARPEN_AWARPSHARP2}} -{{/SHARPEN}} - # ============================================================================ # PASS 8: CHROMA FIXES # ============================================================================ @@ -1760,6 +1700,19 @@ if clip.format.id != _geom_src_format.id: clip = core.resize.Spline36(clip, format=_geom_src_format.id) {{/GEOMETRY}} +# ============================================================================ +# PASS: CROP +# ============================================================================ +# Cropping runs here, with the resize, and not at the top of the script where it +# used to (issue #109). Stabilize shifts the picture and exposes thin edges, and +# a quarter turn swaps width and height, so the crop has to follow both: it then +# removes what Stabilize exposed, and its four sides mean the sides of the +# picture as it will be delivered. The cost is that every earlier pass works on +# the uncropped frame. +{{#CROP}} +clip = core.std.Crop(clip, left={{CROP_LEFT}}, right={{CROP_RIGHT}}, top={{CROP_TOP}}, bottom={{CROP_BOTTOM}}) +{{/CROP}} + # ============================================================================ # PASS 10: RESIZE / UPSCALE # ============================================================================ @@ -1954,6 +1907,63 @@ _aspect_box_h = _even(_box_h) if _box_h > 0 else 0 {{/RESIZE_STANDARD}} {{/RESIZE}} +# ============================================================================ +# PASS: SHARPEN +# ============================================================================ +# Sharpening runs after the resize (issue #109), on the pixels that are actually +# delivered. Sharpened before it, the resampling softens the edges again on a +# downscale and enlarges the sharpening halos on an upscale. It stays ahead of +# the grain, which it would otherwise exaggerate. +{{#SHARPEN}} + +{{#SHARPEN_LSFMOD}} +# LSFmod - Limited Sharpening with overshoot control +clip = haf.LSFmod( + clip, +{{#SHARPEN_STRENGTH}} + strength={{SHARPEN_STRENGTH}}, +{{/SHARPEN_STRENGTH}} +{{#SHARPEN_OVERSHOOT}} + overshoot={{SHARPEN_OVERSHOOT}}, +{{/SHARPEN_OVERSHOOT}} +{{#SHARPEN_UNDERSHOOT}} + undershoot={{SHARPEN_UNDERSHOOT}}, +{{/SHARPEN_UNDERSHOOT}} +{{#SHARPEN_SOFT_EDGE}} + soft={{SHARPEN_SOFT_EDGE}}, +{{/SHARPEN_SOFT_EDGE}} +) +{{/SHARPEN_LSFMOD}} + +{{#SHARPEN_CAS}} +# CAS - Contrast Adaptive Sharpening +clip = core.cas.CAS( + clip, +{{#SHARPEN_CAS_SHARPNESS}} + sharpness={{SHARPEN_CAS_SHARPNESS}}, +{{/SHARPEN_CAS_SHARPNESS}} +) +{{/SHARPEN_CAS}} +{{#SHARPEN_AWARPSHARP2}} +# aWarpSharp2 - sharpens by warping pixels toward edges rather than raising +# local contrast, so it produces no halos. +# +# `chroma` and `planes` are deliberately not passed. This VapourSynth port takes +# chroma as 0 or 1 (NOT Avisynth's 0-6, where 4 means "warp chroma with the luma +# mask") and rejects anything else outright, and by default it processes luma +# only — the conventional use as a sharpener. Passing a guessed value here is how +# this block first shipped broken; expose it deliberately, with a test, or leave +# the plugin's own default alone. +clip = core.warp.AWarpSharp2( + clip, + thresh={{SHARPEN_WARP_THRESH}}, + blur={{SHARPEN_WARP_BLUR}}, + type={{SHARPEN_WARP_TYPE}}, + depth={{SHARPEN_WARP_DEPTH}}, +) +{{/SHARPEN_AWARPSHARP2}} +{{/SHARPEN}} + # ============================================================================ # PASS: FILM GRAIN # ============================================================================ diff --git a/worker/templates/preview_template.vpy b/worker/templates/preview_template.vpy index 191afee..73be6d7 100644 --- a/worker/templates/preview_template.vpy +++ b/worker/templates/preview_template.vpy @@ -76,13 +76,6 @@ def _expr(clips, expr, **kwargs): return _akarin_expr(clips, expr, **kwargs) return core.std.Expr(clips, expr, **kwargs) -# ============================================================================ -# PASS 1: PRE-CROP (before deinterlacing to reduce processing area) -# ============================================================================ -{{#PRE_CROP}} -clip = core.std.Crop(clip, left={{CROP_LEFT}}, right={{CROP_RIGHT}}, top={{CROP_TOP}}, bottom={{CROP_BOTTOM}}) -{{/PRE_CROP}} - # ============================================================================ # PASS 2: DEINTERLACING # ============================================================================ @@ -1178,59 +1171,6 @@ clip = haf.santiag( {{/AA_SANTIAG}} {{/ANTI_ALIAS}} -# ============================================================================ -# PASS 7: SHARPEN -# ============================================================================ -{{#SHARPEN}} - -{{#SHARPEN_LSFMOD}} -# LSFmod - Limited Sharpening with overshoot control -clip = haf.LSFmod( - clip, -{{#SHARPEN_STRENGTH}} - strength={{SHARPEN_STRENGTH}}, -{{/SHARPEN_STRENGTH}} -{{#SHARPEN_OVERSHOOT}} - overshoot={{SHARPEN_OVERSHOOT}}, -{{/SHARPEN_OVERSHOOT}} -{{#SHARPEN_UNDERSHOOT}} - undershoot={{SHARPEN_UNDERSHOOT}}, -{{/SHARPEN_UNDERSHOOT}} -{{#SHARPEN_SOFT_EDGE}} - soft={{SHARPEN_SOFT_EDGE}}, -{{/SHARPEN_SOFT_EDGE}} -) -{{/SHARPEN_LSFMOD}} - -{{#SHARPEN_CAS}} -# CAS - Contrast Adaptive Sharpening -clip = core.cas.CAS( - clip, -{{#SHARPEN_CAS_SHARPNESS}} - sharpness={{SHARPEN_CAS_SHARPNESS}}, -{{/SHARPEN_CAS_SHARPNESS}} -) -{{/SHARPEN_CAS}} -{{#SHARPEN_AWARPSHARP2}} -# aWarpSharp2 - sharpens by warping pixels toward edges rather than raising -# local contrast, so it produces no halos. -# -# `chroma` and `planes` are deliberately not passed. This VapourSynth port takes -# chroma as 0 or 1 (NOT Avisynth's 0-6, where 4 means "warp chroma with the luma -# mask") and rejects anything else outright, and by default it processes luma -# only — the conventional use as a sharpener. Passing a guessed value here is how -# this block first shipped broken; expose it deliberately, with a test, or leave -# the plugin's own default alone. -clip = core.warp.AWarpSharp2( - clip, - thresh={{SHARPEN_WARP_THRESH}}, - blur={{SHARPEN_WARP_BLUR}}, - type={{SHARPEN_WARP_TYPE}}, - depth={{SHARPEN_WARP_DEPTH}}, -) -{{/SHARPEN_AWARPSHARP2}} -{{/SHARPEN}} - # ============================================================================ # PASS 8: CHROMA FIXES # ============================================================================ @@ -1691,6 +1631,19 @@ if clip.format.id != _geom_src_format.id: clip = core.resize.Spline36(clip, format=_geom_src_format.id) {{/GEOMETRY}} +# ============================================================================ +# PASS: CROP +# ============================================================================ +# Cropping runs here, with the resize, and not at the top of the script where it +# used to (issue #109). Stabilize shifts the picture and exposes thin edges, and +# a quarter turn swaps width and height, so the crop has to follow both: it then +# removes what Stabilize exposed, and its four sides mean the sides of the +# picture as it will be delivered. The cost is that every earlier pass works on +# the uncropped frame. +{{#CROP}} +clip = core.std.Crop(clip, left={{CROP_LEFT}}, right={{CROP_RIGHT}}, top={{CROP_TOP}}, bottom={{CROP_BOTTOM}}) +{{/CROP}} + # ============================================================================ # PASS 10: RESIZE / UPSCALE # ============================================================================ @@ -1885,6 +1838,63 @@ _aspect_box_h = _even(_box_h) if _box_h > 0 else 0 {{/RESIZE_STANDARD}} {{/RESIZE}} +# ============================================================================ +# PASS: SHARPEN +# ============================================================================ +# Sharpening runs after the resize (issue #109), on the pixels that are actually +# delivered. Sharpened before it, the resampling softens the edges again on a +# downscale and enlarges the sharpening halos on an upscale. It stays ahead of +# the grain, which it would otherwise exaggerate. +{{#SHARPEN}} + +{{#SHARPEN_LSFMOD}} +# LSFmod - Limited Sharpening with overshoot control +clip = haf.LSFmod( + clip, +{{#SHARPEN_STRENGTH}} + strength={{SHARPEN_STRENGTH}}, +{{/SHARPEN_STRENGTH}} +{{#SHARPEN_OVERSHOOT}} + overshoot={{SHARPEN_OVERSHOOT}}, +{{/SHARPEN_OVERSHOOT}} +{{#SHARPEN_UNDERSHOOT}} + undershoot={{SHARPEN_UNDERSHOOT}}, +{{/SHARPEN_UNDERSHOOT}} +{{#SHARPEN_SOFT_EDGE}} + soft={{SHARPEN_SOFT_EDGE}}, +{{/SHARPEN_SOFT_EDGE}} +) +{{/SHARPEN_LSFMOD}} + +{{#SHARPEN_CAS}} +# CAS - Contrast Adaptive Sharpening +clip = core.cas.CAS( + clip, +{{#SHARPEN_CAS_SHARPNESS}} + sharpness={{SHARPEN_CAS_SHARPNESS}}, +{{/SHARPEN_CAS_SHARPNESS}} +) +{{/SHARPEN_CAS}} +{{#SHARPEN_AWARPSHARP2}} +# aWarpSharp2 - sharpens by warping pixels toward edges rather than raising +# local contrast, so it produces no halos. +# +# `chroma` and `planes` are deliberately not passed. This VapourSynth port takes +# chroma as 0 or 1 (NOT Avisynth's 0-6, where 4 means "warp chroma with the luma +# mask") and rejects anything else outright, and by default it processes luma +# only — the conventional use as a sharpener. Passing a guessed value here is how +# this block first shipped broken; expose it deliberately, with a test, or leave +# the plugin's own default alone. +clip = core.warp.AWarpSharp2( + clip, + thresh={{SHARPEN_WARP_THRESH}}, + blur={{SHARPEN_WARP_BLUR}}, + type={{SHARPEN_WARP_TYPE}}, + depth={{SHARPEN_WARP_DEPTH}}, +) +{{/SHARPEN_AWARPSHARP2}} +{{/SHARPEN}} + # ============================================================================ # PASS: FILM GRAIN # ============================================================================ diff --git a/worker/tests/filter_integration_test.rs b/worker/tests/filter_integration_test.rs index f6808da..d418b3a 100644 --- a/worker/tests/filter_integration_test.rs +++ b/worker/tests/filter_integration_test.rs @@ -4128,11 +4128,15 @@ fn test_109_lut_derainbow_with_ten_bit_guard() { #[test] fn test_110_new_passes_run_in_the_documented_order() { - // The order the passes appear in the script is the order they run, and two - // of these placements are deliberate: anti-aliasing BEFORE sharpening - // (sharpening stair-stepped edges makes the stepping worse), and - // stabilisation LAST before framing (so a crop can remove the borders it - // shifts into view). + // The order the passes appear in the script is the order they run, and + // three of these placements are deliberate: anti-aliasing BEFORE sharpening + // (sharpening stair-stepped edges makes the stepping worse), stabilisation + // LAST before framing (so a crop can remove the borders it shifts into + // view), and sharpening AFTER the resize (issue #109: resampling a + // sharpened picture softens it again on a downscale and enlarges the halos + // on an upscale). The crop runs with the resize, AFTER stabilisation and + // rotation — it used to run before deinterlacing, where it could not remove + // the edges stabilisation exposes. create_output_dir(); let mut job = create_base_job("test_110_order"); job.qtgmc_parameters.enabled = false; @@ -4145,8 +4149,16 @@ fn test_110_new_passes_run_in_the_documented_order() { ..Default::default() }, stabilize: StabilizeParameters { enabled: true, ..Default::default() }, + geometry: GeometryParameters { + enabled: true, + flip_horizontal: true, + ..Default::default() + }, crop_resize: CropResizeParameters { enabled: true, + crop_enabled: true, + crop_left: 8, + crop_right: 8, resize_enabled: true, target_width: Some(640), target_height: Some(480), @@ -4156,11 +4168,21 @@ fn test_110_new_passes_run_in_the_documented_order() { }); let script = script_text(&job); + let crop = script + .find("core.std.Crop(clip, left=8, right=8") + .expect("crop should be present"); + let flip = script.find("FlipHorizontal").expect("flip should be present"); let aa = script.find("haf.daa(").expect("daa should be present"); let sharp = script.find("core.cas.CAS(").expect("CAS should be present"); let stab = script.find("haf.Stab(").expect("Stab should be present"); + let resize = script.find("height=target_h").expect("resize should be present"); assert!(aa < sharp, "anti-aliasing must run before sharpening"); - assert!(sharp < stab, "stabilisation runs after the detail passes"); + assert!(aa < stab, "stabilisation runs after the detail passes"); + assert!(stab < flip, "stabilisation runs before the picture is turned"); + assert!(flip < crop, "the crop must see the turned picture"); + assert!(stab < crop, "the crop must run after stabilisation"); + assert!(crop < resize, "the crop runs before the resize"); + assert!(resize < sharp, "sharpening must run after the resize"); } // ============================================================================