831277 - round glyph positions appropriately in the graphite shaper

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

The graphite shaper doesn't handle the rounding of glyph positions (returned in floating-point by graphite) to appunits or device pixels properly, which results in minor positioning discrepancies between graphite and opentype/harfbuzz shaping even when results might be expected to match. (I think this is the reason for bug 700022 comment 7.) We should fix the rounding to give more consistent results.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: a. make GetRoundOffsetsToPixels a method on gfxContext, instead of private to the harfbuzz shaper

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: b. properly round glyph positions to appunits or pixels (as appropriate for the platform) in the graphite shaper

Comment 3

[John Daggett (:jtd)]

Can you post or link to a simple testcase?  Doesn't need to be a reftest, just something to evaluate the changes being made.  And this only affects WinXP by default, or possibly someone on Win7 if they've messed about with the ClearType params?

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #3)
> Can you post or link to a simple testcase?  Doesn't need to be a reftest,
> just something to evaluate the changes being made.

You can see the effect on OS X if you load
  http://people.mozilla.org/~jkew/charis-test.html
and then (using a separate window), toggle graphite rendering on and off. Note how the glyph positions shift slightly.

With the patch here, the positions stay unchanged when switching between opentype and graphite rendering.

Similarly, on Linux this eliminates the opentype/graphite discrepancy. The adjustments are subtle, but make a definite improvement; see attached screenshots.

>  And this only affects
> WinXP by default, or possibly someone on Win7 if they've messed about with
> the ClearType params?

On Windows, there's a different issue: opentype and graphite metrics/positions differ more significantly than on OS X or Linux, and this patch doesn't eliminate that discrepancy. That's because it turns out gfxGraphiteShaper hasn't actually hooked up the option to use hinted glyph advances from the font, so it's using unhinted widths, while harfbuzz uses hinted widths which on windows can be substantially different. I'll file this as a separate bug.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: graphite rendering on linux, current trunk (poor glyph positioning)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: graphite rendering on linux, patched (correct glyph positioning, matches harfbuzz rendering)

Comment 7

[John Daggett (:jtd)]

Comment on attachment 702800 [details] [diff] [review]
b. properly round glyph positions to appunits or pixels (as appropriate for the platform) in the graphite shaper

> -        uint32_t appAdvance = adv * dev2appUnits;
> +        uint32_t appAdvance = roundX ? NSToIntRound(adv) * dev2appUnits :
> +                                       NSToIntRound(adv * dev2appUnits);
> 
> -                d->mYOffset = -yLocs[j] * dev2appUnits;
> +                d->mYOffset = roundY ? NSToIntRound(-yLocs[j]) * dev2appUnits :
> +                              -yLocs[j] * dev2appUnits;
> 
> -                    d->mXOffset = dev2appUnits *
> -                        (rtl ? (xLocs[j] - clusterLoc) :
> -                               (xLocs[j] - clusterLoc - adv));
> +                    float dx = rtl ? (xLocs[j] - clusterLoc) :
> +                                     (xLocs[j] - clusterLoc - adv);
> +                    d->mXOffset = roundX ? NSToIntRound(dx) * dev2appUnits :
> +                                           dx * dev2appUnits;

Hmmm, why are you introducing the rounding in the *non-rounding* case
(i.e. NSToIntRound(adv * dev2appUnits)) and only for the base
character xAdvance and not for the yAdvance?  That seems like an odd
change.

Comment 8

[John Daggett (:jtd)]

Comment on attachment 702799 [details] [diff] [review]
a. make GetRoundOffsetsToPixels a method on gfxContext, instead of private to the harfbuzz shaper

> +#ifdef XP_WIN
> +#include "gfxWindowsPlatform.h"
> +#endif

Minor nit but this should probably be conditionalized on
CAIRO_HAS_DWRITE_FONT instead of XP_WIN.

r+ with that fixed.

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #7)
> Comment on attachment 702800 [details] [diff] [review]
> b. properly round glyph positions to appunits or pixels (as appropriate for
> the platform) in the graphite shaper
> 
> > -        uint32_t appAdvance = adv * dev2appUnits;
> > +        uint32_t appAdvance = roundX ? NSToIntRound(adv) * dev2appUnits :
> > +                                       NSToIntRound(adv * dev2appUnits);
> > 
> > -                d->mYOffset = -yLocs[j] * dev2appUnits;
> > +                d->mYOffset = roundY ? NSToIntRound(-yLocs[j]) * dev2appUnits :
> > +                              -yLocs[j] * dev2appUnits;
> > 
> > -                    d->mXOffset = dev2appUnits *
> > -                        (rtl ? (xLocs[j] - clusterLoc) :
> > -                               (xLocs[j] - clusterLoc - adv));
> > +                    float dx = rtl ? (xLocs[j] - clusterLoc) :
> > +                                     (xLocs[j] - clusterLoc - adv);
> > +                    d->mXOffset = roundX ? NSToIntRound(dx) * dev2appUnits :
> > +                                           dx * dev2appUnits;
> 
> Hmmm, why are you introducing the rounding in the *non-rounding* case
> (i.e. NSToIntRound(adv * dev2appUnits)) 

Even when we're not rounding positions to whole pixels, we're converting the floating-point advance from graphite to an integer number of appUnits. It's better to round than truncate for that conversion. This fixes the (slight) discrepancy in positioning that you'll see if you toggle graphite on/off on OS X with the Charis sample mentioned above.

>and only for the base
> character xAdvance and not for the yAdvance?  That seems like an odd
> change.

There's no yAdvance, I assume you mean the offsets. The x- and y-offsets are stored as floating-point fields in the DetailedGlyph record, whereas the (x-)advance is an integer. So we can retain floating-point precision for the offsets (if any), but the advance is always quantized to appUnits, even if we aren't doing pixel-snapping.

Comment 10

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d53b9bc5be3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd882f775915

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/d53b9bc5be3e
https://hg.mozilla.org/mozilla-central/rev/cd882f775915