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)
Core
Graphics: Text
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.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #697204 -
Flags: review?(roc)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #697205 -
Flags: review?(roc)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #697206 -
Flags: review?(roc)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #697207 -
Flags: review?(roc)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #697208 -
Flags: review?(roc)
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #697209 -
Flags: review?(roc)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #697210 -
Flags: review?(roc)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #697211 -
Flags: review?(roc)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #697212 -
Flags: review?(roc)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #697213 -
Flags: review?(roc)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #697214 -
Flags: review?(roc)
Assignee | ||
Comment 12•11 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•11 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•11 years ago
|
||
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•11 years ago
|
Attachment #697204 -
Attachment is obsolete: true
Attachment #697204 -
Flags: review?(roc)
Assignee | ||
Comment 16•11 years ago
|
||
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•11 years ago
|
Attachment #697399 -
Attachment is obsolete: true
Attachment #697399 -
Flags: review?(roc)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #697527 -
Flags: review?(roc)
Assignee | ||
Updated•11 years ago
|
Attachment #697205 -
Attachment is obsolete: true
Attachment #697205 -
Flags: review?(roc)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #697528 -
Flags: review?(roc)
Assignee | ||
Updated•11 years ago
|
Attachment #697206 -
Attachment is obsolete: true
Attachment #697206 -
Flags: review?(roc)
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #697529 -
Flags: review?(roc)
Assignee | ||
Updated•11 years ago
|
Attachment #697207 -
Attachment is obsolete: true
Attachment #697207 -
Flags: review?(roc)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #697530 -
Flags: review?(roc)
Assignee | ||
Updated•11 years ago
|
Attachment #697208 -
Attachment is obsolete: true
Attachment #697208 -
Flags: review?(roc)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #697531 -
Flags: review?(roc)
Assignee | ||
Updated•11 years ago
|
Attachment #697209 -
Attachment is obsolete: true
Attachment #697209 -
Flags: review?(roc)
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #697532 -
Flags: review?(roc)
Assignee | ||
Updated•11 years ago
|
Attachment #697210 -
Attachment is obsolete: true
Attachment #697210 -
Flags: review?(roc)
Assignee | ||
Comment 23•11 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).
Attachment #697526 -
Flags: review?(roc) → review+
Attachment #697527 -
Flags: review?(roc) → review+
Attachment #697528 -
Flags: review?(roc) → review+
Attachment #697529 -
Flags: review?(roc) → review+
Attachment #697530 -
Flags: review?(roc) → review+
Attachment #697531 -
Flags: review?(roc) → review+
Attachment #697532 -
Flags: review?(roc) → review+
Attachment #697211 -
Flags: review?(roc) → review+
Attachment #697212 -
Flags: review?(roc) → review+
Attachment #697213 -
Flags: review?(roc) → review+
Attachment #697214 -
Flags: review?(roc) → review+
Assignee | ||
Comment 24•11 years ago
|
||
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.
Assignee | ||
Comment 25•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/39b72947ad79
Target Milestone: --- → mozilla20
Comment 26•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/39b72947ad79
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•