Last Comment Bug 716402 - first in a row of identical letters positioned 1 px too far apart
: first in a row of identical letters positioned 1 px too far apart
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla12
Assigned To: Karl Tomlinson (:karlt)
:
Mentors:
Depends on:
Blocks: 569770 605043
  Show dependency treegraph
 
Reported: 2012-01-08 12:24 PST by Stefan
Modified: 2012-01-27 09:13 PST (History)
5 users (show)
karlt: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (88 bytes, text/html)
2012-01-08 12:24 PST, Stefan
no flags Details
measurement (17.62 KB, image/jpeg)
2012-01-08 12:25 PST, Stefan
no flags Details
screenshot last good nightly (pass) (21.21 KB, image/jpeg)
2012-01-08 13:25 PST, Stefan
no flags Details
screenshot first bad nightly (fail) (21.25 KB, image/jpeg)
2012-01-08 13:26 PST, Stefan
no flags Details
revised Testcase (74 bytes, text/html)
2012-01-08 14:12 PST, Stefan
no flags Details
screenshot last good nightly (pass) (17.87 KB, image/jpeg)
2012-01-08 14:13 PST, Stefan
no flags Details
screenshot first bad nightly (fail) (17.96 KB, image/jpeg)
2012-01-08 14:13 PST, Stefan
no flags Details
improve inter-glyph pixel rounding (6.52 KB, patch)
2012-01-11 19:09 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
improve inter-glyph pixel rounding v1.1 (6.73 KB, patch)
2012-01-12 17:24 PST, Karl Tomlinson (:karlt)
jfkthame: review+
Details | Diff | Splinter Review
screenshot patch 1.1 and kerning (P-A in "SOPA" and "PIPA") (99.86 KB, image/png)
2012-01-19 12:24 PST, Stefan
no flags Details

Description Stefan 2012-01-08 12:24:13 PST
Created attachment 586822 [details]
Testcase

User Agent:  

Steps to reproduce:

1. Open the testcase (with default setting gfx.font_rendering.harfbuzz.scripts;3).


Actual results:

1. The gap between the first and the second '1' is one 1 px wider than the gaps of any other two consecutive 1's.


Expected results:

