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

Hand out descriptors a declaration could produce

Browse Source 
`Operation::descriptor()` returned `&'static OpDescriptor`, and that lifetime
is the whole reason a build-time node is free and a run-time node is
impossible: only a compile-time literal can satisfy it, so no amount of
reading `ops/*.yaml` at startup could ever produce a descriptor the rest of
the application would accept. FR-PLG-2 says a bundled operation and a
third-party plugin are the same kind of thing, differing only in where the
file was found — and a lifetime outsiders cannot meet is exactly the second,
weaker format that requirement forbids.

So a descriptor is now owned and handed out as `Arc<OpDescriptor>`, with `Vec`
where it held `&'static` slices. `Arc` rather than a `&self`-borrowed
reference because the callers want to *keep* it: the develop panel collects
descriptors and then mutates the graph, and a borrow would tie the
descriptor's lifetime to a borrow of the operation it came from, which is the
one thing `&'static` was doing right.

The identifier newtypes deliberately did not follow. `ParamId` is `Copy`, is
compared in `match` arms against generated constants, is a map key in the
sidecar and history, and reaches Slint model rows; an `Arc<str>` there would
cost a refcount on every one of those and would take `match id { EXPOSURE =>
.. }` away from the generated code. They gain an interner instead, which is
honest about its lifetime rather than pretending to one — the set of ids is
bounded by deduplication and is process-lifetime by construction, because the
sidecar on disk names its parameters and an id has to stay resolvable for as
long as any edit naming it can be opened.

No behaviour changes. Every descriptor that was a `static` is a `LazyLock`
initialiser now, `Operation::helpers` borrows from `self` instead of being
`'static` so a future run-time node can own its list, and `Warp` and `Framing`
follow `Operation` so there is one shape rather than two.

