Clamp too large bboxes instead of panicking

[Its-Just-Nans]

Fix #937
Fix #941

This change the API of render() but is still compatible I think
(returns None instead of crashing)

Maybe we could add #[must_use] ?

[Its-Just-Nans]

Should we panic or fail quielty?

[Its-Just-Nans]

The best would be to resturn a Option<usize> or a Result<usize, String>

I don't really like to .expect() but I do it now for this MR for no breaking change

[LaurenzV]

It's fine, it's just for testing.

[Its-Just-Nans]

I understand that I made multiple MRs to improve safety but they contains breaking changes.

Another solution would be to add more public method which are safer instead of editing the current ones (I can do that).

Aka

Instead of modifying render.
Create a new method render_with_result

[Its-Just-Nans]

I understand that I made multiple MRs to improve safety but they contains breaking changes.

Hi

With the upcoming release introducing breaking changes (#922), maybe resvg could also take the opportunity to fix some of the remaining unwraps (and others small issue like the assert!() no?

Thanks

[LaurenzV]

Hi, sorry, I know that the lack of response here is probably frustrating. Just to explain, your (and other) PRs are not being ignored, the problem is simply that no one has really had the bandwidth to work on resvg (at least for me it's lack of time but also a bit lack of motivation), we've managed to pull of the harfrust migration because it's pretty important, but that's about it. This is why there are so many unmerged PRs.

But anyway, a second reason is that even though replacing panics with errors is a good idea in general and seems easy to do on the surface, it still requires a bit more consideration about whether we really want to make that change, and that requires digging into the codebase again which I've not had the time to do!. While in general removing panics is good, we need to distinguish between unwraps that can actually fail (like the filter ones) and unwraps which shouldn't be able to fail and therefore are fine keeping. For example:

let target_size = tiny_skia::IntSize::from_wh(pixmap.width(), pixmap.height())?;

I don't think there is any point in removing the unwrap here because the pixmap size itself is already guaranteed to be non-zero IIRC, so this should never fail. For the filter case, I'm still unsure whether it's really worth it to make rendering fallible because of this single case, because other than that I don't think there are that many fallible operations in resvg. Maybe it is, but maybe it would be better to just clamp instead (because honestly, no one should really be rendering to u32::MAX...). I hope that explains why it's not super straight-forward to merge these changes and why it's been taking so long.

I'll try to make more time for resvg in the near future, but of course I can't make promises, because it's just in my free time after all. :)

[LaurenzV]

I've decided to just clamp now. If you are trying to rasterize SVGs with millions of pixel in a single dimension, I think you have other problems to worry about. 😄

[LaurenzV]

#937 is not fixed by this, but this seems to be a bug in tiny-skia itself with very large paths.
#941 will run into all eternity now, but at least not panic.

The correct fix for both will be to introduce a fallible API and allow restricting the maximum dimensions, but that's a whole other can of worms we don't need to address in this PR now. Apart from that, users can also detect the SVG size themselves and not even start rendering if they want to.

So I think this is good enough for now, thanks!

[Its-Just-Nans]

The correct fix for both will be to introduce a fallible API and allow restricting the maximum dimensions, but that's a whole other can of worms we don't need to address in this PR now.

On my side, I'm also interested in the render() function to return a Result<()> so that the user knows if there was a problem or not.

I think this could give the lib user some error and a way to know if the render failed ^^

Thanks