1. Gaps have same widths.
Comment 1 Stefan 2012-01-08 12:25:56 PST
Created attachment 586824 [details]
measurement
Comment 2 Bill Gianopoulos [:WG9s] 2012-01-08 12:27:40 PST
I am unable to reproduce an issue with current mozilla-central code under Linux.  Will try Windows also, but it seems this is an issue that has already been fixed and the fix will be released as soon as that code gets into the released product.
Comment 3 Bill Gianopoulos [:WG9s] 2012-01-08 12:28:43 PST
So this should be working at least by Firefox version 12 if not before.  I will try to track down the fix closer that that.
Comment 4 Bill Gianopoulos [:WG9s] 2012-01-08 12:30:18 PST
Hmm... but I see this is filed on trunk so there is something here I don't understand.  I can not duplicate this issue on current trunk under Linux.
Comment 5 Bill Gianopoulos [:WG9s] 2012-01-08 12:31:18 PST
Could you try this either in safe mode or with all add-ons disabled?
Comment 6 Stefan 2012-01-08 12:32:30 PST
Produced with 2012-01-08-03-10-24-mozilla-central cf8c9f9aeefc.
Comment 7 Stefan 2012-01-08 12:35:39 PST
(In reply to Bill Gianopoulos [:WG9s] from comment #2)
> I am unable to reproduce an issue with current mozilla-central code under
> Linux.

Can you prepare a screenshot?
Comment 8 Stefan 2012-01-08 12:38:39 PST
Can reproduce with current m-c tip revision 9a230265bad5.
Comment 9 Stefan 2012-01-08 12:43:31 PST
(In reply to Bill Gianopoulos [:WG9s] from comment #5)
> Could you try this either in safe mode or with all add-ons disabled?

Have no add-ons, tried also in safe-mode. The settings in~/.fonts.conf do not change the first gap width.
Comment 10 Bill Gianopoulos [:WG9s] 2012-01-08 12:50:07 PST
(In reply to Stefan from comment #7)
> (In reply to Bill Gianopoulos [:WG9s] from comment #2)
> > I am unable to reproduce an issue with current mozilla-central code under
> > Linux.
> 
> Can you prepare a screenshot?
'
Actually, you need to produce screen shots for what you consider a pass and fail for running your testcase.
Comment 11 Stefan 2012-01-08 13:25:00 PST
Created attachment 586831 [details]
screenshot last good nightly (pass)
Comment 12 Stefan 2012-01-08 13:26:12 PST
Created attachment 586832 [details]
screenshot first bad nightly (fail)
Comment 13 Stefan 2012-01-08 13:26:27 PST
last good nightly: 2010-12-08-03-mozilla-central 95452499f3d6
first bad nightly: 2010-12-09-03-mozilla-central 11e328a49e0a
Comment 14 Bill Gianopoulos [:WG9s] 2012-01-08 13:29:36 PST
Oh great now i need you to explain exactly what is the difference between the pass and fail screenshots because they look the same to me.
Comment 15 Stefan 2012-01-08 13:48:12 PST
(In reply to Bill Gianopoulos [:WG9s] from comment #14)
> they look the same to me.

1. Open the screenshots "pass" and "fail" in adjacent tabs and switch between them back and forth. The 1's number 2-9 seem to shift left/right.

2. Open the "measurement" screenshot. There a multiple red rectangles having all the same width. Every rectangle is left aligned with the pixel right after the stem of the 1's glyph. Only the rectangles between the figures 2-9 are also right-aligned with the rightmost stem-pixel of the adjacent glyph.

3. Enlarge the screenshots and count the pixels between the foots of the ones. The foot-gap is three pixel wide between 2-9 and four pixel wide between 1-2.
Comment 16 Stefan 2012-01-08 13:53:17 PST
(In reply to Stefan from comment #15)
> 3. Enlarge the screenshots

It is necessary to enlarge the screenshots, not the view of the testcase.
Comment 17 Bill Gianopoulos [:WG9s] 2012-01-08 13:59:26 PST
I still do not see any difference (hint if you want to show an issue it would be nice if on the fail case the to lines of 1's did NOT align with each other)
Comment 18 Stefan 2012-01-08 14:05:21 PST
(In reply to Bill Gianopoulos [:WG9s] from comment #17)
> I still do not see any difference (hint if you want to show an issue it
> would be nice if on the fail case the to lines of 1's did NOT align with
> each other)

Which of the three methods to perceive the difference given in Comment 15 did not work out for you?
Comment 19 Stefan 2012-01-08 14:12:29 PST
Created attachment 586837 [details]
revised Testcase
Comment 20 Stefan 2012-01-08 14:13:17 PST
Created attachment 586838 [details]
screenshot last good nightly (pass)
Comment 21 Stefan 2012-01-08 14:13:38 PST
Created attachment 586839 [details]
screenshot first bad nightly (fail)
Comment 22 Stefan 2012-01-08 14:17:43 PST
(In reply to Bill Gianopoulos [:WG9s] from comment #17)
> it would be nice if on the fail case the to lines of 1's did NOT align with
> each other)

The issue is not an alignment issue between the two lines (rows) of the original testcase. The second line was misleading, I have removed it.

The issue is the excess 1 px horizontal distance between one No. 1 and one No. 2 compared to the distances between the other horizontally adjacent ones.
Comment 23 Stefan 2012-01-08 15:59:25 PST
The first bad revision is:
changeset:   58959:11e328a49e0a
user:        Karl Tomlinson <karlt+@karlt.net>
date:        Thu Dec 09 20:29:39 2010 +1300
summary:     b=569770 part 9: use gfxHarfBuzzShaper when suitable r=jfkthame a=blocking

http://hg.mozilla.org/mozilla-central/rev/11e328a49e0a

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=95452499f3d6&tochange=11e328a49e0a
Comment 24 Karl Tomlinson (:karlt) 2012-01-09 12:42:27 PST
I see the problem in attachment 586839 [details], but am not able to reproduce.

Can you tell me which font is used to render these characters, please?
This extension may be useful.
https://addons.mozilla.org/en-US/firefox/addon/fontinfo/

Is your locale "de"?

Just to be sure, you see this with Mozilla builds, right?

(In reply to Stefan from comment #9)
> Have no add-ons, tried also in safe-mode. The settings in~/.fonts.conf do
> not change the first gap width.

Did you try setting hintstyle to hintfull and hintnone?
Comment 25 Stefan 2012-01-09 18:14:34 PST
(In reply to Karl Tomlinson (:karlt) from comment #24)
> Can you tell me which font is used to render these characters, please?

fontinfo says it's "Times New Roman". According to lsof the fontfilename is "times.ttf". Fontforge says 

Fontname TimesNewRomanPSMT
Version  2.82

I can also reproduce the issue with Thorndale AMT and Arial (first gap smaller).

> Is your locale "de"?

"de_DE@euro" (std here seems to be "de_DE.utf8"). The issue does not depend on the locale setting.

> Just to be sure, you see this with Mozilla builds, right?

I use this one:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/01/2012-01-08-03-10-24-mozilla-central/firefox-12.0a1.en-US.linux-x86_64.tar.bz2

> (In reply to Stefan from comment #9)
> > Have no add-ons, tried also in safe-mode. The settings in~/.fonts.conf do
> > not change the first gap width.
> 
> Did you try setting hintstyle to hintfull and hintnone?

This is the .fonts.conf I use, but the wider gap is also visible w/o:

      1 <?xml version="1.0"?>
      2 <!DOCTYPE fontconfig SYSTEM "fonts.dtd">
      3 <fontconfig>
      4  <match target="font" >
      5   <edit mode="assign" name="rgba" >
      6    <const>none</const>
      7   </edit>
      8  </match>
      9  <match target="font" >
     10   <edit mode="assign" name="hinting" >
     11    <bool>false</bool>
     12   </edit>
     13  </match>
     14  <match target="font" >
     15   <edit mode="assign" name="hintstyle" >
     16    <const>hintnone</const>
     17   </edit>
     18  </match>
     19  <match target="font" >
     20   <edit mode="assign" name="antialias" >
     21    <bool>true</bool>
     22   </edit>
     23  </match>
     24 </fontconfig>
Comment 26 Stefan 2012-01-09 18:26:32 PST
(In reply to Karl Tomlinson (:karlt) from comment #24)
> Did you try setting hintstyle to hintfull and hintnone?

Yes, the wider gap does not depend on hinting.
Comment 27 Karl Tomlinson (:karlt) 2012-01-09 23:52:07 PST
(In reply to Stefan from comment #25)
> fontinfo says it's "Times New Roman". According to lsof the fontfilename is
> "times.ttf". Fontforge says 
> 
> Fontname TimesNewRomanPSMT
> Version  2.82

Thanks.  I can reproduce with that font, with hinting set to true or false.
Comment 28 Karl Tomlinson (:karlt) 2012-01-10 16:47:01 PST
This is what we're getting from hb_buffer_get_glyph_positions for the first 3 glyphs in a series of more than 3 "1"s at 16px.  I wonder why the x_offsets are non-zero.  The advances are reduced by the offsets.  I'd expect kerning to change advances, but not offsets.

(gdb) p posInfo[0]@3
$20 = {{
    x_advance = 504832, 
    y_advance = 0, 
    x_offset = 0, 
    y_offset = 0, 
    var = {
      u32 = 0, 
      i32 = 0, 
      u16 = {0, 0}, 
      i16 = {0, 0}, 
      u8 = "\000\000\000", 
      i8 = "\000\000\000"
    }
  }, {
    x_advance = 485376, 
    y_advance = 0, 
    x_offset = -19456, 
    y_offset = 0, 
    var = {
      u32 = 0, 
      i32 = 0, 
      u16 = {0, 0}, 
      i16 = {0, 0}, 
      u8 = "\000\000\000", 
      i8 = "\000\000\000"
    }
  }, {
    x_advance = 485376, 
    y_advance = 0, 
    x_offset = -19456, 
    y_offset = 0, 
    var = {
      u32 = 0, 
      i32 = 0, 
      u16 = {0, 0}, 
      i16 = {0, 0}, 
      u8 = "\000\000\000", 
      i8 = "\000\000\000"
    }
  }}

Disabling kerning by returning 0 from the kerning_func gives consistent advances and removes the offsets.  Comparing advances indicates that kerning was applied to the first advance, but was applied twice to the second advance, as well as to the offset.

(gdb) p posInfo[0]@3
$32 = {{
    x_advance = 524288, 
    y_advance = 0, 
    x_offset = 0, 
    y_offset = 0, 
    var = {
      u32 = 0, 
      i32 = 0, 
      u16 = {0, 0}, 
      i16 = {0, 0}, 
      u8 = "\000\000\000", 
      i8 = "\000\000\000"
    }
  }, {
    x_advance = 524288, 
    y_advance = 0, 
    x_offset = 0, 
    y_offset = 0, 
    var = {
      u32 = 0, 
      i32 = 0, 
      u16 = {0, 0}, 
      i16 = {0, 0}, 
      u8 = "\000\000\000", 
      i8 = "\000\000\000"
    }
  }, {
    x_advance = 524288, 
    y_advance = 0, 
    x_offset = 0, 
    y_offset = 0, 
    var = {
      u32 = 0, 
      i32 = 0, 
      u16 = {0, 0}, 
      i16 = {0, 0}, 
      u8 = "\000\000\000", 
      i8 = "\000\000\000"
    }
  }}
Comment 29 Karl Tomlinson (:karlt) 2012-01-10 21:51:21 PST
The numbers from hb_buffer_get_glyph_positions added together would actually lead to equally spaced glyphs, but I guess it's the rounding to integer pixels that we apply separately to advance and offset that causes the problem.

(HarfBuzz apparently applies the kerning half to the advance of the leading glyph and half to the following glyph.   The offset of the following glyph is adjusted by the same amount so that there is the correct distance between the glyphs.  It might be done this way so that each glyph shares the same change in space, as seen in text selection, for example.)

We can probably come up with a better way to do the rounding.
Comment 30 Karl Tomlinson (:karlt) 2012-01-11 01:40:16 PST
Reftest at http://hg.mozilla.org/try/rev/5b7c39ec6faf
Comment 31 Karl Tomlinson (:karlt) 2012-01-11 11:53:09 PST
As expected the reftest also fails on GDI because we also round to pixels there.
Comment 32 Karl Tomlinson (:karlt) 2012-01-11 19:09:16 PST
Created attachment 587917 [details] [diff] [review]
improve inter-glyph pixel rounding
Comment 33 Karl Tomlinson (:karlt) 2012-01-12 17:24:52 PST
Created attachment 588255 [details] [diff] [review]
improve inter-glyph pixel rounding v1.1

Removed an old comment that no longer applies, and made a couple of formatting changes to make things easier to read.
Comment 34 Behdad Esfahbod 2012-01-14 18:56:42 PST
Thanks Karl.  Your analysis is right.  I changed HarfBuzz to apply kerning (I think TrueType kerning only for now) half each side, for better cursor positioning.
Comment 35 Stefan 2012-01-19 12:13:29 PST
(In reply to Karl Tomlinson (:karlt) from comment #33)

The patch works and additionally alleviates the too tight kerning: Compare the P-A-distances in the Arial rendered words "SOPA" and "PIPA" in http://www.heise.de/tp/blogs/6/151249
Comment 36 Stefan 2012-01-19 12:24:59 PST
Created attachment 589951 [details]
screenshot patch 1.1 and kerning (P-A in "SOPA" and "PIPA")
Comment 37 Jonathan Kew (:jfkthame) 2012-01-20 01:24:12 PST
I pushed this to -inbound, as I'd like it to start baking before we do a harfbuzz update and other font-related stuff.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a9fd06dea54
Comment 38 Ed Morley [:emorley] 2012-01-21 07:05:53 PST
https://hg.mozilla.org/mozilla-central/rev/0a9fd06dea54
Comment 39 Karl Tomlinson (:karlt) 2012-01-26 21:00:23 PST
reftest: https://hg.mozilla.org/integration/mozilla-inbound/rev/f80d6519ad29
Comment 40 Matt Brubeck (:mbrubeck) 2012-01-27 09:13:35 PST
reftest: https://hg.mozilla.org/mozilla-central/rev/f80d6519ad29

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