Closed Bug 1039834 Opened 11 years ago Closed 11 years ago

PNG reftests fail on Mulet

Categories

(Testing :: Reftest, defect)

Product:
Testing
Component:
Reftest
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: jgriffin, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 6 obsolete files)

Fix PNG suite reftest on mulet r=dbaron
21.61 KB, patch
gerard-majax
: review+
Details | Diff | Splinter Review
Most of the PNG reftests fail on Mulet; the reftest analyzer makes it look like Mulet is rendering them with less saturation or something than the reference. See the reftest-analyzer link at https://tbpl.mozilla.org/?tree=Fig&showall=1&rev=2e2c5c14418b Is this acceptable? If so, I'll mark these as skip-if for Mulet. Otherwise, we'll need someone to investigate what's going on.
Blocks: 1027242
No longer blocks: 1031085
What's Mulet, and is it doing something interesting in terms of color-management? The gamma looks way off. Might the reftest harness configuration somehow be failing to set the gfx.color_management.force_srgb preference as it should be (per reftest-preferences.js)?
Also, failing tests should generally be marked as fails-if() rather than skip-if() so that we don't permanently lose test coverage (including after the underlying bug is fixed).
As far as what Mulet is, see this thread: https://groups.google.com/forum/#!topic/mozilla.dev.b2g/2WUg10993OM We do set gfx.color_management.force_srgb. Alexandre is probably the best person to know if what we're seeing is intentional, but he's PTO. I'll needinfo him so he can reply when he returns.
Flags: needinfo?(apoirot)
Most of the pngsuite test files (image/test/reftest/pngsuite-*/*.png) contain a "gAMA 1" chunk, so if gfx.color_management.force_srgb is causing the decoder to overwrite gamma=1.0 with sRGB (gamma approximately == 1/2.2) the images would definitely be displayed incorrectly.
No, force_srgb is forcing our understanding of the display to be sRGB, so that comparisons of uncorrected colors (ugh, yes) and color-managed colors are consistent.
Flags: needinfo?(apoirot) → needinfo?(poirot.alex)
Attached patch Fix reftest on mulet. (obsolete) — — Splinter Review
So it appears to be failing because we set gfx.color_management.mode to 0 on b2g. I tried to ensure setting it to 2 via reftest-preferences.js, but it doesn't seem to be enough (unless I have to clobber?). Commenting this pref in b2g.js does fix png reftest.
Flags: needinfo?(poirot.alex)
Attached patch Fix reftest on mulet (obsolete) — — Splinter Review
Actually, setting the pref from reftest seems to be enough. https://tbpl.mozilla.org/?tree=Try&rev=99b9542cab66
Attachment #8475065 - Attachment is obsolete: true
Attachment #8480546 - Flags: review?(dbaron)
Comment on attachment 8480546 [details] [diff] [review] Fix reftest on mulet I'm ok with this, although I wonder if we're better off setting this pref explicitly in the directories that need it rather than the whole reftest suit. I don't think I have a strong opinion, though. I'd like to run this by Jeff as well, though.
Attachment #8480546 - Flags: review?(jmuizelaar)
Attachment #8480546 - Flags: review?(dbaron)
Attachment #8480546 - Flags: review+
Comment on attachment 8480546 [details] [diff] [review] Fix reftest on mulet Review of attachment 8480546 [details] [diff] [review]: ----------------------------------------------------------------- I also think that just enabling this for the tests that need it is preferable. Other than that it seems reasonable.
Attachment #8480546 - Flags: review?(jmuizelaar) → review+
To be clear, doing it per-directory means adding a default-preferences line to the top of the reftest.list in the relevant directories.
Blocks: 1094369
Since your r+ this a couple of months ago with improvements suggestions, should we land this as is, or should we take the opportunity to rework and refine with your suggestions ?
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(dbaron)
We normally address review comments prior to landing as a matter of general practice. Does it seem particularly hard to do?
Flags: needinfo?(dbaron)
It's more that I am how much work this is, I have not been able to find anything regarding how to play with prefs per directories in those reftests.
Excellent, I was not searching the proper thing :). Thanks !
Flags: needinfo?(jmuizelaar)
Blocks: 1138447
Assignee: nobody → lissyx+mozillians
(In reply to Alexandre LISSY :gerard-majax from comment #18) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=60299c4cbe52 Still waiting on this try to complete successfully before r?
Attachment #8480546 - Attachment is obsolete: true
I'm not sure this is working as expected ... on the try push, I see PNG related failures that should have been fixed by this.
Comment on attachment 8572024 [details] [diff] [review] Fix PNG suite reftest on mulet r=dbaron So far, from the few tests results I could get (it's very very very slow today), it looks like this is not working as I expected. Is there anything obviously wrong there ?
Attachment #8572024 - Flags: feedback?(dbaron)
Does the pref handle dynamic changes correctly?
Flags: needinfo?(jmuizelaar)
I think so, when I see this: > DECL_GFX_PREF(Live, "gfx.color_management.mode", CMSMode, int32_t,-1);
If you change gfx.color_management.mode via about:config, you have to restart Firefox to make the change take effect.
In that case, you should just land the original patch.
Flags: needinfo?(jmuizelaar)
Comment on attachment 8572024 [details] [diff] [review] Fix PNG suite reftest on mulet r=dbaron Seems generally correct, but sounds like it won't work per the above comment, so you should just land the original approach.
Attachment #8572024 - Flags: feedback?(dbaron) → feedback-
Thanks !
Attachment #8572024 - Attachment is obsolete: true
Attachment #8572484 - Attachment is obsolete: true
Comment on attachment 8572487 [details] [diff] [review] Fix PNG suite reftest on mulet r=dbaron,jrmuizel Carrying r+, since it's the same patch.
Attachment #8572487 - Flags: review+
(In reply to Alexandre LISSY :gerard-majax from comment #29) > making sure nothing regress: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=807365a2f0bf Looks like we regress on Android, where we have R1 getting failures on those exact tests. (In reply to Alexandre LISSY :gerard-majax from comment #30) > full reftests: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=627be3f63a32 Good on mulet
(In reply to Alexandre LISSY :gerard-majax from comment #34) > (In reply to Alexandre LISSY :gerard-majax from comment #29) > > making sure nothing regress: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=807365a2f0bf > > Looks like we regress on Android, where we have R1 getting failures on those > exact tests. Those are TEST-UNEXPECTED-PASS. On Android, we have the same pref value as on B2G (0). Now that we are forcing 2, we can re-enable those tests.
Attachment #8572487 - Attachment is obsolete: true
Comment on attachment 8572625 [details] [diff] [review] Fix PNG suite reftest on mulet r=dbaron,jrmuizel So I'm asking for a new review because I had to do a couple of changes compared to the original patch. The fact that we set the pref makes some tests actually pass on Android. So we can remove the |fails-if|.
Attachment #8572625 - Flags: review?(jmuizelaar)
Attachment #8572625 - Flags: review?(dbaron)
Attachment #8572625 - Attachment is obsolete: true
Comment on attachment 8572736 [details] [diff] [review] Fix PNG suite reftest on mulet r=dbaron Carrying r+, just updating r=
Attachment #8572736 - Flags: review+
Blocks: 1139891
https://hg.mozilla.org/mozilla-central/rev/2a85317e8f7e
Status: NEW → RESOLVED
Closed: 11 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Whiteboard: [systemsfe]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: