Make usvg::Tree read-only

[RazrFalcon]

Right now, usvg::Tree can be modified in pretty much any way and can be even constructed from scratch. It might have some benefits, but also introduces a lot of issues:

1. We do not control the content. resvg basically assumes that usvg::Tree was parsed from SVG. There are no guarantee it would work with a manually created tree. By having a mutable tree the user assumes that everything can be changed, but in reality almost nothing can be safely changed.
2. It complicates a tree writing back to SVG, because again, we don't know if it was modified. Someone could remove all id values, which we need during writing. Or even introduce recursive references - we cannot check for that either.
3. We can optimize a lot of things knowing that tree structure before-hand. For example, right now to get a list of clipPaths we need to traverse the tree. But with a read-only tree we can collect them before-hand.
4. All the internals are public. Ideally, the caller shouldn't know if we're using shared references for some data or not. That should be an implementation detail and not part of the public API.

@LaurenzV @yisibl I hope this change would not break your use case. Or anyone else.

[LaurenzV]

@laurmaedje IIRC Typst also manually pushes fonts to the SVG tree, no?

[laurmaedje]

Typst starts out with an empty fontdb, traverses the tree and inspects the TextSpan::font::families to load fonts through its own mechanisms and to perform font fallback. It then loads only the used once into the fontdb and overwrites the text span's font families with the chosen font. It is somewhat hacky, but the best way we found.

[LaurenzV]

I see, so if the tree were to become read-only, we could probably still traverse it to find all fonts, but we can't define our own fallback fonts anymore for each text span.

[RazrFalcon]

@laurmaedje this feature would be removed as well... The problem is that currently parsing and text-to-path conversion are two separate steps which leads to a lot of complexity and issues as well. For one, to do a lot of things with SVG we must know the layer bbox, which we don't know before text layout is finished. A chicken and egg problem.

Will a callback approach work for you? The way we handle images resolving right now. usvg will call a callback for each font not present in fontdb and you could either add it to it or ignore it. I haven't tried implementing it yet, but that's the idea.

In general, the issue here is that usvg is too flexible for me to maintain. Therefore I plan to remove all the features not used by resvg. Ideally, there should be only one way an SVG can be parsed.

[LaurenzV]

The problem is that currently parsing and text-to-path conversion are two separate steps which leads to a lot of complexity and issues as well. For one, to do a lot of things with SVG we must know the layer bbox, which we don't know before text layout is finished.

