Bug 72411

Created attachment 115233 [details] Test case

This depends on the color profile selected for the display, so may be OS X only. If I chose Generic RGB, the colors pass through unchanged. Nearly everything else produces a different result. Interestingly, Firefox remains unchanged in all profiles. In fact, even the unfiltered colors are unaffected by the color profile.

So the bug is probably "Why isn't SVG filters using the same color profile as the rest of the page?"

(In reply to comment #3) > So the bug is probably "Why isn't SVG filters using the same color profile as the rest of the page?" The default color space for SVG Filters is 'linearRGB'. The color space can be set with the CSS property 'color-interpolation-filters'. At the moment 'linearRGB' is the only color space that we support for SVG Filters. Please see also: http://dev.w3.org/Graphics-FX/modules/filters/publish/SVGFilter.html#FilterPrimitivesOverviewIntro http://www.w3.org/TR/2003/REC-SVG11-20030114/painting.html#ColorInterpolationFiltersProperty

(In reply to comment #4) > (In reply to comment #3) > > So the bug is probably "Why isn't SVG filters using the same color profile as the rest of the page?" > > The default color space for SVG Filters is 'linearRGB'. The color space can be set with the CSS property 'color-interpolation-filters'. At the moment 'linearRGB' is the only color space that we support for SVG Filters. Please see also: > > http://dev.w3.org/Graphics-FX/modules/filters/publish/SVGFilter.html#FilterPrimitivesOverviewIntro > http://www.w3.org/TR/2003/REC-SVG11-20030114/painting.html#ColorInterpolationFiltersProperty I guess I misunderstood that property to mean that it applies to the filter operations themselves, not to the rendering of the output into the document. I think this is an issue because it means there currently is no way to provide a filter that doesn't change its input, unless your system is set to linearRGB (or sRGB when we support it).

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > So the bug is probably "Why isn't SVG filters using the same color profile as the rest of the page?" > > > > The default color space for SVG Filters is 'linearRGB'. The color space can be set with the CSS property 'color-interpolation-filters'. At the moment 'linearRGB' is the only color space that we support for SVG Filters. Please see also: > > > > http://dev.w3.org/Graphics-FX/modules/filters/publish/SVGFilter.html#FilterPrimitivesOverviewIntro > > http://www.w3.org/TR/2003/REC-SVG11-20030114/painting.html#ColorInterpolationFiltersProperty > > I guess I misunderstood that property to mean that it applies to the filter operations themselves, not to the rendering of the output into the document. > > I think this is an issue because it means there currently is no way to provide a filter that doesn't change its input, unless your system is set to linearRGB (or sRGB when we support it). The default color space of SVG Masking was changed from linearRGB to sRGB as default (can be changed with 'color-interpolation' property). Maybe the same could be done for filters with changing the initial value of 'color-interpolation-filters'? This needs to get discussed on public-fx, since HTML doesn't support linearRGB yet IIRC. Maybe linearRGB could be added beside the supported color spaces sRGB and DeviceRGB. It is already an option in ColorSpace.h, just the CSS parser doesn't support it yet. Supporting sRGB for filters is not a big deal. When I implemented linearRGB, I had problems to understand if the input of an effect needs to get transformed to linearRGB/sRGB or if the result needs to get transformed (the CSS property gets applied to certain effects). What happens if an effect has two inputs, both in different color spaces? I wrote some tests and couldn't get a sense full explanation of the behavior on testing other SVG Viewers. At the end I decided to use linearRGB as default and support 'color-interpolation-filters' later. We already have a bug report for this: bug 6033. And I think this bug is a duplication of bug 6033 .

<rdar://problem/10588374>

Created attachment 120200 [details] CSS filter test case

We start losing fidelity of the pixel values in the resultImage->context()->drawImageBuffer(filter->sourceImage(), ColorSpaceDeviceRGB, IntPoint()); in SourceGraphic::platformApplySoftware() before we've done any filter or alpha premult stuff.

So right now SourceGraphic has a deviceRGB image buffer and we paint into that. That gets copied to a linearRGB buffer, thence to another linearRGB buffer, and that buffer gets copied to the final context. Somewhere in that process we lose color fidelity. In theory I think CG should be doing colorspace conversion when the deviceRGB buffer is painted to the linearRGB buffer, and again when the lienarRGB buffer is painted to the destination. I'm not sure why that isn't' happening.

(In reply to comment #10) > So right now SourceGraphic has a deviceRGB image buffer and we paint into that. That gets copied to a linearRGB buffer, thence to another linearRGB buffer, and that buffer gets copied to the final context. Somewhere in that process we lose color fidelity. > > In theory I think CG should be doing colorspace conversion when the deviceRGB buffer is painted to the linearRGB buffer, and again when the lienarRGB buffer is painted to the destination. I'm not sure why that isn't' happening. gdb'ing the CSS testcase shows that both of SourceGraphic's buffers (filter->sourceImage and resultImage) are DeviceRGB, so I'm not sure this description is accurate.

(In reply to comment #11) > gdb'ing the CSS testcase shows that both of SourceGraphic's buffers (filter->sourceImage and resultImage) are DeviceRGB, so I'm not sure this description is accurate. Nevermind, I was misunderstanding the GraphicsContextState story.

Created attachment 129616 [details] patch One thing I think we need is an additional bug to look at turning this on for Windows.

Comment on attachment 129616 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129616&action=review > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:119 > + CFBundleRef webCoreBundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebCore")); > + CFURLRef iccProfileURL = CFBundleCopyResourceURL(webCoreBundle, CFSTR("linearSRGB"), CFSTR("icc"), NULL); > + CFDataRef iccProfileData; > + CFURLCreateDataAndPropertiesFromResource(NULL, iccProfileURL, &iccProfileData, NULL, NULL, NULL); > + CFRelease(iccProfileURL); > + linearSRGBSpace = CGColorSpaceCreateWithICCProfile(iccProfileData); > + CFRelease(iccProfileData); And some error checking if the file doesn't exist! I love how I always find things once I'm looking at it in the review interface that I'm totally blind to in Xcode.

Comment on attachment 129616 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=129616&action=review > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:110 > + static CGColorSpaceRef linearSRGBSpace = NULL; statics are initialized to 0 by the compiler. > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:112 > + if (linearSRGBSpace == NULL) { This should be written as if (!linearSRGBSpace). >> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:119 >> + CFRelease(iccProfileData); > > And some error checking if the file doesn't exist! > > I love how I always find things once I'm looking at it in the review interface that I'm totally blind to in Xcode. Please use RetainPtr<> for iccProfileURL and iccProfileData. > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:123 > #endif A common style in WebKit is to write a (static) create() function and initialize the static variable by calling it, rather than doing the manual null check as you do above. An even neater way to do this is to use a block.

Comment on attachment 129616 [details] patch Attachment 129616 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11757201 New failing tests: fast/workers/storage/use-same-database-in-page-and-workers.html

(In reply to comment #15) > (From update of attachment 129616 [details]) > Please use RetainPtr<> for iccProfileURL and iccProfileData. I can see that working for iccProfileURL, but I'm not sure how (in the iccProfileData case) this would interact with the CFData being created by CFURLCreateDataAndPropertiesFromResource. It doesn't seem to "just work", in any case. I'll have to look at it tomorrow.

Created attachment 129641 [details] patch with most of Dan's changes This is just missing the RetainPtr for iccProfileData.

Created attachment 129642 [details] preempting the style bot

Attachment 129642 [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 Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 85 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #20) > Attachment 129642 [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 > Tools/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] > Total errors found: 1 in 85 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Wahaa? I didn't touch that file.

Comment on attachment 129642 [details] preempting the style bot View in context: https://bugs.webkit.org/attachment.cgi?id=129642&action=review > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:80 > + CGColorSpaceRef linearSRGBSpace = deviceRGBColorSpaceRef(); I would have initialized this to 0 here… > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:92 > + return linearSRGBSpace; …and 0-checked it here, and only if it’s 0, call deviceRGBColorSpaceRef(). One reason is to save that function call in the success case. The other is to avoid returning 0 if CGColorSpaceCreateWithICCProfile() failed for some reason.

Thanks for the review! Unfortunately, this is going to need a lot of new baselines. Even more unfortunately, when poking through the new baselines, I noticed that at least svg/filters/feColorMatrix-offset.svg doesn't look right. I'll take a look tomorrow; it's possible that the other browsers I'm trying are getting it wrong instead.

(In reply to comment #23) > Thanks for the review! > > Unfortunately, this is going to need a lot of new baselines. Even more unfortunately, when poking through the new baselines, I noticed that at least svg/filters/feColorMatrix-offset.svg doesn't look right. I'll take a look tomorrow; it's possible that the other browsers I'm trying are getting it wrong instead. Ok, so we're (as of the patch I'm going to post in a second) down to just needing new pixel baselines, which is currently not possible due to https://bugs.webkit.org/show_bug.cgi?id=80571. In the interests of landing this patch sooner rather than later, ... what should I do? I wish I could temporarily skip just the pixel tests for the affected bunch of tests.

Created attachment 130883 [details] patch This patch differs from the last only slightly: - if (iccProfileURL && !CFURLCreateDataAndPropertiesFromResource(0, iccProfileURL.get(), &iccProfileData, 0, 0, 0)) + if (iccProfileURL && CFURLCreateDataAndPropertiesFromResource(0, iccProfileURL.get(), &iccProfileData, 0, 0, 0)) I'd misread the CFURL docs when reorganizing this function and got the expected return value swapped. So, now it's just up to the tests.

(In reply to comment #24) > (In reply to comment #23) > > Thanks for the review! > > > > Unfortunately, this is going to need a lot of new baselines. Even more unfortunately, when poking through the new baselines, I noticed that at least svg/filters/feColorMatrix-offset.svg doesn't look right. I'll take a look tomorrow; it's possible that the other browsers I'm trying are getting it wrong instead. > > Ok, so we're (as of the patch I'm going to post in a second) down to just needing new pixel baselines, which is currently not possible due to https://bugs.webkit.org/show_bug.cgi?id=80571. In the interests of landing this patch sooner rather than later, ... what should I do? I wish I could temporarily skip just the pixel tests for the affected bunch of tests. You can mark the tests as failing pixel tests only by adding appropriate lines to the test_expectations.txt for your platform. The lines would look something like this: // Description of why this set of tests is expected to have pixel diffs BUGWK12345 path/to/test.html = IMAGE ... Then if the tests start failing in any other way (crash, text diffs) they will still go red. This should work for any port using NRWT, which IIRC is everything but windows

> Then if the tests start failing in any other way (crash, text diffs) they will still go red. This should work for any port using NRWT, which IIRC is everything but windows Oh, test_expectations works for Mac? I had no idea!

Comment on attachment 130883 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=130883&action=review This looks fine to me. Are you going to send another patch with the skips or test_expectations? > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:124 > + // FIXME: Windows should be able to use linear sRGB, this is tracked by http://webkit.org/b/80000. I protest that such a nice id is for such a boring bug

(In reply to comment #28) > (From update of attachment 130883 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130883&action=review > > This looks fine to me. Are you going to send another patch with the skips or test_expectations? Thanks! Indeed, I'm trying to work out the test_expectations right now! Which might be leading to me filing another tools bug :-\ > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:124 > > + // FIXME: Windows should be able to use linear sRGB, this is tracked by http://webkit.org/b/80000. > > I protest that such a nice id is for such a boring bug I said the same thing, in the bug :-D

(In reply to comment #29) > (In reply to comment #28) > > (From update of attachment 130883 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=130883&action=review > > > > This looks fine to me. Are you going to send another patch with the skips or test_expectations? > > Thanks! > > Indeed, I'm trying to work out the test_expectations right now! Which might be leading to me filing another tools bug :-\ > > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:124 > > > + // FIXME: Windows should be able to use linear sRGB, this is tracked by http://webkit.org/b/80000. > > > > I protest that such a nice id is for such a boring bug > > I said the same thing, in the bug :-D Landed (with test_expectations updates for Mac) in http://trac.webkit.org/changeset/110221

(In reply to comment #30) > Landed (with test_expectations updates for Mac) in http://trac.webkit.org/changeset/110221 Hrm, this made nrwt --tolerance 0 -p svg useless for me. Without the fix in bug 80571, the pixel test baseline can't be correctly updated - at least I can't find a way to. I wouldn't have landed this with test_expectations.txt changes, until it's clear when and how it can be fixed. I usually mark a test as failing IMAGE, when I know it just needs a rebaseline. The test that you've marked as IMAGE, can't be updated atm due to bug 80571... Anyhow, I'd like to hear how you want to move on with this. I'm reopening this bug for potential rebaselines after 80571 landed.

Closing again, I rebaselined everything using Generic RGB profile in r110292, and restored several missing results that got removed by wrong rebaselining. We have to fix bug 80571 ASAP, to make sure only Generic RGB pixel test results are added to the Mac baseline.