Bug 80571 – [mac] Restore color space switching code to run-webkit-tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Tim Horton]

Mac/CG pixel results on Lion vary if the display color space is changed.

The attached patch will fix this by returning to our old behavior of setting the systemwide color profile. However, it will address the two concerns from https://bugs.webkit.org/show_bug.cgi?id=72424 by using the new (to us) LayoutTestHelper hooks in run-webkit-tests.

With this patch applied, pixel results from WkTR and DRT match both between each other, and between any chosen system color space.

We should probably find a way to share LayoutTestHelper source with the Chromium port (they're still using deprecated ColorSync API in order to support Leopard), though I'm not sure where to put the code if we do. We'll have to work that out.

[Radar WebKit Bug Importer]

<rdar://problem/11008529>

[Tim Horton]

Created attachment 130771 [details]
patch

Patch, but needs more testing (especially that colors output match chromium as much as possible, otherwise required rebaselines will be even more significant). In either case, many mac baselines committed between https://bugs.webkit.org/show_bug.cgi?id=75662 landing and now will probably need to be regenerated.

[WebKit Review Bot]

Attachment 130771 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/D..." exit_code: 1
Tools/Scripts/webkitpy/layout_tests/port/mac.py:150:  trailing whitespace  [pep8/W291] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 130771 [details]
patch

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

> Tools/ChangeLog:11
> +        causes system color profile to not affect results on Lion.

_the_ system color profile?

I'd call it the display color profile, too.

> Tools/DumpRenderTree/mac/LayoutTestHelper.m:58
> +    CFUUIDRef displayRef = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID());
> +    
> +    if (!sInitialProfileURL) {
> +        CFDictionaryRef deviceInfo = ColorSyncDeviceCopyDeviceInfo(kColorSyncDisplayDeviceClass, displayRef);
> +        CFDictionaryRef profileInfo = (CFDictionaryRef)CFDictionaryGetValue(deviceInfo, kColorSyncCustomProfiles);
> +        if (profileInfo)
> +            sInitialProfileURL = (CFURLRef)CFDictionaryGetValue(profileInfo, CFSTR("1"));
> +    }

I guess it doesn't really matter that this function leaks like a sieve?

> Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:56
> -    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());
> +    // Creating this bitmap in generic RGB (which matches the device color space as we have set it) prevents any color conversion when the image of the web view is drawn into it.
> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));

Hrm, are these changes necessary? Will they affect existing pixel results?

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 130771 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130771&action=review
> 
> > Tools/ChangeLog:11
> > +        causes system color profile to not affect results on Lion.
> 
> _the_ system color profile?
> 
> I'd call it the display color profile, too.

*Main* display, even.

> > Tools/DumpRenderTree/mac/LayoutTestHelper.m:58
> > +    CFUUIDRef displayRef = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID());
> > +    
> > +    if (!sInitialProfileURL) {
> > +        CFDictionaryRef deviceInfo = ColorSyncDeviceCopyDeviceInfo(kColorSyncDisplayDeviceClass, displayRef);
> > +        CFDictionaryRef profileInfo = (CFDictionaryRef)CFDictionaryGetValue(deviceInfo, kColorSyncCustomProfiles);
> > +        if (profileInfo)
> > +            sInitialProfileURL = (CFURLRef)CFDictionaryGetValue(profileInfo, CFSTR("1"));
> > +    }
> 
> I guess it doesn't really matter that this function leaks like a sieve?

Not really? But, we might as well clean up anyway.

> > Tools/DumpRenderTree/mac/PixelDumpSupportMac.mm:56
> > -    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());
> > +    // Creating this bitmap in generic RGB (which matches the device color space as we have set it) prevents any color conversion when the image of the web view is drawn into it.
> > +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));
> 
> Hrm, are these changes necessary? Will they affect existing pixel results?