So the idea is that text is still preserved in the usvg tree, but you already need to supply a fontdb when parsing the SVG file, so that when you parse it you already know for each text node which font will be used, and thus you can calculate the bounding boxes appropriately at the parse stage? That might indeed make things simpler, and might even allow to simplify the structure of the tree (for example, iirc one of the main blockers for removing the "ObjectBoundingBox" property was that we don't know the text bbox while parsing)?

[LaurenzV]

So for svg2pdf, the only time I am creating a tree by myself is for filters. But IIRC I only did this because I needed to calculate the bounding boxes, which are currently only exposed via the tree. If it is possible to calculate bounding boxes just from a node (and also render a node directly, which I think is already possible), I wouldn't need anything else, I think.

[laurmaedje]

Will a callback approach work for you? The way we handle images resolving right now. usvg will call a callback for each font not present in fontdb and you could either add it to it or ignore it. I haven't tried implementing it yet, but that's the idea.

As far as font selection goes, this could work. What's also relevant to note here is how the family names are resolved. Typst and fontdb parse family metadata somewhat differently and currently we parse the SVG family names in the Typst way and then, after loading the font into fontdb, patch the name in the SVG to the fontdb variant.

I'd like to bring up one other thing, too: A current problem with Typst's SVG stack is that text isn't selectable. In the current usvg design, I think it would be possible to layout the text with Typst's text stack in the common, simple case (no crazy SVG features on the text), which would make it selectable. Forcing text-to-path in the parsing stage would remove this capability as far as I can tell, which would be unfortunate.

I really liked that usvg offered more control over the text in recent versions. Of course, I can't judge the complexity this forces upon usvg, but I wonder whether it's possible to have some middle ground where consumers can interact with the text beyond just paths.

[LaurenzV]

Forcing text-to-path in the parsing stage would remove this capability as far as I can tell, which would be unfortunate.

As far as I understood, text will still be preserved, but the used font will be decided on at parsing stage so that the bbox is known in advance? If I'm wrong and text would also be converted automatically into paths again, that would be very unfortunate indeed. :(

[yisibl]

These changes may be more impactful, so I also cc @zimond

[RazrFalcon]

@LaurenzV

So the idea is that text is still preserved in the usvg tree, but you already need to supply a fontdb when parsing the SVG file

Yes. Ideally, all you have to do is too call usvg::Tree::parse and thats it. No tree.postprocess() and stuff.

Yes, it will simplify objectBoundingBox as well. But we can implement it already, if we want to.
The bigger problem is that right now we have to parse SVG first and them post-process it via a separate function. Meaning that we have to store some metadata in-between we don't need.

[RazrFalcon]

If it is possible to calculate bounding boxes just from a node (and also render a node directly, which I think is already possible), I wouldn't need anything else, I think.

It's not possible, because we have to know all parent transforms. And you would not be able to create individual Nodes anyway. Just a tree.

That's the problem. To properly parse SVG I need access to the whole tree, not just part of it.

[RazrFalcon]

@laurmaedje

A current problem with Typst's SVG stack is that text isn't selectable.

There are plans to preserve text layout as well. So a Text node would store SVG properties, text layout (a list of glyph + position + style structs) and a flattened (text-to-paths) output.
I'm not sure it would help much. SVG Text layout is pretty complicated. I'm not sure how usvg can help you here.

Forcing text-to-path in the parsing stage would remove this capability as far as I can tell, which would be unfortunate.

The Text node isn't going anywhere. So it's fine.
Turns out text cannot be flattened by design and the current text-as-paths SVG writer is technically incorrect.

after loading the font into fontdb, patch the name in the SVG to the fontdb variant.

This would not be possible anymore. And I'm not sure how can we handle this.
Let's be hones, that's a pretty rare edge case. I don't mind having a callback for such cases, but I'm not really sure what are you doing and why.

Of course, I can't judge the complexity this forces upon usvg, but I wonder whether it's possible to have some middle ground where consumers can interact with the text beyond just paths.

The complexity comes from the fact that the whole reason usvg exists is to serve resvg. It's not designed to be used on its own. And by adding more flexibility we're making resvg more complicated as well.
For example, right now text-to-paths conversion and bbox calculation are optional. But resvg doesn't understand this. The usvg::Tree resvg expects must have all of those properties already.

The fact that usvg::Tree is mutable makes SVG writing pretty hard, if not impossible. Because, as mentioned above, one can modify the Tree in a way that it would have recursive references and so on. And right now both resvg and SVG writer expect that Tree was parsed from a string without any modifications.

To put it bluntly, any modifications to usvg::Tree are undefined behaviour. As simple as that. And by having a read-only tree we can avoid it.

[LaurenzV]

It's not possible, because we have to know all parent transforms. And you would not be able to create individual Nodes anyway. Just a tree.

Hm okay... This might mean that it might break stuff for me then. :/ But it's hard to say, I would have to see the concrete implementation.

[RazrFalcon]

Why do you create filters yourself? To rasterize them?

Also, I will sound like a broken record, but you should not be creating filters by yourself as well. resvg is able to handle only filters created during parsing. All the validation is done on the parser side, not resvg. Meaning your code could break any time.

[laurmaedje]

There are plans to preserve text layout as well. So a Text node would store SVG properties, text layout (a list of glyph + position + style structs) and a flattened (text-to-paths) output.
I'm not sure it would help much. SVG Text layout is pretty complicated. I'm not sure how usvg can help you here.

Then, it should be fine I think.

The complexity comes from the fact that the whole reason usvg exists is to serve resvg. It's not designed to be used on its own.

I think I'm not alone in saying that it is very useful on its own and that's actually a great feature! I just remembered this message from you on reddit and I wholeheartedly agree with it.

[RazrFalcon]

@laurmaedje usvg isn't going anywhere. It will only get better. But to do so we have to make it read-only.

[LaurenzV]

Also, I will sound like a broken record, but you should not be creating filters by yourself as well. resvg is able to handle only filters created during parsing. All the validation is done on the parser side, not resvg. Meaning your code could break any time.

I'm not creating filters myself. Basically, in general, I traverse the usvg tree and convert all of the nodes into PDF objects successively. However, filters are not supported in PDF, so they need to be rasterized. So whenever I encounter a node that has filters, instead of traversing it, I create a new resvg tree (with an empty group root node) and append that node, and then convert it using resvg into an image. And then I just embed the image into the PDF instead.

And it works really well, there are a few edge cases that don't work yet, but all other test cases pass successfully.

[RazrFalcon]

@LaurenzV couldn't you simply render the node itself? resvg has an API for that. I don't see why would you need a new tree for that.

[LaurenzV]

Yes that's what I tried, but I need to know the bounding boxes of the node so that I can set it inside the PDF file (I don't "care" about the parent transforms in this case, I just need the bounding box of the node as if it were the root node). And when I implemented it, the bounding box calculation was private to usvg and only exposed via usvg::Tree. Not sure if that changed.

