1352528 - can we cache the results of gfxFontShaper::GetRoundOffsetsToPixels

Status: RESOLVED FIXED

(Core :: Graphics: Text, enhancement)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In the profile in bug 1329601 comment 13 I noticed this stack to call free:

free
gfxFontShaper::GetRoundOffsetsToPixels(mozilla::gfx::DrawTarget*, bool*, bool*)
gfxHarfBuzzShaper::SetGlyphsFromRun(mozilla::gfx::DrawTarget*, gfxShapedText*, unsigned int, unsigned int, char16_t const*, hb_buffer_t*, bool)
gfxHarfBuzzShaper::ShapeText(mozilla::gfx::DrawTarget*, char16_t const*, unsigned int, unsigned int, mozilla::unicode::Script, bool, gfxShapedText*)


Can we cache the result of GetRoundOffsetsToPixels so we don't have to call this as often?

Comment 1

[Jonathan Kew [:jfkthame]]

Hmm.... we can't readily cache this within the shaper, or within the gfxFont instance, because the answers to GetRoundOffsetsToPixels() depend on the drawTarget that we're aiming for, and a single font instance may be used with multiple drawTargets.

We can't work around that by just hanging the cached values off of the drawTarget (e.g. in its userData), because changes to the DT's transform could invalidate them. So I think we'd have to push the implementation of GetRoundOffsetsToPixels() itself down into the drawTarget, and make DT::SetTransform (and possibly other methods?) aware of the caching so that they can invalidate the cached values. This is all starting to sound too invasive...

An alternative, though: I guess in many cases we end up calling ShapeText() numerous times during construction of a single textrun, and the values will be constant across those calls. So we could hoist the call to GetRoundOffsetsToPixels() up to, say, the beginning of gfxFontGroup::InitTextRun(), and then pass the resulting booleans through to all the ShapeText() calls. The downside of that, though, is that it means we'd call GetRoundOffsetsToPixels() (once) for every InitTextRun(), whereas currently we don't call it at all if it turns out that the textrun can be initialized entirely from the shaped-word caches and so it doesn't call ShapeText() at all.

That prompts me to suspect, though, that we actually have a correctness bug here: AFAICS, it's possible that we'll cache shapedWord records that were initialized according to one set of GetRoundOffsetsToPixels() results, but then end up using them in a context where GetRoundOffsetsToPixels() would have returned a different answer. To avoid this, we should be incorporating the rounding flags into the shaped-word cache keys.

I'll see if I can put together a testcase that confirms whether we do have such a bug; and if so, the "alternative" above will also serve as a basis for fixing it.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Hoist call to GetRoundOffsetsToPixels out of the inner loop of text shaping

To avoid calling GetRoundOffsetsToPixels repeatedly in the inner loop of text shaping (e.g. for each word of a long text block), this patch hoists it up so we call it at the beginning of gfxFont::SplitAndInitTextRun, and then pass the resulting values down to where we shape the individual words.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8855013 [details] [diff] [review]
Hoist call to GetRoundOffsetsToPixels out of the inner loop of text shaping

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

Seems like a good idea to me.

Comment 4

[(no longer active)]

Is this ready to land?

Comment 5

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf453f66c9c384768d7a8d92f129c1328886096
Bug 1352528 - Hoist call to GetRoundOffsetsToPixels out of the inner loop of text shaping. r=jrmuizel

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fdf453f66c9c