dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions 8 Packages Projects Releases 20 Wiki Activity

Run the view transform after the detail stage, in a pass of its own

Browse Source 
The fused pass stops at "linear working values" when a sharpener, a
blur or a repair follows, and the detail passes convolve what it hands
on. Until now it handed on the rendering: the base curve, and since the
last commit the view transform, ran before the store. So every kernel
worked on display-referred values while its comments promised the
opposite — D19's second finding.

A fused pass composed for a detail stage now stops before the view
transform, and carries a second shader, `ComposedShader::view`, composed
from the same inputs. It runs the same prologue, for the positions a
fragment reads (a film's grain seeds from `source_px`) and the corners
it blacks out, takes its colour from the detail stage's result bound
where the sample cache would be, and runs the view transform, the
output transform and the mask reveal. `render_detailed` dispatches it
after the last detail pass, in the same encoder.

So no detail pass encodes any more. Every pass writes an intermediate,
the last one included, which retires three things that existed only to
make the last pass encode: `writes_output` and the runner's second
layout, the body-less resolve pass for an active kernel with nothing to
draw at this scale, and capture sharpening's pass-through, which now
emits no pass at all. An empty chain is a whole render: the view pass
reads the fused result directly. The detail stage no longer takes an
output space either, so `compose_detail_for` folds into
`compose_detail` and the space is named once, on the fused half.

The cost is one full-render read and write per frame when a detail
stage exists, and a third intermediate for a one-pass chain.
This commit is contained in:
dtourolle
2026-09-27 16:52:54 -04:00
parent 92afaebd34
commit c07f81edcb
19 changed files with 618 additions and 566 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/dr-pipeline/src/detail.rs
+101 -237
View File
@@ -122,8 +122,6 @@
use std::fmt::Write as _;
use dr_types::ColourSpace;
use crate::operation::{Helper, Operation, Uniform};
/// Floats the generated detail uniform block always carries, before an
@@ -491,15 +489,6 @@ pub struct ComposedDetailPass {
pub radius: u32,
/// See [`DetailPass::output_scale`].
pub output_scale: u32,
/// Whether this pass writes the display/export texture rather than another
/// linear intermediate.
///
/// True for exactly the last pass in the chain, which carries the output
/// transform — the primaries conversion, the clip and the encode that the
/// fused pass performs when there is no detail stage at all. Folding them
/// into the last pass rather than adding a resolve dispatch keeps the cost
/// of the stage at one dispatch per pass, not one plus one.
pub writes_output: bool,
/// Identifies this pass's *structure*, for the pipeline cache. Covers the
/// generated source, not the uniform values — so moving a slider uploads a
/// buffer and reuses the compiled pipeline, exactly as the fused pass does.
@@ -547,10 +536,11 @@ impl ComposedDetail {
/// an edit with no sharpening produces an empty chain and `dr-gpu` runs the
/// single dispatch it always did.
///
/// `output` is the space the **last** pass encodes into, and it is a parameter
/// for the same reason it is a parameter to [`crate::compose_with_framing`]: a
/// screen render and a Display P3 export are the same edit and different
/// shaders, and neither is more authoritative than the other.
/// No pass encodes. Every pass writes a linear intermediate, the last one
/// included, and the fused pass's view pass ([`crate::ComposedShader::view`])
/// reads the last and performs the view transform and the output transform
/// (D19). So the output space is not a parameter here: a screen render and a
/// Display P3 export share one detail stage.
///
/// # The generated uniform block
///
@@ -562,12 +552,8 @@ impl ComposedDetail {
/// is there because a two-pass operation emitting one body for both directions
/// is a reasonable thing to want, and would otherwise need a uniform of its
/// own purely to say which half it is in.
pub fn compose_detail(
ops: &[Box<dyn Operation>],
scale: RenderScale,
output: ColourSpace,
) -> ComposedDetail {
compose_detail_with(ops, &[], scale, output)
pub fn compose_detail(ops: &[Box<dyn Operation>], scale: RenderScale) -> ComposedDetail {
compose_detail_with(ops, &[], scale)
}
/// TRACES: FR-DEV-8
@@ -592,7 +578,6 @@ pub fn compose_detail_with(
ops: &[Box<dyn Operation>],
spots: &[DetailPass],
scale: RenderScale,
output: ColourSpace,
) -> ComposedDetail {
// Every pass of every active detail operation, flattened, carrying the
// operation it came from for the uniform prefix and the helper set.
@@ -624,45 +609,13 @@ pub fn compose_detail_with(
}
}
// An active detail operation that emitted nothing at this scale.
//
// Legal, and the honest answer for an acutance operation on a heavy proxy
// — a one-source-pixel radius is a third of a render pixel there and no
// kernel represents a third of a pixel (see [`RenderScale`]). But it opens
// a hole between the two halves of the composition: [`compose_full`]
// decides to hand on linear working values from the *operations*, which it
// must, having no scale to consult, so the fused pass has already stopped
// short of the output transform. Returning an empty chain here would leave
// that transform undone and bind an `rgba16float` shader to an
// `rgba8unorm` target, which surfaces as a wgpu validation failure a long
// way from the cause.
//
// So the chain is never empty when the fused pass is expecting one: a
// single pass with no body, which reads the intermediate and performs the
// output transform the fused pass skipped. One dispatch, in the uncommon
// case where a photographer has a kernel switched on at a scale that
// cannot draw it — against the alternative of the preview failing outright
// or `compose_full` growing a resolution argument it has no other use for.
if planned.is_empty() && ops.iter().any(|o| o.is_active() && o.detail().is_some()) {
return ComposedDetail {
passes: vec![compose_one(
RESOLVE_ID,
&[],
&DetailPass {
output_scale: 1,
label: "resolve",
radius: 0,
wgsl: String::new(),
uniforms: Vec::new(),
storage: Vec::new(),
},
0,
scale,
output,
true,
)],
};
}
// An active detail operation may emit nothing at this scale — an
// acutance operation on a heavy proxy, whose one-source-pixel radius is a
// third of a render pixel (see [`RenderScale`]). The chain is then empty
// while the fused pass has stopped at linear working values, and that is
// fine: the fused pass's view pass reads the fused result directly and
// performs the output transform. Before D19 the last detail pass encoded,
// and this case needed a body-less resolve pass to do it.
// TRACES: NFR-P5
// A pass whose body is empty changes nothing but where the pixels are: it
@@ -674,12 +627,10 @@ pub fn compose_detail_with(
// 2560 x 1600 frame on the reference laptop with its clocks held down.
//
// Dropped here, where the chain is still a list, and only where dropping
// it is exact:
// it is exact. The last pass is no exception since D19: it writes an
// `rgba16float` intermediate like the others, and the view pass reads
// whichever one the chain last wrote.
//
// - **Not the last pass.** The last pass performs the output transform on
// what it read from an `rgba16float` intermediate. Moving that transform
// onto the pass before would apply it to that pass's `f32` result
// instead, which is a different rounding of the same picture.
// - **Not after a reduced pass.** A full-resolution pass ends the reduced
// chain (see `DetailRunner::encode`), so one that follows a scaled pass
// is what stops the next operation reading the last one's base. None of
@@ -688,45 +639,29 @@ pub fn compose_detail_with(
// Everywhere else the pass before and the pass after exchange the same
// `rgba16float` texels either way, `aux` included.
let mut kept: Vec<(&str, &[Helper], DetailPass, usize)> = Vec::with_capacity(planned.len());
let total = planned.len();
for (position, entry) in planned.into_iter().enumerate() {
for entry in planned {
let after_full = kept.last().is_none_or(|(_, _, p, _)| p.output_scale <= 1);
let droppable = position + 1 < total && after_full && entry.2.is_identity();
let droppable = after_full && entry.2.is_identity();
if !droppable {
kept.push(entry);
}
}
let planned = kept;
let last = planned.len().saturating_sub(1);
let passes = planned
.into_iter()
.enumerate()
.map(|(position, (id, helpers, pass, index))| {
compose_one(id, helpers, &pass, index, scale, output, position == last)
})
.map(|(id, helpers, pass, index)| compose_one(id, helpers, &pass, index, scale))
.collect();
ComposedDetail { passes }
}
/// The operation id the resolve pass is labelled with.
///
/// Not an operation: no `ops/*.yaml` declares it and nothing in the chain
/// answers to it. It exists so the generated label reads `detail/resolve`
/// rather than borrowing the id of whichever operation happened to fall
/// through, which would send a reader looking for a bug in that operation.
const RESOLVE_ID: &str = "detail";
#[allow(clippy::too_many_arguments)]
fn compose_one(
id: &str,
helpers: &[Helper],
pass: &DetailPass,
index: usize,
scale: RenderScale,
output: ColourSpace,
writes_output: bool,
) -> ComposedDetailPass {
let prefix = format!("{}_{index}", crate::operation::sanitise(id));
@@ -772,41 +707,13 @@ fn compose_one(
let _ = writeln!(helper_src, "{}\n", h.source.trim_end());
}
// The storage format and the tail are the *only* difference between an
// intermediate pass and the final one. Everything above — the taps, the
// uniforms, the body — is identical, which is what lets an operation write
// one kernel without knowing whether it happens to be last in the chain.
let (store_format, tail) = if writes_output {
(
"rgba8unorm",
format!(
"{} // Clip to the output gamut and encode. The one quantisation\n\
\x20 // the pipeline performs (FR-DEV-2), and it is here rather than\n\
\x20 // in the fused pass because this is now the last thing to run.\n\
\x20 c = clamp(c, vec3<f32>(0.0), vec3<f32>(1.0));\n\
\x20 textureStore(output, coord, vec4<f32>(encode_output(c), 1.0));",
crate::operation::primaries_conversion(output)
),
)
} else {
(
"rgba16float",
" // Another linear intermediate: no clip and no encode, because\n\
\x20 // the pass after this one still has to read real values.\n\
\x20 //\n\
\x20 // `aux` rides in alpha. A pass that never touches it hands on\n\
\x20 // whatever it was given, so the lane costs an operation that\n\
\x20 // does not want it exactly one copy of a value it already read.\n\
\x20 textureStore(output, coord, vec4<f32>(c, aux));"
.to_string(),
)
};
let encode_fn = if writes_output {
crate::operation::encode_output_fn(output)
} else {
String::new()
};
// Every pass writes another linear intermediate: no clip and no encode,
// because the view pass after the last one still has to read real values
// (D19). `aux` rides in alpha. A pass that never touches it hands on
// whatever it was given, so the lane costs an operation that does not want
// it exactly one copy of a value it already read.
let store_format = "rgba16float";
let tail = " textureStore(output, coord, vec4<f32>(c, aux));";
let label = format!("{id}/{}", pass.label);
let indented = body
@@ -823,7 +730,7 @@ fn compose_one(
// way back to a coordinate.
//
// In: linear sRGB, scene-referred, **unclipped**, at render resolution.
// Out: {}
// Out: the same, for the next pass or for the view pass after the last.
struct Params {{
{uniform_fields}}}
@@ -907,7 +814,7 @@ fn reduced_at(coord: vec2<i32>) -> f32 {{
return mix(mix(s00, s10, f.x), mix(s01, s11, f.x), f.y);
}}
{helper_src}{encode_fn}
{helper_src}
@compute @workgroup_size(8, 8, 1)
fn main(@builtin(global_invocation_id) gid: vec3<u32>) {{
let dims = textureDimensions(output);
@@ -936,12 +843,7 @@ fn main(@builtin(global_invocation_id) gid: vec3<u32>) {{
{tail}
}}
",
if writes_output {
"display-encoded, in the output space."
} else {
"linear sRGB, for the next pass."
},
"
);
let structure_hash = crate::operation::hash_source(&source);
@@ -956,7 +858,6 @@ fn main(@builtin(global_invocation_id) gid: vec3<u32>) {{
// dispatch size and a declaration is data, which since FR-PLG-2 can
// come from a file this build did not write.
output_scale: pass.output_scale.max(1),
writes_output,
structure_hash,
}
}
@@ -1095,27 +996,19 @@ mod tests {
// dispatch. An unedited photograph must not pay for a sharpener it is
// not using.
let ops = with_blur(0.0);
let composed = compose_detail(
&ops,
RenderScale::full((512, 512)),
dr_types::ColourSpace::Srgb,
);
let composed = compose_detail(&ops, RenderScale::full((512, 512)));
assert!(composed.is_empty());
assert_eq!(fused(&ops).output_mode, OutputMode::Encoded);
}
#[test]
fn a_separable_blur_becomes_two_passes_and_only_the_last_encodes() {
// The multi-pass case, which is the one the ping-pong exists for. The
// first pass writes a linear intermediate and the second writes the
// display texture — so the output transform happens exactly once, at
// the end, wherever the end happens to be.
fn a_separable_blur_becomes_two_passes_and_neither_encodes() {
// The multi-pass case, which is the one the ping-pong exists for. Both
// passes write linear intermediates, and the fused pass's view pass
// reads the second and performs the view transform and the output
// transform — so those happen exactly once, after every kernel (D19).
let ops = with_blur(0.05);
let composed = compose_detail(
&ops,
RenderScale::full((512, 512)),
dr_types::ColourSpace::Srgb,
);
let composed = compose_detail(&ops, RenderScale::full((512, 512)));
assert_eq!(composed.len(), 2);
let first = &composed.passes[0];
@@ -1123,13 +1016,16 @@ mod tests {
assert_eq!(first.label, "detail_probe/horizontal");
assert_eq!(last.label, "detail_probe/vertical");
assert!(!first.writes_output);
assert!(first.source.contains("texture_storage_2d<rgba16float"));
assert!(!first.source.contains("fn encode_output"));
assert!(last.writes_output);
assert!(last.source.contains("texture_storage_2d<rgba8unorm"));
assert!(last.source.contains("fn encode_output"));
for pass in [first, last] {
assert!(pass.source.contains("texture_storage_2d<rgba16float"));
assert!(!pass.source.contains("fn encode_output"));
assert!(!pass.source.contains("view_sigmoid"));
}
let view = fused(&ops)
.view
.expect("a view pass follows the detail stage");
assert!(view.source.contains("fn encode_output"));
assert!(view.source.contains("c = view_sigmoid("));
// Two passes of one operation are two shaders, so they must not share
// a pipeline-cache entry — the classic way a second pass silently runs
@@ -1143,11 +1039,7 @@ mod tests {
// and the composer rewrites it to a prefixed struct field, so two
// operations may both call a uniform `radius` and neither has to know.
let ops = with_blur(0.05);
let composed = compose_detail(
&ops,
RenderScale::full((512, 512)),
dr_types::ColourSpace::Srgb,
);
let composed = compose_detail(&ops, RenderScale::full((512, 512)));
let src = &composed.passes[0].source;
assert!(src.contains("detail_probe_0_radius: f32,"));
assert!(src.contains("let r = i32(u.detail_probe_0_radius);"));
@@ -1164,13 +1056,7 @@ mod tests {
// outright by the WGSL uniform address space rules, and the failure
// arrives as a shader compilation error against generated source.
let ops = with_blur(0.05);
for pass in compose_detail(
&ops,
RenderScale::full((512, 512)),
dr_types::ColourSpace::Srgb,
)
.passes
{
for pass in compose_detail(&ops, RenderScale::full((512, 512))).passes {
assert_eq!(pass.uniforms.len() % 4, 0, "{}", pass.label);
assert!(pass.uniforms.iter().all(|v| v.is_finite()));
// The base block is first and fixed, so a pass never addresses a
@@ -1189,7 +1075,7 @@ mod tests {
// the truth rather than zero.
let ops = with_blur(0.05);
let scale = RenderScale::full((400, 400));
let composed = compose_detail(&ops, scale, dr_types::ColourSpace::Srgb);
let composed = compose_detail(&ops, scale);
let expected = BoxBlur::with_radius(0.05).kernel(scale);
assert_eq!(expected, 20, "5% of a 400px edge");
assert_eq!(composed.radius(), expected);
@@ -1208,7 +1094,7 @@ mod tests {
.iter()
.map(|&(w, h)| {
let scale = RenderScale::full((w, h));
let composed = compose_detail(&ops, scale, dr_types::ColourSpace::Srgb);
let composed = compose_detail(&ops, scale);
composed.radius() as f32 / w.min(h) as f32
})
.collect();
@@ -1226,14 +1112,14 @@ mod tests {
// cannot see each other. `compose_full` decides to hand on linear
// working values from the *operations* — it has no resolution to
// consult — while this composer converts a radius and can legitimately
// decide there is nothing to draw at this size. An empty chain would
// then leave the output transform undone: the fused pass writes
// `rgba16float` and the frontend binds an `rgba8unorm` target to it.
// decide there is nothing to draw at this size.
//
// A photographer meets this by turning on capture sharpening or
// luminance noise reduction while the develop view is fitted to a
// large file, which is the normal way to work, so it is not an edge
// case that can be left to fail.
// large file, which is the normal way to work. Before D19 an empty
// chain left the output transform undone and needed a resolve pass;
// now the view pass does the output transform whatever the chain
// holds.
let ops = with_blur(0.001);
let scale = RenderScale::full((400, 400));
assert!(ops.last().expect("the blur").is_active());
@@ -1243,74 +1129,59 @@ mod tests {
"the premise: a radius too small to draw emits no pass"
);
let composed = compose_detail(&ops, scale, dr_types::ColourSpace::Srgb);
assert_eq!(composed.len(), 1, "the chain must not be empty here");
assert_eq!(composed.radius(), 0, "it reads only the pixel it writes");
let resolve = &composed.passes[0];
assert_eq!(resolve.label, "detail/resolve");
assert!(resolve.writes_output);
assert!(resolve.source.contains("texture_storage_2d<rgba8unorm"));
assert!(resolve.source.contains("fn encode_output"));
// Exactly the fixed base block and no more: a pass with no body has
// nothing of its own to upload, and the block still has to be a
// multiple of sixteen bytes.
assert_eq!(resolve.uniforms.len(), DETAIL_BASE_UNIFORM_FIELDS);
assert_eq!(resolve.uniforms.len() % 4, 0);
// And it really is a copy: the fused pass composed alongside it is the
// one that stopped short, so the two agree about who encodes.
assert_eq!(fused(&ops).output_mode, OutputMode::LinearWorking);
// Empty, and that is fine since D19: nothing in the chain encodes, so
// there is no output transform for an empty chain to leave undone.
// The fused pass stopped at linear values and its view pass reads
// them directly.
let composed = compose_detail(&ops, scale);
assert!(composed.is_empty());
let fused = fused(&ops);
assert_eq!(fused.output_mode, OutputMode::LinearWorking);
let view = fused
.view
.expect("the view pass performs the output transform");
assert_eq!(view.output_mode, OutputMode::Encoded);
assert!(view.source.contains("fn encode_output"));
}
#[test]
fn a_pass_that_changes_nothing_is_dropped_where_that_is_exact() {
// TRACES: NFR-P5
// Capture sharpening at a scale too coarse to draw its radius emits a
// pass with an empty body. Between two other passes it costs a
// render-sized read and write and changes no texel, so it goes; as the
// last pass it performs the output transform on the intermediate, and
// moving that onto the pass before would round differently, so it
// stays.
use crate::ops::{capture_sharpen, CaptureSharpen, NoiseReduction};
let sharpen = || -> Box<dyn Operation> {
let mut op = CaptureSharpen::new();
op.set_param(capture_sharpen::AMOUNT, 60.0);
Box::new(op)
};
// A pass with an empty body costs a render-sized read and write and
// changes no texel, so it goes — wherever it falls since D19, the last
// position included, because the last pass writes an intermediate like
// every other and the view pass reads whichever the chain last wrote.
// Built by hand, and run as a repair so it goes first: no operation
// emits one any more (capture sharpening at a scale too coarse to draw
// its radius used to, and now emits nothing).
use crate::ops::NoiseReduction;
let chroma = || -> Box<dyn Operation> { Box::new(NoiseReduction::with_amounts(0.0, 60.0)) };
// A 24 MP frame fitted to a panel: a one-source-pixel radius is a
// quarter of a render pixel.
let scale = RenderScale::new((1500, 1000), (6000, 4000));
let unresolved = sharpen().detail().expect("a detail stage").passes(scale);
assert!(
unresolved.len() == 1 && unresolved[0].is_identity(),
"the premise: sharpening at this scale is one pass that does nothing"
);
let labels = |ops: &[Box<dyn Operation>]| -> Vec<String> {
compose_detail(ops, scale, dr_types::ColourSpace::Srgb)
let nothing = DetailPass {
output_scale: 1,
label: "nothing",
radius: 0,
wgsl: "// `c` already holds this pixel.".to_string(),
uniforms: Vec::new(),
storage: Vec::new(),
};
assert!(nothing.is_identity(), "the premise");
let labels: Vec<String> =
compose_detail_with(&[chroma()], std::slice::from_ref(&nothing), scale)
.passes
.iter()
.map(|p| p.label.clone())
.collect()
};
// First, ahead of the chroma passes: dropped.
let first = labels(&[sharpen(), chroma()]);
.collect();
assert_eq!(
first,
labels,
[
"noise_reduction/chroma-horizontal",
"noise_reduction/chroma-vertical"
]
);
// Last, after them: kept, and it is the pass that encodes.
let last = labels(&[chroma(), sharpen()]);
assert_eq!(last.len(), 3);
assert_eq!(last[2], "capture_sharpen/unresolved");
// Alone: kept, because the fused pass stopped short and something has
// to finish the frame.
assert_eq!(labels(&[sharpen()]), ["capture_sharpen/unresolved"]);
// Alone: dropped too, and the chain is empty — the view pass
// finishes the frame.
assert!(compose_detail_with(&[], &[nothing], scale).is_empty());
}
#[test]
@@ -1325,11 +1196,7 @@ mod tests {
// A pass that says nothing about `aux` hands on what it was given,
// which is why the box blur below needs no knowledge of it.
let ops = with_blur(0.05);
let composed = compose_detail(
&ops,
RenderScale::full((512, 512)),
dr_types::ColourSpace::Srgb,
);
let composed = compose_detail(&ops, RenderScale::full((512, 512)));
for pass in &composed.passes {
assert!(
@@ -1344,11 +1211,12 @@ mod tests {
.contains("textureStore(output, coord, vec4<f32>(c, aux));"),
"an intermediate must carry the lane to the pass after it"
);
// The last pass writes the display texture, whose alpha is opacity and
// not scratch space. Readable there, not written — which is the right
// way round, because the combining pass is the one that reads it.
assert!(composed.passes[1].writes_output);
assert!(!composed.passes[1].source.contains("vec4<f32>(c, aux)"));
// The last pass carries it too: since D19 it writes an intermediate
// for the view pass rather than the display texture, whose alpha is
// opacity. The view pass reads only the colour.
assert!(composed.passes[1]
.source
.contains("textureStore(output, coord, vec4<f32>(c, aux));"));
}
#[test]
@@ -1357,11 +1225,7 @@ mod tests {
// overwhelmingly common edit: no sharpening means no chain, which
// means `dr-gpu` runs the single fused dispatch it always did.
let ops = crate::ops::chain();
let composed = compose_detail(
&ops,
RenderScale::full((64, 64)),
dr_types::ColourSpace::Srgb,
);
let composed = compose_detail(&ops, RenderScale::full((64, 64)));
assert!(composed.is_empty());
assert_eq!(composed.radius(), 0);
}