EDIT: Nevermind, looks like the method on the group is public... Maybe I missed something, I will look into it later. 😅

[LaurenzV]

Okay, I rewrote it using the node render method, and except for two edge cases that break, it works fine otherwise. So I think read-only tree would be fine in my case, as long as you can still create nodes manually and work with those!

[RazrFalcon]

as long as you can still create nodes manually and work with those

This wouldn't be allowed. That's the whole point. Why do you want this?

[LaurenzV]

Okay, nevermind, I guess I don't strictly need to be able to construct a image node by myself. But I can still at least read the attributes of an image node (such as viewbox, visibility, etc.), right?

I think for me it's just a bit difficult to visualize what the exact API you had in mind would look like, so I guess it's just best to try it out once it's ready. So feel free to write here once it happens and I'll try it out (although I presume that this will probably a long time). :p

[RazrFalcon]

No worries, I will make a pull request soon.

[RazrFalcon]

Created #712 to illustrate the idea.

[LaurenzV]

That was fast. 😄 Will experiment with this.

[LaurenzV]

Okay, I guess I understand now. so basically make all fields private (as they are implementation details) and expose everything necessary as methods.

It does break two things for me where I manually edited the Node, which should be fixable from my side.

The thing with rendering filters seems trickier, I'll have to spend some more time trying it out. Not sure how easy it is going to be.

But I guess in general, the change is fine for me.

[RazrFalcon]

We can add necessary tweaks if needed, but the idea of making struct fields private will probably here to stay. It just makes everything way easier.

[bennobuilder]

Out of context question but can/should the usvg::Tree still be constructed manually (not modified) or is it necessary to go over the svg string parser. Because I’m currently experimenting with a “design editor” which can run on the server to construct like dynamic images based on templates.. And rn I’m like exporting to a svg (using my own implementation) to then parse the svg with usvg parser to then render it using resvg & tiny-skia..

My idea the other day was to like have a flat custom usvg like tree to apply changes from the Bevy ECS too and then construct the real usvg::Tree based on that.

I know my stack is kind off weird and I’m learning by doing, so I’m open for feedback & ideas & roasts :)

What I'm trying to ask is whether my approach just stupid or maybe possible in some degree with usvg as "foundation" / SVG gateway

Thanks 🙏

https://github.com/dyndotart/monorepo/tree/develop/crates/svg_render/src/resources/svg_composition

[RazrFalcon]

@bennoinbeta If I understood you correctly, then no. The only usvg::Tree constructor right now is a string.
And your question kinda highlights the reason for this change. Before, you could construct usvg::Tree from code and not just a string, which could have some possible applications, but it would not be rendered correctly by resvg or written back to SVG anyway. Because both resvg and usvg-svg-writer expects a certein usvg::Tree structure. They do no validate the input usvg::Tree in any way. That's the problem.

[bennobuilder]

@bennoinbeta If I understood you correctly, then no. The only usvg::Tree constructor right now is a string. And your question kinda highlights the reason for this change. Before, you could construct usvg::Tree from code and not just a string, which could have some possible applications, but it would not be rendered correctly by resvg or written back to SVG anyway. Because both resvg and usvg-svg-writer expects a certein usvg::Tree structure. They do no validate the input usvg::Tree in any way. That's the problem.

