Complete rewrite of svg2pdf

[LaurenzV]

This PR contains a complete rewrite of svg2pdf. To summarize it:

• The code has been split up into multiple files so that it is more clear where to find which code responsible for a specific part of the SVG specification.
• A test suite has been added to check the library for correctness.
• Transformations are now completely deferred to the PDF viewer.
• While I did copy parts of the code from the old library, the main logic of the SVG conversion has been written completely from scratch, so a lot of currently existing issues in regard to embedding of images, clip paths, patterns and gradients have been resolved by that as well.

You can find more information about the test suite in the README in the tests folder. If you want to compare the old with the new version, you can find two PDFs below that contain all the svg test files (the old one has a few less because some crashed the program) where you can compare in different PDF viewers how some SVGs were displayed before and how they are displayed now once they are embedded using Typst (again, see the tests folder for more information).

old.pdf new.pdf

I'm aware that there probably still are a lot of things that need to be changed and could be improved on (for example, gradient functions are currently not stored globally but copied for each usage of the gradient. And it would also be nice to update usvg to 0.34), but I just wanted to get this initial PR out now so that there is a basis for discussion and we can talk about what should be prioritized and changed already in this PR, and what can be tackled in a separate PR.

Also, if you do end up wanting to merge this, do let me know before actually doing it, because there are some final checks I want to run once the code has been adapted with the final feedback. And if for some reason you don't want to merge this, don't worry, my main goal in doing this rewrite was to familiarize myself with SVG/PDF and get some hands-on experience with programming in Rust, which I most definitely did. :)

Would appreciate your feedback!

