Closed Bug 1232316 Opened 4 years ago Closed 4 years ago

reftest failures with text-emphasis marks

Categories

(Taskcluster :: Services, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla46

People

(Reporter: armenzg, Assigned: jmaher)

References

Details

Attachments

(2 files, 1 obsolete file)

Hi Jonathan,
We're trying to move Linux64 debug tests into docker images.

I've noticed that one of our reftest chunks fails because of an Asian font difference:
http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/PeZyIzeSTe258t53HdyWrQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

How could I determine which font it is and why it is failing?
Adding a couple more people in case jkew is on holidays. You never know this time of the year :)

^ We're trying to figure out some font discrepancies on reftests.
Indeed, I'm mostly on PTO for now but will try to take a look here and see if I can tell what's going on.
A bit of a close up for the 4 pixels differing.
It looks to me like this isn't really "failing", it's just a minor discrepancy in a couple of the subpixel antialiasing pixels at the point where two characters almost touch (and so their antialiased pixels overlap). This may well be an artifact of different painting orders or text-run breaks in the test vs reference, and doesn't indicate an actual bug that needs fixing.

IMO, it'd be fine to annotate these with "fuzzy" annotations to cover the slight difference in RGB values for a few pixels; or else we could change the text involved to avoid characters whose edges will come so close to each other. If we use characters with at least a pixel of sidebearings, there should be no issue.

Xidorn, would you be OK with modifying the tests to avoid the problem here?
Flags: needinfo?(quanxunzhen)
Yeah, I think it's fine to just add fuzzy there. I have no idea what's the best way to improve the tests to produce a more robust result. Especially given I'm submitting the tests to w3c, I don't really want to change it just for our impl.

