Closed Bug 471917 Opened 14 years ago Closed 13 years ago

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


(Core :: Graphics: ImageLib, defect)

Not set



Tracking Status
status1.9.2 --- .7-fixed


(Reporter: jmaher, Assigned: crowderbt)


(Blocks 1 open bug)


(Keywords: verified1.9.2)


(4 files, 1 obsolete file)

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.
Blocks: 473564
moving to proper component
Component: General → GFX: Color Management
Product: Fennec → Core
QA Contact: general → color-management
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?
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.
Component: GFX: Color Management → ImageLib
QA Contact: color-management → imagelib
Fails on Debian, too.
Mike:  Does that mean this is an expected failure for Linux?
(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.
Attached image the expected result
Assignee: nobody → crowderbt
Attached image the failure result
I've just attached the images coming from the reftests running on the N810
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?

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?
I've ensured that they don't, so I think for now we need to disable this test until we expect it to pass.
(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.
Mike: For fennec in general, we see this:

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: 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.
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.
Mike:  Can you try on a 32-bit system?  I wonder if it's just another version of the same rounding problem.
It might be: it passes on x86 linux.
Shouldn't these tests be marked random, then ?
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.
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)
Attachment #435284 - Flags: superreview?
Attachment #435284 - Flags: review?(mark.finkle)
cc:ing mrbkap since the wrapper magic uses in reftest.js is his.
Attachment #435284 - Attachment is obsolete: true
Attachment #435286 - Flags: superreview?
Attachment #435286 - Flags: review?(mark.finkle)
Attachment #435284 - Flags: superreview?
Attachment #435284 - Flags: review?(mark.finkle)
Attachment #435286 - Flags: superreview? → superreview?(dbaron)
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.
Attachment #435286 - Flags: superreview?(dbaron) → superreview+
One thing, though... I think this should use fails-if rather than skip-if... we should only skip tests that crash.
Attachment #435286 - Flags: review?(mark.finkle) → review+
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.
can we get this landed?
and to fix the accidental deletion of "new" in the first patch.
Closed: 13 years ago
Resolution: --- → FIXED
Is there another bug for the rounding issues already or should a new one be filed ?
We should certainly file one, but I don't think it will be a high-priority bug.
I'll file one when I have the results I talk about in comment 25, then.
can we get this landed on 1.9.2 as well?
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
Attachment #435286 - Flags: approval1.9.2.3?
Attached patch roll-up for 192Splinter Review
Attachment #437339 - Flags: approval1.9.2.4?
Attachment #435286 - Flags: approval1.9.2.4?
Blocks: 557786
Comment on attachment 437339 [details] [diff] [review]
roll-up for 192

a=LegNeato for Please ONLY land this on mozilla-1.9.2 default, as we
are still working on on the relbranch
Attachment #437339 - Flags: approval1.9.2.4? → approval1.9.2.5+
Keywords: checkin-needed
Comment on attachment 437339 [details] [diff] [review]
roll-up for 192

Did this fix get checked in for 1.1?
Attachment #437339 - Flags: approval1.9.2.5+ → approval1.9.2.6+
Doesn't look like it.
Were these tests running on 1.9.2? Is the best way to verify this simply to see that the tests pass now?
Yeah, if the tests pass then this bug is VERIFIED FIXED
Verified for 1.9.2 with passing reftests.
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.