Bug 31370 – SVG filter effects need smarter size calculation

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

[Dirk Schulze]

We stick closely to the current SVG specification to minimize filter effect sizes. This is not enough if content creators take the viewport as filter AND effect size. We need to find out which part of a effect needs to be drawn, independent of the Spec.

[Dirk Schulze]

*** Bug 41224 has been marked as a duplicate of this bug. ***

[Zoltan Herczeg]

Is there any reduction for this bug? The source code of ecma-cloud.svg is not exactly human readable.

[Dirk Schulze]

Created attachment 63996 [details]
Filter with slow performance

This is an example of a very little rect, that gets blurred. The filter size is 5000x5000 and the effect of the same size. It's very slow, because we're using the effect rect with some more (but here useless) optimizations for the ImageBuffer size.

[Dirk Schulze]

I worked on this problem yesterday. And I have a working patch. But the last patch on bug 45612 is needed. I upload the patch after I fixed a problem with filterScale.

[Dirk Schulze]

Created attachment 69566 [details]
Patch

Draw filters in device space and calculate smallest effect region.

[Nikolas Zimmermann]

Comment on attachment 69566 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69566&action=review

Beautiful patch, great job Dirk! Some things left to fix....

> WebCore/ChangeLog:14
> +        Caclculate all filter results in device space instead of the filtered objects user space. This change is similar to

typo: Calculate.

> WebCore/ChangeLog:16
> +        every scale level and is indepent of any transformation to the target element or any root element of the target.

typo: independant.
s/root element/ancestor/

> WebCore/ChangeLog:17
> +        The second part of this patch reduces the size of every effect to the smallest effected region instead of the complete

typo: affected.

> WebCore/ChangeLog:19
> +        as clipping region, like mentioned in the SVG specification, to make the effected region even smaller now.

typo: affected.

> WebCore/ChangeLog:20
> +        This is a huge speedup. The ECMA cloud (http://ejohn.org/files/ecma-cloud.svg) is more than 100 times faster on Gtk and

Newline before this line please.

> WebCore/ChangeLog:22
> +        Some exmaples on svg-wow.org can be fewed the first time now, since the subregions were much bigger than the effected

typo: examples.
typo: affected.
s/fewed/viewed/ cute ;-)

> WebCore/ChangeLog:23
> +        region. This caused endless calculations.

Just remove the entire sentence, it's quite confusing.

> WebCore/ChangeLog:24
> +        There is still more potential to speed up 

Same here, maybe say "There's still more potential to speed up filters, by further reducing the ImageBuffer sizes"

> WebCore/ChangeLog:25
> +        Changed repaintRectInLocalCoordinates to absolutePaintRect, since all coordinates are in device space instead of the

s/Changed/Renamed/

> WebCore/ChangeLog:27
> +        The absolutePaintRect is calculated by determineAbsolutePaintRect and gets called by FilterEffect::effectContext() on

s/The absolutePaintRect/The absolute paint rect/
determineAbsolutePaintRect()

> WebCore/ChangeLog:32
> +        Tests: svg/filters/filterRes/filterRes1.svg

Do we really need a new subdirectory? svg/filters seems just fine.

> WebCore/ChangeLog:36
> +        * platform/graphics/cairo/GraphicsContextCairo.cpp: Update setRepaintRectToLocalCoordinates to setAbsolutePaintRect.

Call setAbsolutePaintRect instead of setRepaintRectInLocalCoordinates.

> WebCore/ChangeLog:39
> +        (WebCore::FEBlend::apply):

Add something like s/repaintRectInLocalCoordinates/absolutePaintRect/.

> WebCore/ChangeLog:41
> +        (WebCore::FEColorMatrix::apply):

Use ditto for the other apply() methods below.

> WebCore/platform/graphics/filters/FEComposite.cpp:120
> +    case FECOMPOSITE_OPERATOR_IN :

Whitespace before :, please remove.

> WebCore/platform/graphics/filters/FEComposite.cpp:122
> +        repaintRect = inputEffect(1)->absolutePaintRect();

