first in a row of identical letters positioned 1 px too far apart

RESOLVED FIXED in mozilla12

Status

()

Core
Layout: Text
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Stefan, Assigned: karlt)

Tracking

({regression})

Trunk
mozilla12
x86_64
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Attachment #586822 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 1

6 years ago
Created attachment 586824 [details]
measurement
(Reporter)

Updated

6 years ago
OS: Other → Linux
Hardware: Other → x86_64
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.
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.
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.
Could you try this either in safe mode or with all add-ons disabled?
(Reporter)

Comment 6

6 years ago
Produced with 2012-01-08-03-10-24-mozilla-central cf8c9f9aeefc.
(Reporter)

Comment 7

6 years ago
(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?
(Reporter)

Comment 8

6 years ago
Can reproduce with current m-c tip revision 9a230265bad5.
(Reporter)

Comment 9

6 years ago
(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.
(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.
(Reporter)

Comment 11

6 years ago
Created attachment 586831 [details]
screenshot last good nightly (pass)
(Reporter)

Updated

6 years ago
Attachment #586831 - Attachment description: last good nightly → screenshot last good nightly (pass)
(Reporter)

Comment 12

6 years ago
Created attachment 586832 [details]
screenshot first bad nightly (fail)
(Reporter)

Comment 13

6 years ago
last good nightly: 2010-12-08-03-mozilla-central 95452499f3d6
first bad nightly: 2010-12-09-03-mozilla-central 11e328a49e0a
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.
(Reporter)

Comment 15

6 years ago
(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.
(Reporter)

Comment 16

6 years ago
(In reply to Stefan from comment #15)
> 3. Enlarge the screenshots

It is necessary to enlarge the screenshots, not the view of the testcase.
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)
(Reporter)

Comment 18

6 years ago
(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?
(Reporter)

Comment 19

6 years ago
Created attachment 586837 [details]
revised Testcase
Attachment #586822 - Attachment is obsolete: true
(Reporter)

Comment 20

6 years ago
Created attachment 586838 [details]
screenshot last good nightly (pass)
Attachment #586831 - Attachment is obsolete: true
(Reporter)

Comment 21

6 years ago
Created attachment 586839 [details]
screenshot first bad nightly (fail)
Attachment #586832 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #586837 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 22

6 years ago
(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.
(Reporter)

Comment 23

6 years ago
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
Component: Untriaged → Layout: Text
Product: Firefox → Core
QA Contact: untriaged → layout.fonts-and-text
Blocks: 569770
(Assignee)

Comment 24

6 years ago
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?
(Reporter)

Comment 25

6 years ago
(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>
(Reporter)

Comment 26

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

Comment 27

6 years ago
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 28

6 years ago
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"
    }
  }}
(Assignee)

Updated

6 years ago
Keywords: regression
(Assignee)

Comment 29

6 years ago
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.
Blocks: 605043
(Assignee)

Comment 30

6 years ago
Reftest at http://hg.mozilla.org/try/rev/5b7c39ec6faf
Assignee: nobody → karlt
(Assignee)

Comment 31

6 years ago
As expected the reftest also fails on GDI because we also round to pixels there.
OS: Linux → All
(Assignee)

Comment 32

6 years ago
Created attachment 587917 [details] [diff] [review]
improve inter-glyph pixel rounding
Attachment #587917 - Flags: review?(jfkthame)
(Assignee)

Comment 33

6 years ago
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.
Attachment #587917 - Attachment is obsolete: true
Attachment #587917 - Flags: review?(jfkthame)
Attachment #588255 - Flags: review?(jfkthame)

Comment 34

6 years ago
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.
Attachment #588255 - Flags: review?(jfkthame) → review+
(Reporter)

Comment 35

6 years ago
(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
(Reporter)

Comment 36

6 years ago
Created attachment 589951 [details]
screenshot patch 1.1 and kerning (P-A in "SOPA" and "PIPA")
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
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/0a9fd06dea54
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 39

6 years ago
reftest: https://hg.mozilla.org/integration/mozilla-inbound/rev/f80d6519ad29
Flags: in-testsuite+
reftest: https://hg.mozilla.org/mozilla-central/rev/f80d6519ad29
You need to log in before you can comment on or make changes to this bug.