Bug 6033 – WebKit+SVG does not support color-interpolation-filters or draw filters in correct colorspace

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

WebKit+SVG does not support color-interpolation-filters or draw filters in correct colorspace

http://www.w3.org/TR/SVG/painting.html#ColorInterpolationFiltersProperty

[Dirk Schulze]

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.

[Florin Malita]

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.

[Florin Malita]

Created attachment 130444 [details]
Patch

Work in progress

[Early Warning System Bot]

Comment on attachment 130444 [details]
Patch

Attachment 130444 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11832564

[WebKit Review Bot]

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

[Early Warning System Bot]

Comment on attachment 130444 [details]
Patch

Attachment 130444 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11852320

[Dirk Schulze]

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

[Florin Malita]

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

[Florin Malita]

Created attachment 155363 [details]
Patch

[Florin Malita]

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?

[Early Warning System Bot]

Comment on attachment 155363 [details]
Patch

Attachment 155363 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13385651

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Early Warning System Bot]

Comment on attachment 155363 [details]
Patch

Attachment 155363 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13388606

[Florin Malita]

Created attachment 155555 [details]
Patch

Updated to address failures.

[Simon Fraser (smfr)]

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.

[Dirk Schulze]

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?

[Florin Malita]

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

[Simon Fraser (smfr)]

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

[Florin Malita]

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

[Florin Malita]

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.

[Florin Malita]

Created attachment 156762 [details]
Patch

[Build Bot]

Comment on attachment 156762 [details]
Patch

Attachment 156762 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13455353

[Florin Malita]

Created attachment 158081 [details]
Patch

Mac build fix

[Dirk Schulze]

Comment on attachment 158081 [details]
Patch

LGTM. r=me

[WebKit Review Bot]

Comment on attachment 158081 [details]
Patch

Clearing flags on attachment: 158081

Committed r125462: <http://trac.webkit.org/changeset/125462>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.