Bug 6021 – WebKit+SVG does not support filterRes attribute

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

WebKit+SVG does not support filterRes attribute
http://www.w3.org/TR/SVG/filters.html#FilterEffectsRegion

[Dirk Schulze]

Created attachment 43422 [details]
Implementation of filterRes

This is the implementation of filterRes. It scales the intermediate images used by the filter and it's effects to set the quality of a filter. It scales the values of any filter effect if needed too. According to the Spec (http://www.w3.org/TR/SVG11/filters.html#FilterUnitsAttribute) we can use filterRes to limit the quality to the needs of the device. I limit the size, so that the maximum size of any  intermediate image is 5000x5000 pixels. A didn't tested the maximum value that is possible, but 10,000x10,000 crashed on cairo. It is also a question of slowness not to have a to big limitation. This fixes bug 26380.
Other platforms can use a different limitations. A maximum size of 200x200 or 100x100 is maybe enough for mobile devices.

[Dirk Schulze]

Created attachment 43448 [details]
Implementation of filterRes

Sorry, _this_ patch checks for the biggest ImageBuffer that we will use and scales the filter down, so that all filters are in the maximum rect of 5000x5000. The scale factor for x and y are calculated independently.

[Nikolas Zimmermann]

Comment on attachment 43448 [details]
Implementation of filterRes

Jeeezz, what a large patch, thank god I'm on a nightshift with nothing else to do ;-)

Several comments:
- You need a much more detailed ChangeLog, as this is a large patch, ideally per file, this time.

> Index: WebCore/platform/graphics/filters/FEGaussianBlur.cpp
...
> +    sdx = std::max(sdx, static_cast<unsigned>(1));
> +    sdy = std::max(sdy, static_cast<unsigned>(1));

Why not just std::max(sdx/y, 1); ?

