SVG filter subregions are bigger than the surfaces we need to propagate through filters

RESOLVED FIXED

Status

()

Core
SVG
P1
normal
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

({perf})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

Resig's "World of Ecmascript" diagram sets primitiveUnits="userSpaceOnUse" and default x/y/width/height on the <filter> elements, and applies filters to a number of elements in the document. Our implementation pretty much follows the spec directly, creating a temporary surface 120% of the size of the viewport in each direction and doing the blur over that surface for each element!

We should optimize the filter implementation by reducing the filter subregions to the minimum size possible. We'll still need to be able to obtain the as-per-spec subregions because the behaviour of elements like feTile depends on them, and of course we'll need to ensure that any computed filter subregions are clipped by a specified subregion.

Updated

10 years ago
Keywords: perf
Flags: blocking1.9?
I think we pretty much decided to make this block
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
I experimented with an approach where we compute a bounding box for each filter image, but it gets complicated because filter processing often wants the input and output images to have the same size, for simplicity ... making that happen requires, in some cases, extra copies to resize input images, which we would rather avoid. The simplest approach with fewest performance gotchas seems to be to make all images the same size --- the minimum size we can get away with. This will require forward propagation of bounding boxes through the entire filter chain, after which we can compute the union of all those bounding boxes which will be the rectangle we use for all temporary surfaces.
Created attachment 302097 [details] [diff] [review]
fix

