Closed Bug 906646 Opened 6 years ago Closed 6 years ago

Glyphs with an SVG representation should never try to use the "contained glyph width" fast-path to stand in for real glyph extents

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: roc, Assigned: jfkthame)

References

Details

Attachments

(1 file, 2 obsolete files)

In gfxFont::Measure, there's a code path for simple glyphs where a mode that's not LOOSE_INK_EXTENTS is handled without actually getting the glyph extents. This seems incorrect and leads to visual bugs with SVG glyphs (and could also lead to visual bugs with other fonts).
Attached patch fix (obsolete) — Splinter Review
Attachment #792161 - Flags: review?(jfkthame)
Comment on attachment 792161 [details] [diff] [review]
fix

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

Sorry to be slow here - I've looked at this a few times, trying to get my head around the tangle of extents and measurement modes, and I'm still not totally convinced that this is what we want to do.

AFAICT, this patch would remove the possibility of gfxFont::Measure ever using the "contained glyph widths" concept from gfxGlyphMetrics, which is intended as an optimization for the (common) case of glyphs that do not exceed their nominal typographic bounding box in any direction. As a result, we'll find ourselves maintaining the (more expensive) full "tight extents" for all glyphs.

I suspect the real reason for visual glitches with SVG fonts is that gfxFont::SetupGlyphExtents relies on cairo_glyph_extents to determine whether a glyph is a "contained glyph". (See http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#3392 and following.) This is unaware of the SVG glyph data, so it may well mark a glyph as "contained" even though its SVG rendering does in fact overflow its nominal typographic bounds.

One option would be to scrap the "contained glyphs" optimization altogether, and simply work with tight extents. However, I'm concerned this could have a significant perf and memory impact. (Maybe we should try instrumenting this to confirm?) Alternatively, we should make SetupGlyphExtents more fully aware of SVG glyphs. And in the animation patches, NotifyGlyphsChanged will need to flush mContainedGlyphWidths as well as mTightGlyphExtents, since animation might cause a formerly-contained glyph to start projecting outside its nominal bounds.
Here's an alternative that I believe will fix the SVG-glyph glitches (at least it does in my testing), without sacrificing the ContainedGlyphWidths optimization for non-SVG glyphs. Because it treats all SVG glyphs as non-contained, rather than trying to actually check their bounds, it wouldn't require animation to flush the mContainedGlyphWidths records after all. WDYT?
Attachment #796352 - Flags: review?(roc)
Comment on attachment 796352 [details] [diff] [review]
glyphs with an SVG representation should never be considered "contained"

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

::: gfx/thebes/gfxFont.cpp
@@ +3528,5 @@
>      int32_t appUnitsPerDevUnit = aExtents->GetAppUnitsPerDevUnit();
>      if (!aNeedTight && extents.x_bearing >= 0 &&
>          extents.y_bearing >= -fontMetrics.maxAscent &&
> +        extents.height + extents.y_bearing <= fontMetrics.maxDescent &&
> +        (!mFontEntry->TryGetSVGData(this) || !mFontEntry->HasSVGGlyph(aGlyphID))) {

You're quite right. However, this makes TryGetSVGData/HasSVGGlyph unconditional in this method, so we may as well do them early and avoid calling them twice.

In fact I think if we have an SVG glyph we should just not call cairo_glyph_extents at all.
Makes sense - we can do the SVG check first, something like this.
Attachment #798223 - Flags: review?(roc)
Assignee: roc → jfkthame
Attachment #798223 - Flags: review?(roc) → review+
Attachment #796352 - Attachment is obsolete: true
Attachment #796352 - Flags: review?(roc)
Attachment #792161 - Attachment is obsolete: true
Attachment #792161 - Flags: review?(jfkthame)
Updating summary to better reflect the issue we actually fixed here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1250c160eaec
OS: Linux → All
Hardware: x86_64 → All
Summary: Text measurement modes other than LOOSE_INK_EXTENTS don't always measure precise extents → Glyphs with an SVG representation should never try to use the "contained glyph width" fast-path to stand in for real glyph extents
Target Milestone: --- → mozilla26
https://hg.mozilla.org/mozilla-central/rev/1250c160eaec
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.