> Index: WebCore/platform/graphics/filters/Filter.h
> ===================================================================
> --- WebCore/platform/graphics/filters/Filter.h	(revision 51070)
> +++ WebCore/platform/graphics/filters/Filter.h	(working copy)
> @@ -22,6 +22,7 @@
>  
>  #if ENABLE(FILTERS)
>  #include "FloatRect.h"
> +#include "FloatSize.h"
>  #include "ImageBuffer.h"
>  #include "StringHash.h"
>  
> @@ -40,15 +41,21 @@ namespace WebCore {
>          void setSourceImage(PassOwnPtr<ImageBuffer> sourceImage) { m_sourceImage = sourceImage; }
>          ImageBuffer* sourceImage() { return m_sourceImage.get(); }
>  
> +        FloatSize filterRes() const { return m_filterRes; }
> +        void setFilterRes(const FloatSize& filterRes) { m_filterRes = filterRes; }

Hrm, I'd prefer to completly avoid abbrevations, aka. change it to "setFilterResolution" here.
Filter* has no direct connection to SVG, and it's not in the "// SVG specific" section below, so why not rename it?
We don't need to reuse SVG naming conventions here.

  
> Index: WebCore/svg/SVGFilterElement.cpp
...
> +    else if (attr->name() == SVGNames::filterResAttr) {
> +        float x, y;
> +        if (parseNumberOptionalNumber(value, x, y)) {
> +            setFilterResXBaseValue(x);
> +            setFilterResYBaseValue(y);
> +        }

An else case reporting errors through SVGDocumentExtensions seems to be missing here.

> +    if (this->hasAttribute(SVGNames::filterResAttr)) {
s/this->//

> +        m_filter->setHasFilterRes(true);
> +        m_filter->setFilterResSize(FloatSize(filterResX(), filterResY()));

Same thoughts as in Filter.h:
s/setHasFilterRes/setHasFilterResolution/
s/setFilterResSize/setFilterResolution/ (Size prefix is superflous IMHO)

> Index: WebCore/svg/graphics/SVGResourceFilter.cpp
> ===================================================================
> --- WebCore/svg/graphics/SVGResourceFilter.cpp	(revision 51070)
> +++ WebCore/svg/graphics/SVGResourceFilter.cpp	(working copy)
> @@ -35,6 +35,10 @@
>  #include "SVGRenderTreeAsText.h"
>  #include "SVGFilterPrimitiveStandardAttributes.h"
>  
> +#define MAX_FILTER_SIZE 5000.f

Evil defines, please use a static constant here. I am not sure about our naming convention, it may have a g, or k prefix, please check for common usage of static constants in other files.

> +bool SVGResourceFilter::matchesMaxImageSize(const FloatSize& size) {

{ neeeds to go on a new line

> +    bool matchesFilterSize = true;
> +    if (size.width() > MAX_FILTER_SIZE) {
> +        m_scaleX *= MAX_FILTER_SIZE / size.width();
> +        matchesFilterSize = false;
> +    }
> +    if (size.height() > MAX_FILTER_SIZE) {
> +        m_scaleY *= MAX_FILTER_SIZE / size.height();
> +        matchesFilterSize = false;
> +    }
> +
> +    return matchesFilterSize;
> +}

matchesMaxImageSize is not a good name. How about "fitsInMaximumImageSize" or something similar.
Matches sounds too much like "is equal".


>  void SVGResourceFilter::prepareFilter(GraphicsContext*& context, const RenderObject* object)
...
> +    // scale to big sourceImage size to MAX_FILTER_SIZE
> +    tempSourceRect.scale(m_scaleX, m_scaleY);
> +    matchesMaxImageSize(tempSourceRect.size());

Hrm, you're ignoring the return value of matchesMaxImageSize here??


> +
>      // prepare Filters
>      m_filter = SVGFilter::create(targetRect, m_filterBBox, m_effectBBoxMode);
> +    m_filter->setFilterRes(FloatSize(m_scaleX, m_scaleY));
>  
>      FilterEffect* lastEffect = m_filterBuilder->lastEffect();
> -    if (lastEffect)
> +    if (lastEffect) {
>          lastEffect->calculateEffectRect(m_filter.get());
> +        // at least one FilterEffect has a to big image size,
s/to/too/

>  void SVGResourceFilter::applyFilter(GraphicsContext*& context, const RenderObject*)
>  {
> +    if (!m_scaleX || !m_scaleY || !m_filterBBox.width() || !m_filterBBox.height())
> +        return;

The same check is used as above, might make sense to refactor in a simple "static inline bool shouldProcessFilter()" helper function?

> Index: WebCore/svg/graphics/SVGResourceFilter.h
> ===================================================================
> --- WebCore/svg/graphics/SVGResourceFilter.h	(revision 51070)
> +++ WebCore/svg/graphics/SVGResourceFilter.h	(working copy)
> @@ -53,6 +53,9 @@ public:
>      
>      virtual SVGResourceType resourceType() const { return FilterResourceType; }
>  
> +    void setFilterResSize(const FloatSize& filterResSize) { m_filterResSize = filterResSize; }
> +    void setHasFilterRes(bool filterRes) { m_filterRes = filterRes; }
Same rename suggestions apply as above.

> +    bool matchesMaxImageSize(const FloatSize&);
Ditto.

> +    FloatSize m_filterResSize;
I'd suggest just "m_filterResolution" here.

> Index: WebCore/svg/graphics/filters/SVGFilter.h
...
> -        bool effectBoundingBoxMode() const { return m_effectBBoxMode; }
> +        virtual bool effectBoundingBoxMode() const { return m_effectBBoxMode; }
>  
> -        FloatRect filterRegion() const { return m_filterRect; }
> -        FloatRect sourceImageRect() const { return m_itemBox; }
> -        void calculateEffectSubRegion(FilterEffect*) const;
> +        virtual FloatRect filterRegion() const { return m_filterRect; }
> +        virtual FloatRect sourceImageRect() const { return m_itemBox; }
> +
> +        virtual FloatSize maxImageSize() const { return m_maxImageSize; }
> +        virtual void calculateEffectSubRegion(FilterEffect*);

I guess these functions have already been marked virtual in the base class, and this is just for clarification?

> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 51126)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,25 @@
> [phone]  Dirk Schulze  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        The first two example have a filter with a size of 20000x20000. We limit
> +        the quality to be able to render this filter.
> +        On the third example the filter qulitiy is limited by different values
> +        to test the bahavior of effects with kernels or other values that need
> +        to be scaled too. It also tests the correctness of the subRegion calculation
> +        for filters with more than one effect.
Better name the tests, instead of relying on the reviewers ability to count ;-)

Just realized the patch is not that big, it's mostly the pngs, that create a frightning big patch size.
Please fix these issues, and upload a new patch. I think the general concept looks sane.

Wouldn't hurt if Simon Fraser / Eric Seidel / Oliver Hunt may have another look. I'll cc them soon.

r- because of the mentioned issues above.

[Dirk Schulze]

(In reply to comment #3)
> (From update of attachment 43448 [details])
> Jeeezz, what a large patch, thank god I'm on a nightshift with nothing else to
> do ;-)
I'll split out the LayoutTests into another patch, to shrink the patch to ~38k

> > Index: WebCore/platform/graphics/filters/FEGaussianBlur.cpp
> ...
> > +    sdx = std::max(sdx, static_cast<unsigned>(1));
> > +    sdy = std::max(sdy, static_cast<unsigned>(1));
> 
> Why not just std::max(sdx/y, 1); ?
Do not understand what you mean? the static_cast was neccessary to get a unsigned 1. What is y in your example?

> > Index: WebCore/svg/SVGFilterElement.cpp
> ...
> > +    else if (attr->name() == SVGNames::filterResAttr) {
> > +        float x, y;
> > +        if (parseNumberOptionalNumber(value, x, y)) {
> > +            setFilterResXBaseValue(x);
> > +            setFilterResYBaseValue(y);
> > +        }
> 
> An else case reporting errors through SVGDocumentExtensions seems to be missing
> here.
I took a look at other SVG*Elements, we never handle parsing errors. We just ignore the attribute, if the arguments are not parseable.

> > Index: WebCore/svg/graphics/filters/SVGFilter.h
> ...
> > -        bool effectBoundingBoxMode() const { return m_effectBBoxMode; }
> > +        virtual bool effectBoundingBoxMode() const { return m_effectBBoxMode; }
> >  
> > -        FloatRect filterRegion() const { return m_filterRect; }
> > -        FloatRect sourceImageRect() const { return m_itemBox; }
> > -        void calculateEffectSubRegion(FilterEffect*) const;
> > +        virtual FloatRect filterRegion() const { return m_filterRect; }
> > +        virtual FloatRect sourceImageRect() const { return m_itemBox; }
> > +
> > +        virtual FloatSize maxImageSize() const { return m_maxImageSize; }
> > +        virtual void calculateEffectSubRegion(FilterEffect*);
> 
> I guess these functions have already been marked virtual in the base class, and
> this is just for clarification?
Yes. Simon suggested this on ImageBufferFilter. I thougt it's the best to have a common style on all childs of Filter.

> Wouldn't hurt if Simon Fraser / Eric Seidel / Oliver Hunt may have another
> look. I'll cc them soon.
I already added all persons, that responsed to bug 26380.

I'll upload a fixed patch.

[Dirk Schulze]

Created attachment 43495 [details]
Implementation of filterRes

Fixed patch. LayoutTests following with the next patch.

[Dirk Schulze]

Created attachment 43509 [details]
Tests for filterRes

The LayoutTests for filterRes.

[Nikolas Zimmermann]

Comment on attachment 43495 [details]
Implementation of filterRes

Looks nice, especially the ChangeLog, so much easier to understand the patch now :-)
r=me, if you change the value to be 5000, as the comments say, it's 3000 in the code at the moment, please fix before landing.

[Nikolas Zimmermann]

Comment on attachment 43509 [details]
Tests for filterRes

LGTM, r=me.

[WebKit Commit Bot]

Comment on attachment 43509 [details]
Tests for filterRes

Clearing flags on attachment: 43509

Committed r51301: <http://trac.webkit.org/changeset/51301>

[Dirk Schulze]

landed in r51310.