Ditto.

> WebCore/platform/graphics/filters/FEComposite.cpp:124
> +    case FECOMPOSITE_OPERATOR_ARITHMETIC :

Ditto.

> WebCore/platform/graphics/filters/FEComposite.cpp:128
> +        return FilterEffect::determineAbsolutePaintRect(filter);

Comments could help here, the code is not really understandable for me at the moment, why arithmetic  needs maxEffectRect(), why in/atop just the first effects absolutePaintRect etc.
And also why you're only calling setAbsolutePaintRect() in the non-default case.

> WebCore/platform/graphics/filters/FEComposite.cpp:130
> +    setAbsolutePaintRect(repaintRect);

It's not clear to me why this function still returns a repaintRect.
It seems it's not needed, the caller can just query absolutePaintRect() after calling determineAbsolutePaintRect.

> WebCore/platform/graphics/filters/FEFlood.cpp:78
> +    if (!color.red() && !color.green() && !color.blue())

Hm, it seems to me a helper function is missing in Color which does just that "bool isBlackAlpha" or something like that, which avoids checking red() green() blue() individually, which all do bit shifts on the RGBA quadruplet.
It should be easier to find this out using a single query on the quadruplet. I'll leave for you to decide wheter you want to add that to Color right now.

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:137
> +        kernelSizeX = max(2U, static_cast<unsigned>(floor(stdX * gGaussianKernelFactor + 0.5f)));

Doesn't max<unsigned>(2, static_cast<unsigned>(...)) work? Just guessing here. If not please use "2u".
Don't you want to use floorf here?

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:141
> +        kernelSizeY = max(2U, static_cast<unsigned>(floor(stdY * gGaussianKernelFactor + 0.5f)));

Ditto.

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:142
> +    repaintRect.inflateX(3 * (kernelSizeX * 0.5f));

That needs a comment.

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:173
> +        kernelSizeX = max(2U, static_cast<unsigned>(floor(stdX * gGaussianKernelFactor + 0.5f)));

Same question as above, doesn't max<unsigned> work? If not use "2u".

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:176
> +    if (stdY)

Ditto.

> WebCore/platform/graphics/filters/FEMorphology.cpp:80
> +    repaintRect.inflateX(static_cast<int>(filter->horizontalScale(m_radiusX)));

static_cast<int> seems to be wrong here. Either floor or ceil, or sth. else, but not truncating. Correct me if I'm wrong.

> WebCore/platform/graphics/filters/FEMorphology.cpp:81
> +    repaintRect.inflateY(static_cast<int>(filter->verticalScale(m_radiusY)));

Ditto.

> WebCore/platform/graphics/filters/FEMorphology.cpp:102
> +    if (m_radiusX <= 0 || m_radiusY <= 0)

Are you sure you want the check here, and not below, after radiusX/Y are calculated?

> WebCore/platform/graphics/filters/FEMorphology.cpp:105
> +    int radiusX = static_cast<int>(filter->horizontalScale(m_radiusX));

Ditto, truncating seems wrong.

> WebCore/platform/graphics/filters/FETurbulence.cpp:338
> +    PaintingData paintingData(static_cast<long>(filter->horizontalScale(m_seed)), imageRect.size());

Why are you casting here?

> WebCore/platform/graphics/filters/Filter.h:48
> +        virtual float horizontalScale(float value) const { return value * m_filterResolution.width(); }
> +        virtual float verticalScale(float value) const { return value * m_filterResolution.height(); }

These names could need some love. How about "applyHorizontalFilterResolutionScale".

> WebCore/platform/graphics/filters/FilterEffect.cpp:79
> +FloatRect FilterEffect::drawingRegionOfInputImage(const IntRect& srcRect) const

Seems unclear, why this is a FloatRect now? Should be an IntRect.

> WebCore/platform/graphics/filters/FilterEffect.cpp:93
> +    IntRect bufferRect = determineAbsolutePaintRect(filter);