EDIT: Also, a small note. The resvg-test-suite contains quite a few test cases which are parsing specific (i.e. they test whether the SVG is parsed correctly, but since we use resvg in the background they probably shouldn't be relevant for us), should I still keep those test cases in there for completeness or should I remove the ones that aren't rendering-specific, even if that means going out-of-sync with the original resvg test suite? If we do decide to remove them, we could also remove the ones that exhibit undefined behavior instead of keeping track of them in the SKIPPED_FILES variable.

[slashformotion]

[laurmaedje]

Let me start by saying that this is really fantastic work! Just looking at the two comparison files makes clear how much better everything works compared to before. The code also seems well structured and is clear to read.

The amount of files added crashes GitHub's review UI, so I'll leave some plain comments instead.

• I made a bunch of style/format nit-picky changes and figured I'd just push them instead of bothering you with this. If there's any change that is problematic or that you disagree with in my commit, just say so.

• Could the test runner use Rust's test targets instead of bin targets so that cargo test --release --workspace in the root actually executes the tests? Like in Typst.

• The fonts and resvg test files included in this PR should be mentioned in a NOTICE file to make sure we give proper attribution. (Also like in Typst, you can check how it's done there.)

I didn't do a full review of the logic itself (and I'm also not that knowledgeable w.r.t. SVG and PDF). But I also don't believe that it makes sense to block this PR on such a review. It very obviously works much better than the status quo and we can just fix issues as they arise. Once the comments above are addressed, I'd be happy to merge this.

To your final question: I did wonder why this PR copies all of resvg-test-suite into the repository instead of somehow depending on it. I do not have the full picture of the trade-offs involved here, but my current position would be as follows: If we inline everything into this repository, I see no reason to keep irrelevant tests. If we want to prevent going out-of-sync with the original test suite, could we somehow depend on resvg-test-suite instead of inlining? How does resvg itself do it?

[LaurenzV]

I made a bunch of style/format nit-picky changes and figured I'd just push them instead of bothering you with this. If there's any change that is problematic or that you disagree with in my commit, just say so.

Fine form my side! I'm only curious about why you replaced the util.rs and render.rs with mod.rs, I had it this way before but then I read that using mod.rs is an old approach and should be replaced with this new style? But maybe I remember this wrong.

Could the test runner use Rust's test targets instead of bin targets so that cargo test --release --workspace in the root actually executes the tests? Like in Typst.

I tried this, I think the problem here was that it didn't seem to be possible to add command line arguments to the program itself (e.g. --replace), that's why I kept it this way. I will try again in the evening.

The fonts and resvg test files included in this PR should be mentioned in a NOTICE file to make sure we give proper attribution. (Also like in Typst, you can check how it's done there.)

I'll have a look at it.

To your final question: I did wonder why this PR copies all of resvg-test-suite into the repository instead of somehow depending on it. I do not have the full picture of the trade-offs involved here, but my current position would be as follows: If we inline everything into this repository, I see no reason to keep irrelevant tests. If we want to prevent going out-of-sync with the original test suite, could we somehow depend on resvg-test-suite instead of inlining? How does resvg itself do it?

I mean resvg-test-suite only is a GitHub repository, I suppose we could add it as a GitHub submodule? Although I have never used it before. I think inlining would be nicer though, so I would prefer removing irrelevant tests if that is okay with you. And if new tests appear in the resvg-test-suite that seem useful to us, we can just add them manually later on.

[laurmaedje]

I'm only curious about why you replaced the util.rs and render.rs with mod.rs, I had it this way before but then I read that using mod.rs is an old approach and should be replaced with this new style? But maybe I remember this wrong.

This style was added more recently, but personally I do not like. It's a matter of preference, some prefer this style and some the other. The supposed benefit is that you can easier find files with things like Cmd+P in VS Code. But I do not like at all how it separates the module's main and side files. In my opinion, they belong together. Long story short: It's a matter of preference and the Typst style is mod.rs.

I tried this, I think the problem here was that it didn't seem to be possible to add command line arguments to the program itself

It should work, Typst does it, too. What you have to do, is add harness = false to your test target. (See Typst for an example.)

I mean resvg-test-suite only is a GitHub repository, I suppose we could add it as a GitHub submodule?

I thought perhaps submodule (although they seem to be generally hated) or maybe there's some other way I'm not thinking of. But I also don't have a problem with inlining.

[LaurenzV]

Update:

• Added a NOTICE with all licenses.
• Doing cargo test --release --workspace will now run all tests. I also updated the CI to use that command instead.
• diffs, references, pdfs and svgs have been renamed to diff, ref, pdf, svg.
• I removed all tests that have been excluded because they have undefined behaviour instead of excluding them manually with a SKIPPED_TESTS variable.

Additionally, above you can find all of the GitHub issues that should be resolved once this gets merged and upstreamed into Typst.

The only thing I need to fix now is a warning that appears while compiling the whole program (related to the test suite target).

[LaurenzV]

The warning is fixed (although it means disabling doc tests unfortunately), so from my side, this would be ready to be merged now.

[laurmaedje]

Thanks again, this is really great work!

[laurmaedje]

I closed all issues except for https://github.com/typst/typst/issues/368 because that one still looks wrong for me in Preview and Safari. Is that an svg2pdf bug or the case where the transformations are broken on Apple's PDF engine?

[LaurenzV]

Can reproduce. It looks fine on Acrobat and Google Chrome, but I get the same issues in Safari. In Firefox, there also are a couple of weird artifacts. Will investigate.

[LaurenzV]

Yeah, I'm pretty sure it's the same bug. To be precise, it's not transformations by themselves that are broken, but transformations in combination with soft masks. There are a couple of clip paths in this SVG which should clip away for example the second horse (whatever the reason is for a second horse being there in the first place) and a couple of other things. These clip paths have a transformation, meaning that they will be embedded as a soft mask in svg2pdf Instead of as a PDF clip path. And that's very likely the reason why the PDF is displayed wrongly in Safari.

Not sure what the cause is for the weird artifacts in Firefox (see below), but it also must be related to the fact that the second horse is not clipped away properly.

[LaurenzV]

I think we might be able to convert clip paths with transforms on the top-level clip pathj into plain PDF clip paths as well in the future, which would fix this issue, but this would require us to get the inverse of a transformation in Rust, which I think should be possible once we update to usvg 0.34. But for now, we have to live with the fact that it doesn't work properly in Safari.