fix: change assert!() to Result with Error

[Its-Just-Nans]

fix #1007

The CLI will produce

Warning (in resvg::filter:351): Filter has an invalid region.

[galister]

Thanks for jumping on this so quickly!

I noticed that the the comments also have the following, which can be removed:

/// # Panics
///
/// When `src1`, `src2` and `dest` have different sizes.

[Its-Just-Nans]

I noticed that the the comments also have the following, which can be removed:

Good catch

Note that my PR doesn't not means a merge, since it needs to be reviewed ^^
(Also since these changes are breaking the API - but it's less unsafe)

Note:

Plz merge before

• Fix https://github.com/linebender/resvg/issues/934 #991 maybe ?

[galister]

In case maintaining API compatibility is important, it would be viable to have 2 variants of the fn:

arithmetic_checked return Result, while the compat fn arithmetic can just call arithmetic_checked and unwrap the result.

That being said, I can't imagine a use-case where resvg is used as a library, and the developer would want it to panic depending on a faulty input file.

[RazrFalcon]

If there is a panic in filters code it means we have parsed the SVG wrong. Therefore fixing the filters code is band-aid.
We should figure out why filter images have a different size to begin with.

[galister]

i have submitted an SVG that triggers this in #1007

[galister]

the filter bounding box sizing looks suspicious.

<filter
        id="point-light-filter-0"
        primitiveUnits="objectBoundingBox"
        x="-500%"
        y="-500%"
        width="1000%"
        height="1000%"
        bx:preset="point-light
        1 0.5 0.5 0.5 50 1 0.2 0 #ffffff">

[Its-Just-Nans]

If there is a panic in filters code it means we have parsed the SVG wrong.

I agree that fixing bugs to have a correct parser and rendered is the good thing to do.

But penalizing crates which uses resvg by allowing it to panic is not great for a library
And I would really be happy to remove all unwrap and panics (some of my MRs are already going this way #989 #991 and this one).

---

I think the problem #1007 is not from the parsing but from this part

// crates/resvg/src/lib.rs
let target_size = tiny_skia::IntSize::from_wh(pixmap.width(), pixmap.height()).unwrap();
let max_bbox = tiny_skia::IntRect::from_xywh(
    -(target_size.width() as i32) * 2,
    -(target_size.height() as i32) * 2,
    target_size.width() * 5,
    target_size.height() * 5,
)
.unwrap();

If I increase the max_bbox, the render works.
Any reason for the chosen value *2 and *5 ?

[RazrFalcon]

Agree that we should remove asserts as well, but they are not the reason why this code fails. I.e. this issue need two fixes.

As for 2 and 5 - this is to limit the filter region to 5x the size. The limit is arbitrary.
Imagine a 5x5 tile with out main image in the center.

[Its-Just-Nans]

As for 2 and 5 - this is to limit the filter region to 5x the size. The limit is arbitrary.

Shouldn't this limit be set by the user using options args/params ? (kinda linked to #815)

For example, if I change the size like in my latest commits, the render is working

`main()` code

fn main() {
        if let Ok(()) = log::set_logger(&LOGGER) {
        log::set_max_level(log::LevelFilter::Warn);
    }
    let width = 96;
    let height = 96;
    let file_path = "534586290-631255f5-efc4-40bf-966e-69d08a6d3d48.svg"; // downloaded from the issue

    let options = resvg::usvg::Options {
        style_sheet: Some("svg { color: white }".into()),
        ..Default::default()
    };

    let data = std::fs::read(file_path).unwrap();

    let tree = resvg::usvg::Tree::from_data(data.as_slice(), &options).unwrap();
    let svg_size = tree.size();
    let scale_x = (width as f32) / svg_size.width();
    let scale_y = (height as f32) / svg_size.height();

    println!("{width} {height}");
    let mut pixmap = resvg::tiny_skia::Pixmap::new(width as _, height as _).unwrap();
    let transform = resvg::usvg::Transform::from_scale(scale_x, scale_y);
    resvg::render(&tree, transform, &mut pixmap.as_mut());
    let buf = pixmap.encode_png().map_err(|e| e.to_string()).unwrap();
    std::fs::write("a.png", &buf).unwrap();
}

[RazrFalcon]

It's such a rare edge case that it's not important.

[yisibl]

A panic error was also reported here: servo/servo#42258

[Its-Just-Nans]

See also

• fix: remove dangerous unwrap #989
• Fix https://github.com/linebender/resvg/issues/934 #991

[galister]

Hi, I would like to revive the conversation about this since there are multiple downstream projects that need this to not panic.

As far as I understand:

• there is an arbitrary limit for the filter region size that is defined in code
• no matter how large the limit, a potentially unsound SVG could still exceed that and cause a panic
• we can't really remove the limit or make it depend on the SVG contents; an unsound SVG could end up consuming a disproportionate amount of resources

If this is the case, I believe the only way forward would be to change the signature to return results instead.