Bug 89767 – SVG Filter Effect sub-region not applied for some filters

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Stephen Chenney]

Layout test svg/filters/shadow-on-rect-with-filter is failing (has incorrect baseline) on all Chromium platforms, and maybe others. The mac expectation is correct, but maybe it is Skipped.

It looks like the filter effect region calculation is not accounting for the CSS webkit-SVG-shadow property.

[Stephen Chenney]

The shadow drawing is apparently done by Skia, just like any other shadow drawing. The SVG repaint region is definitely large enough, so it seems the issue is in the graphics context and Skia, or maybe in the size that is used to set up the context. I will dig deeper.

[Stephen Chenney]

And it is not skipped on other platforms, so definitely something to do with Skia as the back end.

[Stephen Chenney]

Figured it out - it's WebKit and not Skia. Apparently CG handles it differently.

Basically, the SVG code is using the last effect region to clip the final blit of the filter result into the destination. The shadow would get drawn by Skia outside this clip, so it does not show up. My guess is that CG applies the shadow after the clip. The fix is to modify the clip by the shadow before applying it, for Skia platforms.

[Stephen Chenney]

In looking at this, it turns out the problem is twofold. The FEGaussianBlur and FEDropShadow filters apply the filter effect sub-region (the maxEffectRegion in code) _before_ adding the blur radius to the filter's paint region.

We then add a clip region to the final blit of the filter result into the destination, which hides the problem in some cases but which causes webkit-shadow to fail in Chromium, as the shadow is clipped by the filter region. Given webkit-shadow is not a SVG filter, it should not be influenced by the filter effect region.

The patch I'm about to put up breaks some layout tests. I'll deal with those shortly. And I'll probably add a test for the broken subregion code. So please review the code changes but wait on commit.

[Stephen Chenney]

Created attachment 149950 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 149950 [details]
Patch

Looks great to me, I'd r+ this, but want to give DIrk a chance to comment as well.

[Stephen Chenney]

Thanks Niko. I need to explore the test results on mac too, and create a couple of more explicit tests.

[WebKit Review Bot]

Comment on attachment 149950 [details]
Patch

Attachment 149950 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13107548

New failing tests:
platform/chromium/compositing/layout-width-change.html
platform/chromium/compositing/render-surface-alpha-blending.html
platform/chromium/compositing/tiny-layer-rotated.html
platform/chromium/compositing/huge-layer-rotated.html
fast/loader/loadInProgress.html
platform/chromium/compositing/3d-corners.html
platform/chromium/compositing/video-frame-size-change.html
platform/chromium/compositing/perpendicular-layer-sorting.html
platform/chromium/compositing/plugins/webplugin-no-alpha.html
platform/chromium/compositing/child-layer-3d-sorting.html
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/filters/background-filter-blur-outsets.html
platform/chromium/compositing/lost-compositor-context-with-video.html
platform/chromium/compositing/accelerated-drawing/alpha.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
platform/chromium/compositing/plugins/webplugin-alpha.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/backface-visibility-transformed.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
fast/loader/unload-form-post-about-blank.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/img-layer-grow.html
platform/chromium/compositing/filters/background-filter-blur-off-axis.html
platform/chromium/compositing/filters/background-filter-blur.html
platform/chromium/compositing/lost-compositor-context-twice.html

[WebKit Review Bot]

Created attachment 150255 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Stephen Chenney]

Comment on attachment 149950 [details]
Patch

I have looked at the spec and thought about this some more, and the results for feGaussianBlur with a filter effect region smaller than the source image are incorrect.

The spec has this to say in discussion feGaussianBlur: "If the input has infinite extent and is constant (e.g FillPaint where the fill is a solid color), this operation has no effect. If the input has infinite extent and the filter result is the input to an ‘feTile’, the filter is evaluated with periodic boundary conditions." This tells me that the Filter Effect Region limits the output pixels you need to compute, but it does not limit the input pixels that you need to consider. For example, with a blur at a pixel on the edge of the effect region, you need a source image that includes the blur radius beyond the effect region. 

