collective
Browse / github.com/storytold/photocraft / patch
Brush: bound the remaining stroke commands before their dense buffers (#8666)
Brush: bound the remaining stroke commands before their dense buffers (#8666) ### The gap #5992 bounded the stroke commands that render their coverage densely (Clone Stamp, Healing, Spot Healing, Remove, History Brush, Pattern Stamp). Every command that runs `apply_dab_stroke` has the same problem: it reads a dense `Region`, plus a working copy, over the stroke's whole bounding box. The points are only checked against `MAX_COORD`, not against the canvas, so a drag far off the canvas asks for an allocation that aborts the process (on Windows: a freeze, then the app closes). ### The fix Reuse the shared `reject_huge_stroke` (now `pub(crate)`), so the limit (`MAX_STROKE
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- dark13b (imported from pull request #9644, open)
- Base
59cb4c49bd236df04d8e37d4764253f29ca6600a- Size
- +193 −38 in 6 files
- Review
- pending
- Shared
- 2026-10-28
- Id
141258012fd9c19cbe623f85
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/brush_cmds.rs crates/engine/src/brush_cmds/mixer_live.rs crates/engine/src/brush_cmds/mixer_live_tests.rs crates/engine/src/eraser_cmds.rs crates/engine/src/retouch_cmds.rs crates/engine/src/retouch_cmds/tests.rs
@@ -715,7 +715,10 @@ fn color_replacement_setup(s: &Session, p: &Value) -> Result<(Stroke, ReplaceSet background: s.tools.background, }; let id = layer_id(s, p)?;- Ok((Stroke { brush, points: pts }, rs, id))+ let stroke = Stroke { brush, points: pts };+ // The working copy is dense over the stroke's bounds (#8666).+ crate::retouch_cmds::reject_huge_stroke(cmd, &stroke)?;+ Ok((stroke, rs, id)) } fn color_replacement(s: &mut Session, p: &Value) -> Result<Value> {@@ -132,6 +132,7 @@ pub struct LiveMixer { cmyk: Option<Arc<photocraft_color::convert::CmykSpace>>, original: Arc<Document>, points: Vec<StrokePoint>,+ point_bounds: crate::retouch_cmds::PointBounds, original_state: MixerState, } @@ -163,6 +164,8 @@ impl LiveMixer { } let points = parse_points(p, CMD)?; let brush = resolve_brush(s, p, CMD)?;+ // Dabs are tiled, but a stroke far beyond the canvas still means an unbounded dab count (#8666).+ crate::retouch_cmds::reject_huge_stroke(CMD, &Stroke { brush: brush.clone(), points: points.clone() })?; let bm = &brush.mixer; let settings = MixerSettings { wet: pct(p, "wet", bm.wet * 100.0),@@ -233,6 +236,7 @@ impl LiveMixer { cmyk: doc_cmyk_space(s), original: s.active().ok_or(EngineError::NoDocument)?.doc.clone(), points: Vec::new(),+ point_bounds: crate::retouch_cmds::PointBounds::EMPTY, original_state: s.tools.mixer.clone(), }; Ok((live, points))@@ -254,6 +258,13 @@ impl LiveMixer { if self.core.preview_limits && self.points.len().saturating_add(points.len()) > 65_536 { return Err(bad(CMD, "stroke exceeds 65536 recorded input samples")); }+ // A live stroke grows after it starts: admit the grown box before anything is mutated (#8666).+ if !points.is_empty() {+ let mut grown = self.point_bounds;+ grown.add(points);+ grown.reject_huge(CMD, &self.core.ctx.brush)?;+ self.point_bounds = grown;+ } let cmyk = self.cmyk.clone(); photocraft_color::convert::with_cmyk_space(cmyk.as_ref(), || { let old_tail = self.tail;@@ -414,3 +414,16 @@ fn mixer_command_paints_strokes_beyond_the_preview_sample_and_dab_budgets() { assert!(target(&s).rgba(100, 48)[0] > 0.9); } }++/// #8666: a live Mixer stroke that starts small and is pushed far away is refused, unchanged.+#[test]+fn mixer_live_push_refuses_a_stroke_that_grows_too_large() {+ let s = fixture(PixelFormat::RGBA8);+ let mut live = LiveMixer::begin(&s, &json!({"points":[[40,48]],"size":8,"seed":3})).unwrap();+ let (shown, bounds) = (live.doc.clone(), live.bounds());+ let err = live.push(&[StrokePoint::new(900_000.0, 900_000.0, 1.0)]).unwrap_err().to_string();+ assert!(err.contains("stroke too large"), "{err}");+ assert!(Arc::ptr_eq(&live.doc, &shown), "a refused push leaves the preview alone");+ assert_eq!(live.bounds(), bounds);+ live.push(&[StrokePoint::new(60.0, 48.0, 1.0)]).unwrap();+}@@ -143,7 +143,10 @@ fn background_eraser_setup(s: &Session, p: &Value) -> Result<(Stroke, BgEraseSet if d.doc.effective_locks(id).transparency && !d.doc.is_background(id) { return Err(EngineError::Other(format!("Could not use the Background Eraser because the transparency of layer \"{}\" is locked", l.name))); }- Ok((Stroke { brush, points: pts }, bs, id))+ let stroke = Stroke { brush, points: pts };+ // The working copy is dense over the stroke's bounds (#8666).+ crate::retouch_cmds::reject_huge_stroke(CMD, &stroke)?;+ Ok((stroke, bs, id)) } /// `paint.backgroundEraser`: a stroke erasing colours similar to the sampled one.@@ -44,42 +44,60 @@ fn bad(cmd: &str, msg: impl Into<String>) -> EngineError { /// instead of allocating. const MAX_STROKE_PIXELS: u64 = 67_108_864; -/// Refuse a stroke whose footprint would not fit the coverage budget, from the points and brush-/// size alone, before the renderer allocates anything for it.-fn reject_huge_stroke(cmd: &str, stroke: &Stroke) -> Result<()> {- // Dynamics place dabs beyond the input points, so half the tip is not the whole reach:- // Scattering offsets a centre by up to its amount times the tip size, and a Dual Brush dab- // reaches its own radius further. Pad by what those can add so this estimate does not fall- // short of what the renderer will cover. It is only the cheap early-out; the rendered extent- // is admitted for real in `coverage` below.- let b = &stroke.brush;- let size = f64::from(b.size.max(1.0));- let scatter = if b.scattering.enabled { size * f64::from(b.scattering.scatter.jitter.max(0.0)) } else { 0.0 };- let dual = if b.dual_brush.enabled { f64::from(b.dual_brush.size.max(0.0)) * 0.5 } else { 0.0 };- let pad = size.mul_add(0.5, 4.0) + scatter + dual;- let mut min_x = f64::INFINITY;- let mut min_y = f64::INFINITY;- let mut max_x = f64::NEG_INFINITY;- let mut max_y = f64::NEG_INFINITY;- for q in &stroke.points {- min_x = min_x.min(q.x);- min_y = min_y.min(q.y);- max_x = max_x.max(q.x);- max_y = max_y.max(q.y);- }- if !min_x.is_finite() || !min_y.is_finite() || !max_x.is_finite() || !max_y.is_finite() {- return Err(bad(cmd, "point coordinates must be finite"));- }- let w = (max_x - min_x + pad * 2.0).ceil().max(1.0);- let h = (max_y - min_y + pad * 2.0).ceil().max(1.0);- if !w.is_finite() || !h.is_finite() || w > 1.0e9 || h > 1.0e9 {- return Err(bad(cmd, format!("stroke too large (at most {MAX_STROKE_PIXELS} px)")));- }- let area = (w as u64).saturating_mul(h as u64);- if area > MAX_STROKE_PIXELS {- return Err(bad(cmd, format!("stroke too large ({w:.0}×{h:.0} px, at most {MAX_STROKE_PIXELS})")));- }- Ok(())+/// The box the input points of a stroke span, grown as a live stroke is pushed to.+#[derive(Clone, Copy)]+pub(crate) struct PointBounds {+ min_x: f64,+ min_y: f64,+ max_x: f64,+ max_y: f64,+}++impl PointBounds {+ pub(crate) const EMPTY: Self = Self { min_x: f64::INFINITY, min_y: f64::INFINITY, max_x: f64::NEG_INFINITY, max_y: f64::NEG_INFINITY };++ pub(crate) fn add(&mut self, points: &[StrokePoint]) {+ for q in points {+ self.min_x = self.min_x.min(q.x);+ self.min_y = self.min_y.min(q.y);+ self.max_x = self.max_x.max(q.x);+ self.max_y = self.max_y.max(q.y);+ }+ }++ /// Refuse a footprint that would not fit the coverage budget, from these bounds and the+ /// brush size alone, before the renderer allocates anything for it.+ pub(crate) fn reject_huge(&self, cmd: &str, b: &BrushSettings) -> Result<()> {+ // Dynamics place dabs beyond the input points, so half the tip is not the whole reach:+ // Scattering offsets a centre by up to its amount times the tip size, and a Dual Brush dab+ // reaches its own radius further. Pad by what those can add so this estimate does not fall+ // short of what the renderer will cover. It is only the cheap early-out; the rendered extent+ // is admitted for real in `coverage` below.+ let size = f64::from(b.size.max(1.0));+ let scatter = if b.scattering.enabled { size * f64::from(b.scattering.scatter.jitter.max(0.0)) } else { 0.0 };+ let dual = if b.dual_brush.enabled { f64::from(b.dual_brush.size.max(0.0)) * 0.5 } else { 0.0 };+ let pad = size.mul_add(0.5, 4.0) + scatter + dual;+ if !self.min_x.is_finite() || !self.min_y.is_finite() || !self.max_x.is_finite() || !self.max_y.is_finite() {+ return Err(bad(cmd, "point coordinates must be finite"));+ }+ let w = (self.max_x - self.min_x + pad * 2.0).ceil().max(1.0);+ let h = (self.max_y - self.min_y + pad * 2.0).ceil().max(1.0);+ if !w.is_finite() || !h.is_finite() || w > 1.0e9 || h > 1.0e9 {+ return Err(bad(cmd, format!("stroke too large (at most {MAX_STROKE_PIXELS} px)")));+ }+ let area = (w as u64).saturating_mul(h as u64);+ if area > MAX_STROKE_PIXELS {+ return Err(bad(cmd, format!("stroke too large ({w:.0}×{h:.0} px, at most {MAX_STROKE_PIXELS})")));+ }+ Ok(())+ }+}++/// Refuse a stroke whose footprint would not fit the coverage budget (see [`PointBounds::reject_huge`]).+pub(crate) fn reject_huge_stroke(cmd: &str, stroke: &Stroke) -> Result<()> {+ let mut bounds = PointBounds::EMPTY;+ bounds.add(&stroke.points);+ bounds.reject_huge(cmd, &stroke.brush) } /// The stroke's coverage, with the **rendered** extent admitted before the dense buffer exists.@@ -448,6 +466,7 @@ pub struct LiveRetouch { brush: Box<BrushSettings>, /// Every point rendered so far, in order. points: Vec<StrokePoint>,+ point_bounds: PointBounds, } impl LiveRetouch {@@ -514,6 +533,7 @@ impl LiveRetouch { base, brush, points: Vec::new(),+ point_bounds: PointBounds::EMPTY, }; live.push(&stroke.points)?; Ok(live)@@ -567,6 +587,13 @@ impl LiveRetouch { pub fn push(&mut self, pts: &[StrokePoint]) -> Result<Rect> { crate::brush_cmds::check_coords(pts, &self.cmd)?; let pts: Vec<_> = pts.iter().map(|q| StrokePoint::new(q.x, q.y, q.pressure)).collect();+ // A live stroke grows after it starts: admit the grown box before anything is mutated (#8666).+ if !pts.is_empty() {+ let mut grown = self.point_bounds;+ grown.add(&pts);+ grown.reject_huge(&self.cmd, &self.brush)?;+ self.point_bounds = grown;+ } self.points.extend_from_slice(&pts); self.renderer.push(&pts); if self.cmd == "paint.cloneStamp" && flag(&self.params, "wrap", false) {@@ -1128,6 +1155,7 @@ impl DabTool { /// (`apply_dab_stroke`). Blur, Sharpen and Smudge can sample all layers. fn dab_cmd(s: &mut Session, p: &Value, cmd: &str) -> Result<Value> { let (stroke, id) = parse_brush(s, p, cmd)?;+ reject_huge_stroke(cmd, &stroke)?; let (tool, label) = DabTool::parse(s, cmd, p, &stroke)?; let all = matches!(tool, DabTool::Focus { .. } | DabTool::Smudge { .. }) && sample_all_layers(p); // The live stroke the canvas already drew for exactly this commit (`LiveDab::into_commit`):@@ -1267,6 +1295,7 @@ pub struct LiveDab { base: Option<(photocraft_doc::DocId, u64)>, /// Every point rendered so far, in order. points: Vec<StrokePoint>,+ point_bounds: PointBounds, } impl LiveDab {@@ -1339,6 +1368,8 @@ impl LiveDab { neighborhood: DabNeighborhood, effect: impl FnOnce(&Surface) -> DabEffect, ) -> Result<Self> {+ // The working copy is dense over the stroke's bounds (#8666).+ reject_huge_stroke(cmd, &stroke)?; let sel = doc.selection.clone(); let (surf, lock) = crate::channel_cmds::target_surface(&mut doc, id, p)?; let pre_surf = surf.clone();@@ -1365,6 +1396,7 @@ impl LiveDab { tool: None, base: None, points: Vec::new(),+ point_bounds: PointBounds::EMPTY, }; live.push(&stroke.points)?; Ok(live)@@ -1415,6 +1447,13 @@ impl LiveDab { pub fn push(&mut self, pts: &[StrokePoint]) -> Result<Rect> { crate::brush_cmds::check_coords(pts, &self.cmd)?; let pts: Vec<_> = pts.iter().map(|q| StrokePoint::new(q.x, q.y, q.pressure)).collect();+ // A live stroke grows after it starts: admit the grown box before anything is mutated (#8666).+ if !pts.is_empty() {+ let mut grown = self.point_bounds;+ grown.add(&pts);+ grown.reject_huge(&self.cmd, &self.renderer.ctx.brush)?;+ self.point_bounds = grown;+ } self.points.extend_from_slice(&pts); self.renderer.push(&pts); let dmg = self.run_new_dabs();@@ -607,6 +607,53 @@ fn every_retouch_stroke_refuses_a_box_it_cannot_render() { assert!(err.contains("stroke too large"), "paint.remove: {err}"); } +/// #8666: the sequential-dab strokes (Dodge, Burn, Sponge, Blur, Sharpen, Smudge), the Mixer Brush,+/// Color Replacement and the Background Eraser all copy the target densely over the stroke's+/// bounds, and a drag far off the canvas made that copy large enough to abort the process. Each+/// refuses the oversized stroke without touching the pixels or the history, and an ordinary+/// stroke still runs, at every depth.+#[test]+fn every_dab_stroke_refuses_a_box_it_cannot_copy() {+ let huge = json!([[0, 0], [900_000, 900_000]]);+ let ok = json!([[2, 2], [30, 16]]);+ let cases: [(&str, Value); 10] = [+ ("paint.dodge", json!({"size": 8})),+ ("paint.burn", json!({"size": 8})),+ ("paint.sponge", json!({"size": 8})),+ ("paint.blur", json!({"size": 8})),+ ("paint.sharpen", json!({"size": 8})),+ ("paint.smudge", json!({"size": 8})),+ ("paint.mixerBrush", json!({"size": 8})),+ ("paint.mixerBrush", json!({"size": 8, "sampleAllLayers": true})),+ ("paint.colorReplacement", json!({"size": 8})),+ ("paint.backgroundEraser", json!({"size": 8})),+ ];+ for depth in DEPTHS {+ for (cmd, base) in &cases {+ let mut s = session(40, 20, depth, "rgb");+ s.execute("paint.stroke", json!({"points": [[0, 10], [40, 10]], "size": 12, "hardness": 1.0, "color": "#ff0000"})).unwrap();+ let before = rgba(&s, 20, 10);+ let steps = s.active().unwrap().history.past_len();+ let with = |points: &Value| {+ let mut v = base.clone();+ if let Some(o) = v.as_object_mut() {+ o.insert("points".into(), points.clone());+ }+ v+ };+ let err = s.execute(cmd, with(&huge)).unwrap_err().to_string();+ assert!(err.contains("stroke too large"), "{cmd} at {depth}: {err}");+ assert_eq!(rgba(&s, 20, 10), before, "{cmd} at {depth}: a refused stroke leaves the pixels alone");+ assert_eq!(s.active().unwrap().history.past_len(), steps, "{cmd} at {depth}: and records no history step");++ let pixels = layer_pixels(&s);+ s.execute(cmd, with(&ok)).unwrap_or_else(|e| panic!("{cmd} at {depth}: an ordinary stroke must work: {e}"));+ assert_ne!(layer_pixels(&s), pixels, "{cmd} at {depth}: and changes pixels");+ assert_eq!(s.active().unwrap().history.past_len(), steps + 1, "{cmd} at {depth}: and records one step");+ }+ }+}+ /// The review on #6110: the estimate from the input points is not the bound. Brush dynamics place /// dabs beyond those points, so the budget is applied to the extent the renderer actually produced, /// before the dense buffer is allocated. An injected budget makes that observable without asking@@ -2413,3 +2460,42 @@ fn periodic_clone_interior_matches_ordinary_brush_masks_and_transparency() { } } }++/// #8666: a live stroke can start small and then grow past the budget with a pushed point. The push+/// is refused with nothing changed (preview, bounds), and the stroke carries on afterwards.+#[test]+fn live_dab_push_refuses_a_stroke_that_grows_too_large() {+ for (cmd, extra) in dab_cases() {+ let mut s = session(64, 48, 8, "rgb");+ paint_layer(&mut s, texture);+ let mut live = LiveDab::begin(&s, cmd, &dab_opts(&[[10.0, 10.0]], 50, &extra)).unwrap();+ let (shown, bounds) = (live.doc.clone(), live.bounds());+ let before = shown.layer(s.active().unwrap().active_layer.unwrap()).unwrap().surface().unwrap().clone();+ let err = live.push(&[StrokePoint::new(900_000.0, 900_000.0, 1.0)]).unwrap_err().to_string();+ assert!(err.contains("stroke too large"), "{cmd}: {err}");+ assert!(std::sync::Arc::ptr_eq(&live.doc, &shown), "{cmd}: a refused push leaves the preview alone");+ assert_eq!(live.bounds(), bounds, "{cmd}: and the bounds");+ let id = s.active().unwrap().active_layer.unwrap();+ assert_eq!(live.doc.layer(id).unwrap().surface().unwrap().read_region(shown.bounds()), before.read_region(shown.bounds()), "{cmd}: and the pixels");+ live.push(&[StrokePoint::new(30.0, 20.0, 1.0)]).unwrap_or_else(|e| panic!("{cmd}: the stroke must carry on: {e}"));+ }+}++/// #8666: the live Clone Stamp, Healing Brush, History Brush and Pattern Stamp strokes are refused+/// when a pushed point grows them past the budget, with the preview and bounds untouched.+#[test]+fn live_retouch_push_refuses_a_stroke_that_grows_too_large() {+ for cmd in ["paint.cloneStamp", "paint.healingBrush", "paint.historyBrush", "paint.patternStamp"] {+ let mut s = session(64, 48, 8, "rgb");+ paint_layer(&mut s, texture);+ let p = json!({"points": [[10, 10]], "source": [4, 8], "size": 8, "hardness": 50, "pattern": "Checkerboard", "aligned": true});+ let mut live = LiveRetouch::begin(&s, cmd, &p).unwrap();+ let (shown, bounds) = (live.doc.clone(), live.bounds());+ let err = live.push(&[StrokePoint::new(900_000.0, 900_000.0, 1.0)]).unwrap_err().to_string();+ assert!(err.contains("stroke too large"), "{cmd}: {err}");+ assert!(std::sync::Arc::ptr_eq(&live.doc, &shown), "{cmd}: a refused push leaves the preview alone");+ assert_eq!(live.bounds(), bounds, "{cmd}");+ assert_eq!(live.points().len(), 1, "{cmd}: and the recorded points");+ live.push(&[StrokePoint::new(30.0, 20.0, 1.0)]).unwrap_or_else(|e| panic!("{cmd}: the stroke must carry on: {e}"));+ }+}