WebKit+SVG does not support color-interpolation-filters or draw filters in correct colorspace http://www.w3.org/TR/SVG/painting.html#ColorInterpolationFiltersProperty
we have support for the two color space sRGB and linearRGB now. We just need to read color-interpolation-filters and switch between them. (currently linearRGB is used by default according to the spec)
Created attachment 125407 [details] A SVG file - feColorMatrix filter is applied to the object on the left. "color-interpolation-filters" is set to "sRGB" The color of the both objects are supposed to look identical when "color-interpolation-filters" is set to "sRGB". But it is different with Chrome v.16.0.912.77 m on Win 7.
Comment on attachment 125407 [details] A SVG file - feColorMatrix filter is applied to the object on the left. "color-interpolation-filters" is set to "sRGB" feColorMatrix filter is applied to the object on the left. "color-interpolation-filters" is set to "sRGB". Colors of the both objects should be identical. But with Chrome v.16.0.912.77 m, it seems "color-interpolation-filters" is ignored.
Created attachment 125443 [details] An updated SVG file - color-interpolation-filters should be added to a filter primitive, feColorMatrix instead of filter tag.
I have a work in progress patch that implements this by converting all FE inputs to the specified color space. This can have a significant performance impact under adverse circumstances, but I don't think there's a way around it if we must support color-interpolation-filters per filter primitive. It also affects the CSS3 filters (previously only applied in sRGB, now defaulting to linearRGB per spec) so it will require some bulk rebaselining. Let me know if this is headed in the right direction.
Created attachment 130444 [details] Patch Work in progress
Comment on attachment 130444 [details] Patch Attachment 130444 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11832564
Comment on attachment 130444 [details] Patch Attachment 130444 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11853160 New failing tests: css3/filters/filter-with-transform.html css3/filters/effect-combined.html css3/filters/nested-filters.html css3/filters/effect-blur.html css3/filters/effect-grayscale.html css3/filters/effect-sepia.html http/tests/inspector/inspect-element.html accessibility/aria-disabled.html compositing/masks/multiple-masks.html css3/filters/add-filter-rendering.html css2.1/20110323/abspos-containing-block-initial-004d.htm css3/filters/filtered-inline.html css3/filters/nested-filter.html css3/filters/effect-saturate.html css3/filters/effect-hue-rotate.html css3/filters/effect-drop-shadow.html css3/filters/effect-invert.html css3/filters/filter-region.html css3/filters/effect-opacity.html css3/filters/effect-brightness.html css3/filters/effect-contrast.html compositing/culling/filter-occlusion-blur.html css3/filters/crash-hw-sw-switch.html css3/filters/filter-repaint.html css3/filters/multiple-filters-invalidation.html css3/filters/regions-expanding.html css3/filters/crash-filter-change.html compositing/culling/filter-occlusion-blur-large.html css2.1/20110323/abspos-containing-block-initial-004b.htm css3/filters/effect-saturate-hw.html
Comment on attachment 130444 [details] Patch Attachment 130444 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11852320
Comment on attachment 130444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130444&action=review Hi. Great that you work on that. r- because of the lack of tests and description. > Source/WebCore/ChangeLog:4 > + > + WebKit+SVG does not support color-interpolation-filters or draw filters in correct colorspace > + https://bugs.webkit.org/show_bug.cgi?id=6033 You need a detailed description what you do here, how you do it, and how it affects the rendering. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) We need tests for new behaviors. In the case for color-interpolation-filters we would need a lot of pixel results: What happens if you apply sRGB to the <filter> element? What if you add it to a filter effect? What if you do both in different orders? How does it work together with our logic for premultiplied and unpremultiplied effect results? Remember that effects won't use the ImageBuffer results of previous effects, if previous effects already have provide the bit-array. > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:-288 > -#if !USE(CG) > - if (filterData->sourceGraphicBuffer) > - filterData->sourceGraphicBuffer->transformColorSpace(ColorSpaceDeviceRGB, ColorSpaceLinearRGB); > -#endif Have you checked that it does not cause regressions on repainting? IIRC it was added here to prevent calling the transformation multiple times on every repainting (for instance on scrolling, when the filter is not in the backing store any more).
(In reply to comment #10) > > You need a detailed description what you do here, how you do it, and how it affects the rendering. Oh, sure, I didn't intend for this to be submittable yet - just hoping to get some feedback on the general approach. > We need tests for new behaviors. Will do. > In the case for color-interpolation-filters we would need a lot of pixel results: What happens if you apply sRGB to the <filter> element? What if you add it to a filter effect? What if you do both in different orders? It's an inherited property, so if the filter element specifies an sRGB CIF it gets applied to all its constituent FEs (unless they specifically override it). FWIW, this behavior is consistent with FF10 also. > How does it work together with our logic for premultiplied and unpremultiplied effect results? Remember that effects won't use the ImageBuffer results of previous effects, if previous effects already have provide the bit-array. Currently only ImageBuffer color space transforms are supported. So if the result is in {pre,un}multiplied form, it gets converted to IB and cs-transformed in that format then the original bit-array gets invalidated/cleared. If a dependent FE needs the result in pre/un-multiplied form, it will be re-generated from the ImageBuffer. We could avoid the ImageBuffer conversion if we add logic for bit-array color-space transforms, but I was thinking of leaving that optimization for later. > > > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:-288 > > -#if !USE(CG) > > - if (filterData->sourceGraphicBuffer) > > - filterData->sourceGraphicBuffer->transformColorSpace(ColorSpaceDeviceRGB, ColorSpaceLinearRGB); > > -#endif > > Have you checked that it does not cause regressions on repainting? IIRC it was added here to prevent calling the transformation multiple times on every repainting (for instance on scrolling, when the filter is not in the backing store any more). I'm not sure I fully understand that - can you expand a bit? The idea behind removing that conversion is that it should now be handled in FilterEffect::apply(): for (unsigned i = 0; i < size; ++i) { FilterEffect* in = m_inputEffects.at(i).get(); in->apply(); if (!in->hasResult()) return; #if !USE(CG) // Convert the input results to our color space. in->transformResultColorSpace(m_colorSpace); #endif } SourceGraphic (pseudo)FEs are in DevideRGB format, so FEs using them as input will take care of the same transformation (in->transformResultColorSpace(m_colorSpace)). IOW the current chain should be equivalent, with that initial cs-transform deferred to FilterEffect::apply(). Maybe I'm missing something though.
Created attachment 155363 [details] Patch
Reviving this patch. There's one test asserting on Chromium/Skia now: css3/filters/custom/effect-custom-parameters.html STDERR: [11776:11776:1562692191948:FATAL:SkColor.h(34)] ../../third_party/skia/include/core/SkColor.h:34: failed assertion "a <= 255 && r <= 255 && g <= 255 && b <= 255" It seems that the custom CSS filter is generating un-premultiplied results, but Skia's internal buffers are assumed to be pre-multiplied. I'm planning to disable this test for CR and open a separate issue to investigate further - does this sound OK?
Comment on attachment 155363 [details] Patch Attachment 155363 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13385651
Comment on attachment 155363 [details] Patch Attachment 155363 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13389462 New failing tests: css3/filters/custom/effect-custom-parameters.html
Created attachment 155381 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 155363 [details] Patch Attachment 155363 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13388606
Created attachment 155555 [details] Patch Updated to address failures.
Comment on attachment 155555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155555&action=review > Source/WebCore/ChangeLog:8 > + This patch adds support for SVG color-interpolation-filters. All non-CG plarforms now "plarforms". Why not CG? I think this change log should explain why CG doesn't need this code.
Comment on attachment 155555 [details] Patch I would like to see some tests where the property differs between different primitives in one filter. I wonder that the code for CG seems to stay unchanged. Doesn't it perform different on the requested test then?
(In reply to comment #19) > (From update of attachment 155555 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155555&action=review > > > Source/WebCore/ChangeLog:8 > > + This patch adds support for SVG color-interpolation-filters. All non-CG plarforms now > > "plarforms". > > Why not CG? I think this change log should explain why CG doesn't need this code. AFAICT CG is colorspace aware, and handles conversions internally (as long as ImageBuffers are created within the right color space). I'll make the change log more explicit.
> AFAICT CG is colorspace aware, and handles conversions internally (as long as ImageBuffers are created within the right color space). I'll make the change log more explicit. In that case code like this is bad: #if !USE(CG) void FilterEffect::transformResultColorSpace(ColorSpace dstColorSpace) How is the reader supposed to know why transformResultColorSpace() isn't used for CG? It's also prone to build breakage; it would be better for transformResultColorSpace() to exist on all platforms, but do nothing. You may also want to consider the "does this platform need to do color correction" as a runtime setting (maybe something you ask of the GraphicsContext).
(In reply to comment #22) > > AFAICT CG is colorspace aware, and handles conversions internally (as long as ImageBuffers are created within the right color space). I'll make the change log more explicit. > > In that case code like this is bad: > > #if !USE(CG) > void FilterEffect::transformResultColorSpace(ColorSpace dstColorSpace) > > How is the reader supposed to know why transformResultColorSpace() isn't used for CG? It's also prone to build breakage; it would be better for transformResultColorSpace() to exist on all platforms, but do nothing. I did consider that approach, and decided to follow the same #if !USE(CG) pattern as ImageBuffer::transformColorSpace() (assuming that the intent was to avoid any unneeded calls on CG). Ideally, the other platforms should eventually be updated to handle color spaces transparently just like CG, and the ugly #ifdefs would go away. But a cross-platform transformColorSpace is much cleaner; I'll update the patch.
Looks like GraphicsContext is not readily accessible from FilterEffect methods, and making it so feels like overkill just for supporting the runtime color space conversion test. I've moved the CG check inside the method body though - hopefully that addresses your build breakage concerns. A similar cleanup can be done for ImageBuffer::transformColorSpace(), but I'd rather do it separately. Also added alternating color-interpolation-filters to the test, per Dirk's suggestion.
Created attachment 156762 [details] Patch
Comment on attachment 156762 [details] Patch Attachment 156762 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13455353
Created attachment 158081 [details] Patch Mac build fix
Comment on attachment 158081 [details] Patch LGTM. r=me
Comment on attachment 158081 [details] Patch Clearing flags on attachment: 158081 Committed r125462: <http://trac.webkit.org/changeset/125462>
All reviewed patches have been landed. Closing bug.