Our filter code is wrong in this regard. We limit the source image drawing to the filter effect region, when in fact we can only safely limit it to those pixels needed by the filter as inputs for the pixels within the filter's effect region. My guess is that this is why the code was written the way it was previously, even though that previous code was inefficient and produced incorrect results in some cases.

I'll try to clarify all this with tests.

[Dirk Schulze]

(In reply to comment #10)
> (From update of attachment 149950 [details])
> I have looked at the spec and thought about this some more, and the results for feGaussianBlur with a filter effect region smaller than the source image are incorrect.
> 
> The spec has this to say in discussion feGaussianBlur: "If the input has infinite extent and is constant (e.g FillPaint where the fill is a solid color), this operation has no effect. If the input has infinite extent and the filter result is the input to an ‘feTile’, the filter is evaluated with periodic boundary conditions." This tells me that the Filter Effect Region limits the output pixels you need to compute, but it does not limit the input pixels that you need to consider. For example, with a blur at a pixel on the edge of the effect region, you need a source image that includes the blur radius beyond the effect region. 
> 
> Our filter code is wrong in this regard. We limit the source image drawing to the filter effect region, when in fact we can only safely limit it to those pixels needed by the filter as inputs for the pixels within the filter's effect region. My guess is that this is why the code was written the way it was previously, even though that previous code was inefficient and produced incorrect results in some cases.
> 
> I'll try to clarify all this with tests.

Not sure what you mean. The sub regions are output and input limitations. And that is the way gaussian blur works. This was discussed during a SVG WG and it was planned to clarify it more in the spec. This should be done by Filter Effects now. But isn't the case IIRC. See https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html

[Stephen Chenney]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 149950 [details] [details]) 
> > The spec has this to say in discussion feGaussianBlur: "If the input has infinite extent and is constant (e.g FillPaint where the fill is a solid color), this operation has no effect. If the input has infinite extent and the filter result is the input to an ‘feTile’, the filter is evaluated with periodic boundary conditions." This tells me that the Filter Effect Region limits the output pixels you need to compute, but it does not limit the input pixels that you need to consider. For example, with a blur at a pixel on the edge of the effect region, you need a source image that includes the blur radius beyond the effect region. 

> Not sure what you mean. The sub regions are output and input limitations. And that is the way gaussian blur works. This was discussed during a SVG WG and it was planned to clarify it more in the spec. This should be done by Filter Effects now. But isn't the case IIRC. See https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html

Consider the two possible interpretations or filter effect region for an infinite input to the blur.

1. If the filter can use pixels outside the effect region, then every output pixel is computed using exactly the same input and this results in a constant output the same as the input, just clipped to the filter effect region. The discussion in the spec explicitly states this.

2. If the infinite constant color input is also restricted by the filter effect region, then a blur filter applied to a pixel on the edge needs pixels from outside the input, and we clearly use transparent black in this case. The result of the filter is the filter effect region filled with a gradient starting at half the input intensity at the edges coming to full input intensity inside the filter radius. This violates the description in the spec.

We have a LayoutTest for feGaussianBlur that looks at this - it uses a 100 pixel radius filter on a restricted effect region. Currently, we get the same result as Firefox but our filter effect region for the gaussian is incorrect. With my patch, we get the right filter effect region but the result is case 2 above, which now doesn't match Firefox.

Am I to interpret your comment to mean that Firefox is wrong and my new results are right? That's a whole lot easier for me, but then the spec needs to have that discussion about infinite constant input removed, 'cause it just ain't true.

[Stephen Chenney]

Created attachment 153339 [details]
Patch

[Stephen Chenney]

Bug fix, test cases added. Ready to go, I think.

[Dirk Schulze]

Comment on attachment 153339 [details]
Patch

r=me

[Stephen Chenney]

Committed r123210: <http://trac.webkit.org/changeset/123210>

[Stephen Chenney]

Reopened for test updates.

[Stephen Chenney]

Committed r123242: <http://trac.webkit.org/changeset/123242>

[Stephen Chenney]

All tests updated now, I hope.