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

Implement simple example using swash to render to png #54

Merged
merged 15 commits into from
May 23, 2024

Conversation

nicoburns
Copy link
Contributor

@nicoburns nicoburns commented May 21, 2024

This is basically working. But it could definitely do with some actual review and cleanup before merging.

Output

output

Notes

  • Emoji rendering is not working
  • Font fallback for arabic text does seem to be working (note: I pulled the arabic text from cosmic text's sample files. Google translate translate the included arabic text as "arabic", but I cannot otherwise vouch for it's content).
  • Subpixel rendering is not enabled (does this make sense when rendering to image?)
  • The rendered text doesn't look fantastic. Possibly my blending code (used to apply the Swash alpha mask to an RGBA ImageBuffer from the image buffer is incorrect / too naive?). Cosmic-text pulls in tiny-skia in it's examples. I was hoping to avoid that here, but perhaps it's necessary?
  • Converting from a parley::Font to a swash::FontRef is very awkward, specifically when the font file is a collection. The Parley Font stores the index of the font within the collection, but Swash's FontRef expect the byte offset of the font within the collection. I could not find this exposed anywhere in parley/fontique/skrifa/read-fonts so I'm currently doing pointer arithmetic to get it.
  • I'm currently writing to a file called output.png in the users current working directory. Possibly we could do better than that (some kind of gitignored "data" directory in the parley repo?)
  • I've added an impl of Brush for peniko::Color, but possibly we ought to just blanket impl for all types implementing the requisite super traits?
  • I'm not using the NormalizedCoords as they always seemed to be empty in my example (what are they for?)

@nicoburns nicoburns requested review from dfrg and xorgy May 21, 2024 12:33
@nicoburns nicoburns force-pushed the swash-example branch 3 times, most recently from 4ea250c to 4cc54f5 Compare May 21, 2024 14:40
examples/simple.rs Outdated Show resolved Hide resolved
examples/simple.rs Outdated Show resolved Hide resolved
@dfrg
Copy link
Collaborator

dfrg commented May 21, 2024

Emoji rendering is not working

We lost this in the switch to fontique. I can submit a small patch to fix it.

Subpixel rendering is not enabled (does this make sense when rendering to image?)

Probably not unless we’re just doing it for effect. Might be nice to add an option at some point.

The rendered text doesn't look fantastic.

Black on white without gamma correction is going to look “dirty”. Something simple like alpha.pow(1.0/2.2) would improve this a bit.

I'm not using the NormalizedCoords as they always seemed to be empty in my example (what are they for?)

These are the parameters for controlling interpolation in variable fonts. You should be able to feed the slice directly into ScalerBuilder::normalized_coords.

@nicoburns
Copy link
Contributor Author

We lost this in the switch to fontique. I can submit a small patch to fix it.

Ah. Yes, it seems important!

Black on white without gamma correction is going to look “dirty”. Something simple like alpha.pow(1.0/2.2) would improve this a bit.

Perhaps using these functions? https://observablehq.com/@toja/color-blending-with-gamma-correction.

If I'm understanding correctly the correct process is:

  • Convert to float
  • Convert to linear rgb
  • Perform computation
  • Convert srgb
  • Convert to u8

These are the parameters for controlling interpolation in variable fonts. You should be able to feed the slice directly into ScalerBuilder::normalized_coords.

I see. Does this also need to be plugged in with skrifa?

@dfrg
Copy link
Collaborator

dfrg commented May 21, 2024

See #56 for a patch that might get emoji working with this example

@dfrg
Copy link
Collaborator

dfrg commented May 21, 2024

Perhaps using these functions? https://observablehq.com/@toja/color-blending-with-gamma-correction.

If I'm understanding correctly the correct process is:

  • Convert to float
  • Convert to linear rgb
  • Perform computation
  • Convert srgb
  • Convert to u8

We can do full sRGB if you'd like but I think just applying the power function to the alpha component will give us reasonably good results for the example.

These are the parameters for controlling interpolation in variable fonts. You should be able to feed the slice directly into ScalerBuilder::normalized_coords.

I see. Does this also need to be plugged in with skrifa?

It does. We use the LocationRef type there which is really just a wrapper for &[NormalizedCoord]. This unfortunately requires a type conversion and allocation as is done in xilem/masonry. We should probably change parley to use this type in the future.

@nicoburns
Copy link
Contributor Author

nicoburns commented May 21, 2024

We can do full sRGB if you'd like but I think just applying the power function to the alpha component will give us reasonably good results for the example.

The trouble I was having with apply pow to the alpha channel is that it's a u8 and I'm unable to apply a fractional power to a u8.