Could we alternatively disable antialiasing on this dir? I guess supporting disabling that could help solving similiar issues in the future, because it seems to me we hardly want to test antialiasing itself, but contrarily, we have to add fuzzy to work around subpixel difference because of it in reftest like this frequently.
Flags: needinfo?(quanxunzhen)
is there anything at the OS level that we can do to help reduce this?  Maybe certain resolutions, graphics drivers, or fonts and version of fonts?
(In reply to Xidorn Quan [:xidorn] (UTC-5) from comment #5)
> Yeah, I think it's fine to just add fuzzy there. I have no idea what's the
> best way to improve the tests to produce a more robust result. Especially
> given I'm submitting the tests to w3c, I don't really want to change it just
> for our impl.

I think it would be a lot simpler to just make the text sizes larger. This will generally solve this sort of problem, although nothing is guaranteed.
Summary: Font issues → reftest failures with text-emphasis marks
(In reply to Joel Maher (:jmaher) from comment #6)
> is there anything at the OS level that we can do to help reduce this?  Maybe
> certain resolutions, graphics drivers, or fonts and version of fonts?

I believe there are settings you can use in Linux to turn off font antialiasing systemwide, which should take effect on Firefox as well, but I'm not sure whether it is what we want. jfkthame? jtd?
Flags: needinfo?(jfkthame)
Flags: needinfo?(jdaggett)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> (In reply to Joel Maher (:jmaher) from comment #6)
> > is there anything at the OS level that we can do to help reduce this?  Maybe
> > certain resolutions, graphics drivers, or fonts and version of fonts?
> 
> I believe there are settings you can use in Linux to turn off font
> antialiasing systemwide, which should take effect on Firefox as well, but
> I'm not sure whether it is what we want. jfkthame? jtd?

I'd prefer not to do that at a system-wide level, as it means we'd be testing with a configuration that's less similar to what most users will be running.

(In reply to Joel Maher (:jmaher) from comment #8)
> looking at:
> https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/
> submitted/text-decor-3/text-emphasis-color-property-002.html
> 
> how could I make the font bigger?  Maybe adjust line-height to be greater
> than 5?

line-height wouldn't change the font size, only the spacing. You could add something like

  font-size: 400%

to the style of the test <div>s to make everything 4x bigger.

(IIRC, however, Xidorn generated all these tests with a script (see the /support/ subdir there), so if we're going to tweak them to avoid this issue, it should be done by modifying the script and re-generating.)
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #10)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> > (In reply to Joel Maher (:jmaher) from comment #6)
> > > is there anything at the OS level that we can do to help reduce this?  Maybe
> > > certain resolutions, graphics drivers, or fonts and version of fonts?
> > 
> > I believe there are settings you can use in Linux to turn off font
> > antialiasing systemwide, which should take effect on Firefox as well, but
> > I'm not sure whether it is what we want. jfkthame? jtd?
> 
> I'd prefer not to do that at a system-wide level, as it means we'd be
> testing with a configuration that's less similar to what most users will be
> running.

Then is it possible to add a pref for reftests to disable that at individual test level? I'm not quite familiar with how that works.

> (In reply to Joel Maher (:jmaher) from comment #8)
> (IIRC, however, Xidorn generated all these tests with a script (see the
> /support/ subdir there), so if we're going to tweak them to avoid this
> issue, it should be done by modifying the script and re-generating.)

Mostly. Some of the tests are not, and some of the references are not. The range of generated tests have been marked out in reftest.list. But yes, for those generated tests, you should change the templates in the scripts instead of touching the tests directly.

But actually I'm not very happy to see modifying font size to work around.

IIUC, W3C tests generally should not be changed to adapt impls' requirement. They should be changed only if the test itself is not spec-conforming, or it is unstable because of dependency on impl-dependent behavior. And in the later case, it should be changed only if it really improves the compatibility conceptually.

In this specific case, adding letter-space could be a candidate solution, but I suspect that could make ruby layout in references even more confusing.

Note that we may meet more minor test failures like this in the future, and we may not always be able to change the tests especially of those imported.

So I still prefer just adding fuzzy to work around if we can not disable antialiasing.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> (In reply to Jonathan Kew (:jfkthame) from comment #10)
> > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> > > (In reply to Joel Maher (:jmaher) from comment #6)
> > > > is there anything at the OS level that we can do to help reduce this?  Maybe
> > > > certain resolutions, graphics drivers, or fonts and version of fonts?
> > > 
> > > I believe there are settings you can use in Linux to turn off font
> > > antialiasing systemwide, which should take effect on Firefox as well, but
> > > I'm not sure whether it is what we want. jfkthame? jtd?
> > 
> > I'd prefer not to do that at a system-wide level, as it means we'd be
> > testing with a configuration that's less similar to what most users will be
> > running.
> 
> Then is it possible to add a pref for reftests to disable that at individual
> test level? I'm not quite familiar with how that works.

We can set Gecko prefs at the individual test level, but I'm not sure there's a Gecko pref that would help here. Antialiasing is generally controlled by user prefs at the OS level, not within Gecko.

> 
> > (In reply to Joel Maher (:jmaher) from comment #8)
> > (IIRC, however, Xidorn generated all these tests with a script (see the
> > /support/ subdir there), so if we're going to tweak them to avoid this
> > issue, it should be done by modifying the script and re-generating.)
> 
> Mostly. Some of the tests are not, and some of the references are not. The
> range of generated tests have been marked out in reftest.list. But yes, for
> those generated tests, you should change the templates in the scripts
> instead of touching the tests directly.
> 
> But actually I'm not very happy to see modifying font size to work around.
> 
> IIUC, W3C tests generally should not be changed to adapt impls' requirement.
> They should be changed only if the test itself is not spec-conforming, or it
> is unstable because of dependency on impl-dependent behavior. And in the
> later case, it should be changed only if it really improves the
> compatibility conceptually.
> 
> In this specific case, adding letter-space could be a candidate solution,
> but I suspect that could make ruby layout in references even more confusing.
> 
> Note that we may meet more minor test failures like this in the future, and
> we may not always be able to change the tests especially of those imported.
> 
> So I still prefer just adding fuzzy to work around if we can not disable
> antialiasing.

Yeah, I'm fine with that approach here.
I have done the fuzzy-if magic and verified on taskcluster:

before:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbc72991da4b&filter-searchStr=r1

after:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e76f012d0faf&selectedJob=14626354&filter-searchStr=r1


what I don't know is if this will affect other gtkWidget tests not on the same image (i.e. what we run all our existing linux32/64 tests on).
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #8698583 - Flags: review?(quanxunzhen)
had some of this in a different patch- this should be the entire file now.
Attachment #8698583 - Attachment is obsolete: true
Attachment #8698583 - Flags: review?(quanxunzhen)
Attachment #8698602 - Flags: review?(quanxunzhen)
Comment on attachment 8698602 [details] [diff] [review]
fuzzy-if(gtkWidget,3,4) on all related tests that need it

Review of attachment 8698602 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Attachment #8698602 - Flags: review?(quanxunzhen) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)

> So I still prefer just adding fuzzy to work around if we can not disable
> antialiasing.

You could also trace down the reason for the difference in glyph placements which probably at the root of the problem here. :) Using fuzzy is basically a band-aid over the problem, as is increasing the font-size. Pick your poison.

For the reasons Jonathan notes, trying to disable anti-aliasing is a poor choice because it's running the test in a configuration few users would actually use.
Flags: needinfo?(jdaggett)
Well, I guess the difference here is because, in test files, we render the text as a whole, while in reference files, we render each character individually for ruby to simulate the emphasis marks.
https://hg.mozilla.org/mozilla-central/rev/86d03ae71ba2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Moving closed bugs across to new Bugzilla product "TaskCluster".
Component: TaskCluster → Integration
Product: Testing → Taskcluster
Component: Integration → Services
You need to log in before you can comment on or make changes to this bug.