Bug 87437

Summary:

Add tests for JPEG and PNG images with a non-generic RGB color profile

Product:

WebKit

Reporter:

Tony Payne <tpayne>

Component:

New Bugs

Assignee:

Tony Payne <tpayne>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

noel.gordon, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

87434

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Tony Payne

Reported 2012-05-24 16:27:24 PDT

Adds Layout Tests for JPEG and PNG with ICC color profile

Attachments
Patch (907.24 KB, patch) 2012-05-24 16:29 PDT, Tony Payne	no flags	Details Formatted Diff Diff
Patch (907.19 KB, patch) 2012-05-24 16:47 PDT, Tony Payne	no flags	Details Formatted Diff Diff
Patch (502.18 KB, patch) 2012-05-24 20:56 PDT, Tony Payne	no flags	Details Formatted Diff Diff
Patch (501.89 KB, patch) 2012-05-24 21:32 PDT, Tony Payne	no flags	Details Formatted Diff Diff
Patch (740.43 KB, patch) 2012-05-24 23:16 PDT, noel gordon	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Tony Payne

Comment 1 2012-05-24 16:29:47 PDT

Created attachment 143920 [details] Patch

WebKit Review Bot

Comment 2 2012-05-24 16:34:16 PDT

Attachment 143920 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 WARNING: skia test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/skia/skia_test_expectations.txt' does not exist WARNING: chromium test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/webkit/tools/layout_tests/test_expectations.txt' does not exist LayoutTests/platform/chromium/test_expectations.txt:3699: Path does not exist. http/tests/media/media-source/webm/video-media-source-play.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3700: Path does not exist. http/tests/media/media-source/webm/video-media-source-seek.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3701: Path does not exist. http/tests/media/media-source/webm/video-media-source-state-changes.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3702: Path does not exist. http/tests/media/media-source/webm/video-media-source-state-changes.html [test/expectations] [5] Total errors found: 4 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Tony Payne

Comment 3 2012-05-24 16:47:02 PDT

Created attachment 143925 [details] Patch

WebKit Review Bot

Comment 4 2012-05-24 16:49:23 PDT

Attachment 143925 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 WARNING: skia test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/skia/skia_test_expectations.txt' does not exist WARNING: chromium test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/webkit/tools/layout_tests/test_expectations.txt' does not exist LayoutTests/platform/chromium/test_expectations.txt:3699: Path does not exist. http/tests/media/media-source/webm/video-media-source-play.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3700: Path does not exist. http/tests/media/media-source/webm/video-media-source-seek.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3701: Path does not exist. http/tests/media/media-source/webm/video-media-source-state-changes.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3702: Path does not exist. http/tests/media/media-source/webm/video-media-source-state-changes.html [test/expectations] [5] Total errors found: 4 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

noel gordon

Comment 5 2012-05-24 18:33:46 PDT

Comment on attachment 143925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143925&action=review > LayoutTests/ChangeLog:10 > + for a non0identity transform. "non0identity"? > LayoutTests/ChangeLog:12 > + * fast/images/jpeg-with-icc-profile-expected.txt: Added. Sorry for this, could you change "icc-profile" -> "color-profile" in all these file names? For example, red-at-12-oclock-with-color-profile.jpg, etc. > LayoutTests/fast/images/jpeg-with-icc-profile.html:1 > +<!DOCTYPE html> Add the following line here: The red quadrant of the image should be in the 12 o'clock position.<p> > LayoutTests/fast/images/jpeg-with-icc-profile.html:2 > +<img src="resources/red-at-12-oclock-with-icc-profile.jpg" width="800px" height="800px"> DRT has an 800 x 600 screen size during layout tests. Would be nice to make the test result fit in that screen without scroll bars. Try the following: <img src="resources/red-at-12-oclock-with-icc-profile.jpg" width="50%"> > LayoutTests/fast/images/png-with-icc-profile.html:1 > +<!DOCTYPE html> Add the following line here: The red quadrant of the image should be in the 12 o'clock position.<p> > LayoutTests/fast/images/png-with-icc-profile.html:2 > +<img src="resources/red-at-12-oclock-with-icc-profile.png" width="800px" height="800px"> Avoid the scroll bars, use: <img src="resources/red-at-12-oclock-with-icc-profile.png" width="50%"> > LayoutTests/platform/chromium/test_expectations.txt:1164 > +// Implement color profile support. The linux EWS will complain here and won't submit your change due to the test regression I believe (and yes it sucks). Tell the EWS via test_expectations that we'll rebaseline this result in future. Here's the current way BUGCR143 : fast/images/{test-name} = MISSING And the expected result is LayoutTests/platform/chromium-linux/fast/images/png-with-icc-profile-expected.png How will the other chromium ports use this as their expected result? Answer: they won't and their bots will go red on submit :(

Tony Payne

Comment 6 2012-05-24 20:56:29 PDT

Created attachment 143961 [details] Patch

WebKit Review Bot

