Skip to content

Use resvg as library #2588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

JustForFun88
Copy link

@JustForFun88 JustForFun88 commented Apr 8, 2025

This PR replaces the external resvg command with the embedded resvg Rust crate, addressing the out-of-memory issue more reliably than the previous setrlimit-based workaround.

Instead of applying OS-level memory limits, this PR calculates the bounding box of all SVG nodes in advance and scales the image down to fit within max_width and max_height, preventing excessive memory allocation during rendering:

fn target_size_and_scale(tree: &Tree, max_width: u32, max_height: u32) -> (u32, u32, Transform) {
	// It is Ok. The max_width and max_height could not be larger then monitor
	// detentions which much less then f32::MAX
	let max_width = max_width as f32;
	let max_height = max_height as f32;
	let mut width = tree.size().width();
	let mut height = tree.size().height();

	for node in tree.root().children() {
		if let Some(bounding_box) = node.abs_layer_bounding_box() {
			width = width.max(bounding_box.width());
			height = height.max(bounding_box.height());
		}
	}
	if width <= max_width && height <= max_height {
		return (width.floor() as u32, height.floor() as u32, Transform::from_scale(1.0, 1.0));
	}
	let ratio = f32::min(max_width / width, max_height / height);
	(
		(width * ratio).floor() as u32,
		(height * ratio).floor() as u32,
		Transform::from_scale(ratio, ratio),
	)
}

Build impact summary (Windows)

  • Binary size:
    • use_resvg: 21.9 MB (22,964,736 bytes)
    • main: 19.6 MB (20,637,696 bytes)
    • +2.3 MB (~11.3%) increase
  • Build time (measured with cargo build --profile release-windows --locked --timings):
    • use_resvg: 2m 57.9s (177.9 s)
    • main: 3m 0.9s (180.9 s)
    • Slight improvement (~1.7% faster)

TODO

  • Chech the implementation for Ueberzug
  • Chech the implementation for Chafa

@sxyazi
Copy link
Owner

sxyazi commented Apr 8, 2025

Thanks for the PR! The changes are more extensive than I expected, is it possible to simplify it further:

  • SVG should not be treated as a standalone system; instead, it should be a subset of the existing image support. That means it should use the same image API, image_*, rather than creating new svg_* APIs. The image_* API should include a reliable way to detect whether the file is in SVG format, just like it detects any other image format.
  • Ideally, the memory limit part should be contributed upstream since I don't want to maintain it separately within Yazi.
  • The options font_folders, svg_serif_font, svg_sans_serif_font, svg_cursive_font, svg_fantasy_font, and svg_monospace_font are SVG-specific, not generic configurations (for example, max_width, cache_dir, etc., which are used by multiple previewers). Hence, they should not be introduced in yazi.toml — SVG should provide a sensible default behavior without any user configuration and any further configuration should be handled in a plugin-specific way, such as with
    { mime = "image/svg+xml", run = "svg --option1 --option2" }
    or
    require("svg"):setup { ... }

Also, could you provide more specific impact data on build speed and binary size? In the past, some users have complained that Yazi's build speed is too slow (especially on Windows), and I'm very cautious about introducing new crates — I often end up implementing functionality myself to avoid adding them.

@sxyazi sxyazi force-pushed the main branch 2 times, most recently from ce3eb7f to a9c693e Compare April 8, 2025 08:56
@JustForFun88
Copy link
Author

Thanks for the feedback!

  • I'll try to integrate SVG into the existing image_* APIs by detecting SVGs manually via magic bytes.

  • Regarding memory limitation — I think the most I can realistically upstream is the logic for determining the absolute size of the SVG image, since the rest of the logic (scaling, bounding box traversal) feels quite specific to Yazi and aligns with how other image formats are already handled.

  • About the font-related options: the reason I included them is due to how resvg works. If no fonts are loaded into fontdb, resvg doesn't even parse <text> elements in the SVG at all. Once at least one font is loaded, it starts parsing text elements — but if the required font is missing, the rendering might be incorrect (e.g., fallback fonts may not match or glyphs may be missing).

    Since it's difficult to reliably detect all system fonts across platforms, my idea was:

    • If the user explicitly loads system fonts (expensive) or provides a folder with fonts,

    • then we can parse the SVG and show which fonts are referenced (e.g., in the properties pane),

    • so the user can install missing fonts or add more font folders if needed.

    Would you prefer this font logic be moved to plugin-specific config like:

    require("svg"):setup {
      font_folders = ["/path/to/fonts"],
      ...
    }

    Or should I remove font options entirely and rely on resvg's defaults?

I'll also measure build time and binary size impact and post the results later.

@JustForFun88 JustForFun88 force-pushed the use_resvg branch 2 times, most recently from 3bed2f7 to 10e07b0 Compare April 8, 2025 15:15
@JustForFun88
Copy link
Author

JustForFun88 commented Apr 8, 2025

@sxyazi I've now implemented the changes as discussed:

  • SVGs are integrated into the existing image_* APIs. The decoder first attempts to open the image as a raster. If that fails with a format-related error, it then falls back to decoding it as SVG.

  • Font-related configuration has been moved to Lua for flexibility. The plugin now exposes Lua functions such as set_svg_font_family, load_system_fonts, load_font_file, and load_fonts_dir, allowing to modify the resvg font database at runtime.
    As noted, resvg won’t parse <text> elements unless at least one font is loaded. With the Lua API, users should explicitly load fonts.

  • The image scaling logic is left here for now. Some parts of it could potentially be upstreamed later.

The only thing left is to measure build time and binary size impact — I’ll post that shortly. Updated the PR description accordingly.

@JustForFun88 JustForFun88 marked this pull request as ready for review April 8, 2025 19:27
@sxyazi sxyazi force-pushed the main branch 12 times, most recently from 1cc8e5a to a29dbb9 Compare April 21, 2025 18:46
@sxyazi sxyazi force-pushed the main branch 2 times, most recently from d1efbb8 to a0ab614 Compare June 16, 2025 13:25
@sxyazi sxyazi force-pushed the main branch 4 times, most recently from 35d9907 to 2768fd2 Compare June 27, 2025 09:44
@sxyazi sxyazi force-pushed the main branch 3 times, most recently from 9f077f9 to 60a2382 Compare July 14, 2025 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants