Bug 6021 - WebKit+SVG does not support filterRes attribute
Summary: WebKit+SVG does not support filterRes attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P4 Normal
Assignee: Nobody
URL: http://www.w3.org/TR/SVG/filters.html...
Keywords:
Depends on:
Blocks: 68469 26389
  Show dependency treegraph
 
Reported: 2005-12-09 00:17 PST by Eric Seidel (no email)
Modified: 2014-05-12 05:54 PDT (History)
7 users (show)

See Also:


Attachments
Implementation of filterRes (403.56 KB, patch)
2009-11-18 02:57 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Implementation of filterRes (409.10 KB, patch)
2009-11-18 12:35 PST, Dirk Schulze
zimmermann: review-
Details | Formatted Diff | Diff
Implementation of filterRes (42.83 KB, patch)
2009-11-19 04:00 PST, Dirk Schulze
zimmermann: review+
zimmermann: commit-queue-
Details | Formatted Diff | Diff
Tests for filterRes (370.87 KB, patch)
2009-11-19 08:54 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2005-12-09 00:17:23 PST
WebKit+SVG does not support filterRes attribute
http://www.w3.org/TR/SVG/filters.html#FilterEffectsRegion
Comment 1 Dirk Schulze 2009-11-18 02:57:51 PST
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.
Comment 2 Dirk Schulze 2009-11-18 12:35:18 PST
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.
Comment 3 Nikolas Zimmermann 2009-11-18 16:06:33 PST
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 @@
> +2009-11-18  Dirk Schulze  <krit@webkit.org>
> +
> +        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.
Comment 4 Dirk Schulze 2009-11-18 23:49:49 PST
(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.
Comment 5 Dirk Schulze 2009-11-19 04:00:54 PST
Created attachment 43495 [details]
Implementation of filterRes

Fixed patch. LayoutTests following with the next patch.
Comment 6 Dirk Schulze 2009-11-19 08:54:36 PST
Created attachment 43509 [details]
Tests for filterRes

The LayoutTests for filterRes.
Comment 7 Nikolas Zimmermann 2009-11-22 17:20:56 PST
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.
Comment 8 Nikolas Zimmermann 2009-11-22 17:22:46 PST
Comment on attachment 43509 [details]
Tests for filterRes

LGTM, r=me.
Comment 9 WebKit Commit Bot 2009-11-22 17:34:06 PST
Comment on attachment 43509 [details]
Tests for filterRes

Clearing flags on attachment: 43509

Committed r51301: <http://trac.webkit.org/changeset/51301>
Comment 10 Dirk Schulze 2009-11-23 09:28:11 PST
landed in r51310.