471917 - reftest fails when png image is displayed and compared to image created with tables and background colors

Status: RESOLVED FIXED

Description

[Joel Maher ( :jmaher ) (UTC-4)]

In the /modules/libpr0n/test/reftest/pngsuite-ancillary/ccwn3p08.png and /modules/libpr0n/test/reftest/pngsuite-ancillary/ccwn2c08.png tests, there is a .png image which loads up and is compared to the reference file which is a 32x32 table made up of 1 pixel <td> cells where each cell has a background color to create an image.

The problem is appears that the table data and the .png image are displayed in the same alignment.  This causes the image diff to fail.

This passes on firefox (with the same m-c bits as alpha2) but fails on both fennec (linux desktop) and maemo.

Comment 1

[Joel Maher ( :jmaher ) (UTC-4)]

moving to proper component

Comment 2

[Bobby Holley (:bholley)]

Joel - I'm not sure this is the proper component. Color management specifically refers to LCMS and ICC color space transformations. Maybe just GFX: Thebes?

Comment 3

[Joel Maher ( :jmaher ) (UTC-4)]

Thanks for the note.  I looked at the changes to the test file and all the bugs associated with the changes were in GFX: Color management.  The test was originally added under Core:ImageLib.  Let me move it there.

Comment 4

[Mike Hommey [:glandium]]

Fails on Debian, too.

Comment 5

[Brian Crowder]

Mike:  Does that mean this is an expected failure for Linux?

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> Mike:  Does that mean this is an expected failure for Linux?

No idea. It would be interesting to know if other people running reftests on linux have this problem.

Comment 7

[Brian Crowder]

Created attachment 434030 [details]
the expected result

Comment 8

[Brian Crowder]

Created attachment 434031 [details]
the failure result

Comment 9

[Brian Crowder]

I've just attached the images coming from the reftests running on the N810

Comment 10

[Brian Crowder]

Mike:  This fails on x86-linux only if you don't have something like:

user_pref("gfx.color_management.mode", 2);

in your prefs.js   (we need color-management configured correctly to handle these images, as they have gamma tags, or so jrmuziel tells me).  Fennec does not enable this preference by default and Firefox does, that is the source of the disparity on x86.

On ARM, it's a little more fun.  We probably are running into a difference in the way the two processors handle rounding which is causing different results here, even with gfx.color_management.mode=2 set.

So:  how do we disable these tests if gfx.color-management.mode != 2?  And how do we disable them for non-x86 platforms?

Comment 11

[Joel Maher ( :jmaher ) (UTC-4)]

http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#302

mfinkle recently added the MOZ_WIDGET_TOOLKIT stuff in there, so we could look into doing the same.  Basically we build a sandbox of accessible if conditionals.  

Should we ensure that ARM based builds work with gfx.color-management.mode = 2?

Comment 12

[Brian Crowder]

I've ensured that they don't, so I think for now we need to disable this test until we expect it to pass.

Comment 13

[Mike Hommey [:glandium]]

