[webkit-reviews] review denied: [Bug 31370] SVG filter effects need smarter size calculation : [Attachment 69890] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 31370: SVG filter effects need smarter size calculation
https://bugs.webkit.org/show_bug.cgi?id=31370

Attachment 69890: Patch
https://bugs.webkit.org/attachment.cgi?id=69890&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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.