1139893 - REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/1062108-1.html | image comparison (==), max difference: 255, number of differing pixels: 32

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Runtime, defect)

Description

[:gerard-majax]

So we have this test failing on Mulet, and it's not something skipped on B2G/B2G Desktop

Comment 1

[:gerard-majax]

Attachment: test execution screenshot

Comment 2

[:gerard-majax]

Attachment: reference screenshot

Comment 3

[:gerard-majax]

I do have the exact same behavior locally. Hacking fonts is good visually.

Comment 4

[:gerard-majax]

Attachment: Fixing fonts for reftest bugs/1062108-1.html r=...

Comment 5

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11f6bb6d44c8

Comment 6

[:gerard-majax]

Comment on attachment 8573955 [details] [diff] [review]
Fixing fonts for reftest bugs/1062108-1.html r=...

Augmenting the font family fixes the issue. Asking review to assert if you think it's the proper way to fix this :)

Comment 7

[John Daggett (:jtd)]

On which platform was this failing? Mulet on Linux? I'm just curious which font it was picking up such that width(nbsp) != width(x). I think Jonathan picked Droid Mono because there was no bold, so you get fake bolding. I think switching to DejaVu Sans Mono will pick up a real bold face so that's changing the nature of the test.

Comment 8

[:gerard-majax]

Yes, Mulet on Linux.

Comment 9

[:gerard-majax]

Does the font on the screenshot looks like the good one? This is what I had locally, too. Ubuntu 14.04 laptop when testing this.

The other fonts in the list, monano and monospace, were giving worse results. Would it mean there is a real bug ? If so, where should I start looking ?

Comment 10

[:gerard-majax]

Right, I've investigated a little. So far, locally, it looks like I have the expected font (according to :janx). I've noticed that changing the zoom level (150% and 300%) gets me the expected match between both.

Comment 11

[John Daggett (:jtd)]

Jonathan, any ideas on why this would be failing on Mulet on Linux? Maybe picking up monospace fonts that don't support nbsp?

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #11)
> Jonathan, any ideas on why this would be failing on Mulet on Linux? Maybe
> picking up monospace fonts that don't support nbsp?

That shouldn't be a problem, afaik ... I believe we "fake"   support by just using the font's space character anyway. See http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxTextRun.cpp#2903.

