740271 - adding all partial ligature advance measurements should result in the ligature glyph advance

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Cameron McCormack (:heycam)]

Measuring partial ligatures and summing all the parts can result in a value
less than the advance of the ligature glyph, due to rounding.  We should
allocate the remaining width to one of the ligature parts.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Allocate partial ligature advance rounding error to the last part

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 610462 [details] [diff] [review]
Allocate partial ligature advance rounding error to the last part

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

::: gfx/thebes/gfxFont.cpp
@@ +4279,5 @@
> +    // so that measuring all parts of a ligature and summing them is equal to
> +    // the ligature width.
> +    if (aPartEnd == result.mLigatureEnd) {
> +        result.mPartWidth += ligatureWidth % totalClusterCount;
> +    }

Can you prove that this makes the sum of the mPartWidths add up to ligatureWidth? I can't.

It would be more clear if each mPartWidth was computed as partClusterCount*(ligatureWidth/totalClusterCount), mPartAdvance was computed as partClusterIndex*(ligatureWidth/totalClusterCount), and you added ligatureWidth - totalClusterCount*(ligatureWidth/totalClusterCount) to the final mPartWidth.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Can you prove that this makes the sum of the mPartWidths add up to
> ligatureWidth? I can't.

No, but I can't show it sometimes can't!  If for example

  ligatureWidth = 10
  partClusterCount = 3
  totalClusterCount = 6

then if for whatever reason you measured the first three parts together, the the second three parts, and added them up, you would get 10.  But ligatureWidth % totalClusterCount would give you 4.  So that's clearly wrong.

> It would be more clear if each mPartWidth was computed as
> partClusterCount*(ligatureWidth/totalClusterCount), mPartAdvance was
> computed as partClusterIndex*(ligatureWidth/totalClusterCount), and you
> added ligatureWidth - totalClusterCount*(ligatureWidth/totalClusterCount) to
> the final mPartWidth.

Yes that's simpler.  It does mean that more "error" accumulates, but I don't know if it matters much.  For example if

  ligatureWidth = 99
  totalClusterCount = 10

then you'd have the first 9 clusters being given width 9 and the last cluster being given width 18.

Comment 4

[Cameron McCormack (:heycam)]

Actually I didn't realise these values were gfxFloats -- I thought they were nscoords.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It won't matter, because these are all appunits anyway. The maximum error is < totalClusterCount appunits. totalClusterCount is the number of cluster starts in a ligature, so I doubt we'd ever see more than 1/20 of a CSS pixel of error.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Cameron McCormack (:heycam) from comment #4)
> Actually I didn't realise these values were gfxFloats -- I thought they were
> nscoords.

They are, but they must have no fractional part. We keep them in float format to reduce int-to-float conversions later. The divisions happen on ints.

Comment 7

[Cameron McCormack (:heycam)]

Sorry, dur brain.  I see ligatureWidth is a PRInt32.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Allocate partial ligature advance rounding error to the last part. (v1.1)

Comment 9

[Cameron McCormack (:heycam)]

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

Comment 10

[Ed Morley [:emorley]]

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