Tor, let me know if you can't review this soon (and let me know who else I should try). It's a pretty big patch but I think it's not all that complicated at root. Especially let me know where you need more comments/documentation. And also please let me know where I can find more filter testcases ... do we have filter tests in the tree anywhere? Seems like they could be reftested?
Attachment #302097 - Flags: superreview?(tor)
Attachment #302097 - Flags: review?(tor)
I filed bug 416305 on optimizing alpha-only blurs --- one additional piece of low-hanging filter performance fruit that we could/should take for 1.9.
Whiteboard: [needs review]
(In reply to comment #3)
> Created an attachment (id=302097) [details]
> fix
> 
> also please let me know where I can find more filter testcases ... do we have
> filter tests in the tree anywhere? Seems like they could be reftested?

I just added some to test rendering invalid filters. That's all there is in the tree as far as I know.
Comment on attachment 302097 [details] [diff] [review]
fix

I'm happy to provide my 2c...

>   /*
>    * Copy subregion from aSrc to aDest.
>    current sourceImage to targetImage
>    */
>   void CopyImageSubregion(PRUint8 *aDest, const PRUint8 *aSrc);
>+  
>+  nsSVGFilterInstance* GetInstance() { return mInstance; }

Wouldn't it be more consistent to pass instance to FinishScalingFilter and not have the above method.

>nsSVGFilterResource::nsSVGFilterResource(nsSVGFE *aFilter,
>                                          nsSVGFilterInstance* aInstance) :
>   mTargetImage(nsnull),
>   mFilter(aFilter),
>   mInstance(aInstance),
>   mSourceData(nsnull),
>   mTargetData(nsnull),
>-  mWidth(0),
>-  mHeight(0),
>-  mStride(0)
>+  mWidth(aInstance->GetSurfaceWidth()),
>+  mHeight(aInstance->GetSurfaceHeight()),
>+  mStride(aInstance->GetSurfaceStride())
> {
> }

Perhaps mWidth, mHeight and mStride should go altogether and the GetDataStride, GetWidth, GetHeight methods return mInstance->GetSurfaceWidth etc. Even more radical would be to use the instance methods directly and remove GetDataStride etc although that might be too complicated.

>+  nsIntRect filterResRect(0, 0, mInstance->GetFilterResX(), mInstance->GetFilterResY());
>   mInstance->GetFilterSubregion(mFilter, 
>                                 mFilter->SubregionIsUnionOfRegions() ?
>-                                  mSourceRegion : nsRect(0, 0, mWidth, mHeight),
>+                                  mSourceRegion : filterResRect,
>                                 &mFilterSubregion);

I can infer why this next bit here from the GetSurfaceRect comment but it would be nice to reiterate that in a comment line here. 

>+  mSurfaceRect = mInstance->GetSurfaceRect();
>+  nsIntPoint offset = mSurfaceRect.TopLeft();
>+  mSurfaceRect.IntersectRect(mSurfaceRect, mFilterSubregion);
>+  mSurfaceRect -= offset;
> 

> //----------------------------------------------------------------------
> // nsSVGElement methods
> 
> nsSVGElement::LengthAttributesInfo
> nsSVGFE::GetLengthInfo()
> {
>   return LengthAttributesInfo(mLengthAttributes, sLengthInfo,
>                               NS_ARRAY_LENGTH(sLengthInfo));
> }
> 
>+nsIntRect
>+nsSVGFE::ComputeTargetBBox(const nsTArray<nsIntRect>& aSourceBBoxes,
>+        const nsSVGFilterInstance& aInstance)
>+{
>+  nsIntRect r;
>+  for (PRUint32 i = 0; i < aSourceBBoxes.Length(); ++i) {
>+    r.UnionRect(r, aSourceBBoxes[i]);
>+  }
>+  return r;
>+}
>+
>+void
>+nsSVGFE::GetSourceImageNames(nsTArray<nsIDOMSVGAnimatedString*>* aSources)
>+{
>+}
>+

Move these methods up to somewhere under the // Implementation comment rather than having them in the // nsSVGElement methods bit.


> //---------------------Gaussian Blur------------------------
> 
> typedef nsSVGFE nsSVGFEGaussianBlurElementBase;
> 
> class nsSVGFEGaussianBlurElement : public nsSVGFEGaussianBlurElementBase,
>-                                   public nsIDOMSVGFEGaussianBlurElement,
>-                                   public nsISVGFilter

Shouldn't nsISVGFilter be in here somewhere with lots of - signs as it will cease to be as part of this patch won't it?

>   void BoxBlurH(PRUint8 *aInput, PRUint8 *aOutput,
>-                PRInt32 aStride, nsRect aRegion,
>+                PRInt32 aStride, const nsIntRect& aRegion,
>                 PRUint32 leftLobe, PRUint32 rightLobe);
> 
>   void BoxBlurV(PRUint8 *aInput, PRUint8 *aOutput,
>-                PRInt32 aStride, nsRect aRegion,
>+                PRInt32 aStride, const nsIntRect& aRegion,
>                 unsigned topLobe, unsigned bottomLobe);

while you are changing this perhaps you could make topLobe and bottomLobe PRUint32?

> void
> nsSVGFEGaussianBlurElement::BoxBlurV(PRUint8 *aInput, PRUint8 *aOutput,
>-                                     PRInt32 aStride, nsRect aRegion,
>+                                     PRInt32 aStride, const nsIntRect &aRegion,
>                                      unsigned topLobe, unsigned bottomLobe)

and here of course.

>-  PRUint32 dX, dY;
>-  dX = (PRUint32) floor(aStdX * 3*sqrt(2*M_PI)/4 + 0.5);
>-  dY = (PRUint32) floor(aStdY * 3*sqrt(2*M_PI)/4 + 0.5);
>+  *aDX = PRUint32(floor(stdX * 3*sqrt(2*M_PI)/4 + 0.5));
>+  *aDY = PRUint32(floor(stdY * 3*sqrt(2*M_PI)/4 + 0.5));

Maybe we could break out 3 *sqrt(2*M_PI)/4 into a static const scaling factor?

> class nsSVGFEFuncGElement : public nsSVGComponentTransferFunctionElement,
>                             public nsIDOMSVGFEFuncGElement
> {
>   friend nsresult NS_NewSVGFEFuncGElement(nsIContent **aResult,
>-                                          nsINodeInfo *aNodeInfo);
>+                                           nsINodeInfo *aNodeInfo);

???

...

A comment about offset would be nice here.

>+  nsIntPoint offset(instance->GetSurfaceRect().x - tile.x + tile.width,
>+                    instance->GetSurfaceRect().y - tile.y + tile.height);
>   for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
>-    PRUint32 tileY = tile.y + (y - tile.y + tile.height) % tile.height;
>+    PRUint32 tileY = tile.y + (y + offset.y) % tile.height;
>     for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
>-      PRUint32 tileX = tile.x + (x - tile.x + tile.width) % tile.width;
>+      PRUint32 tileX = tile.x + (x + offset.x) % tile.width;
>       *(PRUint32*)(targetData + y * stride + 4 * x) =
>         *(PRUint32*)(sourceData + tileY * stride + 4 * tileX);
>     }
>   }