I wonder if it might be an issue with device-pixel rounding of the glyph advance working differently for the space (for which we use the space-glyph width that's cached in the font) and for the sequence of "x"s (which will go through the usual shaping and measurement codepath). But I haven't attempted to dig deeper to see if that's really relevant or not.

Comment 13

[:gerard-majax]

Jonathan, how could I test your hypothesis ?

Comment 14

[Jonathan Kew [:jfkthame]]

Or maybe bug 1062108 is actually still present on Linux desktop, and just didn't show up because the monospace fonts used there have true bold faces; but running Mulet, we get the Droid Mono face, so synthetic bold kicks in, and the bug surfaces.

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to Alexandre LISSY :gerard-majax from comment #13)
> Jonathan, how could I test your hypothesis ?

If comment 12 is right, it should be possible to demonstrate that the problem goes away at font sizes where the glyph advance is an exact number of pixels; testing a "waterfall" of incrementally-varying sizes should show the bug appearing and disappearing according to how the rounding is working out.

At that point, I think I'd want to run it under a debugger and confirm exactly what glyph metrics are being used for the <space> and <x> glyphs involved, to see what discrepancy there is.

Comment 16

[:gerard-majax]

Like we documented in comment 10 ?

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Alexandre LISSY :gerard-majax from comment #16)
> Like we documented in comment 10 ?

That's certainly suggestive of a pixel-rounding-related issue, though a waterfall test covering a series of sizes such as 10px, 10.5px, 11px, .... 16px or something like that might be clearer and more precise.

Incidentally, a simple test such as

  data:text/html,<pre style="font:12px droid sans mono">hello world<br><b>hello world

on Firefox/Linux (on a system with Droid Sans Mono installed) shows that we are handling synthetic-bold poorly on desktop Linux; the bold line is consistently wider than the regular one. The same test on OS X (again, with Droid Sans Mono installed) has both lines the same width. This doesn't directly account for the test failure here, afaics, but it does suggest that behavior on desktop Linux systems is significantly different from other platforms.

Comment 18

[:gerard-majax]

I've also noticed taskcluster reftests failures on fonts-related tests, that we do not have on try.

Comment 19

[:gerard-majax]

For example, http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/fkSouLU3QBKH4Mab5KkiJQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 20

[:gerard-majax]

I just checked on my firefox nightly coming from mozilla directly (i.e., not self-built) and I'm having the exact same discrepancy than the one I see locally on Mulet.

Comment 21

[:gerard-majax]

hacking different sizes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10e54d142384

Comment 22

[:gerard-majax]

Attachment: Capture du 2015-03-10 13:10:32.png

http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/QGN8jrXbTRiC54M3fqMWlw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 23

[:gerard-majax]

That's from 8px to 18.5px for font-size, with 0.5px increment. So 12px and 12.5px would look good.

Comment 24

[Jonathan Kew [:jfkthame]]

It's pretty clear from the images there -- noting that the discrepancy is sometimes in one direction, sometimes the other -- that the issue does relate to pixel-rounding of fractional glyph advances at some level.

I'm curious what you get if you try an example like

  data:text/html,<div style="font:18.5px droid sans mono"><b>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</b>

and see what document.querySelector("b").getBoundingClientRect() returns; then compare

  data:text/html,<div style="font:18.5px droid sans mono"><b>xxxxx</b>

and see whether one of the results seems to be pixel-rounded, but not the other? In theory, they'd be expected to match.

Comment 25

[:gerard-majax]

They do not match :)

Comment 26

[:gerard-majax]

Attachment: data:text/html,<div style="font:18.5px droid sans mono"><b>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</b>

Comment 27

[:gerard-majax]

Attachment: data:text/html,<div style="font:18.5px droid sans mono"><b>xxxxx</b>

Comment 28

[Jonathan Kew [:jfkthame]]

You're missing a space in the font name in one of those, so it hasn't used the correct font.

Comment 29

[:gerard-majax]

Good catch. The rounding error is more obvious this way.

With the nbsp;:
> DOMRect { x: 8, y: 8, width: 58.83332824707031, height: 23, top: 8, right: 66.83332824707031, bottom: 31, left: 8 }

With the x:
> DOMRect { x: 8, y: 8, width: 60, height: 23, top: 8, right: 68, bottom: 31, left: 8 }

Comment 30

[Jonathan Kew [:jfkthame]]

OK, so it looks like in this example, we're using an unrounded advance width for the space, but a pixel-snapped advance for normal glyphs. Hence the mismatch.

I wonder if we need to check GetRoundOffsetsToPixels somewhere like gfxTextRun::SetSpaceGlyphIfSimple and potentially apply pixel rounding there. Note that we can't simply round the space width when caching it in the font, as rounding is not applied in all situations (e.g. with transformations in effect).

John, care to pursue this? I'm not sure how soon I'll find time to dig further...

Comment 31

[:gerard-majax]

Should I proceed skipping this test for now and file a follow up for you or john to fix it properly ?

Comment 32

[Jonathan Kew [:jfkthame]]

Yeah, I think that's fine. Looks like it's showing a real bug, so we shouldn't just fudge the test to hide the problem; better to explicitly skip it on the problem platform(s), and file a bug to fix the underlying issue. Include the new bug number in the reftest manifest when adding the skip annotation, to help us keep track.

Comment 33

[:gerard-majax]

Attachment: Disable reftest bugs/1062108-1.html on mulet r=jfkthame

There is a real issue underlying, tracked in bug 1141535

Comment 34

[John Daggett (:jtd)]

I'll take a look when I get a chance.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a7cd8a91627

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a7cd8a91627