ok thanks for the quick reply. Makes sense, so I'll stick with my current approach where I construct the SVG and then render it via resvg & tiny-skia when required instead of directly using the usvg::Tree as universal "SVG Gateway".

Just out of curiosity: Is the usvg::Tree so "unforgiving" though as it can represent nearly any SVG in some (internal) form, so it should also be possible to construct it in Rust code. But yeah the user would exactly need to know the required structure and as you said it can't be 100% safe without a overhead.

But never the less resvg & usvg & tiny-skia & fontdb & rustybuzz are really helpful libraries 🙏 Thanks :)

[RazrFalcon]

usvg was never designed to be constructed by hand. It was just an accident. It is a parser by design.
The public API in usvg is permanently in beta + unstable state. The only stable part is the resvg.

[LaurenzV]

@RazrFalcon I tried porting the newest main and I can manage to get nearly everything to work (and parts of the code got much simpler!), except for one edge case:

I'm using the following method to render groups with filters into a pixmap:
https://github.com/RazrFalcon/resvg/blob/96eec97e2413cf7620c1aa0ed32481af1689ea05/crates/resvg/src/lib.rs#L75-L98

I can get it to work for all SVGs with filters except for the ones where the group doesn't have a bounding box, such as for example:

<svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
    <title>On an empty group (1)</title>

    <filter id="filter1" filterUnits="userSpaceOnUse" x="20" y="20" width="160" height="160">
        <feFlood flood-color="seagreen"/>
    </filter>
    <g id="g1" filter="url(#filter1)"/>

    <!-- image frame -->
    <rect id="frame" x="1" y="1" width="198" height="198" fill="none" stroke="black"/>
</svg>

Becaues the call to let bbox = node.abs_bounding_box().to_non_zero_rect()?; returns None since those groups don't have a bounding box. Any idea how to fix that? I guess we should use the layer bounding box instead. But we don't have an abs_layer_bbox right?

[RazrFalcon]

Thanks for the testing! Yes, a lot of things become way more straight-forward.

As for render_node, yes, it's a bug. I should use layer bbox here. Because it wouldn't work with blur either,

[RazrFalcon]

render_node should be fixed now.

[LaurenzV]

Yup now all tests pass, thanks a lot!!

[brianfeucht]

Just as another piece of feedback on this change, it negatively impacts an implementation I have.

The two use cases we have are:

1. Doing some string replacement in TextNodes after the tree was constructed. The primary reason we went with this approach is because the performance cost of parsing the xml multiple times vs making the change in the tree.
2. Add support for text wrapping by detecting when a render pushes a text node outside a bound. If outside a bound, we modify the text node to break it into two. There isn't really an accurate way to do this without rendering text with a font and then measuring.

I understand this use cases were not intentionally supported, but this change will either require us to:

• fork 😬
• trying to translate require changes into xml and reparsing and just living with the performance hit

I appreciate the intention of this change. It does make the API much easier to use. I'm not sure if you have recommendations on how we might support these use cases given the vision you have with this library.

[RazrFalcon]

usvg is not an SVG parsing library. It's a library that prepares an SVG for rendering. It was never designed to be mutable.
The only reason it was mutable before is because I didn't bothered to hide implementation details.
Before, any change to the tree was an undefined behaviour. It might worked in your specific case, but no more.

The problem is quite simple. To render SVG fast I need to know all nodes bboxes. And it's ridiculously hard to do.
Meaning that any change to the tree requires a recalculation of basically everything. And to recalculate everything we need a mutable tree (at least internally). And working with mutable trees (or any shared mutable state) in Rust is pain.

Not to mention that usvg text layout is ridiculously slow to begin with. You're not really optimizing much. In which case you would be better off generating an SVG with just a single text node.

In the end, if you find usvg output useful - great. If not - you need an another library. And I'm not interested in developing one.

PS: if you care about performance, you shouldn't be using SVG to begin with. It's horrible, horrible file format.

[laurmaedje]

