Bug 6021

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.

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 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.

(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.

Created attachment 43495 [details] Implementation of filterRes Fixed patch. LayoutTests following with the next patch.

Created attachment 43509 [details] Tests for filterRes The LayoutTests for filterRes.

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 on attachment 43509 [details] Tests for filterRes LGTM, r=me.

Comment on attachment 43509 [details] Tests for filterRes Clearing flags on attachment: 43509 Committed r51301: <http://trac.webkit.org/changeset/51301>

landed in r51310.