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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Oct 2 00:47:07 PDT 2010 

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 69566: Patch
https://bugs.webkit.org/attachment.cgi?id=69566&action=review

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

More information about the webkit-reviews mailing list