Bug 6033 - WebKit+SVG does not support color-interpolation-filters or draw filters in correct colorspace
Summary: WebKit+SVG does not support color-interpolation-filters or draw filters in co...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P4 Normal
Assignee: Dirk Schulze
URL: http://www.w3.org/TR/SVG/painting.htm...
Keywords:
Depends on: 5972 19835 19991 27844
Blocks: 68469 26389
  Show dependency treegraph
 
Reported: 2005-12-10 15:02 PST by Eric Seidel (no email)
Modified: 2014-05-12 05:54 PDT (History)
14 users (show)

See Also:


Attachments
A SVG file - feColorMatrix filter is applied to the object on the left. "color-interpolation-filters" is set to "sRGB" (9.83 KB, image/svg+xml)
2012-02-03 13:55 PST, hwatanab
no flags Details
An updated SVG file - color-interpolation-filters should be added to a filter primitive, feColorMatrix instead of filter tag. (9.83 KB, image/svg+xml)
2012-02-03 16:11 PST, hwatanab
no flags Details
Patch (12.12 KB, patch)
2012-03-06 14:38 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (725.14 KB, patch)
2012-07-30 14:23 PDT, Florin Malita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-06 (588.95 KB, application/zip)
2012-07-30 15:18 PDT, WebKit Review Bot
no flags Details
Patch (783.85 KB, patch)
2012-07-31 09:03 PDT, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (815.55 KB, patch)
2012-08-06 14:40 PDT, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (725.23 KB, patch)
2012-08-13 13:03 PDT, Florin Malita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2005-12-10 15:02:46 PST
WebKit+SVG does not support color-interpolation-filters or draw filters in correct colorspace

http://www.w3.org/TR/SVG/painting.html#ColorInterpolationFiltersProperty
Comment 1 Dirk Schulze 2009-08-08 14:18:26 PDT
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)
Comment 2 hwatanab 2012-02-03 13:55:18 PST
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 3 hwatanab 2012-02-03 14:03:01 PST
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.
Comment 4 hwatanab 2012-02-03 16:11:01 PST
Created attachment 125443 [details]
An updated SVG file - color-interpolation-filters should be added to a filter primitive, feColorMatrix instead of filter tag.
Comment 5 Florin Malita 2012-03-06 14:36:12 PST
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.
Comment 6 Florin Malita 2012-03-06 14:38:58 PST
Created attachment 130444 [details]
Patch

Work in progress
Comment 7 Early Warning System Bot 2012-03-06 14:53:56 PST
Comment on attachment 130444 [details]
Patch

Attachment 130444 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11832564
Comment 8 WebKit Review Bot 2012-03-07 10:44:45 PST
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 9 Early Warning System Bot 2012-03-07 14:13:51 PST
Comment on attachment 130444 [details]
Patch

Attachment 130444 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11852320
Comment 10 Dirk Schulze 2012-03-07 15:29:57 PST
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).
Comment 11 Florin Malita 2012-03-08 10:56:37 PST
(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.
Comment 12 Florin Malita 2012-07-30 14:23:02 PDT
Created attachment 155363 [details]
Patch
Comment 13 Florin Malita 2012-07-30 14:30:48 PDT
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 14 Early Warning System Bot 2012-07-30 14:42:52 PDT
Comment on attachment 155363 [details]
Patch

Attachment 155363 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13385651
Comment 15 WebKit Review Bot 2012-07-30 15:18:51 PDT
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
Comment 16 WebKit Review Bot 2012-07-30 15:18:57 PDT
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 17 Early Warning System Bot 2012-07-30 18:02:35 PDT
Comment on attachment 155363 [details]
Patch

Attachment 155363 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13388606
Comment 18 Florin Malita 2012-07-31 09:03:08 PDT
Created attachment 155555 [details]
Patch

Updated to address failures.
Comment 19 Simon Fraser (smfr) 2012-07-31 09:07:09 PDT
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 20 Dirk Schulze 2012-07-31 09:16:51 PDT
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?
Comment 21 Florin Malita 2012-07-31 09:19:32 PDT
(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.
Comment 22 Simon Fraser (smfr) 2012-07-31 10:42:22 PDT
> 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).
Comment 23 Florin Malita 2012-07-31 12:01:03 PDT
(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.
Comment 24 Florin Malita 2012-08-06 14:39:19 PDT
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.
Comment 25 Florin Malita 2012-08-06 14:40:11 PDT
Created attachment 156762 [details]
Patch
Comment 26 Build Bot 2012-08-07 22:18:38 PDT
Comment on attachment 156762 [details]
Patch

Attachment 156762 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13455353
Comment 27 Florin Malita 2012-08-13 13:03:37 PDT
Created attachment 158081 [details]
Patch

Mac build fix
Comment 28 Dirk Schulze 2012-08-13 15:05:06 PDT
Comment on attachment 158081 [details]
Patch

LGTM. r=me
Comment 29 WebKit Review Bot 2012-08-13 15:37:50 PDT
Comment on attachment 158081 [details]
Patch

Clearing flags on attachment: 158081

Committed r125462: <http://trac.webkit.org/changeset/125462>
Comment 30 WebKit Review Bot 2012-08-13 15:37:57 PDT
All reviewed patches have been landed.  Closing bug.