Skip to content
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

feat!: switch to resvg as the SVG renderer #2581

Merged
merged 1 commit into from
Apr 7, 2025
Merged

feat!: switch to resvg as the SVG renderer #2581

merged 1 commit into from
Apr 7, 2025

Conversation

sxyazi
Copy link
Owner

@sxyazi sxyazi commented Apr 7, 2025

Follow up to #2533

This PR replaces Yazi's SVG previewer from ImageMagick to resvg to address the following issues:

  • ImageMagick chooses different backends for SVG rendering based on user build parameters and even the runtime environment (depending on whether a certain backend is installed). It can choose between ImageMagick's built-in renderer, rsvg, and inkscape, which introduces many uncontrollable factors.
  • All these backends, except for rsvg, can exhaust all available memory when rendering certain SVGs, leading to system crashes.
  • The parameters required for different backends vary: the built-in backend needs -density and -resize to maintain image quality, while the rsvg backend only needs -size. No single parameter combination works correctly across all backends.

As a result, several users have reported SVG preview issues recently:

Aside from ImageMagick, the remaining two SVG renderers are rsvg-convert and resvg. I chose resvg because:

  • resvg is ~2.5 times faster than rsvg-convert in my benchmarks
  • resvg has better SVG support than rsvg-convert
  • resvg provides an official Windows binary release, allowing Windows users to use it directly, even install it via Scoop

Currently, resvg does not support memory limitation. So, this PR adds memory restrictions for resvg within Yazi:

  • On Linux, memory usage is limited with setrlimit(RLIMIT_AS)
  • On Windows, memory usage is constrained through CreateJobObjectW() and AssignProcessToJobObject()
  • On macOS, where setrlimit is ineffective, memory usage is continuously monitored with proc_pidinfo(), and the process is terminated if it exceeds the limit

⚠️ Breaking change

After this PR, Yazi will use resvg instead of ImageMagick as the SVG previewer, which is a new optional dependency.

You can download it from https://github.com/linebender/resvg/releases

@sxyazi sxyazi requested a review from Copilot April 7, 2025 14:51
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • yazi-plugin/preset/plugins/svg.lua: Language not supported
  • yazi-plugin/preset/plugins/video.lua: Language not supported
Comments suppressed due to low confidence (1)

yazi-plugin/src/process/command.rs:46

  • Check the return value of libc::setrlimit to handle potential errors when setting memory limits.
libc::setrlimit(libc::RLIMIT_AS, &rlp);

@sxyazi sxyazi merged commit 5ad40d9 into main Apr 7, 2025
6 checks passed
@sxyazi sxyazi deleted the pr-23cfd764 branch April 7, 2025 15:02
@sxyazi
Copy link
Owner Author

sxyazi commented Apr 7, 2025

Benchmark with https://commons.wikimedia.org/wiki/File:Cannon-diagram2.svg:

Benchmark 1: resvg -w 2000 -h 2000 Cannon-diagram2.svg /tmp/abc.png
  Time (mean ± σ):     295.5 ms ±   1.2 ms    [User: 257.2 ms, System: 37.0 ms]
  Range (min … max):   294.5 ms … 298.5 ms    10 runs

Benchmark 2: rsvg-convert -w 2000 -h 2000 Cannon-diagram2.svg -o /tmp/abc.png
  Time (mean ± σ):     747.3 ms ±   6.1 ms    [User: 662.8 ms, System: 59.0 ms]
  Range (min … max):   744.1 ms … 763.9 ms    10 runs

Summary
  resvg -w 2000 -h 2000 Cannon-diagram2.svg /tmp/abc.png ran
    2.53 ± 0.02 times faster than rsvg-convert -w 2000 -h 2000 Cannon-diagram2.svg -o /tmp/abc.png

Benchmark with https://commons.wikimedia.org/wiki/File:Moldova_(1483)-ro.svg:

Benchmark 1: resvg -w 2000 -h 2000 Moldova_\(1483\)-ro.svg /tmp/abc.png
  Time (mean ± σ):     601.6 ms ±   1.7 ms    [User: 545.4 ms, System: 50.0 ms]
  Range (min … max):   598.5 ms … 604.2 ms    10 runs

Benchmark 2: rsvg-convert -w 2000 -h 2000 Moldova_\(1483\)-ro.svg -o /tmp/abc.png
  Time (mean ± σ):      1.466 s ±  0.004 s    [User: 1.365 s, System: 0.061 s]
  Range (min … max):    1.460 s …  1.473 s    10 runs

Summary
  resvg -w 2000 -h 2000 Moldova_\(1483\)-ro.svg /tmp/abc.png ran
    2.44 ± 0.01 times faster than rsvg-convert -w 2000 -h 2000 Moldova_\(1483\)-ro.svg -o /tmp/abc.png

sxyazi added a commit that referenced this pull request Apr 7, 2025
@JustForFun88
Copy link

@sxyazi Aw, too bad 😅! I literally just finished adding resvg support. Plugged it in just like the image crate — and the binary only got about 10% bigger!

But with that we can preview SVGs with text and load fonts for it! Wanna check it out?

@sxyazi
Copy link
Owner Author

sxyazi commented Apr 7, 2025 via email

@JustForFun88
Copy link

Is there a way to limit its memory usage if it's built-in rather than being a separate process? Without the limitation, SVGs like the one in linebender/resvg#814 would immediately consume all system memory.

I'm not sure about that — I'll see what I can do. One thing I do know for sure: embedding allows to use about half the memory.

No idea why, but when rendering SVGs, resvg clones the pixel buffer during demultiplying, so we end up with two copies of the pixel data. I changed it to modify the pixels in-place instead. I’ll try to dig into the OOM issue. There's that hint saying “All you can do right now is to walk over nodes and check their bboxes”, though I was kinda hoping to avoid too much headache here.

@JustForFun88
Copy link

@sxyazi Actually, I have an idea! We could manually check the image size using tree::size, and if rendering it would exceed the memory restrictions, just skip it and display a message like “Image too large to render” or something along those lines.

@JustForFun88
Copy link

JustForFun88 commented Apr 8, 2025

@sxyazi I think that I fixed the OOM in #2588. But the implementation for Ueberzug and Chafa may need further refinement, and I'd appreciate help reviewing or improving those parts.

@uncenter uncenter mentioned this pull request Apr 8, 2025
13 tasks
JustForFun88 added a commit to JustForFun88/yazi that referenced this pull request Apr 8, 2025
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