From a3dbe2ab914dc818f79d22fd6faf0a998144c2c5 Mon Sep 17 00:00:00 2001 From: Muhammad Saad Date: Fri, 23 Oct 2026 23:49:37 +0400 Subject: [PATCH] Brush dabs: build only as far as the caller will look (#1520) MAX_STROKE_DABS bounds one stroke at two million, but the two callers that will not look at nearly that many check the length after the vector exists, which bounds nothing: it only discards what was already built. gpu/render.rs rejects a brush mask past MAX_DABS, 4096 across the whole mask, after collecting every stroke's dabs. Each stroke may construct two million first, so the sum that rejects them is reached only once they all exist. spots.rs skips an Auto Mask stroke past MAX_DABS_CONSIDERED, 200,000, after brush_dabs has built the whole thing. brush_dabs_within takes the caller's own budget, and push_dabs_within charges against it instead of MAX_STROKE_DABS. Both callers now ask for one more than they accept, so a stroke past the threshold is still recognised as past it without the rest being built: 4097 rather than two million per stroke on the GPU path, 200,001 rather than two million for the spot window. Behaviour is unchanged for anything either caller keeps. A result short of its budget was never truncated, so it is the whole stroke, and a truncated one only ever reaches a caller that was going to reject it. A truncated stroke is not a prefix: running out of room makes a segment lay its remaining dabs further apart rather than stop mid-segment, which is how push_dabs_counted already behaved at MAX_STROKE_DABS. --- crates/gpu/src/render.rs | 14 ++++++- crates/pipeline/src/masks.rs | 76 +++++++++++++++++++++++++++++++++++- crates/pipeline/src/spots.rs | 4 +- 3 files changed, 90 insertions(+), 4 deletions(-) diff --git a/crates/gpu/src/render.rs b/crates/gpu/src/render.rs index 0c5bd19f0..9fc2b26bc 100644 --- a/crates/gpu/src/render.rs +++ b/crates/gpu/src/render.rs @@ -1012,7 +1012,19 @@ fn masks(cx: &mut Cx<'_>, lin: &Buf, prep: &Prep, plan: &Plan<'_>, host: &mut Ho Some(3) } MaskShape::Brush { strokes } => { - let dabs: Vec<_> = strokes.iter().map(|st| lightcraft_pipeline::masks::brush_dabs(st, frame, w, h)).collect(); + // The kernel takes at most MAX_DABS across the whole mask, so no more than + // that (plus one, to tell "at it" from "past it") is ever built: each stroke + // may otherwise construct two million dabs before the sum below rejects them + // all (#1520). + let mut room = MAX_DABS + 1; + let dabs: Vec<_> = strokes + .iter() + .map(|st| { + let d = lightcraft_pipeline::masks::brush_dabs_within(st, frame, w, h, room); + room -= d.dabs.len().min(room); + d + }) + .collect(); // pen pressure varies radius and flow per dab, which the kernel doesn't take: the CPU rasterizes it if dabs.iter().map(|d| d.dabs.len()).sum::() > MAX_DABS || dabs.iter().any(|d| !d.pressure.is_empty()) { None diff --git a/crates/pipeline/src/masks.rs b/crates/pipeline/src/masks.rs index 36cc29192..a2497a2bd 100644 --- a/crates/pipeline/src/masks.rs +++ b/crates/pipeline/src/masks.rs @@ -524,6 +524,19 @@ pub const MAX_STROKE_DABS: usize = 2_000_000; /// The dabs of stroke `s` in a `w × h` output. pub fn brush_dabs(s: &BrushStroke, frame: &Frame, w: usize, h: usize) -> BrushDabs { + brush_dabs_within(s, frame, w, h, MAX_STROKE_DABS) +} + +/// [`brush_dabs`] stopping at `max` dabs instead of [`MAX_STROKE_DABS`]. +/// +/// For a caller that discards a stroke past a threshold of its own: it builds to that threshold +/// rather than to the stroke budget and throws the difference away. The GPU path rejects a brush +/// mask past 4096 dabs in total and the Auto Mask spot window past 200,000, both far below the +/// two million a single stroke may otherwise construct first (#1520). +/// +/// Ask for one more than the threshold to tell "at it" from "past it": a result shorter than +/// `max` is the whole stroke. +pub fn brush_dabs_within(s: &BrushStroke, frame: &Frame, w: usize, h: usize, max: usize) -> BrushDabs { let to_out = frame.norm_to_out(w, h); let ppl = frame.px_per_long(w); let r = (s.size * ppl).max(0.5); @@ -536,10 +549,14 @@ pub fn brush_dabs(s: &BrushStroke, frame: &Frame, w: usize, h: usize) -> BrushDa let with_pressure = !s.pressure.is_empty(); let mut prev = None; let mut prev_p = 1.0; + let mut emitted = 0usize; for (i, p) in s.points.iter().enumerate() { + if emitted >= max { + break; + } let q = to_out.apply(*p); let before = dabs.len(); - if push_dabs(&mut dabs, prev, q, r) { + if push_dabs_within(&mut dabs, &mut emitted, prev, q, r, max) { let pq = s.pressure_at(i); if with_pressure { // the first dab of the stroke has nothing to interpolate from @@ -585,7 +602,16 @@ pub fn push_dabs(dabs: &mut Vec, prev: Option, q: Point, r: f64) - /// dabs no longer held in `dabs`. Incremental callers retain `emitted` when clearing their buffer; /// it advances only by the number actually appended. Acceptance and spacing match [`push_dabs`]. pub fn push_dabs_counted(dabs: &mut Vec, emitted: &mut usize, prev: Option, q: Point, r: f64) -> bool { - let room = MAX_STROKE_DABS.saturating_sub(*emitted); + push_dabs_within(dabs, emitted, prev, q, r, MAX_STROKE_DABS) +} + +/// [`push_dabs_counted`] against a caller's own budget rather than [`MAX_STROKE_DABS`]. +/// +/// A caller that will not look at more than `max` dabs should build no more than `max`: a length +/// check after the vector exists bounds nothing, it only throws away what was already paid for +/// (#1520). +pub fn push_dabs_within(dabs: &mut Vec, emitted: &mut usize, prev: Option, q: Point, r: f64, max: usize) -> bool { + let room = max.min(MAX_STROKE_DABS).saturating_sub(*emitted); if room == 0 || !(q.x.is_finite() && q.y.is_finite()) { return false; } @@ -856,6 +882,52 @@ mod tests { assert!(e[0].alpha.get(50, 50) < 0.01); } + /// #1520: a caller that will not look past its own threshold builds to that threshold, not to + /// the stroke budget. Each segment here is admissible on its own and together they want far + /// more than any threshold the callers apply, which is the aggregate the issue describes + /// rather than one hostile segment. + #[test] + fn a_stroke_is_built_only_as_far_as_the_caller_will_look() { + let f = frame(4000, 4000); + // Twenty long sweeps across the output: tens of thousands of dabs between them. + let points: Vec = + (0..20).map(|i| if i % 2 == 0 { Point::new(0.02, 0.02 + i as f64 * 0.04) } else { Point::new(0.98, 0.02 + i as f64 * 0.04) }).collect(); + let s = BrushStroke { points, size: 0.002, feather: 50.0, flow: 100.0, density: 100.0, ..Default::default() }; + + let whole = brush_dabs(&s, &f, 4000, 4000); + assert!(whole.dabs.len() > 20_000, "the stroke really is large: {}", whole.dabs.len()); + assert!(whole.dabs.len() <= MAX_STROKE_DABS); + + // Each caller gets what it asked for and no more, however large the stroke is. 4097 and + // 200_001 are what the GPU path and the Auto Mask spot window now ask for. + for max in [1usize, 2, 4097, 200_001] { + let got = brush_dabs_within(&s, &f, 4000, 4000, max); + assert!(got.dabs.len() <= max, "asked for at most {max}, built {}", got.dabs.len()); + assert_eq!(got.r, whole.r, "the brush itself is unchanged"); + } + + // A budget the stroke never reaches changes nothing: it only ever truncates. + let generous = brush_dabs_within(&s, &f, 4000, 4000, whole.dabs.len() + 1000); + assert_eq!(generous.dabs, whole.dabs, "an unreached budget leaves the stroke alone"); + + // The guarantee the callers rely on: a result short of the budget was never truncated, + // so it is the stroke itself. Both callers ask for one more than they will accept, so + // anything they keep is identical to what they drew before this change, and anything + // truncated is something they were going to reject anyway. + // + // A truncated one is not a prefix: running out of room makes a segment lay its remaining + // dabs further apart rather than stop mid-segment, which is how `push_dabs_counted` + // already behaved at MAX_STROKE_DABS. That only ever reaches a caller that rejects it. + let cut = brush_dabs_within(&s, &f, 4000, 4000, 500); + assert_eq!(cut.dabs.len(), 500, "a stroke past the budget fills it exactly"); + for max in [700usize, 5_000, whole.dabs.len()] { + let got = brush_dabs_within(&s, &f, 4000, 4000, max); + if got.dabs.len() < max { + assert_eq!(got.dabs, whole.dabs, "a result short of its budget is the whole stroke"); + } + } + } + /// Issue #517: dabs along a long segment (a fast stroke: two pointer samples far apart) are at /// most [`dab_spacing`] apart, painting dab by dab (the live stroke) or the whole path at once /// (the committed one). diff --git a/crates/pipeline/src/spots.rs b/crates/pipeline/src/spots.rs index 315f7f956..a875ffb7f 100644 --- a/crates/pipeline/src/spots.rs +++ b/crates/pipeline/src/spots.rs @@ -206,7 +206,9 @@ pub fn window_for_reads_checked( // refine the stroke over a couple of radii: each dab matters to pixels within 2.5 radii let mut dab_boxes: Vec<[f64; 4]> = Vec::new(); for st in autos { - let dabs = crate::masks::brush_dabs(st, frame, full_w, full_h); + // One more than the window takes, so a stroke past it is still recognised as past it + // without the other 1.8 million dabs being built first (#1520). + let dabs = crate::masks::brush_dabs_within(st, frame, full_w, full_h, MAX_DABS_CONSIDERED + 1); let r = dabs.r; if dabs.dabs.len() > MAX_DABS_CONSIDERED { continue;