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
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | .7-fixed |
People
(Reporter: jmaher, Assigned: crowderbt)
References
(Blocks 1 open bug)
Details
(Keywords: verified1.9.2)
Attachments
(4 files, 1 obsolete file)
7.39 KB,
image/png
|
Details | |
6.91 KB,
image/png
|
Details | |
1.92 KB,
patch
|
mfinkle
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
1.74 KB,
patch
|
dveditz
:
approval1.9.2.7+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•14 years ago
|
||
moving to proper component
Component: General → GFX: Color Management
Product: Fennec → Core
QA Contact: general → color-management
Comment 2•14 years ago
|
||
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•14 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
Comment 4•13 years ago
|
||
Fails on Debian, too.
Assignee | ||
Comment 5•13 years ago
|
||
Mike: Does that mean this is an expected failure for Linux?
Comment 6•13 years ago
|
||
(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•13 years ago
|
||
Assignee: nobody → crowderbt
Assignee | ||
Comment 8•13 years ago
|
||
Assignee | ||
Comment 9•13 years ago
|
||
I've just attached the images coming from the reftests running on the N810
Assignee | ||
Comment 10•13 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•13 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•13 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.
Comment 13•13 years ago
|
||
(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•13 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.
Comment 15•13 years ago
|
||
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•13 years ago
|
||
Mike: Can you try on a 32-bit system? I wonder if it's just another version of the same rounding problem.
Comment 17•13 years ago
|
||
It might be: it passes on x86 linux.
Comment 18•13 years ago
|
||
Shouldn't these tests be marked random, then ?
Assignee | ||
Comment 19•13 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•13 years ago
|
||
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•13 years ago
|
||
cc:ing mrbkap since the wrapper magic uses in reftest.js is his.
Assignee | ||
Comment 22•13 years ago
|
||
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•13 years ago
|
Attachment #435286 -
Flags: superreview? → superreview?(dbaron)
Comment 23•13 years ago
|
||
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+
Comment 24•13 years ago
|
||
One thing, though... I think this should use fails-if rather than skip-if... we should only skip tests that crash.
Updated•13 years ago
|
Attachment #435286 -
Flags: review?(mark.finkle) → review+
Comment 25•13 years ago
|
||
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•13 years ago
|
||
can we get this landed?
Assignee | ||
Comment 27•13 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
Closed: 13 years ago
Resolution: --- → FIXED
Comment 28•13 years ago
|
||
Is there another bug for the rounding issues already or should a new one be filed ?
Assignee | ||
Comment 29•13 years ago
|
||
We should certainly file one, but I don't think it will be a high-priority bug.
Comment 30•13 years ago
|
||
I'll file one when I have the results I talk about in comment 25, then.
Reporter | ||
Comment 31•13 years ago
|
||
can we get this landed on 1.9.2 as well?
Assignee | ||
Comment 32•13 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•13 years ago
|
||
Attachment #437339 -
Flags: approval1.9.2.4?
Assignee | ||
Updated•13 years ago
|
Attachment #435286 -
Flags: approval1.9.2.4?
Comment 34•12 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•12 years ago
|
Keywords: checkin-needed
Comment 35•12 years ago
|
||
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•12 years ago
|
||
Doesn't look like it.
Comment 37•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/43bf210715d2
status1.9.2:
--- → .6-fixed
Keywords: checkin-needed
Comment 38•12 years ago
|
||
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•12 years ago
|
||
Yeah, if the tests pass then this bug is VERIFIED FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•