From 1d8b30000605fc28d34e39a42ac07c910dfb732c Mon Sep 17 00:00:00 2001 From: Arman Uguray Date: Tue, 2 Apr 2024 21:43:50 +0000 Subject: [PATCH] Revert "[vello] Enable the bump allocation estimator" This reverts commit d63b4892245184c71df1319c79e7d0bbd04048af. Reason for revert: path_huge_crbug_800804 is causing overflow due to multiply in the estimator code. This requires a fix in a third_party repo so fixing forward will take time. Original change's description: > [vello] Enable the bump allocation estimator > > The bump allocation estimator computes a conservative estimate of the > required buffer allocations based on heuristics over the encoded path > data. See the description of vello PR #454 for some examples scenes and > the current factors of overestimation: https://github.com/linebender/vello/pull/454 > > Bug: b/285193099 > Change-Id: I131305914307591ad04d6158f24a94235826ac10 > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/828997 > Commit-Queue: Arman Uguray > Reviewed-by: Jim Van Verth > Reviewed-by: James Godfrey-Kittle Bug: b/285193099 Change-Id: I3604d62d5b55dd4680ee2a8dc49e37cfdac01500 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://skia-review.googlesource.com/c/skia/+/835421 Auto-Submit: Arman Uguray Commit-Queue: Arman Uguray Bot-Commit: Rubber Stamper --- bazel/external/vello/BUILD.bazel | 1 - third_party/vello/src/encoding.rs | 137 ++++++++++-------------------- 2 files changed, 47 insertions(+), 91 deletions(-) diff --git a/bazel/external/vello/BUILD.bazel b/bazel/external/vello/BUILD.bazel index c873cb74c84d..c6e4a559fb45 100644 --- a/bazel/external/vello/BUILD.bazel +++ b/bazel/external/vello/BUILD.bazel @@ -26,7 +26,6 @@ rust_library( include = ["crates/encoding/src/**/*.rs"], allow_empty = False, ), - crate_features = ["bump_estimate"], visibility = ["//visibility:public"], deps = [ "@vello_deps//:bytemuck", diff --git a/third_party/vello/src/encoding.rs b/third_party/vello/src/encoding.rs index d64848424463..a453fe3b7e08 100644 --- a/third_party/vello/src/encoding.rs +++ b/third_party/vello/src/encoding.rs @@ -10,14 +10,11 @@ use { Brush, Color, Fill, Mix, }, std::pin::Pin, - vello_encoding::{ - BumpEstimator, Encoding as VelloEncoding, PathEncoder, RenderConfig, Transform, - }, + vello_encoding::{Encoding as VelloEncoding, RenderConfig, Transform}, }; pub(crate) struct Encoding { encoding: VelloEncoding, - estimator: BumpEstimator, } pub(crate) fn new_encoding() -> Box { @@ -32,7 +29,7 @@ impl Encoding { // the encoding as non-fragment achieves this. let mut encoding = VelloEncoding::new(); encoding.reset(); - Encoding { encoding, estimator: BumpEstimator::new(), } + Encoding { encoding } } pub fn is_empty(&self) -> bool { @@ -41,7 +38,6 @@ impl Encoding { pub fn reset(&mut self) { self.encoding.reset(); - self.estimator.reset(); } pub fn fill( @@ -51,10 +47,10 @@ impl Encoding { brush: &ffi::Brush, path_iter: Pin<&mut ffi::PathIterator>, ) { - let t = Transform::from_kurbo(&transform.into()); - self.encoding.encode_transform(t); + self.encoding + .encode_transform(Transform::from_kurbo(&transform.into())); self.encoding.encode_fill_style(style.into()); - if self.encode_path(path_iter, &t, None) { + if self.encode_path(path_iter, /*is_fill=*/ true) { self.encoding.encode_brush(&Brush::from(brush), 1.0) } } @@ -66,24 +62,23 @@ impl Encoding { brush: &ffi::Brush, path_iter: Pin<&mut ffi::PathIterator>, ) { - let t = Transform::from_kurbo(&transform.into()); - self.encoding.encode_transform(t); - + self.encoding + .encode_transform(Transform::from_kurbo(&transform.into())); // TODO: process any dash pattern here using kurbo's dash expander unless Graphite // handles dashing already. - let stroke = style.into(); - self.encoding.encode_stroke_style(&stroke); - if self.encode_path(path_iter, &t, Some(&stroke)) { - self.encoding.encode_brush(&Brush::from(brush), 1.0); + self.encoding.encode_stroke_style(&style.into()); + if self.encode_path(path_iter, /*is_fill=*/ false) { + self.encoding.encode_brush(&Brush::from(brush), 1.0) } } pub fn begin_clip(&mut self, transform: ffi::Affine, path_iter: Pin<&mut ffi::PathIterator>) { - let t = Transform::from_kurbo(&transform.into()); - self.encoding.encode_transform(t); + self.encoding + .encode_transform(Transform::from_kurbo(&transform.into())); self.encoding.encode_fill_style(Fill::NonZero); - self.encode_path(path_iter, &t, None); - self.encoding.encode_begin_clip(Mix::Clip.into(), /*alpha=*/ 1.0); + self.encode_path(path_iter, /*is_fill=*/ true); + self.encoding + .encode_begin_clip(Mix::Clip.into(), /*alpha=*/ 1.0); } pub fn end_clip(&mut self) { @@ -98,83 +93,45 @@ impl Encoding { ) -> Box { let mut packed_scene = Vec::new(); let layout = vello_encoding::resolve_solid_paths_only(&self.encoding, &mut packed_scene); - let mut config = RenderConfig::new(&layout, width, height, &background.into()); - - let bump_estimate = self.estimator.tally(None); - //println!("bump: {bump_estimate}"); - config.buffer_sizes.bin_data = bump_estimate.binning; - config.buffer_sizes.seg_counts = bump_estimate.seg_counts; - config.buffer_sizes.segments = bump_estimate.segments; - config.buffer_sizes.lines = bump_estimate.lines; - config.gpu.binning_size = bump_estimate.binning.len(); - config.gpu.segments_size = bump_estimate.segments.len(); - + let config = RenderConfig::new(&layout, width, height, &background.into()); Box::new(RenderConfiguration { packed_scene, config, }) } - fn encode_path( - &mut self, - iter: Pin<&mut ffi::PathIterator>, - transform: &Transform, - stroke: Option<&Stroke>, - ) -> bool { - let mut encoder = self.encoding.encode_path(/*is_fill=*/ stroke.is_none()); - - // Wrap the input iterator inside a custom iterator, so that the path gets - // encoded as the estimator runs through it. - let path = IterablePathEncoder { iter, encoder: &mut encoder }; - self.estimator.count_path(path, transform, stroke); - encoder.finish(/*insert_path_marker=*/ true) != 0 - } -} - -// This is path element iterator that encodes path elements as it gets polled. -struct IterablePathEncoder<'a, 'b> { - iter: Pin<&'a mut ffi::PathIterator>, - encoder: &'a mut PathEncoder<'b>, -} - -impl Iterator for IterablePathEncoder<'_, '_> { - type Item = PathEl; - - fn next(&mut self) -> Option { - let mut path_el = ffi::PathElement::default(); - if !unsafe { self.iter.as_mut().next_element(&mut path_el) } { - return None; - } - Some(match path_el.verb { - ffi::PathVerb::MoveTo => { - let p = &path_el.points[0]; - self.encoder.move_to(p.x, p.y); - PathEl::MoveTo(p.into()) - } - ffi::PathVerb::LineTo => { - let p = &path_el.points[1]; - self.encoder.line_to(p.x, p.y); - PathEl::LineTo(p.into()) - } - ffi::PathVerb::QuadTo => { - let p0 = &path_el.points[1]; - let p1 = &path_el.points[2]; - self.encoder.quad_to(p0.x, p0.y, p1.x, p1.y); - PathEl::QuadTo(p0.into(), p1.into()) + fn encode_path(&mut self, mut path_iter: Pin<&mut ffi::PathIterator>, is_fill: bool) -> bool { + let segments = { + let mut path_encoder = self.encoding.encode_path(is_fill); + let mut path_el = ffi::PathElement::default(); + while unsafe { path_iter.as_mut().next_element(&mut path_el) } { + match path_el.verb { + ffi::PathVerb::MoveTo => { + let p = &path_el.points[0]; + path_encoder.move_to(p.x, p.y); + } + ffi::PathVerb::LineTo => { + let p = &path_el.points[1]; + path_encoder.line_to(p.x, p.y); + } + ffi::PathVerb::QuadTo => { + let p0 = &path_el.points[1]; + let p1 = &path_el.points[2]; + path_encoder.quad_to(p0.x, p0.y, p1.x, p1.y); + } + ffi::PathVerb::CurveTo => { + let p0 = &path_el.points[1]; + let p1 = &path_el.points[2]; + let p2 = &path_el.points[3]; + path_encoder.cubic_to(p0.x, p0.y, p1.x, p1.y, p2.x, p2.y); + } + ffi::PathVerb::Close => path_encoder.close(), + _ => panic!("invalid path verb"), + } } - ffi::PathVerb::CurveTo => { - let p0 = &path_el.points[1]; - let p1 = &path_el.points[2]; - let p2 = &path_el.points[3]; - self.encoder.cubic_to(p0.x, p0.y, p1.x, p1.y, p2.x, p2.y); - PathEl::CurveTo(p0.into(), p1.into(), p2.into()) - } - ffi::PathVerb::Close => { - self.encoder.close(); - PathEl::ClosePath - } - _ => panic!("invalid path verb"), - }) + path_encoder.finish(/*insert_path_marker=*/ true) + }; + segments != 0 } }