569770 - [harfbuzz] use harfbuzz shaper under Linux

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[John Daggett (:jtd)]

The work on bug 449292 implements harfbuzz support on Windows and Mac but not on Linux.  The Linux code will need to be restructured, similar to the restructuring done for bug 502906.  Not sure whether support for complex scripts in harfbuzz needs to be done first or if some fallback to existing Pango code for this is possible.

Comment 1

[Behdad Esfahbod]

The current plan is to move all the complex shapers to HarfBuzz by around September time and make Pango solely use that.

Comment 2

[Joe Drew (not getting mail)]

I think we need to bite the bullet and do this, but I don't know how much this is going to impact our schedule. It needs to be in beta 5 because that's feature freeze.

Comment 3

[Karl Tomlinson (:karlt)]

Starting on this.

Comment 4

[Karl Tomlinson (:karlt)]

This is not going to make beta 5 tomorrow.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: part 1: keep a reference from font table blob to font entry to ensure cache lives until removal

So that we don't need to be concerned about blobs getting destroyed before their font entries.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: part 2: make it clearer that the blob owns the cache entry

The FontTableCacheEntry no longer needs to hold the blob pointer.

If hb_blob_create returns a nil blob, FontTableCacheEntry::Destroy can still
try to remove the cache entry before (and not after) it is added, but now only
the nil blob lives on in the cache instead of the FontTableCacheEntry.

Although there is no way it can happen yet, these changes also provide that
the world won't end if an existing table entry gets knocked out by a new entry
with the same tag.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: part 2: make it clearer that the blob owns the cache entry v1.1

GetBlob -> MakeBlob (and this version compiles)

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: part 2: make it clearer that the blob owns the cache entry v1.2

qref

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: part 3: provide public access to gfxFontEntry font table cache

The cairo API only provides access to the FT_Face through the scaled font, not
through the font face.

That's OK because usually we want the FT_Face for getting info about a scaled
font, but to share the font tables via the font entry the gfxFont needs access
to the table cache.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: part 4: gfxFT2FontBase::GetFontTable implementation

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: part 5: gfxFT2FontBase::GetHintedGlyphWidth implementation

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: part 6: use font group style to look for CSS features

gfxFcFonts are associated with a cairo scaled font rather than an particular set of CSS properties.

Here, these CSS font features are a property of the font group, so it makes sense to look up the features on the font group.

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: part 7: allow the font to provide its own GetGlyph function

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: part 8: gfxFT2FontBase::GetGlyph implementation

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: part 9: use gfxHarfBuzzShaper for SFNT fonts

This patch switches to using gfxHarfBuzzShaper only for SFNT fonts.
With GetGlyph provided by the font, it would also work for shaping with
other fonts, but AFAIK there is no point.

I assume SIL Graphite fonts are SFNT fonts.
Is the script check sufficient to cause fallback to Pango for these fonts do
you think?

Comment 16

[Karl Tomlinson (:karlt)]

Added this bug's patches to
https://hg.mozilla.org/users/ktomlinson_mozilla.com/bug597147/

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 479300 [details] [diff] [review]
part 4: gfxFT2FontBase::GetFontTable implementation

This looks ok, but I wonder whether it'd be worth bypassing Freetype altogether for table loading in the (usual) case where the font is a .ttf or .otf font on disk, and we have access to the file path (from fontconfig or gfxFT2FontList). We could then mmap() the font file and create blobs directly from the mapped file, without having to copy the table data at all.

I'm OK with leaving that as a potential future optimization, though. Just something to consider.

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 479295 [details] [diff] [review]
part 2: make it clearer that the blob owns the cache entry v1.2

I'm ok with the rearrangement you're doing here, but I think for better clarity you should rename the FontTableCacheEntry struct - as it is no longer the thing that actually gets entered in the cache. Something like CachedBlobUserData? (I don't like it much, but can't think of a better name at the moment.)

I also don't think the MakeBlob function really belongs as a member of this struct; it would more logically be something like gfxFontEntry::PrepareCachedBlob. But given that it's only a couple of lines of code, and only used in one place, how about just doing it in-line in gfxFontEntry::GetFontTable()? After all, it's not a general blob-making function; it's specific to the fontEntry caching usage.

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 479299 [details] [diff] [review]
part 3: provide public access to gfxFontEntry font table cache

This will need updating along with part 2.

