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)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: heycam, Assigned: heycam)
References
Details
Attachments
(1 file, 1 obsolete file)
1.72 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Attachment #610462 -
Flags: review?(roc)
Assignee | ||
Updated•12 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•12 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•12 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•12 years ago
|
||
Sorry, dur brain. I see ligatureWidth is a PRInt32.
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #610786 -
Flags: review?(roc)
Assignee | ||
Updated•12 years ago
|
Attachment #610462 -
Attachment is obsolete: true
Attachment #610462 -
Flags: review?(roc)
Attachment #610786 -
Flags: review?(roc) → review+
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2571157f636
Comment 10•12 years ago
|
||
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.
Description
•