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

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: jmaher, Assigned: Brian Crowder)

Tracking

(Blocks: 1 bug, {verified1.9.2})

Trunk
verified1.9.2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.2 .7-fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Blocks: 473564
(Reporter)

Comment 1

9 years ago
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?
(Reporter)

Comment 3

9 years ago
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.
(Assignee)

Comment 5

8 years ago
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.
(Assignee)

Comment 7

8 years ago
Created attachment 434030 [details]
the expected result
Assignee: nobody → crowderbt
(Assignee)

Comment 8

8 years ago
Created attachment 434031 [details]
the failure result
(Assignee)

Comment 9

8 years ago
I've just attached the images coming from the reftests running on the N810
(Assignee)

Comment 10

8 years ago
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?
(Reporter)

Comment 11

8 years ago
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?
(Assignee)

Comment 12

8 years ago
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.
(Reporter)

Comment 14

8 years ago
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.
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.
(Assignee)

Comment 16

8 years ago
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 ?
(Assignee)

Comment 19

8 years ago
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.
(Assignee)

Comment 20

8 years ago
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)
Attachment #435284 - Flags: superreview?
Attachment #435284 - Flags: review?(mark.finkle)
(Assignee)

Comment 21

8 years ago
cc:ing mrbkap since the wrapper magic uses in reftest.js is his.
(Assignee)

Comment 22

8 years ago
Created attachment 435286 [details] [diff] [review]
fixing a bug spotted by mrbkap
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)
(Assignee)

Updated

8 years ago
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.
(Reporter)

Comment 26

8 years ago
can we get this landed?
(Assignee)

Comment 27

8 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Is there another bug for the rounding issues already or should a new one be filed ?
(Assignee)

Comment 29

8 years ago
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.
(Reporter)

Comment 31

8 years ago
can we get this landed on 1.9.2 as well?
(Assignee)

Comment 32

8 years ago
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?
(Assignee)

Comment 33

8 years ago
Created attachment 437339 [details] [diff] [review]
roll-up for 192
Attachment #437339 - Flags: approval1.9.2.4?
(Assignee)

Updated

8 years ago
Attachment #435286 - Flags: approval1.9.2.4?
Blocks: 557786

Comment 34

8 years ago
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
Attachment #437339 - Flags: approval1.9.2.4? → approval1.9.2.5+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
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?
Attachment #437339 - Flags: approval1.9.2.5+ → approval1.9.2.6+

Comment 36

8 years ago
Doesn't look like it.

Comment 37

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/43bf210715d2
status1.9.2: --- → .6-fixed
Keywords: checkin-needed
Were these tests running on 1.9.2? Is the best way to verify this simply to see that the tests pass now?
(Assignee)

Comment 39

8 years ago
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.