Support SVG format #168
Support SVG format #168
Conversation
There was a problem hiding this comment.
Thank you for the contribution, svg support is a great addition, which I've been wanting to do for a long time.
I would like to point out that optimally the SVG should be re-rasterized ("re-rendered") when the user zooms in, so that you can zoom right into the tiniest detail and the image would remain sharp and not pixelated. I gave it bit of thought and implementing that properly is a much more difficult and time consuming task so the approch that you took in this PR should be good enough as a start.
I did leave some remarks that I'd like you to address. I'm happy to give further guidance if needed.
src/image_cache/image_loader.rs
Outdated
| let mut pixmap = tiny_skia::Pixmap::new(width, height).unwrap(); | ||
| resvg::render(&rtree, usvg::FitTo::Zoom(zoom as f32), pixmap.as_mut()).unwrap(); | ||
| Ok(image::RgbaImage::from_raw(width, height, pixmap.data().to_vec()).unwrap()) |
There was a problem hiding this comment.
Yes, this is exactly what I was thinking about. 😊
There is one more improvement that can be made here. pixmap.data() only gives you a reference to the pixel array within pixmap, then to_vec takes the reference and creates a new array, copying all the data, then you throw away the entire array contained within pixmap. As you can see there's a copy which we don't really need. We can avoid creating a copy by moving the pixmap's internal data into the from_raw function. Unfortunately the data inside Pixmap is not pub so we can't reach it directly. But there's a function for Pixmap which was created exatcly for this purpose, as shown below. (Notice that this function takes self instead of &self, this means that self is moved)
/// Consumes the internal data.
///
/// Bytes are ordered as RGBA.
pub fn take(self) -> Vec<u8> {
self.data
}There was a problem hiding this comment.
Thanks for the detailed pointers, they really helped. It should be fine now.
src/image_cache/image_loader.rs
Outdated
| pub fn load_svg(path: &std::path::Path) -> Result<image::RgbaImage> { | ||
| let opt = usvg::Options::default(); | ||
| let rtree = usvg::Tree::from_file(path, &opt)?; | ||
| let (width, height) = (rtree.svg_node().size.width(), rtree.svg_node().size.height()); |
There was a problem hiding this comment.
EDIT: What I suggested earlier does not compile 🤦♂️
May I suggest
let size = rtree.svg_node().size;
let (width, height) = (size.width(), size.height());There was a problem hiding this comment.
Changed. I believe that kind of pattern matching on let assignments is still WIP.
There was a problem hiding this comment.
It's actually not. The problem was that the fields of Size are not public. The reason for that is beyond me.
|
Thanks again! |
Closes #85
Description
Support SVG format with resvg, since the blocking dep
harfbuzz_rswas replaced with a pure rust crate -rustybuzz- a while ago (see https://github.com/RazrFalcon/resvg/blob/master/CHANGELOG.md).Implementation
svgwas added (I mirrored theavifsupport but I don't know if this should be a feature).resvg,usvgandtiny-skiawere added as optional dependencies.usvg), rendered (resvg) and encoded as PNGs (tiny-skia).