go-text / typesetting

High quality text shaping in pure Go.
Other
88 stars 11 forks source link

Font scan #63

Closed benoitkugler closed 1 year ago

benoitkugler commented 1 year ago

As discussed in #17, I've rebased this branch to include the latest changes.

The PR is long : you may want to start by the readme before diving into the implementation.

As a summary, this package provides two functionalities :

I'm open to all your suggestions !

whereswaldon commented 1 year ago

We use one here, which is the functionality Gio is consuming to load system fonts.

Wow, my mistake, I forgot we have also implemented the general approach.

No worries!

OK, so it is probably best to keep your approach (not changing the Fontmap interface) and bear with the "duplicated" ResolveFace and ResolveFaceAndMetadata.

I don't mind changing the API if we have a technical reason to, but the current thing is certainly the least churn. Is there a structure that seems superior?

benoitkugler commented 1 year ago

We use one here, which is the functionality Gio is consuming to load system fonts.

Wow, my mistake, I forgot we have also implemented the general approach.

No worries!

OK, so it is probably best to keep your approach (not changing the Fontmap interface) and bear with the "duplicated" ResolveFace and ResolveFaceAndMetadata.

I don't mind changing the API if we have a technical reason to, but the current thing is certainly the least churn. Is there a structure that seems superior?

We could perhaps use a new method Fontmap.Metadata() similar to FaceLocation(). Cleaner API, but probably a bit less performant when the number of faces actually used grows

whereswaldon commented 1 year ago

I was unable to set up a FreeBSD system capable of testing this within a few hours, so I think we're just going to have to hope it works. I did get confirmation from a friend that it's working on OpenBSD.

whereswaldon commented 1 year ago

We use one here, which is the functionality Gio is consuming to load system fonts.

Wow, my mistake, I forgot we have also implemented the general approach.

No worries!

OK, so it is probably best to keep your approach (not changing the Fontmap interface) and bear with the "duplicated" ResolveFace and ResolveFaceAndMetadata.

I don't mind changing the API if we have a technical reason to, but the current thing is certainly the least churn. Is there a structure that seems superior?

We could perhaps use a new method Fontmap.Metadata() similar to FaceLocation(). Cleaner API, but probably a bit less performant when the number of faces actually used grows

I wouldn't want to traverse the entire set of loaded fonts in the way that FaceLocation does. I'd be open to this if we used a more efficient strategy. It seems like that would require maintaining multiple maps of the loaded fonts though (in order to support querying by different keys), which introduces many opportunities for mistakes. Would you prefer that?

benoitkugler commented 1 year ago

I wouldn't want to traverse the entire set of loaded fonts in the way that FaceLocation does. I'd be open to this if we used a more efficient strategy. It seems like that would require maintaining multiple maps of the loaded fonts though (in order to support querying by different keys), which introduces many opportunities for mistakes. Would you prefer that?

Hum.. a map[Face] -> (Location, Metadata) would be sufficient for efficient FaceLocation and FaceMetadata methods right ? Maybe it is not to dangerous to use this approach : I see only two writes to the cache field, and we could use an helper method to ensure consistency.

whereswaldon commented 1 year ago

@benoitkugler I've tried to implement a separate Metadata() method. Let me know what you think.

As far as I currently know, the only TODOs on this branch are:

I'd appreciate it if you could handle the rebase, and I believe that you've already got some work for monospace stuff. Is there anything I can do to help right now? If not, I'll focus on the Gio side of this feature.

whereswaldon commented 1 year ago

I found a hole in the API that I've tried to plug in #80. Since it's new API design and a feature we haven't already discussed, I thought it made sense to discuss it in a separate draft PR with this as the base branch.

benoitkugler commented 1 year ago

@benoitkugler I've tried to implement a separate Metadata() method. Let me know what you think.

As far as I currently know, the only TODOs on this branch are:

  • [ ] rebase on main to resolve the harfbuzz-related conflicts
  • [ ] implement something for resolving monospace fonts

I'd appreciate it if you could handle the rebase, and I believe that you've already got some work for monospace stuff. Is there anything I can do to help right now? If not, I'll focus on the Gio side of this feature.

Thank you very much for the many improvements ! There were so many gaps you have filled ! I can indeed get a stab at the two remaining points.

whereswaldon commented 1 year ago

@benoitkugler thinking on it (and discussing the issue with @dominikh), I'm starting to think I was wrong to want a special case for monospace fonts. It's expensive for us to check if a font is monospace, and no other font resolution platform implements it like this. They expect you to either specify a known monospace font by family name or to rely on the generic family names like monospace. Maybe that is the better option.

Based on your current view of things, does it seem better to not handle that as a special case, or to persist in adding special resolution mechanisms for it?

whereswaldon commented 1 year ago

I think we may need to do some extra work to honor this CSS requirement

If the font family name [in @font-face] is the same as a font family available in a given user's environment, it effectively hides the underlying font for documents that use the stylesheet. This permits a web author to freely choose font-family names without worrying about conflicts with font family names present in a given user's environment.

Right now, manually loading a font into the fontmap doesn't prevent a system font of the same name from being used. In fact, the system font may win always depending on load ordering.

I'm really struggling with how to best ensure that fonts loaded by applications are used whenever possible. #80 handles the case in which there are no system fonts loaded, allowing us to fall back to manually loaded fonts. However, in the presence of system fonts, the default rule will add sans-serif and derivatives, ensuring that a manually loaded sans-serif font is never chosen if the system provides any.

Maybe that's okay, as we expect application authors to request their custom faces by name. It's simply irritating because Gio historically didn't work this way, and it's unclear how to migrate to the new behavior smoothly.

benoitkugler commented 1 year ago

@benoitkugler thinking on it (and discussing the issue with @dominikh), I'm starting to think I was wrong to want a special case for monospace fonts. It's expensive for us to check if a font is monospace, and no other font resolution platform implements it like this. They expect you to either specify a known monospace font by family name or to rely on the generic family names like monospace. Maybe that is the better option.

Based on your current view of things, does it seem better to not handle that as a special case, or to persist in adding special resolution mechanisms for it?

Ah this is very interesting !

As far as my personal use of go-text is concerned, I would not have use the IsMonospace Query flag, so...

I've noticed that the flag behavior may be surprising : for instance, for Query{Families: []string{"NotoSans"}, EnforceMonospace: true}, the NotoSansMono would not always match ! (because the family names do no match, and assuming they are other monospace fonts in the database).

So the only "well-defined" use case (in my opinion), is to combine a generic family like "serif" or "sans-serif" with the IsMonospace flag. Does it often happens in practice ?

benoitkugler commented 1 year ago

I think we may need to do some extra work to honor this CSS requirement

If the font family name [in @font-face] is the same as a font family available in a given user's environment, it effectively hides the underlying font for documents that use the stylesheet. This permits a web author to freely choose font-family names without worrying about conflicts with font family names present in a given user's environment.

Right now, manually loading a font into the fontmap doesn't prevent a system font of the same name from being used. In fact, the system font may win always depending on load ordering.

I'm really struggling with how to best ensure that fonts loaded by applications are used whenever possible. #80 handles the case in which there are no system fonts loaded, allowing us to fall back to manually loaded fonts. However, in the presence of system fonts, the default rule will add sans-serif and derivatives, ensuring that a manually loaded sans-serif font is never chosen if the system provides any.

Maybe that's okay, as we expect application authors to request their custom faces by name. It's simply irritating because Gio historically didn't work this way, and it's unclear how to migrate to the new behavior smoothly.

Could this be handled by a tag on the footprint ("user"/"system"), then giving higher priority to "user" ? The fonts with the same family name would have the same score, so this additional sort would yield the expected order (I think !)

whereswaldon commented 1 year ago

@benoitkugler thinking on it (and discussing the issue with @dominikh), I'm starting to think I was wrong to want a special case for monospace fonts. It's expensive for us to check if a font is monospace, and no other font resolution platform implements it like this. They expect you to either specify a known monospace font by family name or to rely on the generic family names like monospace. Maybe that is the better option. Based on your current view of things, does it seem better to not handle that as a special case, or to persist in adding special resolution mechanisms for it?

Ah this is very interesting !

As far as my personal use of go-text is concerned, I would not have use the IsMonospace Query flag, so...

I've noticed that the flag behavior may be surprising : for instance, for Query{Families: []string{"NotoSans"}, EnforceMonospace: true}, the NotoSansMono would not always match ! (because the family names do no match, and assuming they are other monospace fonts in the database).

So the only "well-defined" use case (in my opinion), is to combine a generic family like "serif" or "sans-serif" with the IsMonospace flag. Does it often happens in practice ?

In practice, I think Gio users are spoiled by our previous font matcher explicitly allowing you to say "give me the first available monospace font", but we shouldn't let that drive the design.