I did attempt to port Typst to usvg 40.0 in the meantime. Because fonts may be loaded lazily from the network, I can't provide a populated fontdb. What I tried is parsing the SVG without fonts and traversing it to find all text nodes and then loading the specified fonts plus any fonts that may be required for font fallback. Then, I call has_text_nodes and if that's the case reparse with a populated fontdb.

The unfortunate problem is that usvg does not generate any Node::Text items if the fontdb is empty. (I took a quick peek into the code and it seems to be caused by a ? on a non-zero bounding box, but not sure.) I understand that it cannot provide flatten paths, but would it make sense for the text node itself to still be there?

Alternatively, as discussed above, it'd be great if there was an API for on-demand font loading. Basically just getting access to the text properties and the ability to populate the fontdb. Ideally, this would also provide access to the text itself so that callers can provide extra fonts that usvg's font fallback can make use of if the primary family isn't available.

[RazrFalcon]

@laurmaedje This can be done only via some sort of callback. We can add one to usvg::Options that would be called for each text node, so you could populate the db.
But the problem is that we have to accept a mutable fontdb now. Which means no multi-treaded parsing...

On the other hand, if we want to add embedded fonts support in the future, we need a mutable fontdb anyway, external or internal.
And external would probably not work, because we need an ability to add and remove fonts per SVG file. Which would quickly exhaust the internal fontdb ID generator.

As always, there is no such thing as "simple" in SVG...

Will see what can be done here. We might end up with internally cloning the fontdb when needed.

The unfortunate problem is that usvg does not generate any Node::Text items if the fontdb is empty.

This is by design. If a node doesn't have a valid bbox - it should be removed.

[laurmaedje]

@RazrFalcon What concrete design did you have in mind for the text callback? I think one where the callback is called with a mutable fontdb would work well enough, but one where the callee has control over the font selection itself would be even more powerful and flexible.

Either way, it would be great to get the ball rolling here, so that maybe this could be part of the next release. Right now, Typst is still on usvg 0.38 because otherwise it can't display any text. If there's anything I can help with to move this forward, let me know.

[RazrFalcon]

As I've mentioned above, fontdb cannot be mutable, because we pass an immutable one to the usvg right now.
Meaning that the only sane solution I see is to pass Arc<fontdb::Database> and deep-clone it when necessary.
Not optimal, but I don't see any other solutions.

Also, I'm not really sure what exactly do you need. Do you need a callback that would be called for each font or for all fonts at once? What do you plan to do in this callback? Simply populate the fontdb with your fonts or explicitly return the fontdb::ID of the font that must be used in that case?
Something like:

fn font_callback(font: &FontStyle, db: &mut fontdb::Database) -> Option<fontdb::ID> {}

?

I haven't looked much into it myself. I will try looking into it on the weekend.

[laurmaedje]

Do you need a callback that would be called for each font or for all fonts at once?

Either one is fine for me.

What do you plan to do in this callback?

Find a suitable font, load it via Typst's lazy font loading mechanism and then provide it to usvg.

Simply populate the fontdb with your fonts or explicitly return the fontdb::ID of the font that must be used in that case?

The latter version, where we get a fontdb::Query and can return an Option<ID> would be ideal as that would allow us to use our own font matching logic and the results would be consistent with how Typst behaves generally. If other users would want to provide font data, but not their own face selection logic, they could instead just populate the fontdb and then forward to fontdb::Database::query

I haven't looked much into it myself. I will try looking into it on the weekend.

Thank you!

[RazrFalcon]

So you do not plan to modify the fontdb in this callback? This simplifies things a bit.

What should happen when the callback returns None? The whole text element should be skipped or should we try to fallback to some random last resort font?

[laurmaedje]

So you do not plan to modify the fontdb in this callback?

I do need some way to provide the font data to usvg. So the fontdb would need to be modified. Basically, I can't construct the fontdb upfront because I only want to lazily load the font once I know that usvg will need it.

What should happen when the callback returns None? The whole text element should be skipped or should we try to fallback to some random last resort font?

Ideally, we could also influence the selection of the fallback font. There could be a second callback that can request a fallback font for a specific text run that we could serve in the same way. Or it's all one callback and the callback also provides the text itself so that we can do the fallback in there. Then, None could mean skip.