1412355 - Revise the storage of glyph offsets in DetailedGlyph records so as to avoid x/y swapping in the inner loop of gfxFont::DrawGlyphs

Status: RESOLVED FIXED

(Core :: Graphics: Text, enhancement)

Description

[Jonathan Kew [:jfkthame]]

As per bug 1408612 comment 18 and 21.

When shaping in vertical mode, we currently store glyph positioning data such that the mXOffset/mYOffset fields are being (ab)used such that the "x" offset is really an offset along the baseline of the text, and the "y" offset is perpendicular to this. During rendering, we end up having to check for vertical mode and swap the interpretations to get the physical offsets we need.

If we do an equivalent of this swap during shaping instead, this if-test can be removed from the DrawGlyphs inner loop. This should (marginally) improve text-drawing performance, given that we're likely to shape a run once but then paint it multiple times.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Replace the mXOffset/mYOffset fields in DetailedGlyph records with a gfx::Point that stores glyph offsets in line-orientation-relative coordinates

Tryserver says this passes reftests successfully (including the newly-added test from bug 1395926):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a5e580a70bcfbdbbcb4d61ad9578058c16c61f3

(The android build failure there has been fixed.)

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4905048de8d1
Replace the mXOffset/mYOffset fields in DetailedGlyph records with a gfx::Point that stores glyph offsets in line-orientation-relative coordinates. r=jrmuizel

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequently failing reftest layout/reftests/writing-mode/1248248-1-orientation-break-glyphrun.html on Linux x64 debug and asan:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0eb1f08953b31362483a415465d2964a67a5f0c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4905048de8d11e8bec261886d68be9c3f99d312b&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=140443531&repo=mozilla-inbound
Seems to be a different cropping at the bottom.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #3)
> Backed out for frequently failing reftest
> layout/reftests/writing-mode/1248248-1-orientation-break-glyphrun.html on
> Linux x64 debug and asan:

Interesting.... I'm not really sure why this is intermittent and only seemed to affect certain platforms. I guess it must be timing-dependent, related to exactly how dirty-rects and repaint operations work out. The root of the issue is that the test font we have in layout/reftest/fonts/gw1270797.ttf has incorrect 'vmtx' (vertical glyph metrics) values, which results in the glyphs getting painted much lower than they should -- largely outside their intended rects. Depending exactly how the textrun bounds and resulting invalidation areas get handled, they may end up mostly clipped.

The straightforward fix is to correct the 'vmtx' table topSideBearing values, such that the glyphs render in the proper positions relative to the font's em-square, and the (unreliable) clipping won't happen.

(Incidentally, I also noticed a bug in the patch here -- I failed to remove the obsolete "if (orientation == eVertical)..." branch from the IsSimpleGlyph() code path in gfxFont::Measure. So I'll clean that up before re-landing, although in practice it was unlikely to matter because I believe most fonts will never hit the IsSimpleGlyph() path when shaped in vertical mode, because of how vertical-origin offsets work.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4558b0224f93
Replace the mXOffset/mYOffset fields in DetailedGlyph records with a gfx::Point that stores glyph offsets in line-orientation-relative coordinates. r=jrmuizel

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4558b0224f93

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/4558b0224f93