7.39 KB, image/png
6.91 KB, image/png
1.92 KB, patch
|Details | Diff | Splinter Review|
1.74 KB, patch
|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.
moving to proper component
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.
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.
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?
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?
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: 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.
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.
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)
cc:ing mrbkap since the wrapper magic uses in reftest.js is his.
Created attachment 435286 [details] [diff] [review] fixing a bug spotted by mrbkap
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.
One thing, though... I think this should use fails-if rather than skip-if... we should only skip tests that crash.
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?
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.
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
Created attachment 437339 [details] [diff] [review] roll-up for 192
Comment on attachment 437339 [details] [diff] [review] roll-up for 192 a=LegNeato for 184.108.40.206. Please ONLY land this on mozilla-1.9.2 default, as we are still working on 220.127.116.11 on the relbranch
Comment on attachment 437339 [details] [diff] [review] roll-up for 192 Did this fix get checked in for 18.104.22.168/Fennec 1.1?
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.