Closed Bug 825871 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Graphics: Text, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(12 files, 8 obsolete files)

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
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.
Blocks: 825875
Attachment #697205 - Flags: review?(roc)
Attachment #697206 - Flags: review?(roc)
Attachment #697207 - Flags: review?(roc)
Attachment #697208 - Flags: review?(roc)
Attachment #697210 - Flags: review?(roc)
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?
(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.
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)
Attachment #697204 - Attachment is obsolete: true
Attachment #697204 - Flags: review?(roc)
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)
Attachment #697399 - Attachment is obsolete: true
Attachment #697399 - Flags: review?(roc)
Attachment #697205 - Attachment is obsolete: true
Attachment #697205 - Flags: review?(roc)
Attachment #697206 - Attachment is obsolete: true
Attachment #697206 - Flags: review?(roc)
Attachment #697207 - Attachment is obsolete: true
Attachment #697207 - Flags: review?(roc)
Attachment #697208 - Attachment is obsolete: true
Attachment #697208 - Flags: review?(roc)
Attachment #697209 - Attachment is obsolete: true
Attachment #697209 - Flags: review?(roc)
Attachment #697210 - Attachment is obsolete: true
Attachment #697210 - Flags: review?(roc)
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).
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
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 832440
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: