Bug 47174 – Moving all bounding box related calculation to RenderSVGResourceFilterPrimitive

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

[Zoltan Herczeg]

The whole logic should be in RenderSVGResourceFilterPrimitive. But one patch is not enough for the whole refactor.

[Zoltan Herczeg]

Created attachment 69780 [details]
patch

This patch is tested and working on mac leopard. I think this is a good first step, since it keeps the logic (in a refactored way). There is no more regression, and I think it is finally ready for review.

[WebKit Review Bot]

Attachment 69780 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:61:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Can you wait before you update the patch please? The patch on bug 31370 is nearly done.

[Dirk Schulze]

Comment on attachment 69780 [details]
patch

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

Good start. But you still forgot some of the things we spoke of, like calculating the subregions in buildPrimitives and not to make the functions in RenderSVGResourceFilterPrimitives static.

> WebCore/platform/graphics/filters/Filter.h:50
>          virtual bool effectBoundingBoxMode() const = 0;

Do we still need this function?

> WebCore/rendering/RenderSVGResourceFilter.cpp:200
> +    FloatRect subRegion = lastEffect->repaintRectInLocalCoordinates();
>      // At least one FilterEffect has a too big image size,
>      // recalculate the effect sizes with new scale factors.
> -    if (!fitsInMaximumImageSize(filterData->filter->maxImageSize(), scale)) {
> +    if (!fitsInMaximumImageSize(subRegion.size(), scale)) {

Interesting, sound like a good idea to take repaintRectInLocalCoordinates (alias absolutePaintRect). But please name the variable paintRect.

> WebCore/rendering/RenderSVGResourceFilter.cpp:202
> +        RenderSVGResourceFilterPrimitive::determineFilterPrimitiveSubregion(lastEffect, filterData->filter.get());

We already talked about it on IRC. I wouldn't make this static. Its better to manage it in the class.

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:47
> +    if (effect->isSourceInput()) {
> +        // For SourceAlpha and SourceGraphic
> +        FloatRect clippedSourceRect = filter->sourceImageRect();
> +        if (filter->sourceImageRect().x() < filter->filterRegion().x())
> +            clippedSourceRect.setX(filter->filterRegion().x());
> +        if (filter->sourceImageRect().y() < filter->filterRegion().y())
> +            clippedSourceRect.setY(filter->filterRegion().y());
> +        effect->setFilterPrimitiveSubregion(clippedSourceRect);
> +        clippedSourceRect.scale(filter->filterResolution().width(), filter->filterResolution().height());
> +        effect->setRepaintRectInLocalCoordinates(clippedSourceRect);
> +        return filter->filterRegion();
> +    }

This isn't needed anymore

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:64
> +    if (!effect->isTile()) {
> +        // FETurbulence, FEImage and FEFlood don't have input effects, take the filter region as unite rect.
> +        if (unsigned numberOfInputEffects = effect->inputEffects().size()) {
> +            for (unsigned i = 0; i < numberOfInputEffects; ++i)
> +                uniteRect.unite(determineFilterPrimitiveSubregion(effect->inputEffect(i), filter));
> +        } else
> +            uniteRect = filter->filterRegion();
> +    }
> +    else {
> +        determineFilterPrimitiveSubregion(effect->inputEffect(0), filter);
> +        uniteRect = filter->filterRegion();
> +    }

Maybe we should move this to an inline

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:95
> +    if (filter->effectBoundingBoxMode()) {
> +        newSubRegion = uniteRect;
> +        // Avoid the calling of a virtual method several times.
> +        FloatRect targetBoundingBox = filter->sourceImageRect();
> +
> +        if (effect->hasX())
> +            newSubRegion.setX(targetBoundingBox.x() + subRegionBoundingBox.x() * targetBoundingBox.width());
> +
> +       if (effect->hasY())
> +            newSubRegion.setY(targetBoundingBox.y() + subRegionBoundingBox.y() * targetBoundingBox.height());
> +
> +        if (effect->hasWidth())
> +            newSubRegion.setWidth(subRegionBoundingBox.width() * targetBoundingBox.width());
> +
> +        if (effect->hasHeight())
> +            newSubRegion.setHeight(subRegionBoundingBox.height() * targetBoundingBox.height());
> +    } else {
> +        if (!effect->hasX())
> +            newSubRegion.setX(uniteRect.x());
> +
> +        if (!effect->hasY())
> +            newSubRegion.setY(uniteRect.y());
> +
> +        if (!effect->hasWidth())
> +            newSubRegion.setWidth(uniteRect.width());
> +
> +        if (!effect->hasHeight())
> +            newSubRegion.setHeight(uniteRect.height());
> +    }
> +

We really don't want the effects to store hasX, hasY and so on anymore. You can ask your node() directly now.

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:105
> +    FloatRect scaledSubRegion(newSubRegion);
> +    scaledSubRegion.scale(filter->filterResolution().width(), filter->filterResolution().height());
> +    effect->setRepaintRectInLocalCoordinates(scaledSubRegion);
> +    return newSubRegion;
>  }

I already begun to give the variables better name. Maybe you can continue this here.

> WebCore/rendering/RenderSVGResourceFilterPrimitive.h:43
> +    RenderSVGResourceFilterPrimitive(SVGFilterPrimitiveStandardAttributes* filterPrimitiveElement)
> +        : RenderSVGHiddenContainer(filterPrimitiveElement)
> +    {
> +    }
> +

I would move the ctor to the cpp. Looks more like the other classes. Not sure if we have a style rule for it yet. But still..

> WebCore/rendering/RenderSVGResourceFilterPrimitive.h:46
> +    static FloatRect determineFilterPrimitiveSubregion(FilterEffect* effect, Filter* filter);

hm...

[Zoltan Herczeg]

> We already talked about it on IRC. I wouldn't make this static. Its better to manage it in the class.

Me neither. But moving all the logic to one file caused enough regression fails to fix them in one patch. That is why I tried to stress that the static function is temporary.

[Dirk Schulze]

(In reply to comment #5)
> > We already talked about it on IRC. I wouldn't make this static. Its better to manage it in the class.
> 
> Me neither. But moving all the logic to one file caused enough regression fails to fix them in one patch. That is why I tried to stress that the static function is temporary.

If you have a followup that fixes this, I'm fine with a temporary static function. Over read this in you ChangeLog, sorry.
To my last comment. You can't use absolutePaintRect, since it is calculated during the drawing phase (on calling FilterEffect::apply(Filter*)). You have to use maxEffectRect, the subregion in absolute coordinate space.

[Zoltan Herczeg]

Created attachment 71283 [details]
patch 2

Still keeping the static function for the first step.

[Dirk Schulze]

Comment on attachment 71283 [details]
patch 2

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

Just style issues remaining. Great Patch! Looking forward to see a fixed version of this patch and a followup with turning determineFilterPrimitiveSubregion into a member of the Renderer. r- for now.

> WebCore/platform/graphics/filters/ImageBufferFilter.h:-46
> -    virtual FloatSize maxImageSize() const { return FloatSize(); }

This file doesn't exist any longer. Revert this file in your local copy and update to trunk. Shouldn't cause any problems.

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:39
> +    FloatRect subRegionBoundingBox = effect->effectBoundaries();
> +    FloatRect newSubRegion = subRegionBoundingBox;

We use just subregion, not subRegion now.

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:49
> +    }
> +    else {

} else {

> WebCore/rendering/RenderSVGResourceFilterPrimitive.h:46
> +    // These two arguments are depend on the RenderObject

s/depend/dependent/ or just they depend on...

> WebCore/svg/graphics/filters/SVGFilter.cpp:@
>  SVGFilter::SVGFilter(const AffineTransform& absoluteTransform, const FloatRect&

Remove the SVGFEImage.h include in this file.

[Zoltan Herczeg]

Created attachment 71533 [details]
patch

Thanks for the review. The updated patch is attached.

[Dirk Schulze]

Comment on attachment 71533 [details]
patch

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

LGTM. r=me. Please fix the name of absoluteSubRegion. You can do it before uploading it.

> WebCore/rendering/RenderSVGResourceFilterPrimitive.cpp:85
> +    FloatRect absoluteSubRegion = filter->mapLocalRectToAbsoluteRect(subregion);

s/absoluteSubRegion/absoluteSubregion/

[Zoltan Herczeg]

Landed in http://trac.webkit.org/changeset/70296

Closing bug.