(In reply to comment #10)
> Mike:  This fails on x86-linux only if you don't have something like:
> 
> user_pref("gfx.color_management.mode", 2);

This pref is set to 2 at GRE level in modules/libpref/src/init/all.js, and only 0 when building for WINCE.

So that's not the reason I'm seeing this disparity.

Comment 14

[Joel Maher ( :jmaher ) (UTC-4)]

Mike: For fennec in general, we see this:
http://mxr.mozilla.org/mobile-browser/source/app/mobile.js#289

Brian: I don't think there is a good way to ignore this test on ARM only.  We can look at XPCOMABI (no value on ARM: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#308) in the sandbox.  I would rather us look at the pref to see if it is !=2.  

I had misspoke earlier about what mfinkle changed, he added support for nativeThemePref in bug 471700 and fixed the manifest file to prepend a 'skip-if(!nativeTheme)' to the specific test cases.  We should do the same for Fennec as it is disabled for all Fennec platforms.  If we do Firefox on ARM (say on Android) then we have other problems.

Comment 15

[Mike Hommey [:glandium]]

Let me put it this way: I'm seeing a reftest fail on these 2 tests with a xulrunner x86-64 linux build, where gfx.color_management.mode *is* 2. Though I'm using system libpng (thus without apng support, but that's another story) if that makes a difference, but that really shouldn't.

Comment 16

[Brian Crowder]

Mike:  Can you try on a 32-bit system?  I wonder if it's just another version of the same rounding problem.

Comment 17

[Mike Hommey [:glandium]]

It might be: it passes on x86 linux.

Comment 18

[Mike Hommey [:glandium]]

Shouldn't these tests be marked random, then ?

Comment 19

[Brian Crowder]

No, we know where and when they fail and they fail consistently in those cases.  I'll have a fix for this today or early tomorrow.

Comment 20

[Brian Crowder]

Created attachment 435284 [details] [diff] [review]
Adds prefs.getIntPref and uses it

This is sufficient to fix this particular issue for the default configuration of Fennec.  We probably need better build/platform information, but I will leave that for another bug.  (getting dbaron's review for the reftest.js part)

Comment 21

[Brian Crowder]

cc:ing mrbkap since the wrapper magic uses in reftest.js is his.

Comment 22

[Brian Crowder]

Created attachment 435286 [details] [diff] [review]
fixing a bug spotted by mrbkap

Comment 23

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Comment on attachment 435286 [details] [diff] [review]
fixing a bug spotted by mrbkap

looks ok to me, although it's mostly black magic to me.  I'm glad mrbkap reviewed it.

Comment 24

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

One thing, though... I think this should use fails-if rather than skip-if... we should only skip tests that crash.

Comment 25

[Mike Hommey [:glandium]]

FWIW, I'll have the results for this test for arm eabi, hppa, ia64, mips, mipsel, ppc, s390 and sparc (all linux) in a few days.

Comment 26

[Joel Maher ( :jmaher ) (UTC-4)]

can we get this landed?

Comment 27

[Brian Crowder]

http://hg.mozilla.org/mozilla-central/rev/bb7491eaf570
and
http://hg.mozilla.org/mozilla-central/rev/6037af8facb9 to fix the accidental deletion of "new" in the first patch.

Comment 28

[Mike Hommey [:glandium]]

Is there another bug for the rounding issues already or should a new one be filed ?

Comment 29

[Brian Crowder]

We should certainly file one, but I don't think it will be a high-priority bug.

Comment 30

[Mike Hommey [:glandium]]

I'll file one when I have the results I talk about in comment 25, then.

Comment 31

[Joel Maher ( :jmaher ) (UTC-4)]

can we get this landed on 1.9.2 as well?

Comment 32

[Brian Crowder]

Comment on attachment 435286 [details] [diff] [review]
fixing a bug spotted by mrbkap

Need approval for this (with the "new " removal chunk gone, and the skip-if => fails-if changes made) on 1.9.2

Comment 33

[Brian Crowder]

Created attachment 437339 [details] [diff] [review]
roll-up for 192

Comment 34

[LegNeato]

Comment on attachment 437339 [details] [diff] [review]
roll-up for 192

a=LegNeato for 1.9.2.5. Please ONLY land this on mozilla-1.9.2 default, as we
are still working on 1.9.2.4 on the relbranch

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 437339 [details] [diff] [review]
roll-up for 192

Did this fix get checked in for 1.9.2.5/Fennec 1.1?

Comment 36

[LegNeato]

Doesn't look like it.

Comment 37

[:Ehsan Akhgari]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/43bf210715d2

Comment 38

[Al Billings [:abillings]]

Were these tests running on 1.9.2? Is the best way to verify this simply to see that the tests pass now?

Comment 39

[Brian Crowder]

Yeah, if the tests pass then this bug is VERIFIED FIXED

Comment 40

[Al Billings [:abillings]]

Verified for 1.9.2 with passing reftests.