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

Continue support COLRv1 table #145

Merged
merged 54 commits into from
May 9, 2024
Merged

Conversation

LaurenzV
Copy link
Contributor

No description provided.

@LaurenzV
Copy link
Contributor Author

@RazrFalcon As far as I can tell, the current way we deal with outlines is not going to be good enough to work with COLRv1 correctly... In particular, filling a shape must also create a clip path, so that if we have nested fills, the clip paths are combined. See also https://learn.microsoft.com/en-us/typography/opentype/spec/colr#filling-shapes. Currently, we only have the outline function which takes a glyph ID and the user must generate the path for it and store it until the next operation.

My suggestions would be the following: Instead of having a single outline function, we implement methods for building paths (similar to OutLineBuilder) like move_to, line_to etc. and in the end, and then we also add a clip, fill and clip_and_fill method where the user should take the path that has been built and apply the corresponding operation. This would also allow us to define arbitrary clip paths, which I think will be necessary for ClipBox.

What do you think?

@RazrFalcon
Copy link
Collaborator

Sure. Sounds good.

@LaurenzV
Copy link
Contributor Author

Small update: Making decent progress, but some parts of it are a bit tricky, especially dealing with transforms that only apply to a paint and not to an outline... But I'll think of something and make the API a bit nicer while I'm at it.

Only major thing missing is variable fonts, not sure how difficult that will be to implement.

@LaurenzV
Copy link
Contributor Author

https://github.com/RazrFalcon/ttf-parser/blob/ad4449de309b06d27a0d36ad48f75c5ad6711001/src/tables/colr.rs#L364-L366

What exactly is the "Explain why" comment referring to? And could we merge all of the scale, rotate etc. functions into transform or is there a good reason to keep them separate?

@RazrFalcon
Copy link
Collaborator

What exactly is the "Explain why" comment referring to?

I already forgot 😄

I would guess this is because rotate to affine transform requires trigonometry functions, which are not available in no_std. And we do not support libm hack in ttf-parser (yet).

I don't mind having just the transform callback, but we would have to port the libm hack from tiny-skia.

src/tables/colr.rs Outdated Show resolved Hide resolved
src/tables/colr.rs Outdated Show resolved Hide resolved
@@ -20,7 +20,7 @@ impl<'a> Table<'a> {
let mut s = Stream::new(data);

let version = s.read::<u16>()?;
if version != 0 {
if version > 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

v0 and v1 are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first part of the table should be the same. That's why we have an early return in the function if version is 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we support v1? Does it even have a useful info for us?

examples/font2svg.rs Outdated Show resolved Hide resolved
@RazrFalcon
Copy link
Collaborator

Once again a superb job! The code looks like if I wrote it myself, back in those days when I had time...

I guess it was a good introduction into TrueType insanity, especially since you had to deal with variable fonts as well.

And with this change I guess tff-parser become one of the most feature-rich TrueType libraries out there. There is not much left to do after that.

Also update the readme. We currently have COLR table ~ (only v0)

Have you thought about rasterizing COLRv1 via tiny-skia in font2svg? Maybe that would be a better choice since SVG is a bit limited for this case.
And we would probably have to do the same in resvg anyway.

@LaurenzV
Copy link
Contributor Author

LaurenzV commented May 9, 2024

I guess it was a good introduction into TrueType insanity, especially since you had to deal with variable fonts as well.

It was indeed. 😄 It's crazy how many different features there are....

Have you thought about rasterizing COLRv1 via tiny-skia in font2svg? Maybe that would be a better choice since SVG is a bit limited for this case. And we would probably have to do the same in resvg anyway.

Yeah, this would in theory be the next step. The main blocker for this is that we don't have sweep gradients in tiny-skia, and I'm honestly not sure if I really want to also look into how skia works... I think dealing with OpenType is already more than enough for me. 😄

For now, my basic idea was to port the font2svg part into usvg, so that usvg basically has a best-effort implementation for flattening text. We should remark that it is only best-effort, and that if the user wants accurate COLRv1, they will have to convert the layouted glyphs themselves. And once we have sweep gradients in tiny-skia, we basically do the same in resvg. But as I said, not sure when that will happen, for now I will just try to get the best-effort version into usvg. It probably works correctly for more than 95% of the emojis, anyway.

@LaurenzV
Copy link
Contributor Author

LaurenzV commented May 9, 2024

Btw it looks like ttf-parser without the std feature is broken? Try running cargo run --no-default-features --example font2svg -- some_font.ttf out.svg, it seems to complain about the parse function. It's unrelated to this PR though, it doesn't work on main either. Just so you know.

@RazrFalcon
Copy link
Collaborator

Got it. I will look into adding sweep gradients to tiny-skia somewhere this month and will try to implement a COLRv1 rasterizer as well.

Examples are not intended to be running without default features. It's fine.

Ready to merge?

@LaurenzV
Copy link
Contributor Author

LaurenzV commented May 9, 2024

Got it. I will look into adding sweep gradients to tiny-skia somewhere this month and will try to implement a COLRv1 rasterizer as well.

Okay, good luck with that, feel free to ping me if there is a test case you can't get to render properly. Keep in mind that I have a feeling that it will be tricky to get some edge cases to work properly.

Ready to merge?

From my side yes!

@RazrFalcon RazrFalcon merged commit f8c1b25 into harfbuzz:master May 9, 2024
2 checks passed
@LaurenzV
Copy link
Contributor Author

LaurenzV commented May 9, 2024

@RazrFalcon Should I port the "best-effort" solution to usvg? Or do you prefer if usvg only supports COLRv0, and COLRv1 always needs to be implemented on the on the caller's side?

@RazrFalcon
Copy link
Collaborator

Sure. Something would be better then nothing.

@LaurenzV
Copy link
Contributor Author

I'll wait for the next rustybuzz release then. Somehow I'm not able to patch the ttf-parser dependencyp properly on the resvg side...

@RazrFalcon
Copy link
Collaborator

Strange. Will bump rustybuzz then.

You probably have to path fontdb as well.

@RazrFalcon
Copy link
Collaborator

@LaurenzV rustybuzz fails after ttf-parser update. https://github.com/RazrFalcon/rustybuzz/actions/runs/9034679635/job/24827768167

I guess something got changed in the variable fonts support.

@LaurenzV
Copy link
Contributor Author

Oof... Okay, I will try to look into it. Sorry, maybe I should've tested the integration with rustybuzz before. 😅

@RazrFalcon
Copy link
Collaborator

Bumped ttf-parser in fontdb and rustybuzz.

@LaurenzV
Copy link
Contributor Author

@RazrFalcon How hard do you think would it be to add sweep gradients to tiny-skia for someone who has never looked at the Skia code base? 😅 If you think it's manageable maybe I'll take a look.

@RazrFalcon
Copy link
Collaborator

I think it would be a waster of time. You would have to know how Skia works and what changes I had to do to adapt it to Rust and tiny-skia. Unlike rustybuzz, tiny-skia is very different to Skia.

I will try to look into it soon™.

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