Nondeterministic line height in testcase with U+2007 (FIGURE SPACE)

RESOLVED FIXED

Status

()

Core
Layout: Block and Inline
P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: roc, Assigned: jtd)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

This is a weird testcase. Reloading the testcase can sometimes make the gap between the two lines differ by one pixel. Repeated reloads generally don't change anything, but if you do something else in the browser for a while and then come back you might see it change. That suggests use of uninitialized memory. It's a problem because my testcase is derived from a reftest (non-breakable-1-ref.html, in particular).
Created attachment 297630 [details]
testcase
Blocks: 400813
The difference is in the second line. Sometimes it's

            line 0x272e738: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,1152,3107,1236} <
              Text(0)@0x272e6f4[8,8,T]  prev-continuation=0x272382c {0,1152,3107,1140} [state=10600204] SELECTED [content=0x27186f0] sc=0x272e300 pst=:-moz-non-element<
                "\u2007abcdef\n"
              >
            >

After reloading it's

            line 0x4081c138: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,1152,3134,1296} <
              Text(0)@0x4081c0f4[8,8,T]  prev-continuation=0x40899a2c {0,1152,3134,1200} [state=10600204] SELECTED [content=0x23918f0] sc=0x4081bd00 pst=:-moz-non-element<
                "\u2007abcdef\n"
              >
            >

Note that it's taller and wider. I wonder if this is nondeterminism in font selection.
For the first load we're using DejaVuSerifCondensed for the FIGURE SPACE. After reload we're using Monaco.
The reason for that is, in the first load we're constructing a textrun for the whole string "abcdef &#x2007;abcdef", so the first characters matched Times-Roman and aPrevFont is non-null. Thus Monaco gets a score of 20 for having the glyph plus 2 for having nearly the right weight. In the second load (due to the textrun cache, I guess) we're only constructing a textrun for the string "&#x2007;abcdef", so aPrevFont is null and we give Monaco a score of 20 for having the glpyh and 5 for not being bold or italic.

I'm not sure why this second string isn't hitting in the textrun cache too, it should, so I'll look into that.

Do we want to allow font selection for a character to depend on the font selected for the previous character? Apart from the ranking dependency, there's also this:

    // 3. use fallback fonts
    // -- before searching for something else check the font used for the previous character
    if (!selectedFont && aPrevMatchedFont && aPrevMatchedFont->TestCharacterMap(aCh)) {
        selectedFont = aPrevMatchedFont;
        return selectedFont.forget();
    }

I had thought we were trying to avoid this, apart from the ZWJ special case.

If we want to allow this, then I think after encountering whitespace we should clear mFirstPass in CmapFontMatcher. ("mFirstPass" is a misleading name, BTW.) This will make the font selection results at least independent of the contents of the textrun cache.
Also, even if we allow stuff like the code I cited above, I think the font we pass to gfxQuartzFontCache::FindFontForChar for matching should always be the first font in the font group, not the previous font.
(Assignee)

Comment 6

10 years ago
(In reply to comment #4)

Yeah, using the first font group is better, that's what the Windows code does.  So a change on the line below should fix the problem:

http://mxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxAtsuiFonts.cpp#804

aPrevMatchedFont should instead be GetFontAt(0), that way the ranking within gfxQuartzFontCache::FindFontForCharProc will be consistent in both cases.

> Do we want to allow font selection for a character to depend on the font
> selected for the previous character?

This is an optimization in both the Windows/Mac font fallback code, we're trying to avoid walking the list of system fonts.  This only applies to the fallback situation, where all the fonts in the font group and in the list of pref fonts has already been tried.  But you're right, this does make the font selection dependent on how the text runs get chopped up.  The characters involved would need to (1) not be in pref fonts and (2) exist in different fonts with different style characteristics (italic/weight) for this dependency to crop up.

> "mFirstPass" is a misleading name, BTW

mFirstChar would be better maybe?
mFirstRange?

I can live with the previous-font thing, but we need to make sure it's null after we've processed whitespace. Do you want to write patches here or should I?

I've figured out why we're not hitting the textrun cache for the second line when we reload. Basically every page transition flushes the docshell's devicecontext font metrics cache, so in the new page we have to create a new fontmetrics for the text, and that creates a new fontgroup (fontgroups aren't cached like fonts are). For cases like this where a word contains glyphs from fonts that aren't the first font in the fontgroup, the textrun word cache uses the fontgroup pointer as the cache key, and now the fontgroup pointers are different.

I wonder if it's worth doing a fontgroup cache for 1.9. What do you think Stuart?
Created attachment 297675 [details] [diff] [review]
textrun cache dumping code

This is helpful debug dumping code that I wrote, it might as well land.
Attachment #297675 - Flags: review?(pavlov)
(Assignee)

Comment 9

10 years ago
(In reply to comment #7)

> I can live with the previous-font thing, but we need to make sure it's null
> after we've processed whitespace. Do you want to write patches here or should
> I?

Sure, I can set up a patch for this.
(Assignee)

Comment 10

10 years ago
This is hard to reproduce.  DejaVu is a Windows font?  A Vista font?  Sorry for the trouble, but could you attach a list of ~/Library/Fonts?
(Assignee)

Comment 11

10 years ago
Created attachment 297686 [details]
testcase that should show the different font matching without refresh

Comment 12

10 years ago
DejaVu is the sort-of successor to the Bitstream Vera fonts
http://dejavu.sourceforge.net/wiki/index.php/Main_Page
(Assignee)

Comment 13

10 years ago
With DejaVu fonts installed, and DUMP_TEXT_RUN enabled, you get:

InitTextRun 372067b0 fontgroup 4035bd90 (serif) len 14 TEXTRUN "abcdef  abdcef" ENDTEXTRUN
InitTextRun font: Times-Roman
InitTextRun 372067b0 fontgroup 4035bd90 font 40379260 match Times-Roman (1-7)
InitTextRun 372067b0 fontgroup 4035bd90 font 3720c260 match DejaVuSerifCondensed (8-1)
InitTextRun 372067b0 fontgroup 4035bd90 font 40379260 match Times-Roman (9-6)

InitTextRun 3720c9f0 fontgroup 4035bd90 (serif) len 7 TEXTRUN " abcdef" ENDTEXTRUN
InitTextRun font: Times-Roman
InitTextRun 3720c9f0 fontgroup 4035bd90 font 3720cb50 match Monaco (1-1)
InitTextRun 3720c9f0 fontgroup 4035bd90 font 40379260 match Times-Roman (2-6)
(Assignee)

Comment 14

10 years ago
Created attachment 297692 [details] [diff] [review]
patch to use firstFont instead of prevFont

This should fix the problem.  It doesn't change the previous font rules, I need to fiddle with that a bit more.  And that should be done both on Windows and on the Mac.
(Assignee)

Updated

10 years ago
Attachment #297692 - Flags: superreview?(roc)
Attachment #297692 - Flags: review?(roc)
Attachment #297692 - Flags: superreview?(roc)
Attachment #297692 - Flags: superreview+
Attachment #297692 - Flags: review?(roc)
Attachment #297692 - Flags: review+

Updated

10 years ago
Attachment #297675 - Flags: review?(pavlov) → review+
(Assignee)

Updated

10 years ago
Attachment #297692 - Flags: approval1.9?

Updated

10 years ago
Attachment #297692 - Flags: approval1.9? → approval1.9+

Updated

10 years ago
Flags: in-testsuite?
Checked in that debug-only dumping code.
(Assignee)

Comment 16

10 years ago
Checked in patch for this on Monday.

Comment 17

10 years ago
Should this be marked as fixed?
(Assignee)

Comment 18

10 years ago
Nope, not yet.  roc feels we should clear out the "previous font" setting at white-space boundaries, so I still need to implement that.

Updated

10 years ago
Blocks: 412192
(Assignee)

Updated

10 years ago
Assignee: roc → jdaggett
Priority: -- → P4
I think this should be higher priority than P4. It leads to nondeterministic rendering, which can cause various hard-to-diagnose problems.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Priority: P4 → P3
(Assignee)

Comment 20

10 years ago
(In reply to comment #7)

> I can live with the previous-font thing, but we need to make sure it's null
> after we've processed whitespace.

This relates to this code below.  In this case, "whitespace" really means just the space character, since that's what the word cache code uses to split text runs.

// 3. use fallback fonts
// -- before searching for something else check the font used for the previous character
if (!selectedFont && aPrevMatchedFont && aPrevMatchedFont->TestCharacterMap(aCh)) {
    selectedFont = aPrevMatchedFont;
    return selectedFont.forget();
}
    
http://mxr.mozilla.org/mozilla/source/gfx/thebes/src/gfxAtsuiFonts.cpp#817

After reviewing the code, this is already effectively the case, the character after a space will never fall through to this case.  This is because *all* fonts have a codepoint for the space character, the previous matched font after a space will always be the first font in the font group.  Font group fonts are the first thing tested in gfxAtsuiFontGroup::FindFontForChar so there is no way that aPrevMatchedFont->TestCharacterMap(aCh) will be true at this point in the code for a character after a space.

If I'm missing something here, please reopen.

Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.