There you go, just call determineAbsolutePaintRect(filter), then use IntRect bufferRect = absolutePaintRect();
Can you avoid the local variable, is it only used in the ImageBuffer::create() call, or even below?

> WebCore/platform/graphics/filters/FilterEffect.h:68
> +    IntRect maxEffectRect() const { return m_maxEffectRect; }

Hm, I'd love this to be more descriptive. Isn't it something like the minimal effect rect needed, rather than the max effect rect? :-)

> WebCore/platform/graphics/filters/FilterEffect.h:95
> +    // FIXME: FETile still needs special handling.

Can you explain to me, what's still different about FETile? Is it the only exception?

> WebCore/platform/graphics/filters/FilterEffect.h:115
> +    // The maximum size of a filter primitive. On SVG this is the primitive subregion in absolute coordinate space.

s/On SVG/For SVG/

> WebCore/rendering/RenderSVGResourceFilter.cpp:191
> +    // than kMaxFilterSize.

Do you need to break lines here?

> WebCore/svg/graphics/filters/SVGFilter.cpp:27
> +SVGFilter::SVGFilter(AffineTransform absoluteTransform, const FloatRect& absoluteSourceDrawingRegion, const FloatRect& targetBoundingBox, const FloatRect& filterRegion, bool effectBBoxMode)

Use const AffineTransform& here!

> WebCore/svg/graphics/filters/SVGFilter.cpp:91
> +    return value * filterResolution().width() * (m_absoluteFilterRegion.width() / m_filterRegion.width());

Useless braces.

> WebCore/svg/graphics/filters/SVGFilter.cpp:98
> +    return value * filterResolution().height() * (m_absoluteFilterRegion.height() / m_filterRegion.height());

Useless braces.

> WebCore/svg/graphics/filters/SVGFilter.cpp:101
> +PassRefPtr<SVGFilter> SVGFilter::create(AffineTransform absoluteTransform, const FloatRect& absoluteSourceDrawingRegion, const FloatRect& targetBoundingBox, const FloatRect& filterRegion, bool effectBBoxMode)

const AffineTransform&.

> LayoutTests/svg/filters/filterRes/filterRes3.svg:1
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

Can you add other tests, that do the same but have a viewBox, and or a parent <g transform="something-complex-here-skewed-rotated-scaled, etc."> ?

[Dirk Schulze]

Created attachment 69833 [details]
Patch

[Dirk Schulze]

Created attachment 69839 [details]
Patch

[Dirk Schulze]