I have implemented an attempt at sRGB <-> Linear RGB conversion. Not sure if I've done it correctly (it's interesting that you suggest applying the curve to the alpha channel as the solution I've ended up with applies it to everything but the alpha channel!) as the result isn't obviously better (but it's good enough that it looks plausibly correct to me). I've left both functions in the code for the time being if you want to inspect them.

Naive:

output_naive

Gamma corrected:

output_gamma

It does. We use the LocationRef type there which is really just a wrapper for &[NormalizedCoord]. This unfortunately requires a type conversion and allocation as is done in xilem/masonry. We should probably change parley to use this type in the future.

Ah, gotcha. I'll have a look at plugging this in tomorrow.

@dfrg
Copy link
Collaborator

dfrg commented May 21, 2024

Assuming this is using system-ui on macOS, setting the normalized coordinates should also get you the requested bold on the first four characters since San Francisco is a variable font.

@nicoburns
Copy link
Contributor Author

Assuming this is using system-ui on macOS, setting the normalized coordinates should also get you the requested bold on the first four characters since San Francisco is a variable font.

Ah, that would be good. Bold was working with Helvetica, but it generally looked pretty bad with Helvetica so I switched it out. And I'd like to leave it on system-ui as that will presumably pick something sensible cross-platform. I guess maybe I could also use sans-serif?

@nicoburns
Copy link
Contributor Author

Pretty sure my gamma correction function is wrong as the serif font looks much better with the naive blending.

examples/simple.rs Outdated Show resolved Hide resolved
@dfrg
Copy link
Collaborator

dfrg commented May 22, 2024

Pretty sure my gamma correction function is wrong as the serif font looks much better with the naive blending.

Yeah, this is exceptionally tricky to get right. I’ll play with it later this evening.

Copy link
Contributor

@waywardmonkeys waywardmonkeys left a comment

Choose a reason for hiding this comment

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

I'm assuming that once the other (tiny-skia) lands, this will move to a crate as well?

@nicoburns
Copy link
Contributor Author

I'm assuming that once the other (tiny-skia) lands, this will move to a crate as well?

Yes. Hoping to land the tiny-skia one (#55) first. Will update this one once we've settled on a design there.

@nicoburns
Copy link
Contributor Author

nicoburns commented May 22, 2024

Pretty sure my gamma correction function is wrong as the serif font looks much better with the naive blending.

Yeah, this is exceptionally tricky to get right. I’ll play with it later this evening.

Hmm... the image crate (that we're already using here) has a blend function (https://docs.rs/image/latest/image/struct.Rgba.html#method.blend). Perhaps we could just use that. Looks the same as my "naive" version to me, but it certainly makes the example shorter.

EDIT: I've update this PR to use image's built in blend function. Old code is still available in an older commit if you want to play with it.

@nicoburns
Copy link
Contributor Author

These are the parameters for controlling interpolation in variable fonts. You should be able to feed the slice directly into ScalerBuilder::normalized_coords.

@dfrg Hmm.. I've added this, but I still don't seem to get bold text.

@dfrg
Copy link
Collaborator

dfrg commented May 22, 2024

These are the parameters for controlling interpolation in variable fonts. You should be able to feed the slice directly into ScalerBuilder::normalized_coords.

@dfrg Hmm.. I've added this, but I still don't seem to get bold text.

Was just working on this :) Found a bug in swash's avar table. Update to swash 0.1.16 should fix it.

@dfrg
Copy link
Collaborator

dfrg commented May 22, 2024

Overall, this is looking pretty good to me now. I'm happy to punt on the gamma issue, get the tiny-skia version landed and then merge this one after making the crate extraction changes.

github-merge-queue bot pushed a commit that referenced this pull request May 23, 2024
This example uses `skrifa` and `tiny-skia` rather than `swash` and
`image` as in #54.

## Output


![tiny_skia_render](https://github.com/linebender/parley/assets/1007307/74cbf3b6-380c-4dff-91e4-976612982fac)

## Notes

- This has the same issue as #54 around emoji rendering not working /
being implemented
- There is an issue with the centers of closed letter forms incorrectly
being filled
- The code for this is IMO much nicer than the swash version (at the
cost of the heavier tiny-skia dependency)
- This example does not have hinting enabled
@nicoburns nicoburns marked this pull request as draft May 23, 2024 14:32
}
}
}
Content::SubpixelMask => unimplemented!(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dfrg What are the semantics of the SubpixelMask? (/ how would one apply that to an RGBA image?). I reckon it would be good to demonstrate that even if we don't actually enable subpixel mode in the example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an rgba image with an alpha component per color channel (the alpha in the mask is ignored). This document (https://github.com/servo/webrender/blob/main/webrender/doc/text-rendering.md#subpixel-text-blending) has a good explanation of the math.

@nicoburns nicoburns marked this pull request as ready for review May 23, 2024 21:45
@nicoburns nicoburns requested review from dfrg and waywardmonkeys May 23, 2024 21:45
@nicoburns
Copy link
Contributor Author

I've rebased this on top of latest main and made it fit the new directory structure. It is ready for another round of review.

Copy link
Collaborator

@dfrg dfrg left a comment

Choose a reason for hiding this comment

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

LGTM with a couple suggestions and some confusion about the code duplication.

examples/simple.rs Outdated Show resolved Hide resolved
examples/simple.rs Outdated Show resolved Hide resolved
examples/swash_render/src/main.rs Show resolved Hide resolved
@nicoburns nicoburns dismissed waywardmonkeys’s stale review May 23, 2024 22:34

Requested changes addressed

@nicoburns nicoburns added this pull request to the merge queue May 23, 2024
Merged via the queue into linebender:main with commit 8e10403 May 23, 2024
19 checks passed
@nicoburns nicoburns deleted the swash-example branch May 23, 2024 22:37
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.

3 participants