Bug 83799 – Convert layout test resource PNGs to sRGB

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Payne]

Convert layout test resource PNGs to sRGB

[Tony Payne]

Created attachment 136946 [details]
Patch

[Adam Barth]

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?

[Tony Payne]

Created attachment 136977 [details]
Patch

[Tony Payne]

(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.

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 136977 [details]
Patch

Clearing flags on attachment: 136977

Committed r114056: <http://trac.webkit.org/changeset/114056>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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.

[Tony Payne]

(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.

[Adam Barth]

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?

[Alexey Proskuryakov]

> 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.

[Simon Fraser (smfr)]

I think many sample images have no color profile. So now we have some with sRGB, and many with no profile.