Given that the flag behaves in a counterintuitive way unless you use it with generic families, I think we should drop it.

We may even be able to drop the monospace information from footprint and metadata. It was all an effort to implement this older Gio behavior that I now think was both poorly-defined and cannot be implemented efficiently for system fonts. Of course, it cay stay if someone does have a use for it.

I feel quite silly, if I'm honest. I approached this with the goal of implementing a compatible interface to what Gio already did, but Gio was doing something super unrealistic. The more that I come to understand the mechanics here, the more I think Gio has to change to match the reality of what metadata is available.

whereswaldon commented 1 year ago

I think we may need to do some extra work to honor this CSS requirement

If the font family name [in @font-face] is the same as a font family available in a given user's environment, it effectively hides the underlying font for documents that use the stylesheet. This permits a web author to freely choose font-family names without worrying about conflicts with font family names present in a given user's environment.

Right now, manually loading a font into the fontmap doesn't prevent a system font of the same name from being used. In fact, the system font may win always depending on load ordering. I'm really struggling with how to best ensure that fonts loaded by applications are used whenever possible. #80 handles the case in which there are no system fonts loaded, allowing us to fall back to manually loaded fonts. However, in the presence of system fonts, the default rule will add sans-serif and derivatives, ensuring that a manually loaded sans-serif font is never chosen if the system provides any. Maybe that's okay, as we expect application authors to request their custom faces by name. It's simply irritating because Gio historically didn't work this way, and it's unclear how to migrate to the new behavior smoothly.

Could this be handled by a tag on the footprint ("user"/"system"), then giving higher priority to "user" ? The fonts with the same family name would have the same score, so this additional sort would yield the expected order (I think !)

Yes, I definitely think this would work.

benoitkugler commented 1 year ago

I feel quite silly, if I'm honest.

Thank you for you intellectual honesty, very much appreciated !

Given that the flag behaves in a counterintuitive way unless you use it with generic families, I think we should drop it.

We may even be able to drop the monospace information from footprint and metadata. It was all an effort to implement this older Gio behavior that I now think was both poorly-defined and cannot be implemented efficiently for system fonts. Of course, it cay stay if someone does have a use for it.

Alright, let's drop it then.

I think we can keep it in the metadata package, but ensure we don't pay the price to compute it in fontscan. I'll try to do so tomorrow.

Could this be handled by a tag on the footprint ("user"/"system"), then giving higher priority to "user" ? The fonts with the same family name would have the same score, so this additional sort would yield the expected order (I think !)

Yes, I definitely think this would work.

Great, I'll try to tackle it as well tomorrow.

benoitkugler commented 1 year ago

By the way, should we keep the FindFont API (filename based, without rune coverage) ? I've initialy written it to propose a faster alternative to the Fontmap API, but do you think it is still useful for the toolkits ? (It won't be for my use case.)

whereswaldon commented 1 year ago

By the way, should we keep the FindFont API (filename based, without rune coverage) ? I've initialy written it to propose a faster alternative to the Fontmap API, but do you think it is still useful for the toolkits ? (It won't be for my use case.)

I don't think Gio needs it, and I suspect Fyne will be in the same boat. If we want to resolve a system font, we're probably trying to fill a gap in rune coverage. We might just be loading it for the sake of having a prettier font, but the scan isn't actually as slow as I would have guessed, so it's usable in practice by the toolkits anyway.

benoitkugler commented 1 year ago

I've applied the changes we have discussed, along with some memory allocation optimization.

I would like to go a bit further, but it will require a small API change in metadata, so I'll open a separate PR : see #81

andydotxyz commented 1 year ago

Sorry all, just catching up on this long PR - great work, and highly relevant to a task I am about to pick up too. Apologies for the delay.

  • [ ] ~FreeBSD~ I was unable to get a working graphical FreeBSD system in a reasonable amount of time, so I think we're just going to have to hope it works similarly enough to OpenBSD for now.

Can I help on this one? Fyne has a FreeBSD test target which I should be able to run appropriate test / searches on.

I feel quite silly, if I'm honest. I approached this with the goal of implementing a compatible interface to what Gio already did, but Gio was doing something super unrealistic. The more that I come to understand the mechanics here, the more I think Gio has to change to match the reality of what metadata is available.

I think you're being harsh on yourself here. Fyne also expects the ability to have a "monospace font" automatically known - as you say it is a user expectation that typing code blocks etc will pick appropriately. I don't know if a query lookup in the way discussed is necessarily the only way to do it - but as the main text/font library for Go graphics I think we do need a solution to this problem. Perhaps, as we have platform specific code, we can have a simplified version of "what is the normal monospace font" - I.e. not a full scan, but something we know exists in the OS install?