I notice you made gfxFontEntry::GetCachedFontTable(), which sort-of-replaces gfxFontEntry::GetFontTable(aTag), non-virtual in this patch. That's ok at this point, but it leaves us without an obvious override point if we want to implement an mmap()-based table blob accessor at the font entry level. (We could do it in the gfxFont subclass, but that means we'd be repeatedly mmap()ing the font file for each instantiation, instead of once in the font entry.) So we may want to reconsider this in the future.

r=me, modulo FontTableCacheEntry changes from part 2.

Comment 20

[Jonathan Kew [:jfkthame]]

Comment on attachment 479301 [details] [diff] [review]
part 5: gfxFT2FontBase::GetHintedGlyphWidth implementation

Looks fine.

Actually, I wonder if we should implement a version of GetHintedGlyphWidth based on cairo_scaled_font_glyph_extents() in the base gfxFont class, in place of the existing GDI-specific version in gfxGDIFont. We may also want it for DirectWrite; we're currently using unhinted widths there but might give better results.

Depending on the performance of cairo_scaled_font_glyph_extents(), it might also be worth caching widths here, like gfxGDIFont::GetHintedGlyphWidth currently does.

But let's get this in the tree for a start; then we can look at possibly using it elsewhere, and do some profiling if performance is an issue.

Comment 21

[Karl Tomlinson (:karlt)]