(In reply to comment #8)
> Created an attachment (id=69839) [details]
> Patch

Added checks for apply() to SourceGraphic and SourceAlpha if the sourceImage is present, return if not.

[Dirk Schulze]

Created attachment 69890 [details]
Patch

[Dirk Schulze]

(In reply to comment #10)
> Created an attachment (id=69890) [details]
> Patch

Last change. We have to take the subregion size in absolute coordinates for lightning effects instead of the smallest effect rect. I thought it was a progression, since the results looked like in Firefox with the previous patch, but Firefox is wrong here.

[Dirk Schulze]

(In reply to comment #6)
> (From update of attachment 69566 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69566&action=review
ChangeLog issues fixed.

> 
> > WebCore/platform/graphics/filters/FEComposite.cpp:120
> > +    case FECOMPOSITE_OPERATOR_IN :
> 
> Whitespace before :, please remove.
> 
> > WebCore/platform/graphics/filters/FEComposite.cpp:122
> > +        repaintRect = inputEffect(1)->absolutePaintRect();
> 
> Ditto.
> 
> > WebCore/platform/graphics/filters/FEComposite.cpp:124
> > +    case FECOMPOSITE_OPERATOR_ARITHMETIC :
> 
> Ditto.
fixed.

> 
> > WebCore/platform/graphics/filters/FEComposite.cpp:128
> > +        return FilterEffect::determineAbsolutePaintRect(filter);
> 
> Comments could help here, the code is not really understandable for me at the moment, why arithmetic  needs maxEffectRect(), why in/atop just the first effects absolutePaintRect etc.
> And also why you're only calling setAbsolutePaintRect() in the non-default case.
Added some comments to make clear why this is an optimization, and how it works.

> 
> > WebCore/platform/graphics/filters/FEComposite.cpp:130
> > +    setAbsolutePaintRect(repaintRect);
> 
> It's not clear to me why this function still returns a repaintRect.
> It seems it's not needed, the caller can just query absolutePaintRect() after calling determineAbsolutePaintRect.
determineAbsolutePaintRect doesn't return anything now. Fixed.

> 
> > WebCore/platform/graphics/filters/FEFlood.cpp:78
> > +    if (!color.red() && !color.green() && !color.blue())
> 
> Hm, it seems to me a helper function is missing in Color which does just that "bool isBlackAlpha" or something like that, which avoids checking red() green() blue() individually, which all do bit shifts on the RGBA quadruplet.
> It should be easier to find this out using a single query on the quadruplet. I'll leave for you to decide wheter you want to add that to Color right now.
Right, should be in Color, removed this from the patch.

> 
> > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:137
> > +        kernelSizeX = max(2U, static_cast<unsigned>(floor(stdX * gGaussianKernelFactor + 0.5f)));
> 
> Doesn't max<unsigned>(2, static_cast<unsigned>(...)) work? Just guessing here. If not please use "2u".
> Don't you want to use floorf here?
> 
> > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:141
> > +        kernelSizeY = max(2U, static_cast<unsigned>(floor(stdY * gGaussianKernelFactor + 0.5f)));
> 
> Ditto.
Works and is fixed.

> 
> > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:142
> > +    repaintRect.inflateX(3 * (kernelSizeX * 0.5f));
> 
> That needs a comment.
Comment added.

> 
> > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:173
> > +        kernelSizeX = max(2U, static_cast<unsigned>(floor(stdX * gGaussianKernelFactor + 0.5f)));
> 
> Same question as above, doesn't max<unsigned> work? If not use "2u".
> 
> > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:176
> > +    if (stdY)
> 
> Ditto.
Ditto. :-)

> 
> > WebCore/platform/graphics/filters/FEMorphology.cpp:80
> > +    repaintRect.inflateX(static_cast<int>(filter->horizontalScale(m_radiusX)));
> 
> static_cast<int> seems to be wrong here. Either floor or ceil, or sth. else, but not truncating. Correct me if I'm wrong.
> 
> > WebCore/platform/graphics/filters/FEMorphology.cpp:81
> > +    repaintRect.inflateY(static_cast<int>(filter->verticalScale(m_radiusY)));
> 
> Ditto.
Added floorf. Fixed.

> 
> > WebCore/platform/graphics/filters/FEMorphology.cpp:102
> > +    if (m_radiusX <= 0 || m_radiusY <= 0)
> 
> Are you sure you want the check here, and not below, after radiusX/Y are calculated?
yes, the spec want us to return, if the radius that was specified is <=0. But this can still happen on the transformation of the radii to absolute coordinates. But this is allowed here. (In my eyes it should be allowed in general, but this is an issue with the spec).

> 
> > WebCore/platform/graphics/filters/FEMorphology.cpp:105
> > +    int radiusX = static_cast<int>(filter->horizontalScale(m_radiusX));
> 
> Ditto, truncating seems wrong.
fixed.

> 
> > WebCore/platform/graphics/filters/FETurbulence.cpp:338
> > +    PaintingData paintingData(static_cast<long>(filter->horizontalScale(m_seed)), imageRect.size());
> 
> Why are you casting here?
Was wrong, fixed.

> 
> > WebCore/platform/graphics/filters/Filter.h:48
> > +        virtual float horizontalScale(float value) const { return value * m_filterResolution.width(); }
> > +        virtual float verticalScale(float value) const { return value * m_filterResolution.height(); }
> 
> These names could need some love. How about "applyHorizontalFilterResolutionScale".
After a discussion on IRC, I take applyHorizontalScale and applyVerticalScale now.

> 
> > WebCore/platform/graphics/filters/FilterEffect.cpp:79
> > +FloatRect FilterEffect::drawingRegionOfInputImage(const IntRect& srcRect) const
> 
> Seems unclear, why this is a FloatRect now? Should be an IntRect.
Is IntRect now.

> 
> > WebCore/platform/graphics/filters/FilterEffect.cpp:93
> > +    IntRect bufferRect = determineAbsolutePaintRect(filter);
> 
> There you go, just call determineAbsolutePaintRect(filter), then use IntRect bufferRect = absolutePaintRect();
> Can you avoid the local variable, is it only used in the ImageBuffer::create() call, or even below?
Done.

> 
> > WebCore/platform/graphics/filters/FilterEffect.h:68
> > +    IntRect maxEffectRect() const { return m_maxEffectRect; }
> 
> Hm, I'd love this to be more descriptive. Isn't it something like the minimal effect rect needed, rather than the max effect rect? :-)
maxEffectRect is the primitive subregion in absolute coordinates and also the clipping region for primitives in general. Because FilterEffect should be as independent of SVG Filter as possible, I decided to take another name.

> 
> > WebCore/platform/graphics/filters/FilterEffect.h:95
> > +    // FIXME: FETile still needs special handling.
> 
> Can you explain to me, what's still different about FETile? Is it the only exception?
Yes, feTile need a special handling according to the SVG Spec. See http://www.w3.org/TR/SVG/filters.html#FilterPrimitiveSubRegion and the 5th paragraph.

> 
> > WebCore/platform/graphics/filters/FilterEffect.h:115
> > +    // The maximum size of a filter primitive. On SVG this is the primitive subregion in absolute coordinate space.
> 
> s/On SVG/For SVG/
> 
> > WebCore/rendering/RenderSVGResourceFilter.cpp:191
> > +    // than kMaxFilterSize.
> 
> Do you need to break lines here?
fixed.

> 
> > WebCore/svg/graphics/filters/SVGFilter.cpp:27
> > +SVGFilter::SVGFilter(AffineTransform absoluteTransform, const FloatRect& absoluteSourceDrawingRegion, const FloatRect& targetBoundingBox, const FloatRect& filterRegion, bool effectBBoxMode)
> 
> Use const AffineTransform& here!
fixed.

> 
> > WebCore/svg/graphics/filters/SVGFilter.cpp:91
> > +    return value * filterResolution().width() * (m_absoluteFilterRegion.width() / m_filterRegion.width());
> 
> Useless braces.
removed.

> 
> > WebCore/svg/graphics/filters/SVGFilter.cpp:98
> > +    return value * filterResolution().height() * (m_absoluteFilterRegion.height() / m_filterRegion.height());
> 
> Useless braces.
Ditto.

> 
> > WebCore/svg/graphics/filters/SVGFilter.cpp:101
> > +PassRefPtr<SVGFilter> SVGFilter::create(AffineTransform absoluteTransform, const FloatRect& absoluteSourceDrawingRegion, const FloatRect& targetBoundingBox, const FloatRect& filterRegion, bool effectBBoxMode)
> 
> const AffineTransform&.
Fixed.

[Nikolas Zimmermann]

Comment on attachment 69890 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69890&action=review

Ok, I could give r+, but I'd like to review the second version first...

> WebCore/platform/graphics/filters/FEComposite.cpp:126
> +        // Arithmetic may influnce the comple filter primitive region. So we can't

typo: complete.

> WebCore/platform/graphics/filters/FEDisplacementMap.h:56
> +    virtual void determineAbsolutePaintRect(Filter*) { setAbsolutePaintRect(maxEffectRect()); }

FEDisplacmentMap, FEFlood, FEConvolveMatrix, FELighting, FETile, FETurbulence and SVGFEImage all need this function.
How about introducing "bool usesMaxEffectRectAsAbsolutePaintRect() const" returning true for those renderers. Then you don't need to override determineAbsolutePaintRect here.
But can instead use "if (usesMaxEffectRectAsAbsolutePaintRect()) m_absolutePaintRect = maxEffectRect(); " in FilterEffect directly.

Makes sense?

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:128
> +inline void calculateKernelSize(unsigned& kernelSizeX, unsigned& kernelSizeY, float stdX, float stdY, Filter* filter)

I'd prefer if you passed transformed stdX/stdY values here, and leave out the Filter* param.
Ok, just saw that you're using calculateKernelSize twice, so it's fine to do it here. Can you just move the Filter* param to be the first one :-) ?

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:141
> +    // inflates the absolute paint rect to much.

Please add "Compatible with FireFox trunk".

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:142
> +    if (kernelSizeX > 1000)

Introduce a static const unsigned kMaxKernelSize or something like this, on top of this file, like it's done for kMaxFilterSize.

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:159
> +    absolutePaintRect.inflateX(3 * (kernelSizeX * 0.5f));
> +    absolutePaintRect.inflateY(3 * (kernelSizeY * 0.5f));

Useless braces.

> WebCore/platform/graphics/filters/FEMorphology.cpp:81
> +    paintRect.inflateX(floorf(filter->applyHorizontalScale(m_radiusX)));
> +    paintRect.inflateY(floorf(filter->applyVerticalScale(m_radiusY)));

Why does this compile? You still need a static_cast<int>(floorf(....)), as the floorf return type is float, and paintRect is integer based.

> WebCore/platform/graphics/filters/FEMorphology.cpp:104
> +    int radiusX = floorf(filter->applyHorizontalScale(m_radiusX));

Ditto, need to cast to int first.

> WebCore/platform/graphics/filters/FEOffset.cpp:67
> +    FloatRect paintRect = inputEffect(0)->absolutePaintRect();

This is inconsistent, compared to eg. FEMorphology, which uses floorf after applyHorizontalScale, and operates on IntRect.
Here you are operating on FloatRects, and use enclosingIntRect afterwards.
I think the FEOffset solution is better, and should be deployed everywhere.

> WebCore/platform/graphics/filters/FETile.cpp:74
> +    if (!SVGImageBufferTools::createImageBuffer(tileRect, tileRect, tileImage, DeviceRGB))

We're sure DeviceRGB is correct here?

> WebCore/platform/graphics/filters/FETile.cpp:78
> +    tileImageContext->translate(-in->maxEffectRect().x(), -in->maxEffectRect().y());

maxEffectRect() should be cached in a local variable, same for in->maxEffectRect, it's used multiple times here.

> WebCore/platform/graphics/filters/FilterEffect.cpp:95
> +    m_effectBuffer = ImageBuffer::create(m_absolutePaintRect.size(), LinearRGB);

LinearRGB is used here, and DeviceRGB in FETile - not sure wheter we want this difference??

> WebCore/platform/graphics/filters/FilterEffect.h:95
> +    // FIXME: FETile still needs special handling.

Why is this a FIXME? Is it related to zoltans patch who introduces isFETile() and removes this special function?

> WebCore/rendering/RenderSVGResourceFilter.cpp:167
> +    // Eliminate shear of the absolute transformation matrix to increase the quality of results in feTile. 

The comment is wrong :-)
Eliminate shear of the absolute transformation matrix, to be able to produce unsheared tile images in feTile.
It's not about quality, there's no other way we could ever do tiling correctly :-)