If it's not possible to do something like that I think Fyne, like Gio, will have to continue packaging a monospace font, or implementing complex lookup logic to determine what is a good monospace guess and it feels like that belongs better here than in the toolkit.

benoitkugler commented 1 year ago

I don't know if a query lookup in the way discussed is necessarily the only way to do it - but as the main text/font library for Go graphics I think we do need a solution to this problem. Perhaps, as we have platform specific code, we can have a simplified version of "what is the normal monospace font" - I.e. not a full scan, but something we know exists in the OS install?

If it's not possible to do something like that I think Fyne, like Gio, will have to continue packaging a monospace font, or implementing complex lookup logic to determine what is a good monospace guess and it feels like that belongs better here than in the toolkit.

Note that, with the current solution, using Query{Families: []string{"monospace"}} is a valid way to get a monospace font (providing the system has one), which hopefully is enough in practice. The only behavior not covered is "I want a serif font which is mono-spaced", but I hope it is not required too often..

benoitkugler commented 1 year ago
  • [ ] ~FreeBSD~ I was unable to get a working graphical FreeBSD system in a reasonable amount of time, so I think we're just going to have to hope it works similarly enough to OpenBSD for now.

Can I help on this one? Fyne has a FreeBSD test target which I should be able to run appropriate test / searches on.

That would be great ! I guess that checking if the tests pass are a first start, and then trying to actually display something using the Fontmap would be nice. A pseudo code you could use :

fm := fontscan.NewFontMap()
fm.UseSystemFonts() 
input := shaping.Input{Text: []rune("my text")}
inputs := shaping.SplitByFace(input, fm)
// then shape with harfbuzz and display outputs

Perhaps @whereswaldon has other suggestions ?

whereswaldon commented 1 year ago

I feel quite silly, if I'm honest. I approached this with the goal of implementing a compatible interface to what Gio already did, but Gio was doing something super unrealistic. The more that I come to understand the mechanics here, the more I think Gio has to change to match the reality of what metadata is available.

I think you're being harsh on yourself here. Fyne also expects the ability to have a "monospace font" automatically known - as you say it is a user expectation that typing code blocks etc will pick appropriately. I don't know if a query lookup in the way discussed is necessarily the only way to do it - but as the main text/font library for Go graphics I think we do need a solution to this problem. Perhaps, as we have platform specific code, we can have a simplified version of "what is the normal monospace font" - I.e. not a full scan, but something we know exists in the OS install?

If it's not possible to do something like that I think Fyne, like Gio, will have to continue packaging a monospace font, or implementing complex lookup logic to determine what is a good monospace guess and it feels like that belongs better here than in the toolkit.

The ability to resolve a monospace font is easy enough. As @benoitkugler says, you just say monospace as the family. What Gio used to offer was a way to specify all of:

However, in practice the variation is part of the family. Those variations aren't exposed in font metadata that can be queried efficiently. Allowing callers to request "Go" as the family and "Mono" as the variation seems fine on the surface, but that can't be queried efficiently. However, such users can just request "Go Mono" as the family to get what they wanted. Gio dodged this by having the user annotate the fonts manually to specify their variation, but we can't do that for system fonts.

I'm planning to just drop the "variation" from the API, expecting users to encode what they want in the family name instead. This matches how every other font resolution system seems to work. I'll also be adding a way to specify the fallback chain so that you can say "Go Mono, Courier, monospace". Gio currently doesn't allow this sort of configurable fallback.

whereswaldon commented 1 year ago
  • [ ] ~FreeBSD~ I was unable to get a working graphical FreeBSD system in a reasonable amount of time, so I think we're just going to have to hope it works similarly enough to OpenBSD for now.

Can I help on this one? Fyne has a FreeBSD test target which I should be able to run appropriate test / searches on.

That would be great ! I guess that checking if the tests pass are a first start, and then trying to actually display something using the Fontmap would be nice. A pseudo code you could use :

fm := fontscan.NewFontMap()
fm.UseSystemFonts() 
input := shaping.Input{Text: []rune("my text")}
inputs := shaping.SplitByFace(input, fm)
// then shape with harfbuzz and display outputs

Perhaps @whereswaldon has other suggestions ?

@andydotxyz Here's a complete program that you can use to test font support:

