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.

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

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.

(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.

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

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.

(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.

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

Created attachment 610786 [details] [diff] [review] Allocate partial ligature advance rounding error to the last part. (v1.1)

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

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