...

>+    nsIntRect r;
>+    r.IntersectRect(entry->mRegion, mSurfaceRect);
>+    r -= mSurfaceRect.TopLeft();
>+
>     if (entry->mColorModel.mAlphaChannel == ColorModel::PREMULTIPLIED)
>-      nsSVGUtils::UnPremultiplyImageDataAlpha(data, stride, entry->mRegion);
>+      nsSVGUtils::UnPremultiplyImageDataAlpha(data, stride, r);
> 
>     if (aRequiredColorModel.mColorSpace != entry->mColorModel.mColorSpace) {
> 
>       if (aRequiredColorModel.mColorSpace == ColorModel::LINEAR_RGB)
>-        nsSVGUtils::ConvertImageDataToLinearRGB(data, stride, entry->mRegion);
>+        nsSVGUtils::ConvertImageDataToLinearRGB(data, stride, r);
>       else
>-        nsSVGUtils::ConvertImageDataFromLinearRGB(data, stride, entry->mRegion);
>+        nsSVGUtils::ConvertImageDataFromLinearRGB(data, stride, r);
>     }
>     if (aRequiredColorModel.mAlphaChannel == ColorModel::PREMULTIPLIED)
>-      nsSVGUtils::PremultiplyImageDataAlpha(data, stride, entry->mRegion);
>+      nsSVGUtils::PremultiplyImageDataAlpha(data, stride, r);

In that case all the nsSVGUtils methods here should take an nsIntRect also change the surface nsRect to nsIntRect in nsSVGMaskFrame.

>+++ mozilla-trunk/layout/svg/base/src/nsSVGFilterInstance.h	2008-02-08 21:39:53.000000000 +1300
>@@ -60,76 +60,94 @@ public:
>     enum AlphaChannel { UNPREMULTIPLIED, PREMULTIPLIED };
> 
>     ColorModel(ColorSpace aColorSpace, AlphaChannel aAlphaChannel) :
>       mColorSpace(aColorSpace), mAlphaChannel(aAlphaChannel) {}
>     PRBool operator==(const ColorModel& aOther) const {
>       return mColorSpace == aOther.mColorSpace &&
>              mAlphaChannel == aOther.mAlphaChannel;
>     }
>     ColorSpace   mColorSpace;
>     PRPackedBool mAlphaChannel;

Oops, that should be AlphaChannel mAlphaChannel.