package main

import (
    "log"
    "os"

    "github.com/go-text/typesetting/fontscan"
)

func main() {
    cwd, err := os.Getwd()
    if err != nil {
        panic(err)
    }
    fontMap := fontscan.NewFontMap(log.Default())
    err = fontMap.UseSystemFonts(cwd)
    if err != nil {
        panic(err)
    }
    fontMap.SetQuery(fontscan.Query{
        Families: []string{""},
    })
    face := fontMap.ResolveFace('a')
    family, aspect := fontMap.FontMetadata(face.Font)
    log.Printf("got face: %p %s %#+v", face, family, aspect)
}

If the final log line prints a real face family name and aspect, we successfully resolved a system font. I don't think displaying text graphically is strictly necessary, though it was how I was doing my testing.

whereswaldon commented 1 year ago

From where I stand, this PR is ready for review. I'm not aware of feature gaps or fundamental concerns with the approach. Since clicking the "ready for review" button doesn't do anything except legitimize the PR in GitHub, I think I'll do that now. We can continue with code review as per any other PR.

whereswaldon commented 1 year ago

A friend confirmed that it's working on FreeBSD for me, so all of my target platforms are covered.

andydotxyz commented 1 year ago

My apologies for delays once again. FreeBSD gets this from the test app: got face: 0x4000126810 luxisans metadata.Aspect{Style:0x1, Weight:400, Stretch:1} And latest Darwin works too got face: 0x14003c7ba40 verdana metadata.Aspect{Style:0x1, Weight:400, Stretch:1}

Were there iOS and Android tests completed as well or was that out of scope at this time?

andydotxyz commented 1 year ago

iOS results are positive. but Android seems to crash, investigating.

Screenshot 2023-07-04 at 16 24 07

andydotxyz commented 1 year ago

Android log...

07-04 16:25:49.406  5512  5546 I Fyne    : using system font dirs ["/data/fonts" "/system/fonts"]
07-04 16:25:49.406  5512  5546 I Fyne    : error walking font directory "/data/fonts": open /data/fonts: permission denied

Do you think this can be fixed to work before merge, or is Android additional work that is later?

Screenshot 2023-07-04 at 16 32 45
andydotxyz commented 1 year ago

Oh, my apologies I see now that it was expected that the demo code would have some android item in it. Resolved once utilised appropriately:

Screenshot 2023-07-04 at 16 40 36
andydotxyz commented 1 year ago

One last comment, not sure if it matters or not... But it may fail to find matching faces for some runes - emoji was a test that seemed to fail for me:

2023/07/04 16:47:01 using system font dirs ["/Library/Fonts" "/System/Library/Fonts" "/Users/andy/Library/Fonts"]
2023/07/04 16:47:01 No font matched for [] and rune U+1F600 (😀) -> returning arbitrary face

Could it be a multi-byte rune issue? Or are emoji fonts problematic

benoitkugler commented 1 year ago

One last comment, not sure if it matters or not... But it may fail to find matching faces for some runes - emoji was a test that seemed to fail for me:

2023/07/04 16:47:01 using system font dirs ["/Library/Fonts" "/System/Library/Fonts" "/Users/andy/Library/Fonts"]
2023/07/04 16:47:01 No font matched for [] and rune U+1F600 (😀) -> returning arbitrary face

Could it be a multi-byte rune issue? Or are emoji fonts problematic

Very interesting point. One could expect the FontMap to be smart enough to select a proper "emoji font" to handle such runes, but it is not the case. At the FontMap level, the solution is to explicitly ask for an emoji font, by providing Query{Families: []string{"emoji"}}. This will require more work from the toolkits. I think we may want to add in the future an "emoji iterator", whose job would be to detect emoji runes, so that the Fontmap consumers may update the query accordingly. Note that because of multi-runes emoji (emoji + modifier), the Fontmap API (limited to one rune at a time) may not do this automatically.

Such an iterator may also be seen as one step of text segmentation, alongside with bidi and script, so it may be worth thinking of it as part of larger scope.

benoitkugler commented 1 year ago

I've opened #83 to export the common generic families; Serif, SansSerif, Emoji and Monospace being probably the most useful ones.

whereswaldon commented 1 year ago

I've rebased this atop main, as GitHub was unable to resolve the conflicts itself.

whereswaldon commented 1 year ago

My rebase made no logic changes (just had to fix conflicts in go.mod over and over and over and a couple of obvious other conflicts). As CI is still happy, I think this can be merged.