Last Comment Bug 740271 - adding all partial ligature advance measurements should result in the ligature glyph advance
: adding all partial ligature advance measurements should result in the ligatur...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Cameron McCormack (:heycam)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: svgtext
  Show dependency treegraph
 
Reported: 2012-03-28 23:53 PDT by Cameron McCormack (:heycam)
Modified: 2012-03-30 13:01 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allocate partial ligature advance rounding error to the last part (1.35 KB, patch)
2012-03-28 23:53 PDT, Cameron McCormack (:heycam)
no flags Details | Diff | Splinter Review
Allocate partial ligature advance rounding error to the last part. (v1.1) (1.72 KB, patch)
2012-03-29 18:35 PDT, Cameron McCormack (:heycam)
roc: review+
Details | Diff | Splinter Review

Description Cameron McCormack (:heycam) 2012-03-28 23:53:47 PDT
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) 2012-03-28 23:53:57 PDT
Created attachment 610462 [details] [diff] [review]
Allocate partial ligature advance rounding error to the last part
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-29 00:44:39 PDT
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) 2012-03-29 18:06:55 PDT
(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) 2012-03-29 18:13:11 PDT
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) 2012-03-29 18:16:05 PDT
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) 2012-03-29 18:19:06 PDT
(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) 2012-03-29 18:26:23 PDT
Sorry, dur brain.  I see ligatureWidth is a PRInt32.
Comment 8 Cameron McCormack (:heycam) 2012-03-29 18:35:37 PDT
Created attachment 610786 [details] [diff] [review]
Allocate partial ligature advance rounding error to the last part. (v1.1)
Comment 9 Cameron McCormack (:heycam) 2012-03-29 19:38:32 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2571157f636
Comment 10 Ed Morley [:emorley] 2012-03-30 13:01:17 PDT
https://hg.mozilla.org/mozilla-central/rev/e2571157f636

Note You need to log in before you can comment on or make changes to this bug.