RFC: SVG images/icons support #19932

There are some concerns about possible security issues.

luke-jr:

... the concern is importing something complex like qtsvg into the overall codebase/runtime process... Any vulnerability compromises the entire program.

OTOH, NicolasDorier:

Given SVG files are auditable (it's only text), and we don't accept SVG from any untrusted source, I don't think this is a good concern luke-jr

And one more concern: MarcoFalke:

Concept ACK if this worked out of the box without the build system changes.

The build system changes are required for static builds.

The questions are:

1. If the project really sticks to the PNG only image/icon format, why src/qt/res/src/*.svg are still present in the repo?

2. If concerns about SVG are not good, why not switch all icons to SVG (now we have designers to help with that), and get rid of src/qt/res/icons/*.png?

cc @jonasschnelli @luke-jr @NicolasDorier @MarcoFalke

If the project really sticks to the PNG only image/icon format, why src/qt/res/src/*.svg are still present in the repo?

and

If concerns about SVG are not good, why not switch all icons to SVG (now we have designers to help with that), and get rid of src/qt/res/icons/*.png?

The supported OSes and windows manager are AFAIK all pixel based. Having a pixel based format like PNG should make drawing things "faster". Also, it should look always the same regardless of the SVG render engine. Which is IMO preferable for a GUI application.

IMO the GUI should use PNG files. It makes little sense to render the current SVG at runtime. The SVG files are still important to have as some sort of "source" file. One can edit the SVG files and or make higher resolution PNG files out of it.

The SVG files are still important to have as some sort of "source" file. One can edit the SVG files and or make higher resolution PNG files out of it.

Great point!

No opinion on using SVG for more things. Like with QML there has to be a good reason to include more parts of Qt, but if results in a much better GUI why not.

FWIW, png and jpeg codecs have also had exploitable vulnerabilities in the past. This is not unique to svg. The important part is to not load them from remote sources.

The SVG files are still important to have as some sort of "source" file. One can edit the SVG files and or make higher resolution PNG files out of it.

Yes, it's a bit like asking why we still have .cpp files in the repository while we're compiling them anyway.

Concept ACK on SVG -> PNG migration

I don't have any opinion on either case. I never experienced problem with different svg engine rendering same image differently

I don't see any reason to render SVGs at runtime. At best, it trivially slows down startup. But at worst, SVG rendering is often prone to rendering engine bugs (for Liquid, the logo was mis-rendered by stock librsvg), and adds a lot of code (QtSvg) to the footprint of the program we otherwise have no use for.

Rendering them at build time might make a little more sense (Knots does this, mainly to avoid adding binary files that can't be made into a text patch), but has most of the same "worst case" issues. On top of that, the most common tool to do CLI SVG->PNG rendering is librsvg, which is problematic because it was rewritten in Rust.

Designer @Bosch-0 touches on this in PR#178 in the GUI Repo.

Why Include SVG: "SVGs are used as source files because they can scale while retaining image quality, but are not used in production due to limited application support. In the event that different-sized production (PNG) icons are required, they can be generated from the associated SVG source file in a vector-based tool"

Why Include PNG: "PNGs are used in production due to wide application support, transparency support, and better image quality in comparison to competing file types such as JPEG."

This can be closed

the reason for sticking with PNG for production and keeping SVG files around in the repo are laid out in this comment and the PR linked in the comment.