[harfbuzz] use harfbuzz shaper under Linux

RESOLVED FIXED in mozilla2.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: jtd, Assigned: karlt)

Tracking

Trunk
mozilla2.0b8
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(8 attachments, 9 obsolete attachments)

4.83 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
2.78 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
6.75 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
5.31 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
7.07 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
8.92 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
537 bytes, text/html
Details
17.53 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
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.
Depends on: 449292
The current plan is to move all the complex shapers to HarfBuzz by around September time and make Pango solely use that.
OS: Mac OS X → Linux
blocking2.0: --- → ?
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.
blocking2.0: ? → beta5+
Starting on this.
Assignee: jfkthame → karlt
This is not going to make beta 5 tomorrow.
blocking2.0: beta5+ → betaN+
Depends on: 597147
Depends on: 600452
So that we don't need to be concerned about blobs getting destroyed before their font entries.
Attachment #479289 - Flags: review?(jfkthame)
Attachment #479289 - Attachment description: keep a reference from font table blob to font entry to ensure cache lives until removal r?jfkthame → part 1: keep a reference from font table blob to font entry to ensure cache lives until removal
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.
Attachment #479291 - Flags: review?(jfkthame)
GetBlob -> MakeBlob (and this version compiles)
Attachment #479291 - Attachment is obsolete: true
Attachment #479293 - Flags: review?(jfkthame)
Attachment #479291 - Flags: review?(jfkthame)
qref
Attachment #479293 - Attachment is obsolete: true
Attachment #479295 - Flags: review?(jfkthame)
Attachment #479293 - Flags: review?(jfkthame)
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.
Attachment #479299 - Flags: review?(jfkthame)
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.
Attachment #479302 - Flags: review?(jfkthame)
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?
Attachment #479310 - Flags: review?(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.
Attachment #479300 - Flags: review?(jfkthame) → review+
Attachment #479289 - Flags: review?(jfkthame) → review+
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.
Attachment #479295 - Flags: review?(jfkthame) → review-
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.
Attachment #479299 - Flags: review?(jfkthame) → review+
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.
Attachment #479301 - Flags: review?(jfkthame) → review+
(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.
(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?
Blocks: 605347
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.
Attachment #479302 - Attachment is obsolete: true
Attachment #484224 - Flags: review?(jfkthame)
Attachment #479302 - Flags: review?(jfkthame)
Depends on: 605872
(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).
(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.
Attachment #484224 - Flags: review?(jfkthame) → review+
(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.
Attachment #479305 - Flags: review?(jfkthame) → review+
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.
Attachment #479306 - Flags: review?(jfkthame) → review+
(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 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]
Attachment #479289 - Attachment is obsolete: true
Attachment #479295 - Attachment is obsolete: true
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.
Attachment #479299 - Attachment is obsolete: true
Attachment #486856 - Flags: review?(jfkthame)
Depends on: 608876
Depends on: 605043
Attachment #486856 - Flags: review?(jfkthame)
Attachment #486856 - Attachment is obsolete: true
Attachment #490033 - Attachment description: provide public access to gfxFontEntry font table hashtable v2.1 → part 3: provide public access to gfxFontEntry font table hashtable v2.1
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.
Blocks: 609649
Comment on attachment 490033 [details] [diff] [review]
part 3: provide public access to gfxFontEntry font table hashtable v2.1

LGTM, let's do it!
Attachment #490033 - Flags: review?(jfkthame) → review+
Depends on: 614468
Depends on: 482596
(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.
Attachment #479310 - Flags: review?(jfkthame)
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.
Depends on: 617231
Posted file 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.
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.
Attachment #479310 - Attachment is obsolete: true
Attachment #495741 - Flags: review?(jfkthame)
Attachment #495741 - Flags: review?(jfkthame) → review+
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.)
Attachment #496364 - Flags: review?(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.
Attachment #496364 - Flags: review?(karlt) → review-
No longer depends on: 482596
(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.
Pushed parts 3 to 9:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9ab76fbcb6d7&tochange=11e328a49e0a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
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).
Attachment #496364 - Attachment is obsolete: true
Blocks: 571518
Blocks: 569350
Blocks: 580962
No longer depends on: 618406
Depends on: 618406
Blocks: 464168
Blocks: 463413
Depends on: 717478
You need to log in before you can comment on or make changes to this bug.