I expect they could be necessary in the case where interpretation of deviceRGB (which is unfortunately not actually defined to mean anything specific) changes, causing conversion between data in other color spaces and this to be different. It's one dimension I haven't had a chance to double check yet (and one of the reasons I didn't set r? yet). The old code (before https://bugs.webkit.org/show_bug.cgi?id=75662) also used Generic RGB here, just in a slightly more roundabout way.

[Tim Horton]

Created attachment 130886 [details]
patch for safekeeping

Patch, but still needs investigation of buffer colorspace choice vs. Chromium and going forward; just for safekeeping.

[Nikolas Zimmermann]

(In reply to comment #6)
> Created an attachment (id=130886) [details]
> patch for safekeeping
> 
> Patch, but still needs investigation of buffer colorspace choice vs. Chromium and going forward; just for safekeeping.

I've generated most results of the SVG pixel tests by forcing Generic RGB profile on my iMac before.
I've rebaselined to trunk, and here are my findings:

- When I switch to Generic RGB profile manually, all tests pass, except the ones you've added to platform/mac/test_expectations.txt (they're now failing)
- When I switch to iMac profile, more tests are failing, but also including those that you marked as IMAGE failing in the test_expectations.txt file on mac.

It's quite frustrating for me, as the trunk pixel test baseline (before the recent changes) passed with tolerance 0 on my both 64 bit machines, one running Lion, one running SL, and I'm used to test this way.
Anyhow, now it's broken and I don't know which route to take to fix it.
What's your plan to move on here? Why can't we add this now for Mac, until we find consensus with the cr guys on how to hook it into their TestShell.

[Nikolas Zimmermann]

(In reply to comment #7)
Okay, I partly have to take back what I wrote: It's partly related to Ojans massive rebaselining, which moved dozens of platform/mac/svg results to platform/mac-future/svg/, which is wrong. I'll restore some results, to clean up this mess. Mailed Ojan about this as well.

[Nikolas Zimmermann]

(In reply to comment #8)
> (In reply to comment #7)
> Okay, I partly have to take back what I wrote: It's partly related to Ojans massive rebaselining, which moved dozens of platform/mac/svg results to platform/mac-future/svg/, which is wrong. I'll restore some results, to clean up this mess. Mailed Ojan about this as well.
I fixed the SVG pixel test baseline on Lion - generated all results using the Generic RGB profile where needed (I already did that before for most of them) in r10292. No more skipped svg/ tests because of this bug, still some filters/ test need rebaselines.

Anyhow just wanted to let you know that I rebaselined everything in Generic RGB. If you want to run tests, you have to manually switch to Generic RGB profile on mac now, as long as this bug is fixed.

[Tim Horton]

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=130886) [details] [details]
> > patch for safekeeping
> > 
> > Patch, but still needs investigation of buffer colorspace choice vs. Chromium and going forward; just for safekeeping.
> 
> I've generated most results of the SVG pixel tests by forcing Generic RGB profile on my iMac before.
> I've rebaselined to trunk, and here are my findings:
> 
> - When I switch to Generic RGB profile manually, all tests pass, except the ones you've added to platform/mac/test_expectations.txt (they're now failing)
> - When I switch to iMac profile, more tests are failing, but also including those that you marked as IMAGE failing in the test_expectations.txt file on mac.
> 
> It's quite frustrating for me, as the trunk pixel test baseline (before the recent changes) passed with tolerance 0 on my both 64 bit machines, one running Lion, one running SL, and I'm used to test this way.
> Anyhow, now it's broken and I don't know which route to take to fix it.
> What's your plan to move on here? Why can't we add this now for Mac, until we find consensus with the cr guys on how to hook it into their TestShell.

My plan to move on here is to make sure that our results match cr-mac in the very simplest of cases, and then go with it! This is one of my plans for today.

I agree that it's very unfortunate, that's why I brought it up and am fixing it :-D

Thanks for rebaselining all of the tests, by the way! Hopefully those results will stay stable.

[Tim Horton]

Created attachment 132867 [details]
patch

[Tim Horton]

Created attachment 132868 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 132868 [details]
patch

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

> Tools/DumpRenderTree/mac/LayoutTestHelper.m:38
> +// This is a simple helper app that changes the ColorSync profile to the

I think the comment should say "the color profile for the main display".

> Tools/DumpRenderTree/mac/LayoutTestHelper.m:51
> +    CFUUIDRef mainDisplay = CGDisplayCreateUUIDFromDisplayID(CGMainDisplayID());

mainDisplayID would be a better name.

> Tools/DumpRenderTree/mac/LayoutTestHelper.m:84
> +        fprintf(stderr, "Failed to set color profile for main display!  Many pixel tests may fail as a result.\n");

One space after the ! pls.

> Tools/DumpRenderTree/mac/LayoutTestHelper.m:117
> +    exit(128 + sig);

What is 128 + sig?

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:59
> -    [m_window setColorSpace:[[NSScreen mainScreen] colorSpace]];
> +    [m_window setColorSpace:[NSColorSpace genericRGBColorSpace]];

Does DRT need a similar change?

[Tim Horton]

(In reply to comment #13)
> > Tools/DumpRenderTree/mac/LayoutTestHelper.m:117
> > +    exit(128 + sig);
> 
> What is 128 + sig?

Those are the error codes reserved for death-by-signal.

[Tim Horton]

(In reply to comment #13)
> > Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:59
> > -    [m_window setColorSpace:[[NSScreen mainScreen] colorSpace]];
> > +    [m_window setColorSpace:[NSColorSpace genericRGBColorSpace]];
> 
> Does DRT need a similar change?

Actually, WKTR doesn't need that change, since the color space of the main screen *is* GenericRGB. Tests continue to pass without it.

[Tim Horton]

Landed as http://trac.webkit.org/changeset/111429

[Tim Horton]

This broke SL tools build because it uses API that was made public in Lion. I have an ugly patch that basically #ifdefs the whole file to use the old API if on SL or below.

[Tim Horton]

Created attachment 132908 [details]
ugly, dirty patch (not tested on SL! however, very similar code is in DRT/chromium tree so it should be ok)

[Tim Horton]

Follow up SL DRT build fix in http://trac.webkit.org/changeset/111452.