Skip to content

Commit

Permalink
Revert "[vello] Enable the bump allocation estimator"
Browse files Browse the repository at this point in the history 
This reverts commit d63b489.

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: linebender/vello#454
>
> Bug: b/285193099
> Change-Id: I131305914307591ad04d6158f24a94235826ac10
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/828997
> Commit-Queue: Arman Uguray <[email protected]>
> Reviewed-by: Jim Van Verth <[email protected]>
> Reviewed-by: James Godfrey-Kittle <[email protected]>

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 <[email protected]>
Commit-Queue: Arman Uguray <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
  • Loading branch information
@armansito
armansito authored and SkCQ committed Apr 2, 2024
1 parent a65f274 commit 1d8b300
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 91 deletions.
1 change: 0 additions & 1 deletion bazel/external/vello/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
137 changes: 47 additions & 90 deletions third_party/vello/src/encoding.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Encoding> {
Expand All @@ -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 {
Expand All @@ -41,7 +38,6 @@ impl Encoding {

pub fn reset(&mut self) {
self.encoding.reset();
self.estimator.reset();
}

pub fn fill(
Expand All @@ -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)
}
}
Expand All @@ -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) {
Expand All @@ -98,83 +93,45 @@ impl Encoding {
) -> Box<RenderConfiguration> {
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<Self::Item> {
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
}
}

Expand Down

0 comments on commit 1d8b300

Please sign in to comment.