Comment 7 2012-05-24 20:58:39 PDT

Attachment 143961 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 WARNING: skia test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/skia/skia_test_expectations.txt' does not exist WARNING: chromium test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/webkit/tools/layout_tests/test_expectations.txt' does not exist LayoutTests/platform/chromium/test_expectations.txt:1165: Path does not exist. fast/images/jpeg-with-icc-profile.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:1166: Path does not exist. fast/images/png-with-icc-profile.html [test/expectations] [5] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Tony Payne

Comment 8 2012-05-24 21:00:19 PDT

(In reply to comment #5) > (From update of attachment 143925 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143925&action=review > > > LayoutTests/ChangeLog:10 > > + for a non0identity transform. > > "non0identity"? Fixed > > > LayoutTests/ChangeLog:12 > > + * fast/images/jpeg-with-icc-profile-expected.txt: Added. > > Sorry for this, could you change "icc-profile" -> "color-profile" in all these file names? For example, red-at-12-oclock-with-color-profile.jpg, etc. > Done > > LayoutTests/fast/images/jpeg-with-icc-profile.html:1 > > +<!DOCTYPE html> > > Add the following line here: > > The red quadrant of the image should be in the 12 o'clock position.<p> > Done > > LayoutTests/fast/images/jpeg-with-icc-profile.html:2 > > +<img src="resources/red-at-12-oclock-with-icc-profile.jpg" width="800px" height="800px"> > > DRT has an 800 x 600 screen size during layout tests. Would be nice to make the test result fit in that screen without scroll bars. Try the following: > > <img src="resources/red-at-12-oclock-with-icc-profile.jpg" width="50%"> Went with 400px, 400px > > LayoutTests/fast/images/png-with-icc-profile.html:1 > > +<!DOCTYPE html> > > Add the following line here: > > The red quadrant of the image should be in the 12 o'clock position.<p> > done > > LayoutTests/fast/images/png-with-icc-profile.html:2 > > +<img src="resources/red-at-12-oclock-with-icc-profile.png" width="800px" height="800px"> > > Avoid the scroll bars, use: > > <img src="resources/red-at-12-oclock-with-icc-profile.png" width="50%"> > Fixed > > LayoutTests/platform/chromium/test_expectations.txt:1164 > > +// Implement color profile support. > > The linux EWS will complain here and won't submit your change due to the test regression I believe (and yes it sucks). Tell the EWS via test_expectations that we'll rebaseline this result in future. Here's the current way > > BUGCR143 : fast/images/{test-name} = MISSING > Done > And the expected result is LayoutTests/platform/chromium-linux/fast/images/png-with-icc-profile-expected.png > Removed > How will the other chromium ports use this as their expected result? Answer: they won't and their bots will go red on submit :(

Tony Payne

Comment 9 2012-05-24 21:32:23 PDT

Created attachment 143964 [details] Patch

noel gordon

Comment 10 2012-05-24 23:10:41 PDT

Thanks Tony. A discussed, I'll generate the mac port test result, and update ChangeLog, etc.

noel gordon

Comment 11 2012-05-24 23:16:23 PDT

Created attachment 143984 [details] Patch

noel gordon

Comment 12 2012-05-24 23:25:31 PDT

Comment on attachment 143984 [details] Patch tamura-san: Tony's patch looks fine to me, rubber stamp please?

Kent Tamura

Comment 13 2012-05-24 23:29:25 PDT

Comment on attachment 143984 [details] Patch ok

WebKit Review Bot

Comment 14 2012-05-25 00:26:50 PDT

Comment on attachment 143984 [details] Patch Clearing flags on attachment: 143984 Committed r118487: <http://trac.webkit.org/changeset/118487>

WebKit Review Bot

Comment 15 2012-05-25 00:26:55 PDT

All reviewed patches have been landed. Closing bug.

Tony Payne

Comment 16 2012-05-25 15:41:04 PDT

Comment on attachment 143984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143984&action=review > LayoutTests/fast/images/jpeg-with-color-profile.html:2 > +<img src="resources/red-at-12-oclock-with-color-profile.jpg" width="50%"> You changed the test file to use width="50%" which is really not what you want. That's 50%, not of the image size, but of the page size. Page size changes from platform to platform depending on the window manager's chrome. I highly recommend putting that back to "400px" which will make the image size be stable across platforms > LayoutTests/fast/images/png-with-color-profile.html:2 > +<img src="resources/red-at-12-oclock-with-color-profile.png" width="50%"> You changed the test file to use width="50%" which is really not what you want. That's 50%, not of the image size, but of the page size. Page size changes from platform to platform depending on the window manager's chrome. I highly recommend putting that back to "400px" which will make the image size be stable across platforms

noel gordon

Comment 17 2012-05-25 22:30:09 PDT

This bug is closed. File new bugs if you have concerns. Note that webkit layout tests snapshot 800 x 600 px regardless of platform.

Note You need to log in before you can comment on or make changes to this bug.