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

RESOLVED FIXED in mozilla14

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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)

Comment 1

5 years ago
Created attachment 610462 [details] [diff] [review]
Allocate partial ligature advance rounding error to the last part
Attachment #610462 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Sorry, dur brain.  I see ligatureWidth is a PRInt32.
(Assignee)

Comment 8

5 years ago
Created attachment 610786 [details] [diff] [review]
Allocate partial ligature advance rounding error to the last part. (v1.1)
Attachment #610786 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #610462 - Attachment is obsolete: true
Attachment #610462 - Flags: review?(roc)
Attachment #610786 - Flags: review?(roc) → review+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2571157f636
(Assignee)

Updated

5 years ago
Blocks: 655877
https://hg.mozilla.org/mozilla-central/rev/e2571157f636
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.