refactor gfxTextRun and gfxShapedWord to have a common base interface for font shapers to use

RESOLVED FIXED in mozilla20

Status

()

Core
Graphics: Text
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 8 obsolete attachments)

2.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.11 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.47 KB, patch
roc
: review+
Details | Diff | Splinter Review
20.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
75.40 KB, patch
roc
: review+
Details | Diff | Splinter Review
16.64 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.72 KB, patch
roc
: review+
Details | Diff | Splinter Review
12.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
183.25 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Currently, font shapers (i.e., subclasses of gfxFontShaper, of which we have about half a dozen for the various platform and in-tree shaping backends) are designed to record the shaped glyph data into a gfxShapedWord, which is cached by the associated font; the glyph data is copied into textruns as needed from these cached words.

This implies that *all* text shaping needed for text-run construction goes through shaped-word objects. In general, that's good: it allows fonts to cache the results of shaping, and thus improve performance when the same word (in the same font) occurs many times.

However, there are a couple of cases where it is suboptimal, and it would be better if the font shapers could write the shaped glyph data directly into a text-run. The cases I have in mind are (a) bug 761442, where per-word shaping breaks contextual lookups that involve the inter-word spaces -- to fix this, we need to shape the run as a whole instead of the individual words in isolation; and (b) as word length increases, the usefulness of the font word caches decreases -- long "words" (which are often not real words but things like URLs, etc) are unlikely to be frequently reused, so it's probably not worth the memory cost of caching them at all.

So my plan here is to refactor gfxTextRun and gfxShapedWord to have a common (abstract) base class, which I'm calling gfxShapedText. This will provide an interface to the stuff that is common to both text-runs and individual words, and is needed for the actual shaping process: mainly, the array of CompressedGlyph records and the associated DetailedGlyph records (where needed). The font shapers will then use this interface to store their shaped output.

This will allow us to fix bug 761442 by bypassing the per-word shaping code and caches entirely when the font has features that depend on space, and will also allow us to shape long "words" directly into the target run, so we don't bloat the caches with items that are unlikely to be reused.
(Assignee)

Updated

6 years ago
Blocks: 825875
(Assignee)

Comment 1

6 years ago
Created attachment 697204 [details] [diff] [review]
(a) introduce gfxShapedText abstract base for gfxTextRun and gfxShapedWord
Attachment #697204 - Flags: review?(roc)
(Assignee)

Comment 2

6 years ago
Created attachment 697205 [details] [diff] [review]
(b) adapt gfxHarfBuzzShaper to work with gfxShapedText
Attachment #697205 - Flags: review?(roc)
(Assignee)

Comment 3

6 years ago
Created attachment 697206 [details] [diff] [review]
(c) adapt gfxGraphiteShaper to work with gfxShapedText
Attachment #697206 - Flags: review?(roc)
(Assignee)

Comment 4

6 years ago
Created attachment 697207 [details] [diff] [review]
(d) adapt gfxCoreTextShaper to work with gfxShapedText
Attachment #697207 - Flags: review?(roc)
(Assignee)

Comment 5

6 years ago
Created attachment 697208 [details] [diff] [review]
(e) adapt gfxGDIShaper to work with gfxShapedText
Attachment #697208 - Flags: review?(roc)
(Assignee)

Comment 6

6 years ago
Created attachment 697209 [details] [diff] [review]
(f) adapt gfxUniscribeShaper to work with gfxShapedText
Attachment #697209 - Flags: review?(roc)
(Assignee)

Comment 7

6 years ago
Created attachment 697210 [details] [diff] [review]
(g) adapt gfxDWriteShaper to work with gfxShapedText
Attachment #697210 - Flags: review?(roc)
(Assignee)

Comment 8

6 years ago
Created attachment 697211 [details] [diff] [review]
(h) adapt gfxMacFont to work with gfxShapedText
Attachment #697211 - Flags: review?(roc)
(Assignee)

Comment 9

6 years ago
Created attachment 697212 [details] [diff] [review]
(i) adapt gfxGDIFont to work with gfxShapedText
Attachment #697212 - Flags: review?(roc)
(Assignee)

Comment 10

6 years ago
Created attachment 697213 [details] [diff] [review]
(j) adapt gfxFT2Fonts to work with gfxShapedText
Attachment #697213 - Flags: review?(roc)
(Assignee)

Comment 11

6 years ago
Created attachment 697214 [details] [diff] [review]
(k) adapt gfxPangoFonts to work with gfxShapedText
Attachment #697214 - Flags: review?(roc)
(Assignee)

Comment 12

6 years ago
This set of patches introduce the gfxShapedText interface as a base for gfxShapedWord and gfxTextRun, and adapt all the shaping code (subclasses of gfxFontShaper that implement shaping, and subclasses of gfxFont that call shapers) accordingly. Although it looks like a lot of patch, it mostly consists of moving existing code into the new base class, and then semi-mechanically transforming all implementers and callers. (The whole set of patches will clearly need to be folded together for landing, as the tree won't build in an intermediate state with mismatched font-shaper interfaces.)
Comment on attachment 697204 [details] [diff] [review]
(a) introduce gfxShapedText abstract base for gfxTextRun and gfxShapedWord

Review of attachment 697204 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +2439,5 @@
>  
>  bool
> +gfxFont::ShapeText(gfxContext    *aContext,
> +                   const uint8_t *aText,
> +                   uint32_t       aOffset,

Why are we passing aText and aOffset here? Couldn't we just add aText and aOffset in the caller?

@@ +2517,5 @@
> +        } else if (i > 0 &&
> +                   NS_IS_HIGH_SURROGATE(aText[i - 1]) &&
> +                   NS_IS_LOW_SURROGATE(aText[i])) {
> +            aShapedText->SetIsLowSurrogate(aOffset + i);
> +        }

Having to make another pass over the text here isn't so great. Why can't we do it on the fly?
(Assignee)

Comment 14

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> >  bool
> > +gfxFont::ShapeText(gfxContext    *aContext,
> > +                   const uint8_t *aText,
> > +                   uint32_t       aOffset,
> 
> Why are we passing aText and aOffset here? Couldn't we just add aText and
> aOffset in the caller?

No, we need aOffset to tell us the position within the destination shaped-text to store the glyphs -- i.e., aOffset and aLength tell us the range of the destination that we're writing to. In the case of a shaped-word, they'll always be 0 and the word length, but in the case of a text-run we may be shaping only a section of it.

> Having to make another pass over the text here isn't so great. Why can't we
> do it on the fly?

Hmm... that's how gfxFont::GetShapedWord did it, I just moved the code to its new home. But maybe we could do this as part of SetupClusterBoundaries instead, provided we ensure the shapers will preserve any flags this has set in the CompressedGlyph records. I'll have a look at that possibility.
(Assignee)

Comment 15

5 years ago
Created attachment 697399 [details] [diff] [review]
(a) introduce gfxShapedText abstract base for gfxTextRun and gfxShapedWord

OK, this integrates the marking of low-surrogates and spaces into SetupClusterBoundaries, as we're already making a pass over the text there. I've pushed it to tryserver to check that nothing breaks as a result... https://tbpl.mozilla.org/?tree=Try&rev=55819449dd1c.
Attachment #697399 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #697204 - Attachment is obsolete: true
Attachment #697204 - Flags: review?(roc)
(Assignee)

Comment 16

5 years ago
Created attachment 697526 [details] [diff] [review]
(a) introduce gfxShapedText abstract base for gfxTextRun and gfxShapedWord

Well, that worked except that some shapers overwrite the IsLowSurrogate flag. Here's a minor tidy-up of the first patch; the following updates will fix the surrogate issues in the shapers.
Attachment #697526 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #697399 - Attachment is obsolete: true
Attachment #697399 - Flags: review?(roc)
(Assignee)

Comment 17

5 years ago
Created attachment 697527 [details] [diff] [review]
(b) adapt gfxHarfBuzzShaper to work with gfxShapedText
Attachment #697527 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #697205 - Attachment is obsolete: true
Attachment #697205 - Flags: review?(roc)
(Assignee)

Comment 18

5 years ago
Created attachment 697528 [details] [diff] [review]
(c) adapt gfxGraphiteShaper to work with gfxShapedText
Attachment #697528 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #697206 - Attachment is obsolete: true
Attachment #697206 - Flags: review?(roc)
(Assignee)

Comment 19

5 years ago
Created attachment 697529 [details] [diff] [review]
(d) adapt gfxCoreTextShaper to work with gfxShapedText
Attachment #697529 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #697207 - Attachment is obsolete: true
Attachment #697207 - Flags: review?(roc)
(Assignee)

Comment 20

5 years ago
Created attachment 697530 [details] [diff] [review]
(e) adapt gfxGDIShaper to work with gfxShapedText
Attachment #697530 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #697208 - Attachment is obsolete: true
Attachment #697208 - Flags: review?(roc)
(Assignee)

Comment 21

5 years ago
Created attachment 697531 [details] [diff] [review]
(f) adapt gfxUniscribeShaper to work with gfxShapedText
Attachment #697531 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #697209 - Attachment is obsolete: true
Attachment #697209 - Flags: review?(roc)
(Assignee)

Comment 22

5 years ago
Created attachment 697532 [details] [diff] [review]
(g) adapt gfxDWriteShaper to work with gfxShapedText
Attachment #697532 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #697210 - Attachment is obsolete: true
Attachment #697210 - Flags: review?(roc)
(Assignee)

Comment 23

5 years ago
Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=fa2b9089e091 (the "surrogate-fixes" changeset there has now been folded into the updated patches (a) through (g) above).
(Assignee)

Comment 24

5 years ago
Created attachment 697981 [details] [diff] [review]
refactor gfxTextRun and gfxShapedWord to share a common abstract base class (gfxShapedText) and shaping interface.

Roll-up of parts (a) through (k) all folded into one patch, with a little extra cleanup, ready for landing. Carrying forward r=roc from the individual pieces above.
https://hg.mozilla.org/mozilla-central/rev/39b72947ad79
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.