Bug 83799 - Convert layout test resource PNGs to sRGB
Summary: Convert layout test resource PNGs to sRGB
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Payne
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-12 11:48 PDT by Tony Payne
Modified: 2012-04-13 10:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (703.59 KB, patch)
2012-04-12 11:48 PDT, Tony Payne
no flags Details | Formatted Diff | Diff
Patch (703.91 KB, patch)
2012-04-12 14:40 PDT, Tony Payne
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Payne 2012-04-12 11:48:17 PDT
Convert layout test resource PNGs to sRGB
Comment 1 Tony Payne 2012-04-12 11:48:56 PDT
Created attachment 136946 [details]
Patch
Comment 2 Adam Barth 2012-04-12 14:32:32 PDT
Comment on attachment 136946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136946&action=review

This seems fine.  We have PNGs for testing color profiles, so I think its ok to remove the profiles from these PNGs, which are for testing other aspects of the engine.

> LayoutTests/ChangeLog:6
> +        Convert layout test resource PNGs to sRGB
> +        https://bugs.webkit.org/show_bug.cgi?id=83799
> +
> +        Reviewed by NOBODY (OOPS!).

Can you add some information to the ChangeLog about why we're making this change?
Comment 3 Tony Payne 2012-04-12 14:40:50 PDT
Created attachment 136977 [details]
Patch
Comment 4 Tony Payne 2012-04-12 14:41:35 PDT
(In reply to comment #2)
> (From update of attachment 136946 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136946&action=review
> 
> This seems fine.  We have PNGs for testing color profiles, so I think its ok to remove the profiles from these PNGs, which are for testing other aspects of the engine.
> 
> > LayoutTests/ChangeLog:6
> > +        Convert layout test resource PNGs to sRGB
> > +        https://bugs.webkit.org/show_bug.cgi?id=83799
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Can you add some information to the ChangeLog about why we're making this change?

Done.
Comment 5 Adam Barth 2012-04-12 14:52:15 PDT
Comment on attachment 136977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136977&action=review

> LayoutTests/ChangeLog:9
> +        https://bugs.webkit.org/show_bug.cgi?id=83799

The usual format for ChangeLogs is to have the bug title, followed by the bug link on the next line, followed by a blank line and then the description.  It's probably not worth re-uploading this patch, but it's something for next time.
Comment 6 WebKit Review Bot 2012-04-12 17:26:49 PDT
Comment on attachment 136977 [details]
Patch

Clearing flags on attachment: 136977

Committed r114056: <http://trac.webkit.org/changeset/114056>
Comment 7 WebKit Review Bot 2012-04-12 17:26:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Alexey Proskuryakov 2012-04-13 09:26:55 PDT
Multiple people recently worked on ensuring that layout tests don't fail because of color matching issues. Did you consult with any of the people involved before suggesting this change? Adam, did you consult with them before reviewing?

In this bug itself, there is no indication that this is a thought through change - the system is convoluted enough for obvious answer to not necessarily be correct.

See also: bug 67880.
Comment 9 Tony Payne 2012-04-13 09:37:51 PDT
(In reply to comment #8)
> Multiple people recently worked on ensuring that layout tests don't fail because of color matching issues. Did you consult with any of the people involved before suggesting this change? Adam, did you consult with them before reviewing?

I posted on webkit-dev and got no reply.

> In this bug itself, there is no indication that this is a thought through change - the system is convoluted enough for obvious answer to not necessarily be correct.
> 
> See also: bug 67880.

Note that this is a separate issue from color profiles in the expectation PNGs. Here, the issue is color profiles in the test resources.

For me, the fact that the image-color-matching test was expecting a non-sRGB image to match an image without a profile was extremely suspect. I've spent almost a month trying to understand why the LayoutTests are the way they are. I won't claim to be an expert, but I've had no luck finding anyone who can explain the current situation.
Comment 10 Adam Barth 2012-04-13 09:43:55 PDT
Alexey, I don't understand what you're concerned about.  Bug 67880 seems unrelated to this issue.  Can you explain more explicitly what you're worried about?
Comment 11 Alexey Proskuryakov 2012-04-13 09:53:27 PDT
> I posted on webkit-dev and got no reply.

That's sufficient, thank you.

It usually helps to mention previous webkit-dev discussion in Bugzilla, because bugs do feel like they have incomplete rationale and/or vetting otherwise.
Comment 12 Simon Fraser (smfr) 2012-04-13 10:31:30 PDT
I think many sample images have no color profile. So now we have some with sRGB, and many with no profile.