> WebCore/rendering/RenderSVGResourceFilter.cpp:218
> +    // Even if the target objectBoundingBox() is empty, we still have to draw something.

s/something/the last effect result image in postApplyResource/.

> LayoutTests/ChangeLog:10
> +        primitiveUnits "objectBoundingBox" for stdDeviation on feGaussianBlu correctly.

typo: feGaussianBlur.

> LayoutTests/ChangeLog:11
> +        

One newline too much.

[Dirk Schulze]

(In reply to comment #13)
> (From update of attachment 69890 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69890&action=review
> 
> Ok, I could give r+, but I'd like to review the second version first...
> 
> > WebCore/platform/graphics/filters/FEComposite.cpp:126
> > +        // Arithmetic may influnce the comple filter primitive region. So we can't
> 
> typo: complete.
> 
> > WebCore/platform/graphics/filters/FEDisplacementMap.h:56
> > +    virtual void determineAbsolutePaintRect(Filter*) { setAbsolutePaintRect(maxEffectRect()); }
> 
> FEDisplacmentMap, FEFlood, FEConvolveMatrix, FELighting, FETile, FETurbulence and SVGFEImage all need this function.
> How about introducing "bool usesMaxEffectRectAsAbsolutePaintRect() const" returning true for those renderers. Then you don't need to override determineAbsolutePaintRect here.
> But can instead use "if (usesMaxEffectRectAsAbsolutePaintRect()) m_absolutePaintRect = maxEffectRect(); " in FilterEffect directly.
> 
> Makes sense?
It would mean introducing a new virtual function, without replacing another one. determineAbsolutePaintRect still needs to be virtual for effects like GaussianBlur, feOffset, feMorphology, peseudo primitives.

> 
> > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:128
> > +inline void calculateKernelSize(unsigned& kernelSizeX, unsigned& kernelSizeY, float stdX, float stdY, Filter* filter)
> 
> I'd prefer if you passed transformed stdX/stdY values here, and leave out the Filter* param.
> Ok, just saw that you're using calculateKernelSize twice, so it's fine to do it here. Can you just move the Filter* param to be the first one :-) ?
done.

> 
> > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:141
> > +    // inflates the absolute paint rect to much.
> 
> Please add "Compatible with FireFox trunk".
added.

> 
> > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:142
> > +    if (kernelSizeX > 1000)
> 
> Introduce a static const unsigned kMaxKernelSize or something like this, on top of this file, like it's done for kMaxFilterSize.
Took gMaxFilterSize regarding the comments on webkit-dev about static consts.

> 
> > WebCore/platform/graphics/filters/FEGaussianBlur.cpp:159
> > +    absolutePaintRect.inflateX(3 * (kernelSizeX * 0.5f));
> > +    absolutePaintRect.inflateY(3 * (kernelSizeY * 0.5f));
> 
> Useless braces.
right. Still have 3 * .. * 0.5 in the patch instead of 1.5. Otherwise it would be to confusing to read. The compiler should optimize it anyway.
> 
> > WebCore/platform/graphics/filters/FEMorphology.cpp:81
> > +    paintRect.inflateX(floorf(filter->applyHorizontalScale(m_radiusX)));
> > +    paintRect.inflateY(floorf(filter->applyVerticalScale(m_radiusY)));
> 
> Why does this compile? You still need a static_cast<int>(floorf(....)), as the floorf return type is float, and paintRect is integer based.
Don't know why it compiles. Fixed :-) Take enclosingIntRect at the end and use FloatRect instead of the IntRect. This is more exact.

> 
> > WebCore/platform/graphics/filters/FEMorphology.cpp:104
> > +    int radiusX = floorf(filter->applyHorizontalScale(m_radiusX));
> 
> Ditto, need to cast to int first.
fixed.

> 
> > WebCore/platform/graphics/filters/FEOffset.cpp:67
> > +    FloatRect paintRect = inputEffect(0)->absolutePaintRect();
> 
> This is inconsistent, compared to eg. FEMorphology, which uses floorf after applyHorizontalScale, and operates on IntRect.
> Here you are operating on FloatRects, and use enclosingIntRect afterwards.
> I think the FEOffset solution is better, and should be deployed everywhere.
Done.

> 
> > WebCore/platform/graphics/filters/FETile.cpp:74
> > +    if (!SVGImageBufferTools::createImageBuffer(tileRect, tileRect, tileImage, DeviceRGB))
> 
> We're sure DeviceRGB is correct here?
I tried it on MacOS 10.5, because this was confusing to me too. But it looked just correct with DeviceRGB. I was told, that CG maps the ColorSpace to linearRGB for its own.

> 
> > WebCore/platform/graphics/filters/FETile.cpp:78
> > +    tileImageContext->translate(-in->maxEffectRect().x(), -in->maxEffectRect().y());
> 
> maxEffectRect() should be cached in a local variable, same for in->maxEffectRect, it's used multiple times here.
Just cached the location of maxEffectRect() and in->maxEffectRect(), since we just need x() and y().

> 
> > WebCore/platform/graphics/filters/FilterEffect.cpp:95
> > +    m_effectBuffer = ImageBuffer::create(m_absolutePaintRect.size(), LinearRGB);
> 
> LinearRGB is used here, and DeviceRGB in FETile - not sure wheter we want this difference??
See comment above.

> 
> > WebCore/platform/graphics/filters/FilterEffect.h:95
> > +    // FIXME: FETile still needs special handling.
> 
> Why is this a FIXME? Is it related to zoltans patch who introduces isFETile() and removes this special function?
Thats right!

> 
> > WebCore/rendering/RenderSVGResourceFilter.cpp:167
> > +    // Eliminate shear of the absolute transformation matrix to increase the quality of results in feTile. 
> 
> The comment is wrong :-)
> Eliminate shear of the absolute transformation matrix, to be able to produce unsheared tile images in feTile.
> It's not about quality, there's no other way we could ever do tiling correctly :-)
Hehe, fixed.

> 
> > WebCore/rendering/RenderSVGResourceFilter.cpp:218
> > +    // Even if the target objectBoundingBox() is empty, we still have to draw something.
> 
> s/something/the last effect result image in postApplyResource/.
> 
> > LayoutTests/ChangeLog:10
> > +        primitiveUnits "objectBoundingBox" for stdDeviation on feGaussianBlu correctly.
> 
> typo: feGaussianBlur.
> 
> > LayoutTests/ChangeLog:11
> > +        
> 
> One newline too much.
Fixed.

[Dirk Schulze]

Created attachment 69902 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 69902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69902&action=review

> WebCore/WebCore.xcodeproj/project.pbxproj:21023
> +			developmentRegion = English;

Can you revert this line? Newer Xcodes fault :-)

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:38
>  static const float gGaussianKernelFactor = (3 * sqrtf(2 * piFloat) / 4.f);

While you're at it removes those braces, and say "3 / 4.f * sqrtf(2 * piFloat)" here :-)

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:152
> +    IntRect absolutePaintRect = inputEffect(0)->absolutePaintRect();

Stil a problem, use FloatRect absolutePaintRect here.

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:160
> +    absolutePaintRect.inflateX(3 * kernelSizeX * 0.5f);

Otherwhise this inflateX will produce artefacts.
How about using 1.5f * kernelSizeX here?

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:162
> +    setAbsolutePaintRect(absolutePaintRect);

Then use enclosingIntRect here.

> WebCore/platform/graphics/filters/FETile.cpp:76
> +    if (!SVGImageBufferTools::createImageBuffer(tileRect, tileRect, tileImage, DeviceRGB))

Won't DeviceRGB be problematic for non-cg platforms here? Still unsure...
RenderSVGResourceMasker uses LinearRGB for instance.

> WebCore/rendering/RenderSVGResourceFilter.cpp:167
> +    // Eliminate shear of the absolute transformation matrix, to be able to produce unsheared tile images in feTile.

s/in feTile/for feTile/

Other than these issues, it looks great! Looking forward to have non-pixelated filters :-)

[Dirk Schulze]

*** Bug 5526 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

Landed new pixel test baseline in r69188.

[Dirk Schulze]

Comment on attachment 69902 [details]
Patch

Landed with http://trac.webkit.org/changeset/69181

Fixes:
http://trac.webkit.org/changeset/69181
http://trac.webkit.org/changeset/69187
http://trac.webkit.org/changeset/69193
http://trac.webkit.org/changeset/69194