>   void GetFilterBox(float *x, float *y, float *width, float *height) {

Could be a const method too.

>+  const nsIntRect& GetSurfaceRect() { return mSurfaceRect; }
>+  PRInt32 GetSurfaceWidth() { return mSurfaceRect.width; }
>+  PRInt32 GetSurfaceHeight() { return mSurfaceRect.height; }
>+  PRInt32 GetSurfaceStride() { return mSurfaceStride; }
>+  
>+  PRUint32 GetFilterResX() { return mFilterResX; }
>+  PRUint32 GetFilterResY() { return mFilterResY; }

const methods for all.
And one thing on a more general note, should we do something equally clever like this for masks and possibly patterns?
(In reply to comment #5)
> (In reply to comment #3)
> > Created an attachment (id=302097) [details] [details]
> > fix
> > 
> > also please let me know where I can find more filter testcases ... do we have
> > filter tests in the tree anywhere? Seems like they could be reftested?
> 
> I just added some to test rendering invalid filters. That's all there is in the
> tree as far as I know.

Erk. Care to write some? I guess I'll try to write some too but the more the merrier.

(In reply to comment #6)
> (From update of attachment 302097 [details] [diff] [review])
> >   /*
> >    * Copy subregion from aSrc to aDest.
> >    current sourceImage to targetImage
> >    */
> >   void CopyImageSubregion(PRUint8 *aDest, const PRUint8 *aSrc);
> >+  
> >+  nsSVGFilterInstance* GetInstance() { return mInstance; }
> 
> Wouldn't it be more consistent to pass instance to FinishScalingFilter and not
> have the above method.

Yeah OK.

> >nsSVGFilterResource::nsSVGFilterResource(nsSVGFE *aFilter,
> >                                          nsSVGFilterInstance* aInstance) :
> >   mTargetImage(nsnull),
> >   mFilter(aFilter),
> >   mInstance(aInstance),
> >   mSourceData(nsnull),
> >   mTargetData(nsnull),
> >-  mWidth(0),
> >-  mHeight(0),
> >-  mStride(0)
> >+  mWidth(aInstance->GetSurfaceWidth()),
> >+  mHeight(aInstance->GetSurfaceHeight()),
> >+  mStride(aInstance->GetSurfaceStride())
> > {
> > }
> 
> Perhaps mWidth, mHeight and mStride should go altogether and the
> GetDataStride, GetWidth, GetHeight methods return mInstance->GetSurfaceWidth
> etc. Even more radical would be to use the instance methods directly and
> remove GetDataStride etc although that might be too complicated.

I'll leave these as is, I'm trying not to churn the code more than necessary (although I failed in some places).

> >+  nsIntRect filterResRect(0, 0, mInstance->GetFilterResX(), mInstance->GetFilterResY());
> >   mInstance->GetFilterSubregion(mFilter, 
> >                                 mFilter->SubregionIsUnionOfRegions() ?
> >-                                  mSourceRegion : nsRect(0, 0, mWidth, mHeight),
> >+                                  mSourceRegion : filterResRect,
> >                                 &mFilterSubregion);
> 
> I can infer why this next bit here from the GetSurfaceRect comment but it would
> be nice to reiterate that in a comment line here. 

OK

> >+  mSurfaceRect = mInstance->GetSurfaceRect();
> >+  nsIntPoint offset = mSurfaceRect.TopLeft();
> >+  mSurfaceRect.IntersectRect(mSurfaceRect, mFilterSubregion);
> >+  mSurfaceRect -= offset;
> > 
> 
> > //----------------------------------------------------------------------
> > // nsSVGElement methods
> > 
> > nsSVGElement::LengthAttributesInfo
> > nsSVGFE::GetLengthInfo()
> > {
> >   return LengthAttributesInfo(mLengthAttributes, sLengthInfo,
> >                               NS_ARRAY_LENGTH(sLengthInfo));
> > }
> > 
> >+nsIntRect
> >+nsSVGFE::ComputeTargetBBox(const nsTArray<nsIntRect>& aSourceBBoxes,
> >+        const nsSVGFilterInstance& aInstance)
> >+{
> >+  nsIntRect r;
> >+  for (PRUint32 i = 0; i < aSourceBBoxes.Length(); ++i) {
> >+    r.UnionRect(r, aSourceBBoxes[i]);
> >+  }
> >+  return r;
> >+}
> >+
> >+void
> >+nsSVGFE::GetSourceImageNames(nsTArray<nsIDOMSVGAnimatedString*>* aSources)
> >+{
> >+}
> >+
> 
> Move these methods up to somewhere under the // Implementation comment rather
> than having them in the // nsSVGElement methods bit.

Sure OK

> > //---------------------Gaussian Blur------------------------
> > 
> > typedef nsSVGFE nsSVGFEGaussianBlurElementBase;
> > 
> > class nsSVGFEGaussianBlurElement : public nsSVGFEGaussianBlurElementBase,
> >-                                   public nsIDOMSVGFEGaussianBlurElement,
> >-                                   public nsISVGFilter
> 
> Shouldn't nsISVGFilter be in here somewhere with lots of - signs as it will
> cease to be as part of this patch won't it?

Yeah OK. (Removing nsISVGFilter was one of the things I probably should have eschewed, but adding methods to a dud interface galled me.)

> >   void BoxBlurH(PRUint8 *aInput, PRUint8 *aOutput,
> >-                PRInt32 aStride, nsRect aRegion,
> >+                PRInt32 aStride, const nsIntRect& aRegion,
> >                 PRUint32 leftLobe, PRUint32 rightLobe);
> > 
> >   void BoxBlurV(PRUint8 *aInput, PRUint8 *aOutput,
> >-                PRInt32 aStride, nsRect aRegion,
> >+                PRInt32 aStride, const nsIntRect& aRegion,
> >                 unsigned topLobe, unsigned bottomLobe);
> 
> while you are changing this perhaps you could make topLobe and bottomLobe
> PRUint32?

Yeah OK.

> >-  PRUint32 dX, dY;
> >-  dX = (PRUint32) floor(aStdX * 3*sqrt(2*M_PI)/4 + 0.5);
> >-  dY = (PRUint32) floor(aStdY * 3*sqrt(2*M_PI)/4 + 0.5);
> >+  *aDX = PRUint32(floor(stdX * 3*sqrt(2*M_PI)/4 + 0.5));
> >+  *aDY = PRUint32(floor(stdY * 3*sqrt(2*M_PI)/4 + 0.5));
> 
> Maybe we could break out 3 *sqrt(2*M_PI)/4 into a static const scaling factor?

No thanks, not this time :-).

> > class nsSVGFEFuncGElement : public nsSVGComponentTransferFunctionElement,
> >                             public nsIDOMSVGFEFuncGElement
> > {
> >   friend nsresult NS_NewSVGFEFuncGElement(nsIContent **aResult,
> >-                                          nsINodeInfo *aNodeInfo);
> >+                                           nsINodeInfo *aNodeInfo);
> 
> ???

OOps

> ...
> 
> A comment about offset would be nice here.

OK

> >+    nsIntRect r;
> >+    r.IntersectRect(entry->mRegion, mSurfaceRect);
> >+    r -= mSurfaceRect.TopLeft();
> >+
> >     if (entry->mColorModel.mAlphaChannel == ColorModel::PREMULTIPLIED)
> >-      nsSVGUtils::UnPremultiplyImageDataAlpha(data, stride, entry->mRegion);
> >+      nsSVGUtils::UnPremultiplyImageDataAlpha(data, stride, r);
> > 
> >     if (aRequiredColorModel.mColorSpace != entry->mColorModel.mColorSpace) {
> > 
> >       if (aRequiredColorModel.mColorSpace == ColorModel::LINEAR_RGB)
> >-        nsSVGUtils::ConvertImageDataToLinearRGB(data, stride, entry->mRegion);
> >+        nsSVGUtils::ConvertImageDataToLinearRGB(data, stride, r);
> >       else
> >-        nsSVGUtils::ConvertImageDataFromLinearRGB(data, stride, entry->mRegion);
> >+        nsSVGUtils::ConvertImageDataFromLinearRGB(data, stride, r);
> >     }
> >     if (aRequiredColorModel.mAlphaChannel == ColorModel::PREMULTIPLIED)
> >-      nsSVGUtils::PremultiplyImageDataAlpha(data, stride, entry->mRegion);
> >+      nsSVGUtils::PremultiplyImageDataAlpha(data, stride, r);
> 
> In that case all the nsSVGUtils methods here should take an nsIntRect also
> change the surface nsRect to nsIntRect in nsSVGMaskFrame.

I actually think I'll back out all the nsIntRect usage, it's unnecessary churn. Something to do later.

> >+++ mozilla-trunk/layout/svg/base/src/nsSVGFilterInstance.h	2008-02-08 21:39:53.000000000 +1300
> >@@ -60,76 +60,94 @@ public:
> >     enum AlphaChannel { UNPREMULTIPLIED, PREMULTIPLIED };
> > 
> >     ColorModel(ColorSpace aColorSpace, AlphaChannel aAlphaChannel) :
> >       mColorSpace(aColorSpace), mAlphaChannel(aAlphaChannel) {}
> >     PRBool operator==(const ColorModel& aOther) const {
> >       return mColorSpace == aOther.mColorSpace &&
> >              mAlphaChannel == aOther.mAlphaChannel;
> >     }
> >     ColorSpace   mColorSpace;
> >     PRPackedBool mAlphaChannel;
> 
> Oops, that should be AlphaChannel mAlphaChannel.

OK, I'll fix that.

> >   void GetFilterBox(float *x, float *y, float *width, float *height) {
> 
> Could be a const method too.
> 
> >+  const nsIntRect& GetSurfaceRect() { return mSurfaceRect; }
> >+  PRInt32 GetSurfaceWidth() { return mSurfaceRect.width; }
> >+  PRInt32 GetSurfaceHeight() { return mSurfaceRect.height; }
> >+  PRInt32 GetSurfaceStride() { return mSurfaceStride; }
> >+  
> >+  PRUint32 GetFilterResX() { return mFilterResX; }
> >+  PRUint32 GetFilterResY() { return mFilterResY; }
> 
> const methods for all.

OK

(In reply to comment #7)
> And one thing on a more general note, should we do something equally clever
> like this for masks and possibly patterns?

I don't know the costs and benefits of doing that. The answer is "not now" unless we encounter wild testcases where it would help a lot.
I'm working on a test suite for SVG filters. I've found some bugs in my patch but also a number of trunk bugs. So far:
-- The numbers k1,k2,k3,k4 for the feComposite "arithmetic" operator are interpreted as lengths. This seems very wrong.
-- k4 is divided by 255.0 where it should be multiplied
-- feComposite's cairo path does not clip rendering to the filter primitive subregion.
-- filter primitive subregions in objectBoundingBox units should be interpreted relative to the origin of the object bounding box; currently they're interpreted relative to the origin of user space

I'm fixing these in my patch as I go.
(In reply to comment #9)
> I'm working on a test suite for SVG filters. I've found some bugs in my patch
> but also a number of trunk bugs. So far:
> -- The numbers k1,k2,k3,k4 for the feComposite "arithmetic" operator are
> interpreted as lengths. This seems very wrong.

The feComposite testcase in the svg test spec uses SVG images as its sources which makes it trickier to verify I guess.

> -- k4 is divided by 255.0 where it should be multiplied
> -- feComposite's cairo path does not clip rendering to the filter primitive
> subregion.

Does feMerge have the same problem?

> Does feMerge have the same problem?

Possibly, I haven't got to that yet :-)

> The feComposite testcase in the svg test spec uses SVG images as its sources
> which makes it trickier to verify I guess.

If you could find a way to verify, that would be useful. I think it makes absolutely no sense for these to be lengths and the spec just says they're "numbers" which I'm pretty sure means they should not be scaled by the bounding box size.
There's a composite testcase in the original implementation bug 361070. It looks like it is derived from the svg testsuite avoiding svg images.
Found another bug, the WRAP() macro for feConvolveMatrix is completely broken for negative 'val'.
Whiteboard: [needs review]
Found anothe bug in feDisplacementMap ... we round the source coordinates towards zero, but we should always round down, otherwise both -0.5 and 0.5 map to 0 which leads to inconsistent results.
According to bug 389746 there may be other testcases for filters you could find by searching the inkscape mailing list, assuming it's archived somewhere.
As you predicted, feMerge needs to clip to its filter primitive subregion. Easily fixed.
Another bug: feMorphology uses the filter primitive subregion XMost and YMost as upper bounds for the area that it searches in the erosion/dilation kernel. This is a mistake, it needs to be able to read outside the filter primitive subregion (in fact it already does search pixels above and to the left of 'rect').
Another problem with feMorphology is that when an rx or ry radius value is very slightly greater than an integer (due to machine imprecision), we effectively round it up to the next integer when searching above or to the left of the current pixel, which is really one pixel too far. We should use ceil to convert rx and ry to integer radii, but subtract an epsilon value before ceil to ensure that N.0001 is treated as N (it probably should be N but for machine imprecision).
Bug in feOffset: it limits both its source area and its output to the filter primitive subregion. That seems wrong, it should only limit its output to that subregion. Admittedly the spec is not 100% clear on this. Anyone else got an opinion?
Urgh, here's another crazy thing:

  mInstance->GetFilterSubregion(mFilter, 
                                mFilter->SubregionIsUnionOfRegions() ?
                                  mSourceRegion : nsRect(0, 0, mWidth, mHeight),
                                &mFilterSubregion);

So when SubregionIsUnionOfRegions() is true, we use mSourceRegion as the default filter primitive subregion ... but that's just the source region for *one* of the sources, not the union for all of them!!! I should be able to fix this pretty easily now that in my new patch, it's easy to get a list of all sources for a filter primitive.
Created attachment 302920 [details] [diff] [review]
reftests

These are my filter reftests. They all pass on my Mac build, but they fail miserably on trunk.

Having fixed all these bugs, I now discover that the original optimization here isn't working anymore. The problem is that the feFlood element conceptually fills the entire filter effects region, so we decide that we need temporary surfaces the size of the filter effects region.

I need to juice up my optimization so that after propagating bounding boxes forward through the filter pipeline, I then propagate "needed-boxes" *backwards* through the filter pipeline so we can discover that the results of the feFlood element are only needed over a small area. This is going to be slightly tricky because of the way named images can be overwritten by filter primitives ... I basically need to do a kind of SSA-conversion here.
Created attachment 302966 [details] [diff] [review]
fix v2

This fixes all the bugs I mentioned and passes the tests. (Along the way I somehow fixed a bug I noticed in http://www.w3.org/TR/SVG/images/filters/filters01.svg on trunk when zooming in ... the lighting was getting all washed out. Not sure what fixed that.)

It does the two pass analysis that I just mentioned and thus solves the major performance issues with ecma-cloud.svg. The analysis is easily extensible for additional optimizations in the future. One major one that I just realized is now easy --- we could easily plug in the "dirty rect" for the drawing operation and restrict filter computation based on it. Food for another bug but could be a huge optimization for many cases.

I also addressed all of rlongson's comments.
Attachment #302097 - Attachment is obsolete: true
Attachment #302966 - Flags: superreview?(tor)
Attachment #302966 - Flags: review?(tor)
Attachment #302097 - Flags: superreview?(tor)
Attachment #302097 - Flags: review?(tor)
Whiteboard: [needs review]
Comment on attachment 302966 [details] [diff] [review]
fix v2

>+nsRect
>+nsSVGFEImageElement::ComputeTargetBBox(const nsTArray<nsRect>& aSourceBBoxes,
>+        const nsSVGFilterInstance& aInstance)
>+{
>+  // XXX can do better here

In what way? An additional comment sentence would be nice.

>+  // Build graph of Info nodes describing filter primitives
>+  nsresult SetupGraph(nsIContent* aFilterElement);
>+  // Compute bounding boxes of the filter primitive outputs
>+  void ComputeResultBoundingBoxes();
>+  // Compute bounding boxes of what we actually *need* from the filter
>+  // primitive outputs
>+  void ComputeNeededBoxes();
>+  nsRect GetUnionOfAllNeededBoxes();
>+  const nsRect& GetSourceColorAlphaNeeded()
>+  { return mSourceColorAlphaInfo.mResultNeededBox; }
>+  const nsRect& GetSourceAlphaNeeded()
>+  { return mSourceAlphaInfo.mResultNeededBox; }

Some of these could be const methods.

>+  // Figure out the size of the intermediate buffers we're going to need.
>+  // Also figures out 
>+  nsresult AnalyzeRequirements(Requirements *aRequirements,
>+          const nsRect& aSourceBBox, const nsRect& aFilterRegion,
>+          const nsSVGFilterInstance& aInstance);
>+    

Anticipation of comment continuing ;-)

>+  // Allocates an image surface that covers mSurfaceRect (it uses
>+  // device offsets so that its origin is positioned at mSurfaceRect.TopLeft()
>+  // when using cairo to draw into the surface). The surface is cleared
>+  // to transprent black.

s/transprent/transparent/
Adding dependency based on comment 22
Blocks: 411023
> In what way? An additional comment sentence would be nice.

We could check what we know of the source image bounds and compute an accurate bounding box for the filter primitive result.

> Some of these could be const methods.

Doesn't really matter since we don't have a const FilterAnalysis reference anywhere.

> Anticipation of comment continuing ;-)

"Also figures out whether we need the source graphic and/or alpha."

Thanks...
One more thing... The patch does not build on Windows.

You have changed the filter method declarations throughout

>-  NS_IMETHOD Filter(nsSVGFilterInstance *instance);
...
> +  virtual nsresult Filter(nsSVGFilterInstance *aInstance);

But you have not changed the definitions.

 NS_IMETHODIMP
 nsSVGFEGaussianBlurElement::Filter(nsSVGFilterInstance *instance)


Testing reveals that the reftests filter-basic-02.svg and filter-basic-03.svg fail. Presumably you need to integrate the filters.cpp changes from bug 414550 into your new design. See also http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-felem-01-b.html which should display as the reference image.

(In reply to comment #27)

Wrong bug, I meant bug 407463
Created attachment 303105 [details] [diff] [review]
fix v3

Updated to fix those issues.

I decided to basically inline AnalyzeRequirements into FilterPaint, because it's small now, this removes the need for the Requirements struct, and makes FilterAnalysis available in the body of FilterPaint which will be useful if/when we start using it more.

When we have the opportunity to make bigger changes we should combine nsSVGFilterInstance with FilterAnalysis ... we can get rid of the image dictionary and drive everything from the analysis graph. We can also rework AcquireSourceImage/AcquireTargetImage ... basically remove them since we can get the source image list directly from the filter primitive now.
Attachment #302966 - Attachment is obsolete: true
Attachment #303105 - Flags: superreview?(tor)
Attachment #303105 - Flags: review?(tor)
Attachment #302966 - Flags: superreview?(tor)
Attachment #302966 - Flags: review?(tor)
Attachment #303105 - Flags: review?(tor) → review?(longsonr)
Comment on attachment 303105 [details] [diff] [review]
fix v3

Great stuff.
Attachment #303105 - Flags: review?(longsonr) → review+
One nit that I pointed out before still remains

>+  // Allocates an image surface that covers mSurfaceRect (it uses
>+  // device offsets so that its origin is positioned at mSurfaceRect.TopLeft()
>+  // when using cairo to draw into the surface). The surface is cleared
>+  // to transprent black.

s/transprent/transparent/

Updated

10 years ago
Attachment #303105 - Flags: superreview?(tor) → superreview+
checked in, with tests.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I had to back this out because it caused the crashtest for bug 381777 to crash. The reason was a stray "rect = fr.GetFilterSubregion()" which caused us to loop over the subregion rect instead of surface rect, causing us to index outside the surface. I'll reland this tonight.
Whiteboard: [needs review] → [needs landing]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
relanded, all tests green.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
I think nsISVGFilter.h still needs deleting.
Depends on: 1219406
You need to log in before you can comment on or make changes to this bug.