Connect the lens vignetting correction to the pipeline
`ops/vignetting.rs` has carried a complete descriptor, polynomial, helper and test suite without an entry in `ops/`, so it was never in `chain()`. It reached no photograph and no panel, and `Attribute::Optics` was an empty category in consequence — filtered out of the tab strip for having no rows, by a chain that had never been given its only member. Declaring it needs the one thing the operation was written against and which did not exist. `wgsl_body` reads `radius`, and the module claimed "the composer publishes `radius` in the shader prologue for exactly this reason". It did not. `sample_source` now does, in both sampling branches, beside the `source_px` it already published for the same class of caller. It is corner-normalised there, which is the part that is easy to leave out. `p` spans ±0.5·aspect, so its length at the corner is 0.5·length(aspect) — about 0.901 on a 3:2 frame, not 1. Lensfun's polynomials are fitted against a corner radius of 1, so passing `length(p)` straight in evaluates every one of them short of where it was measured, by a factor that changes with the aspect ratio. It would have read as a correction that is simply too weak, which is indistinguishable from a bad profile. Both `lens.rs` and `framing.rs` asserted the normalisation `p` does not have; corrected. `order: 5` puts the correction ahead of the tonal stages, and the ordering is load-bearing rather than tidy. Recovering a corner means dividing by an attenuation below one — about two stops for a fast prime wide open — so run after the highlights have been rolled off and clipped, the lift has nowhere to go and the corners posterise instead of brightening. `layer_chain` now drops `Optics` as well as the neighbourhood operations. A local vignetting slider would have worked, which is what makes it worth excluding: `radius` measures from the centre of the whole photograph and a mask cannot move the optical axis, so it would lay a frame-centred radial ramp across the picture and multiply it by the mask. The existing exclusion covers operations that move and do nothing; this one covers an operation that moves and does something its name does not promise. The rule both share is now written down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,40 @@
|
|||||||
|
# A hand-written node, and the first thing in `Attribute::Optics`.
|
||||||
|
#
|
||||||
|
# Written, tested and unreferenced until now: `src/ops/vignetting.rs` has
|
||||||
|
# existed with a full descriptor and a working polynomial, and without an entry
|
||||||
|
# here it was never in `chain()` — so it reached no photograph and no panel.
|
||||||
|
# This file is the whole of what was missing, which is the point of `ops/`
|
||||||
|
# being the one place the pipeline's order is written down.
|
||||||
|
id: vignetting
|
||||||
|
order: 5
|
||||||
|
|
||||||
|
attributes: [optics]
|
||||||
|
rust: Vignetting
|
||||||
|
|
||||||
|
why_rust: |
|
||||||
|
It carries a lens profile's `pa` coefficients, which are not parameters: they
|
||||||
|
come from the body and lens that took the photograph, not from the
|
||||||
|
photographer, and no `uniforms:` expression could produce them. The one
|
||||||
|
parameter that *is* theirs — the manual trim — is composed with `k1` in Rust,
|
||||||
|
because the profile and the trim have to reach the shader as a single
|
||||||
|
polynomial rather than as two the fragment would have to add up.
|
||||||
|
|
||||||
|
placement: |
|
||||||
|
First in the chain, ahead of white balance and exposure.
|
||||||
|
|
||||||
|
Vignetting is what the lens did to the light before the sensor measured it,
|
||||||
|
so undoing it belongs with reading the file rather than with editing the
|
||||||
|
picture — everything downstream is then working on the frame the lens would
|
||||||
|
have delivered had it been even.
|
||||||
|
|
||||||
|
The ordering is load-bearing rather than tidy. Correcting a corner means
|
||||||
|
*dividing* by an attenuation below one, which pushes those pixels up: a fast
|
||||||
|
prime wide open needs about two stops there. Run after the tonal stages, that
|
||||||
|
recovery happens once the highlights have already been rolled off and
|
||||||
|
clipped, so it lifts values that no longer have anywhere to go and the
|
||||||
|
corners posterise instead of brightening. Run here, the headroom to hold them
|
||||||
|
still exists (ARCH §5.2).
|
||||||
|
|
||||||
|
Before distortion and chromatic aberration in intent, though those are
|
||||||
|
`lens::Warp`s rather than nodes and compose ahead of the fetch, so no `order:`
|
||||||
|
relates the two.
|
||||||
@@ -893,9 +893,16 @@ impl Framing {
|
|||||||
/// The WGSL mapping an output pixel to a **normalised centred** source
|
/// The WGSL mapping an output pixel to a **normalised centred** source
|
||||||
/// position, ready for the warp chain.
|
/// position, ready for the warp chain.
|
||||||
///
|
///
|
||||||
/// Leaves the result in `p`: centre `(0, 0)`, `r == 1` at the corner —
|
/// Leaves the result in `p`: centre `(0, 0)`, spanning `±0.5 * aspect` —
|
||||||
/// exactly the space [`crate::lens`] documents, so lens correction
|
/// the space [`crate::lens`] documents, so lens correction composes on top
|
||||||
/// composes on top of this without either stage naming the other.
|
/// of this without either stage naming the other.
|
||||||
|
///
|
||||||
|
/// **`p` is centred but not corner-normalised**, and this used to claim it
|
||||||
|
/// was. Its length at the corner is `0.5 * length(aspect)`, not 1. The
|
||||||
|
/// corner-normalised radius the radial corrections need is published
|
||||||
|
/// separately as `radius` by `operation::sample_source`, which divides by
|
||||||
|
/// exactly that; anything reading `length(p)` as a lens radius is off by
|
||||||
|
/// an aspect-dependent factor.
|
||||||
///
|
///
|
||||||
/// `aspect` is left in scope alongside it, since the warp chain and the
|
/// `aspect` is left in scope alongside it, since the warp chain and the
|
||||||
/// sampler both need it to return to texture coordinates.
|
/// sampler both need it to return to texture coordinates.
|
||||||
|
|||||||
@@ -41,6 +41,12 @@
|
|||||||
//! `(0, 0)`, and the radius is scaled so that `r == 1` at the corner. Both
|
//! `(0, 0)`, and the radius is scaled so that `r == 1` at the corner. Both
|
||||||
//! properties matter.
|
//! properties matter.
|
||||||
//!
|
//!
|
||||||
|
//! Note which variable carries which. The shader's `p` supplies the centring
|
||||||
|
//! and spans `±0.5 * aspect`; the corner normalisation is applied on top of it
|
||||||
|
//! by `operation::sample_source`, which publishes the result as `radius`. A
|
||||||
|
//! warp reading `length(p)` and calling it `r` would be evaluating its
|
||||||
|
//! polynomial short of where the profile was fitted.
|
||||||
|
//!
|
||||||
//! Centring is what makes the polynomial meaningful — lens distortion is
|
//! Centring is what makes the polynomial meaningful — lens distortion is
|
||||||
//! radially symmetric about the optical axis, so a formula written about any
|
//! radially symmetric about the optical axis, so a formula written about any
|
||||||
//! other origin would need cross terms to say the same thing.
|
//! other origin would need cross terms to say the same thing.
|
||||||
|
|||||||
@@ -55,7 +55,7 @@ use std::fmt::Write as _;
|
|||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
|
||||||
use crate::coverage::Coverage;
|
use crate::coverage::Coverage;
|
||||||
use crate::descriptor::{OpDescriptor, ParamId};
|
use crate::descriptor::{Attribute, OpDescriptor, ParamId};
|
||||||
use crate::operation::Operation;
|
use crate::operation::Operation;
|
||||||
use crate::ops;
|
use crate::ops;
|
||||||
|
|
||||||
@@ -777,8 +777,8 @@ pub struct MaskLayer {
|
|||||||
pub ops: Vec<Box<dyn Operation>>,
|
pub ops: Vec<Box<dyn Operation>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
/// The chain a mask layer holds: every point operation, and none of the
|
/// The chain a mask layer holds: every point operation, and neither the
|
||||||
/// neighbourhood ones.
|
/// neighbourhood ones nor the optical corrections.
|
||||||
///
|
///
|
||||||
/// A layer's adjustments are fused into the colour dispatch and multiplied by
|
/// A layer's adjustments are fused into the colour dispatch and multiplied by
|
||||||
/// the mask afterwards, which is exactly why a layer needs no per-operation
|
/// the mask afterwards, which is exactly why a layer needs no per-operation
|
||||||
@@ -795,10 +795,26 @@ pub struct MaskLayer {
|
|||||||
/// Filtering here means a local sharpening or denoise control simply does not
|
/// Filtering here means a local sharpening or denoise control simply does not
|
||||||
/// appear until there is a stage that can honour it, which is the honest
|
/// appear until there is a stage that can honour it, which is the honest
|
||||||
/// state of affairs.
|
/// state of affairs.
|
||||||
|
/// **The optical corrections are excluded on their own grounds**, which are
|
||||||
|
/// not the neighbourhood argument above: they would work perfectly and mean
|
||||||
|
/// nothing.
|
||||||
|
///
|
||||||
|
/// `Attribute::Optics` describes what the *lens* did to the whole frame. Its
|
||||||
|
/// corrections are radial about the optical axis, and the `radius` they read
|
||||||
|
/// is the distance from the centre of the photograph — a layer's mask does not
|
||||||
|
/// and cannot move it. So a vignetting slider inside a mask would not correct
|
||||||
|
/// the falloff within the selected region; it would lay a frame-centred radial
|
||||||
|
/// ramp over the picture and then multiply it by the mask. That is a
|
||||||
|
/// well-defined operation nobody would ever want, and worse, it is one whose
|
||||||
|
/// name promises something else entirely.
|
||||||
|
///
|
||||||
|
/// The general rule the two exclusions share: a layer holds an operation only
|
||||||
|
/// where restricting it to a region is a thing a photographer could mean.
|
||||||
fn layer_chain() -> Vec<Box<dyn Operation>> {
|
fn layer_chain() -> Vec<Box<dyn Operation>> {
|
||||||
ops::chain()
|
ops::chain()
|
||||||
.into_iter()
|
.into_iter()
|
||||||
.filter(|o| o.detail().is_none())
|
.filter(|o| o.detail().is_none())
|
||||||
|
.filter(|o| !o.descriptor().attributes.contains(&Attribute::Optics))
|
||||||
.collect()
|
.collect()
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1463,33 +1479,49 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn a_layer_offers_only_the_operations_it_can_actually_apply() {
|
fn a_layer_offers_only_the_operations_it_can_actually_apply() {
|
||||||
|
// Two exclusions, for two different reasons, and the test states both
|
||||||
|
// because they fail in different ways.
|
||||||
|
//
|
||||||
// A layer's adjustments are fused into the colour dispatch and then
|
// A layer's adjustments are fused into the colour dispatch and then
|
||||||
// multiplied by the mask. A neighbourhood operation cannot take that
|
// multiplied by the mask. A neighbourhood operation cannot take that
|
||||||
// route: it is a dispatch of its own, run after the fused pass and
|
// route: it is a dispatch of its own, run after the fused pass and
|
||||||
// after the masks are already applied, so there is nowhere to hand it
|
// after the masks are already applied, so there is nowhere to hand it
|
||||||
// one layer's mask.
|
// one layer's mask. Left in, it moves and nothing happens.
|
||||||
|
//
|
||||||
|
// An optical correction *can* take that route, and that is the
|
||||||
|
// problem. It is radial about the frame's optical axis, which a mask
|
||||||
|
// cannot move, so it would lay a frame-centred ramp over the whole
|
||||||
|
// picture and multiply it by the mask. Left in, it moves and the wrong
|
||||||
|
// thing happens — under a name that promises the right one.
|
||||||
//
|
//
|
||||||
// The panel builds itself from `capabilities()` and names no
|
// The panel builds itself from `capabilities()` and names no
|
||||||
// operation, so anything left in this chain becomes a control. One
|
// operation, so anything left in this chain becomes a control.
|
||||||
// that cannot work is worse than one that is missing: it moves, the
|
|
||||||
// picture does not change, and nothing says why.
|
|
||||||
let layer = lit_layer("m1", 1.0);
|
let layer = lit_layer("m1", 1.0);
|
||||||
let ids: Vec<&str> = layer.capabilities().iter().map(|c| c.id.0).collect();
|
let ids: Vec<&str> = layer.capabilities().iter().map(|c| c.id.0).collect();
|
||||||
|
|
||||||
let global = crate::ops::chain();
|
let global = crate::ops::chain();
|
||||||
|
let local_means_something = |op: &Box<dyn Operation>| {
|
||||||
|
op.detail().is_none()
|
||||||
|
&& !op
|
||||||
|
.descriptor()
|
||||||
|
.attributes
|
||||||
|
.contains(&crate::descriptor::Attribute::Optics)
|
||||||
|
};
|
||||||
|
|
||||||
for op in &global {
|
for op in &global {
|
||||||
let id = op.descriptor().id.0;
|
let id = op.descriptor().id.0;
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
ids.contains(&id),
|
ids.contains(&id),
|
||||||
op.detail().is_none(),
|
local_means_something(op),
|
||||||
"{id} is offered as a local adjustment but cannot be one, \
|
"{id} is offered as a local adjustment but cannot meaningfully \
|
||||||
or is a point operation and has gone missing from a layer"
|
be one, or is an ordinary point operation and has gone \
|
||||||
|
missing from a layer"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
assert!(
|
assert!(
|
||||||
ids.len() < global.len() || global.iter().all(|o| o.detail().is_none()),
|
ids.len() < global.len() || global.iter().all(local_means_something),
|
||||||
"the filter dropped nothing, so either it is not running or the \
|
"the filter dropped nothing, so either it is not running or the \
|
||||||
chain has no neighbourhood operation left to drop"
|
chain has nothing left that a layer cannot carry"
|
||||||
);
|
);
|
||||||
|
|
||||||
// And a clone must rebuild the same chain: it copies parameters across
|
// And a clone must rebuild the same chain: it copies parameters across
|
||||||
|
|||||||
@@ -1074,6 +1074,23 @@ pub(crate) fn sample_source(interpolate: bool) -> &'static str {
|
|||||||
let source_px = uv_src * vec2<f32>(src_dims);
|
let source_px = uv_src * vec2<f32>(src_dims);
|
||||||
// A free angle puts output pixels between source pixels. Nearest-neighbour
|
// A free angle puts output pixels between source pixels. Nearest-neighbour
|
||||||
// here is what makes a straightened horizon stair-step, so interpolate.
|
// here is what makes a straightened horizon stair-step, so interpolate.
|
||||||
|
// Distance from the optical axis, normalised so the corner is exactly 1.
|
||||||
|
//
|
||||||
|
// Published beside `source_px` and for the same reason: a fragment is
|
||||||
|
// handed a colour with no way back to a coordinate, and the radial
|
||||||
|
// corrections need one. Derived from `p` after the whole coordinate stage,
|
||||||
|
// so it measures the *source* frame — which is what a lens profile is
|
||||||
|
// calibrated against, and why an off-centre crop still gets the falloff
|
||||||
|
// its corner actually had rather than one centred on the crop.
|
||||||
|
//
|
||||||
|
// **The division is the part that is easy to leave out.** `p` spans
|
||||||
|
// `+/-0.5 * aspect`, so at the corner its length is `0.5 * length(aspect)`
|
||||||
|
// -- about 0.901 on a 3:2 frame, not 1. Lensfun's polynomials are fitted
|
||||||
|
// against a corner radius of 1, so passing `length(p)` straight in
|
||||||
|
// evaluates every one of them short of where it was measured, and by an
|
||||||
|
// amount that changes with the aspect ratio. It reads as a correction that
|
||||||
|
// is simply too weak, which is indistinguishable from a bad profile.
|
||||||
|
let radius = length(p) / (0.5 * length(aspect));
|
||||||
var c = sample_bilinear(uv_src, src_dims);
|
var c = sample_bilinear(uv_src, src_dims);
|
||||||
"
|
"
|
||||||
} else {
|
} else {
|
||||||
@@ -1101,6 +1118,23 @@ pub(crate) fn sample_source(interpolate: bool) -> &'static str {
|
|||||||
// put, and its *amount* is handled separately by how much film a pixel
|
// put, and its *amount* is handled separately by how much film a pixel
|
||||||
// covers -- see `dr_film::Grain`.
|
// covers -- see `dr_film::Grain`.
|
||||||
let source_px = uv_src * vec2<f32>(src_dims);
|
let source_px = uv_src * vec2<f32>(src_dims);
|
||||||
|
// Distance from the optical axis, normalised so the corner is exactly 1.
|
||||||
|
//
|
||||||
|
// Published beside `source_px` and for the same reason: a fragment is
|
||||||
|
// handed a colour with no way back to a coordinate, and the radial
|
||||||
|
// corrections need one. Derived from `p` after the whole coordinate stage,
|
||||||
|
// so it measures the *source* frame — which is what a lens profile is
|
||||||
|
// calibrated against, and why an off-centre crop still gets the falloff
|
||||||
|
// its corner actually had rather than one centred on the crop.
|
||||||
|
//
|
||||||
|
// **The division is the part that is easy to leave out.** `p` spans
|
||||||
|
// `+/-0.5 * aspect`, so at the corner its length is `0.5 * length(aspect)`
|
||||||
|
// -- about 0.901 on a 3:2 frame, not 1. Lensfun's polynomials are fitted
|
||||||
|
// against a corner radius of 1, so passing `length(p)` straight in
|
||||||
|
// evaluates every one of them short of where it was measured, and by an
|
||||||
|
// amount that changes with the aspect ratio. It reads as a correction that
|
||||||
|
// is simply too weak, which is indistinguishable from a bad profile.
|
||||||
|
let radius = length(p) / (0.5 * length(aspect));
|
||||||
var c = textureLoad(source, coord, 0).rgb;
|
var c = textureLoad(source, coord, 0).rgb;
|
||||||
"
|
"
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -54,6 +54,14 @@
|
|||||||
//! than `Operation`, because they rewrite *coordinates* before the source is
|
//! than `Operation`, because they rewrite *coordinates* before the source is
|
||||||
//! sampled rather than transforming a colour after it. They are not part of
|
//! sampled rather than transforming a colour after it. They are not part of
|
||||||
//! the develop chain and do not appear in `ops/`.
|
//! the develop chain and do not appear in `ops/`.
|
||||||
|
//!
|
||||||
|
//! [`vignetting`] is the exception, and the reason the split is drawn at
|
||||||
|
//! coordinates rather than at "lens correction": it applies a gain to the
|
||||||
|
//! pixel already fetched, so it is an ordinary node in `ops/` like any other.
|
||||||
|
//! What it needs that a colour fragment is not otherwise given is the pixel's
|
||||||
|
//! distance from the optical axis, which the sampler publishes as `radius`
|
||||||
|
//! — corner-normalised there, because the profile coefficients are fitted
|
||||||
|
//! against a corner radius of 1 and the prologue's `p` is not.
|
||||||
|
|
||||||
// Hand-written nodes. Each is listed in `ops/` with `rust:`, which is what
|
// Hand-written nodes. Each is listed in `ops/` with `rust:`, which is what
|
||||||
// places it in the chain; these are the implementations that entry points at.
|
// places it in the chain; these are the implementations that entry points at.
|
||||||
|
|||||||
@@ -402,6 +402,7 @@ fn every_declared_node_is_checked() {
|
|||||||
"noise_reduction",
|
"noise_reduction",
|
||||||
"texture",
|
"texture",
|
||||||
"tone_curve",
|
"tone_curve",
|
||||||
|
"vignetting",
|
||||||
],
|
],
|
||||||
"the set of hand-written nodes changed; if that is deliberate, update \
|
"the set of hand-written nodes changed; if that is deliberate, update \
|
||||||
this list and the module documentation above"
|
this list and the module documentation above"
|
||||||
|
|||||||
File diff suppressed because one or more lines are too long
@@ -146,6 +146,14 @@ fn catalogued(key: &str) -> Option<&'static str> {
|
|||||||
// see from there.
|
// see from there.
|
||||||
"op.capture_sharpen" => "Sharpening",
|
"op.capture_sharpen" => "Sharpening",
|
||||||
"op.framing" => "Crop & Rotate",
|
"op.framing" => "Crop & Rotate",
|
||||||
|
// "Lens Vignetting", not the "Vignetting" `derive` would produce, and
|
||||||
|
// the qualifier is doing real work. This operation corrects the corner
|
||||||
|
// falloff a lens imposed; the creative vignette that darkens corners on
|
||||||
|
// purpose is a different operation, belongs to `Effect` rather than
|
||||||
|
// `Optics`, and will want the plain word when it exists. Naming this
|
||||||
|
// one for what it corrects means the two can sit in the same panel
|
||||||
|
// without either having to be renamed to make room.
|
||||||
|
"op.vignetting" => "Lens Vignetting",
|
||||||
|
|
||||||
// Parameters
|
// Parameters
|
||||||
"param.temperature" => "Temperature",
|
"param.temperature" => "Temperature",
|
||||||
|
|||||||
Reference in New Issue
Block a user