The one place a descriptor is read per frame is `compose_full`, which takes
`descriptor().id` to prefix each active operation's uniforms, and `dr-ui`
composes on every frame it draws. That is a dozen atomic increments beside a
composition that is already building several kilobytes of WGSL on the same
call; it is noted at the trait method rather than left for a profiler to find.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-27 19:02:25 +02:00
co-authored by Claude Opus 5
parent 725f7bf77f
commit 0c3b8cb1c4
23 changed files with 772 additions and 490 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/dr-pipeline/src/operation.rs
+111 -40
View File
@@ -21,6 +21,7 @@
//! generator emits readable, commented output — see [`compose`].
use std::fmt::Write as _;
use std::sync::Arc;
use dr_types::{ColourSpace, Transfer};
@@ -171,7 +172,7 @@ impl Invalidation {
pub(crate) fn hash_op(h: u64, op: &dyn Operation) -> u64 {
let desc = op.descriptor();
let mut h = hash_bytes(h, desc.id.0.as_bytes());
for p in desc.params {
for p in &desc.params {
h = hash_bytes(h, p.id.0.as_bytes());
h = mix(h, u64::from(canonical_bits(op.param(p.id))));
}
@@ -213,6 +214,12 @@ pub(crate) const FNV_OFFSET: u64 = 0xcbf2_9ce4_8422_2325;
pub struct Uniform {
/// Field name as it appears in WGSL. Prefixed with the op id by the
/// composer, so two operations may both declare `amount`.
///
/// `&'static str` for the reason [`crate::descriptor::intern`] gives about
/// ids: a uniform name is a small, deduplicated, process-lifetime piece of
/// vocabulary, and a declared operation interns its names once when it is
/// parsed rather than allocating them on every `uniforms()` call — which
/// happens per composition, and composition happens per frame.
pub name: &'static str,
pub value: f32,
}
@@ -222,8 +229,32 @@ pub struct Uniform {
/// Object-safe: the pipeline holds `Box<dyn Operation>` in graph order, so
/// order is data rather than code (ARCH §3.4).
pub trait Operation: Send + Sync {
/// Static description, driving UI generation (FR-DEV-3a).
fn descriptor(&self) -> &'static OpDescriptor;
/// TRACES: FR-DEV-3a | FR-PLG-2
/// This operation's description, driving UI generation (FR-DEV-3a).
///
/// **Shared and owned rather than `&'static`.** See [`OpDescriptor`] for
/// why — in short, a `&'static` descriptor is one a compile-time literal
/// can produce and a load-time declaration cannot, which would make a
/// plugin a second-class kind of operation for a reason that is purely an
/// artefact of how the built-ins happen to be written.
///
/// # What this costs, and where
///
/// One `Arc` clone and drop per call. Descriptors are read when a panel is
/// built (`EditGraph::capabilities`), when a sidecar is written or read,
/// and when the history names what changed — none of which is a per-frame
/// path.
///
/// There is **one** exception, and it is worth stating plainly rather than
/// letting somebody discover it with a profiler: [`compose_full`] reads
/// `descriptor().id` once per *active* operation to prefix its uniforms,
/// and `dr-ui` composes on every frame it draws. That is a handful of
/// atomic increments — a dozen or so, against a composition that is
/// already building several kilobytes of WGSL text from scratch on the
/// same call. If composition ever stops being a per-frame operation, this
/// stops being a question at all; while it is one, the refcount is not
/// what makes it expensive.
fn descriptor(&self) -> Arc<OpDescriptor>;
/// Set a parameter. Values arrive already clamped to the descriptor.
fn set_param(&mut self, id: ParamId, value: f32);
@@ -321,7 +352,13 @@ pub trait Operation: Send + Sync {
/// Emitted once per *distinct* function name even if several operations
/// request it, so shared helpers (luminance, soft clipping) are declared
/// exactly once.
fn helpers(&self) -> &'static [Helper] {
///
/// Borrowed from `self` rather than `'static`, for the reason
/// [`Self::descriptor`] is owned: a generated operation returns a
/// `&'static [Helper]` and coerces, while an operation built from a
/// declaration at load time owns its list. The [`Helper`] *strings*
/// themselves stay `&'static` — they are interned, like the ids.
fn helpers(&self) -> &[Helper] {
&[]
}
@@ -1184,31 +1221,37 @@ pub(crate) fn sanitise(id: &str) -> String {
#[cfg(test)]
mod tests {
use super::*;
use std::sync::LazyLock;
use crate::descriptor::Attribute;
use crate::descriptor::{LocalizedKey, OpId, ParamDescriptor};
static DESC_A: OpDescriptor = OpDescriptor {
id: OpId("op_a"),
label: LocalizedKey("a"),
params: &[ParamDescriptor::amount("amount", "a.amount")],
attributes: &[Attribute::Tone],
};
static DESC_B: OpDescriptor = OpDescriptor {
id: OpId("op_b"),
label: LocalizedKey("b"),
params: &[ParamDescriptor::amount("amount", "b.amount")],
attributes: &[Attribute::Tone],
};
static DESC_A: LazyLock<Arc<OpDescriptor>> = LazyLock::new(|| {
Arc::new(OpDescriptor {
id: OpId("op_a"),
label: LocalizedKey("a"),
params: vec![ParamDescriptor::amount("amount", "a.amount")],
attributes: vec![Attribute::Tone],
})
});
static DESC_B: LazyLock<Arc<OpDescriptor>> = LazyLock::new(|| {
Arc::new(OpDescriptor {
id: OpId("op_b"),
label: LocalizedKey("b"),
params: vec![ParamDescriptor::amount("amount", "b.amount")],
attributes: vec![Attribute::Tone],
})
});
struct Fake {
desc: &'static OpDescriptor,
desc: Arc<OpDescriptor>,
amount: f32,
helper: Option<Helper>,
}
impl Operation for Fake {
fn descriptor(&self) -> &'static OpDescriptor {
self.desc
fn descriptor(&self) -> Arc<OpDescriptor> {
self.desc.clone()
}
fn set_param(&mut self, _id: ParamId, value: f32) {
self.amount = value;
@@ -1241,7 +1284,7 @@ mod tests {
source: "fn luma(c: vec3<f32>) -> f32 { return c.g; }",
}];
fn fake(desc: &'static OpDescriptor, amount: f32, helper: bool) -> Box<dyn Operation> {
fn fake(desc: Arc<OpDescriptor>, amount: f32, helper: bool) -> Box<dyn Operation> {
Box::new(Fake {
desc,
amount,
@@ -1253,7 +1296,7 @@ mod tests {
fn an_inactive_operation_contributes_nothing() {
// The point of composing rather than branching: an op at neutral
// must not appear in the source at all.
let ops = vec![fake(&DESC_A, 0.0, false)];
let ops = vec![fake(DESC_A.clone(), 0.0, false)];
let shader = compose(&ops);
assert!(
!shader.source.contains("op_a"),
@@ -1274,7 +1317,7 @@ mod tests {
#[test]
fn an_active_operation_appears_once() {
let ops = vec![fake(&DESC_A, 2.0, false)];
let ops = vec![fake(DESC_A.clone(), 2.0, false)];
let shader = compose(&ops);
assert!(shader.source.contains("---- op_a ----"));
assert!(shader.source.contains("u.op_a_amount"));
@@ -1285,7 +1328,10 @@ mod tests {
// Both fakes declare a uniform called `amount`. Without prefixing,
// the generated struct would have a duplicate field and fail to
// compile — the failure mode that makes naive concatenation fragile.
let ops = vec![fake(&DESC_A, 1.0, false), fake(&DESC_B, 2.0, false)];
let ops = vec![
fake(DESC_A.clone(), 1.0, false),
fake(DESC_B.clone(), 2.0, false),
];
let shader = compose(&ops);
assert!(shader.source.contains("op_a_amount: f32"));
assert!(shader.source.contains("op_b_amount: f32"));
@@ -1295,7 +1341,10 @@ mod tests {
#[test]
fn uniform_values_follow_declaration_order() {
let ops = vec![fake(&DESC_A, 1.5, false), fake(&DESC_B, 2.5, false)];
let ops = vec![
fake(DESC_A.clone(), 1.5, false),
fake(DESC_B.clone(), 2.5, false),
];
let shader = compose(&ops);
assert_eq!(shader.uniforms[PREAMBLE_FIELDS], 1.5);
assert_eq!(shader.uniforms[PREAMBLE_FIELDS + 1], 2.5);
@@ -1305,7 +1354,10 @@ mod tests {
fn a_shared_helper_is_emitted_once() {
// Two operations wanting the same helper must not produce a
// duplicate function definition.
let ops = vec![fake(&DESC_A, 1.0, true), fake(&DESC_B, 1.0, true)];
let ops = vec![
fake(DESC_A.clone(), 1.0, true),
fake(DESC_B.clone(), 1.0, true),
];
let shader = compose(&ops);
assert_eq!(
shader.source.matches("fn luma(").count(),
@@ -1319,7 +1371,17 @@ mod tests {
// WGSL rejects a uniform struct whose size is not a multiple of 16.
for n in 0..6 {
let ops: Vec<Box<dyn Operation>> = (0..n)
.map(|i| fake(if i % 2 == 0 { &DESC_A } else { &DESC_B }, 1.0, false))
.map(|i| {
fake(
if i % 2 == 0 {
DESC_A.clone()
} else {
DESC_B.clone()
},
1.0,
false,
)
})
.collect();
let shader = compose(&ops);
assert_eq!(
@@ -1335,11 +1397,14 @@ mod tests {
fn structure_hash_ignores_values_but_tracks_the_op_set() {
// The property the shader cache depends on: moving a slider must not
// trigger a recompile, but enabling an operation must.
let a1 = compose(&[fake(&DESC_A, 1.0, false)]).structure_hash;
let a2 = compose(&[fake(&DESC_A, 9.0, false)]).structure_hash;
let a1 = compose(&[fake(DESC_A.clone(), 1.0, false)]).structure_hash;
let a2 = compose(&[fake(DESC_A.clone(), 9.0, false)]).structure_hash;
assert_eq!(a1, a2, "a value change must reuse the compiled pipeline");
let both = compose(&[fake(&DESC_A, 1.0, false), fake(&DESC_B, 1.0, false)]);
let both = compose(&[
fake(DESC_A.clone(), 1.0, false),
fake(DESC_B.clone(), 1.0, false),
]);
assert_ne!(a1, both.structure_hash, "a different op-set must recompile");
}
@@ -1347,8 +1412,14 @@ mod tests {
fn structure_hash_is_order_sensitive() {
// Operation order is data (ARCH §3.4); two orders are different
// shaders and must not share a cache entry.
let ab = compose(&[fake(&DESC_A, 1.0, false), fake(&DESC_B, 1.0, false)]);
let ba = compose(&[fake(&DESC_B, 1.0, false), fake(&DESC_A, 1.0, false)]);
let ab = compose(&[
fake(DESC_A.clone(), 1.0, false),
fake(DESC_B.clone(), 1.0, false),
]);
let ba = compose(&[
fake(DESC_B.clone(), 1.0, false),
fake(DESC_A.clone(), 1.0, false),
]);
assert_ne!(ab.structure_hash, ba.structure_hash);
}
@@ -1412,7 +1483,7 @@ mod tests {
// Exposure and the tonal controls act on white-balanced values; if
// the multiply came afterwards, every operation would be reasoning
// about a green-cast image.
let ops = vec![fake(&DESC_A, 2.0, false)];
let ops = vec![fake(DESC_A.clone(), 2.0, false)];
let source = compose(&ops).source;
let wb = source.find("u.as_shot_wb").expect("wb applied");
let op = source.find("---- op_a ----").expect("op present");
@@ -1472,7 +1543,7 @@ mod tests {
// The other half, and the one that would fail silently: a bug that
// suppressed the tail unconditionally renders every ordinary edit
// flat and uncorrected, which reads as a broken camera profile.
let source = compose(&[fake(&DESC_A, 2.0, false)]).source;
let source = compose(&[fake(DESC_A.clone(), 2.0, false)]).source;
assert!(source.contains("base_curve_last.z > 0.5"));
assert!(source.contains("Camera space -> linear sRGB"));
}
@@ -1484,7 +1555,7 @@ mod tests {
// stock loaded is the default state of every photograph in the
// catalogue, and it must not disturb the camera's own rendering.
let film: Box<dyn Operation> = Box::new(crate::ops::FilmSim::new());
let source = compose(&[film, fake(&DESC_A, 2.0, false)]).source;
let source = compose(&[film, fake(DESC_A.clone(), 2.0, false)]).source;
assert!(source.contains("base_curve_last.z > 0.5"));
assert!(source.contains("Camera space -> linear sRGB"));
}
@@ -1493,7 +1564,7 @@ mod tests {
fn the_camera_matrix_is_applied_after_the_operations() {
// Adjustments are meaningful in sensor-native space, where highlight
// headroom still exists; converting first would clip it away.
let ops = vec![fake(&DESC_A, 2.0, false)];
let ops = vec![fake(DESC_A.clone(), 2.0, false)];
let source = compose(&ops).source;
let op = source.find("---- op_a ----").expect("op present");
let matrix = source.find("u.cam_to_srgb_0").expect("matrix applied");
@@ -1514,7 +1585,7 @@ mod tests {
// Before the matrix: the curve was tuned against this body's own
// primaries. Applied after the conversion it would be a Canon
// rendering acting on sRGB values, which is a different curve.
let ops = vec![fake(&DESC_A, 2.0, false)];
let ops = vec![fake(DESC_A.clone(), 2.0, false)];
let source = compose(&ops).source;
let op = source.find("---- op_a ----").expect("op present");
let curve = source
@@ -1592,7 +1663,7 @@ mod tests {
// it must not pick up an identity matrix multiply for the sake of
// generality. Asserted against the source rather than against timing,
// which would not fail reliably.
let ops = vec![fake(&DESC_A, 1.0, false)];
let ops = vec![fake(DESC_A.clone(), 1.0, false)];
let srgb = compose_to(&ops, ColourSpace::Srgb).source;
assert!(
!srgb.contains("Linear sRGB -> linear sRGB"),
@@ -1607,7 +1678,7 @@ mod tests {
// in linear sRGB, the primaries conversion carries it into the wider
// space, and only then is it clipped — clipping first would discard
// exactly the colours the wider space was chosen to keep.
let source = compose_to(&[fake(&DESC_A, 1.0, false)], ColourSpace::DisplayP3).source;
let source = compose_to(&[fake(DESC_A.clone(), 1.0, false)], ColourSpace::DisplayP3).source;
let camera = source.find("u.cam_to_srgb_0").expect("camera matrix");
let convert = source
.find("Linear sRGB -> linear Display P3")
@@ -1659,7 +1730,7 @@ mod tests {
// an export that came out sRGB and claimed to be Display P3.
let mut seen: Vec<u64> = Vec::new();
for space in ColourSpace::ALL {
let h = compose_to(&[fake(&DESC_A, 1.0, false)], space).structure_hash;
let h = compose_to(&[fake(DESC_A.clone(), 1.0, false)], space).structure_hash;
assert!(!seen.contains(&h), "{space:?} collides with another space");
seen.push(h);
}
@@ -1669,7 +1740,7 @@ mod tests {
fn generated_source_carries_a_do_not_edit_banner() {
// Someone will eventually find this in a debugger and try to fix it
// in place.
let shader = compose(&[fake(&DESC_A, 1.0, false)]);
let shader = compose(&[fake(DESC_A.clone(), 1.0, false)]);
assert!(shader.source.starts_with("// GENERATED"));
}