(In reply to comment #17)
> This looks ok, but I wonder whether it'd be worth bypassing Freetype altogether
> for table loading in the (usual) case where the font is a .ttf or .otf font on
> disk, and we have access to the file path (from fontconfig or gfxFT2FontList).
> We could then mmap() the font file and create blobs directly from the mapped
> file, without having to copy the table data at all.

Yes.  I had been thinking mmap would make sense at least with 48-bit virtual address spaces.  I was wondering whether or not it would still be acceptable with 32-bit address spaces, but more space is the future anyway.

I'm guessing we could even create the cairo font faces from FT_Faces from mmapped fonts, if we did more FcPattern processing ourselves.  That would mean that FT would skip reading some of its preloaded tables.

I think if we're going to mmap, then  we should go the whole way and use mmap both for tables and glyph rasterizing/measuring (rather than opening the same file with another descriptor for conventional read in glyph operations).

Even now, for downloaded fonts, bypassing FreeType for table loading would allow us to create blobs directly from memory without copying.  If we are going to cache the blobs (I assume we should), that would require switching from nsTArray- to hb_blob-based gfxFontEntry::mFontTableCache interfaces.

Comment 22

[Karl Tomlinson (:karlt)]

(In reply to comment #18)
> Comment on attachment 479295 [details] [diff] [review]
> part 2: make it clearer that the blob owns the cache entry v1.2
> 
> I'm ok with the rearrangement you're doing here, but I think for better clarity
> you should rename the FontTableCacheEntry struct - as it is no longer the thing
> that actually gets entered in the cache. Something like CachedBlobUserData? (I
> don't like it much, but can't think of a better name at the moment.)

I thought of it still as a cache entry, just that it is referenced through the
blob; it only exists as long as its reference from the cache.  But it sounds
like it would be useful to also include in the table blobs with different
destroy functions, and your suggestions would help us move in that direction.

I could call it SharedTableBlobData.  That would make it clearer that nothing
is really cached, just reused if already in use.

> I also don't think the MakeBlob function really belongs as a member of this
> struct; it would more logically be something like
> gfxFontEntry::PrepareCachedBlob. But given that it's only a couple of lines of
> code, and only used in one place, how about just doing it in-line in
> gfxFontEntry::GetFontTable()? After all, it's not a general blob-making
> function; it's specific to the fontEntry caching usage.

If I rename gfxFontEntry::CacheFontTable in part 3 to CreateSharedTableBlob
then that would be a sensible place to do this.

I don't know how to make PreloadFontTable work with this though.
It actually wants more than sharing, i.e. keeping the data when not in use
(Bug 527276 comment 50).  Do you have any ideas what to do about that?

If the hash table actually kept a reference to the blob (so that it became a cache), I assume we'd use quite a bit more memory?  System font gfxFontEntries are forever on other platforms are they?

Comment 23

[Karl Tomlinson (:karlt)]

Attachment: part 6: use font group style to look for language and CSS features

Extending part 6 (see comment 12) now that bug 597147 part 8 has been removed and gfxHarfBuzzShaper still finds the language.

This also moves the language code out of the "if (!mHBFace)" block so that InitTextRun can be called on the same shaper object for FontGroups with different languages.

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to comment #22)
> I could call it SharedTableBlobData.  That would make it clearer that nothing
> is really cached, just reused if already in use.
....
> If I rename gfxFontEntry::CacheFontTable in part 3 to CreateSharedTableBlob
> then that would be a sensible place to do this.

Yes, sounds reasonable.

> I don't know how to make PreloadFontTable work with this though.
> It actually wants more than sharing, i.e. keeping the data when not in use
> (Bug 527276 comment 50).  Do you have any ideas what to do about that?
....
> If the hash table actually kept a reference to the blob (so that it became a
> cache), I assume we'd use quite a bit more memory?  System font gfxFontEntries
> are forever on other platforms are they?

Yes, they are (usually -- they may be destroyed and re-created if the system font configuration changes, so that we rebuild the platform font list).

Because blobs are refcounted and sharable, the idea is that they'll live in the fontEntry "cache" as long as any actual gfxFont instance is alive. Each font will "destroy" its blobs when finished with them, and when they're all gone, the blob's destroy function will actually release it and remove itself from the cache.

So we don't want the fontEntry hashtable to hold on to such blobs, because then they'd never get released. But in the case of PreloadFontTable, we do want the hashtable entries to persist as long as the fontEntry itself. Which means we need to take care of deleting them when the fontEntry dies (filed as bug 605872).

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to comment #13)
> Created attachment 479305 [details] [diff] [review]
> part 7: allow the font to provide its own GetGlyph function

(In reply to comment #14)
> Created attachment 479306 [details] [diff] [review]
> part 8: gfxFT2FontBase::GetGlyph implementation

The code here looks fine, but I'm curious whether we need it - are there specific reasons (either functionality or performance) that it's better to go via Freetype than to use the code we have in-tree that reads the cmap directly? In particular, using our own code would mean that variation selector support isn't dependent on the installed freetype version.

Comment 26

[Karl Tomlinson (:karlt)]

(In reply to comment #25)
> (In reply to comment #13)
> > part 7: allow the font to provide its own GetGlyph function
> 
> (In reply to comment #14)
> > part 8: gfxFT2FontBase::GetGlyph implementation
> 
> The code here looks fine, but I'm curious whether we need it - are there
> specific reasons (either functionality or performance) that it's better to go
> via Freetype than to use the code we have in-tree that reads the cmap directly?

Oh, sorry.  I did write a big explanation of why this was done, but it looks
like I forgot to paste it in.  Here's a rewrite.  There are two reasons, one
functionality and the other footprint (and so possibly performance).

gfxPangoFontGroup::FindFontForChar uses fontconfig's character-support-by-font
database to choose the font.  This is an efficient database that is shared
across processes through mmap.  It would be a shame to construct our own
copy of this database.  The fontconfig database is constructed using FcFreeTypeCharIndex, which is a little different from the in-tree code.

The algorithm used by FcFreeTypeCharIndex looks at the best Unicode cmap
subtable as well as the microsoft/symbol subtables and mac/roman subtables,
taking the union of these three (with appropriate conversion to Unicode).
AIUI the in-tree code looks at only the preferred Unicode subtable.

I'm not trying to suggest that FcFreeTypeCharIndex is better, but it is
important that FindFontForChar and GetGlyph are consistent.  For example, some
fonts use the mac/roman subtable for latin glyphs but a unicode subtable for
native glyphs.  If FindFontForChar said that such a font supported Latin
characters but GetGlyph did not find the glyph, then we'd end up with hex
boxes for the characters.

The other suspected advantage of using FreeType for GetGlyph comes from the
fact that FreeType preloads the cmap tables into memory when the font is
opened.  If the font is open, then using FreeType for GetGlyph would save
another copy.  (This would no longer be an advantage if/when we switch to
mmapping fonts.)  This is actually not necessarily always a win at the moment
as the font is not always open, and copying the table might save opening the
font, if there were no other reason (rasterizing glyphs, checking if it is
sfnt, etc.) to open the font.  gfxFT2Fonts.cpp always has the fonts open
though so this seems the right approach for that backend, at least.

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 479306 [details] [diff] [review]
part 8: gfxFT2FontBase::GetGlyph implementation

OK, let's do this, it seems like the safest way to go for now. Eventually, I think it would be interesting to compare performance of the alternative where we read the cmap directly (especially with mmap'ed fonts), but let's get the basic functionality in place first.

Comment 28

[Karl Tomlinson (:karlt)]

(In reply to comment #20)
> Comment on attachment 479301 [details] [diff] [review]
> part 5: gfxFT2FontBase::GetHintedGlyphWidth implementation

> Actually, I wonder if we should implement a version of GetHintedGlyphWidth
> based on cairo_scaled_font_glyph_extents() in the base gfxFont class, in place
> of the existing GDI-specific version in gfxGDIFont. We may also want it for
> DirectWrite; we're currently using unhinted widths there but might give better
> results.

Perhaps.  I'm not so familiar with other platforms to know whether those fonts always have a cairo font.

> Depending on the performance of cairo_scaled_font_glyph_extents(), it might
> also be worth caching widths here, like gfxGDIFont::GetHintedGlyphWidth
> currently does.

I had chosen not to do that because cairo_scaled_font already had a cache, and i didn't like the idea of caches on top of caches.

However, I had forgotten that PangoCairoFont caches the results of 
cairo_scaled_font_glyph_extents(), and given that we only use part of the data from the extents, it might make sense to have a cache with more (smaller) entries than cairo_scaled_font keeps.  Something to watch out for in profiles, anyway.

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 479289 [details] [diff] [review]
part 1: keep a reference from font table blob to font entry to ensure cache lives until removal

Part 1 and part 2 are obsoleted by attachment 486852 [details] [diff] [review]

Comment 30

[Karl Tomlinson (:karlt)]

Attachment: part 3: provide public access to gfxFontEntry font table hashtable v2

v1 was reviewed, but there have been changes with bug 527276 and bug 605872, as well as method name changes and reference counting is now a little more consistent with all callers that get blobs needing to blob_destroy, so requesting re-review.

Comment 31

[Karl Tomlinson (:karlt)]

Attachment: part 3: provide public access to gfxFontEntry font table hashtable v2.1

Updated for changes in bug 605872.

Comment 32

[Jonathan Kew [:jfkthame]]

Comment on attachment 479310 [details] [diff] [review]
part 9: use gfxHarfBuzzShaper for SFNT fonts

Could we go a bit further in restructuring the code here, either in this patch or as a separate patch in the queue? I'd like us to use the gfxFont implementation of InitTextRun(), so that we get the benefit of its handling of long textruns, rather than overriding that method completely.

To do this, I envisage moving the InitGlyphRun* methods over to a gfxPangoShaper class, and you'd have an override of CreatePlatformShaper() to instantiate this.

You might still want an InitTextRun() override in order to decide whether to use harfbuzz or not for the particular font, similar to gfxMacFont::InitTextRun() which favors Core Text shaping for AAT fonts, but all this needs to do is pass the appropriate flag to the base class implementation.

In looking at the functionality that would end up in gfxPangoShaper (and its helpers), I also notice that there's a (static) SetupClusterBoundaries() function being used. I think it should be possible to remove that now, as the code in gfxFontGroup::MakeTextRun() now sets cluster boundaries prior to calling the InitTextRun() methods of the specific fonts used.

Comment 33

[Jonathan Kew [:jfkthame]]

Comment on attachment 490033 [details] [diff] [review]
part 3: provide public access to gfxFontEntry font table hashtable v2.1

LGTM, let's do it!

Comment 34

[Karl Tomlinson (:karlt)]

(In reply to comment #15)
> I assume SIL Graphite fonts are SFNT fonts.
> Is the script check sufficient to cause fallback to Pango for these fonts do
> you think?

The script check is a blacklist and so not sufficient.
And, paraphrasing what jfkthame said on irc, it is not appropriate because we'd still prefer to use HarfBuzz for unusual scripts if the font does not require Graphite shaping.

The SIL Graphite Pango shaper choses to shape fonts when the "capability"
property of the fontconfig pattern includes ttable::Silf (which means the font
has an SILF table).  We could perform a similar check, which would be the simplest approach.

As jfkthame pointed out in irc discussion, the remaining question is what to
do with fonts that contain both Graphite and GPOS/GSUB tables.

The capability property also includes info on whether the font contains DFLT
or script-specific GPOS or GSUB tables.  We could use this to test for the
existence of a table for run script, using harfbuzz if one is found.

e.g. Charis SIL has capability "ttable:Silf  otlayout:cyrl otlayout:latn"

If the font had SILF as well as a DFLT GPOS or GSUB table, but no script-specific table, then I guess we'd choose Graphite.

Comment 35

[Behdad Esfahbod]

Every time I enabled the graphite shaper, either the pango one or the harfbuzz one, it crashed my firefox easily.  Deep in the graphite code.  I wouldn't want it on my machine.

Comment 36

[Karl Tomlinson (:karlt)]

Attachment: Graphite testcase

This testcase needs pango-graphite, Dai Banna SIL Book, and Tai Heritage Pro installed.
http://scripts.sil.org/DaiBannaSIL
http://scripts.sil.org/TaiHeritage

Dai Banna SIL Book has capability "ttable:Silf  otlayout:latn" and Tai Heritage Pro has only "ttable:Silf ".

The testcase detected bug 617203, which may lead to some crashes, though probably not crashes deep in the Graphite code.

Comment 37

[Karl Tomlinson (:karlt)]

Attachment: part 9: use gfxHarfBuzzShaper when suitable

This version of part 9 removes the InitGlyphRunFast code, which would only
have been used for non-OpenType fonts with 8-bit characters.  This allows the
shaper-choice logic to be simpler, and removes a code path that was of little
use.  gfxHarfBuzzShaper is therefore used even for non SFNT fonts.

The Graphite logic is as indicated in comment 34, preferring HarfBuzz if the
font looks like it has (regular) OpenType support for the script.

Comment 38

[Jonathan Kew [:jfkthame]]

Attachment: part 10 - refactor gfxFont::InitTextRun etc so that Linux code also benefits from splitting of long runs

This rearranges the code from gfxTextRun::InitTextRun so that the handling of huge strings (to split them into reasonable-length chunks for shaping) applies to all platforms; the virtual InitTextRun method, which may be overridden, will never be called with strings > 32K chars.

Also optimizes the 8-bit path by avoiding script-run itemization in this case, as 8-bit text must be purely Latin script.

(This will overlap slightly with the patch in bug 553981, so if that lands first it will need a minor refresh.)

Comment 39

[Karl Tomlinson (:karlt)]

Comment on attachment 496364 [details] [diff] [review]
part 10 - refactor gfxFont::InitTextRun etc so that Linux code also benefits from splitting of long runs

I like the SplitAndInitTextRun changes here.  It's even the same method name
that I was considering using.

But, I think we should do this in another bug, probably two other bugs.
Especially for changes at this late stage of the release cycle, drivers want
to be able to look back and pin-point which particular changes caused behavior
changes.  I don't see this patch as really related to using HarfBuzz on linux.
(It is more related to bug 597147, but we weren't doing any splitting at that
stage.)

I expect SplitAndInitTextRun makes Uniscribe::CopyItemSplitOversize
unnecessary (because MAX_SHAPING_LENGTH < 43680).

And I like the s/InitTextRun/InitScriptRun/ change.

> Also optimizes the 8-bit path by avoiding script-run itemization in this case,
> as 8-bit text must be purely Latin script.

The problem is that it might be a run of common characters for use with
another script.

I expect changing from HB_SCRIPT_COMMON to HB_SCRIPT_LATIN will mess up font
selection for common runs in non-Latin languages because
pango_language_includes_script will return false.  Possibly we could come up
with some different algorithm, but the UI font would end up looking
different from other Pango apps in some situations.  It's getting late in the
cycle to start playing with the font selection algorithm.

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to comment #39)
> Comment on attachment 496364 [details] [diff] [review]
> part 10 - refactor gfxFont::InitTextRun etc so that Linux code also benefits
> from splitting of long runs
> 
> I like the SplitAndInitTextRun changes here.  It's even the same method name
> that I was considering using.
> 
> But, I think we should do this in another bug, probably two other bugs.

Fair enough, let's do that.

> I expect SplitAndInitTextRun makes Uniscribe::CopyItemSplitOversize
> unnecessary (because MAX_SHAPING_LENGTH < 43680).

Yes, once we do this, I was expecting to file a followup on removing that.

> > Also optimizes the 8-bit path by avoiding script-run itemization in this case,
> > as 8-bit text must be purely Latin script.
> 
> The problem is that it might be a run of common characters for use with
> another script.
> 
> I expect changing from HB_SCRIPT_COMMON to HB_SCRIPT_LATIN will mess up font
> selection for common runs in non-Latin languages because
> pango_language_includes_script will return false.

Good point. Ok, we shouldn't do that part, or at least we'll need to look at it separately.

Comment 41

[Karl Tomlinson (:karlt)]

Pushed parts 3 to 9:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9ab76fbcb6d7&tochange=11e328a49e0a

Comment 42

[Jonathan Kew [:jfkthame]]

Comment on attachment 496364 [details] [diff] [review]
part 10 - refactor gfxFont::InitTextRun etc so that Linux code also benefits from splitting of long runs

Filed bug 617905 to deal with refactoring gfxFont::InitTextRun and avoid shaping massive strings (comment 38 and following).