Closed Bug 740271 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Summary: imported patch ligature-width-rounding → adding all partial ligature advance measurements should result in the ligature glyph advance
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.
Attachment #610462 - Attachment is obsolete: true
Attachment #610462 - Flags: review?(roc)
Blocks: svgtext
https://hg.mozilla.org/mozilla-central/rev/e2571157f636
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: