375846 - Implement feConvolveMatrix filter primitive

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jeff Schiller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Gecko/20070328 Minefield/3.0a4pre

SVG spec: http://www.w3.org/TR/SVG11/filters.html#feConvolveMatrix

SVG test suite: http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-conv-01-f.html

Lots of cool effects with this filter, would be great to get done before Fx3 arrives.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[tor]

Attachment: work in progress

Comment 2

[tor]

Attachment: missed a file

Comment 3

[tor]

Attachment: update to tip,

Comment 4

[Robert Longson [:longsonr]]

Comment on attachment 268504 [details] [diff] [review]
update to tip, 

>Index: dom/public/idl/svg/nsIDOMSVGAnimatedBoolean.idl
>===================================================================
>RCS file: dom/public/idl/svg/nsIDOMSVGAnimatedBoolean.idl
>diff -N dom/public/idl/svg/nsIDOMSVGAnimatedBoolean.idl
>--- /dev/null	1 Jan 1970 00:00:00 -0000

>+ * The Initial Developer of the Original Code is IBM Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2004
>+ * the Initial Developer. All Rights Reserved.

s/2004/2007/

Also in content/svg/content/src/nsSVGAnimatedBoolean.cpp,
        content/svg/content/src/nsSVGAnimatedBoolean.h

>+/**
>+ * The nsIDOMSVGAnimatedNumber interface is the interface to an SVG
>+ * Animated Number.
>+ *

Erm yes, but this is AnimatedBoolean.

>+
>+interface nsIDOMSVGNumber;

I don't think this is necessary/

>+NS_IMETHODIMP
>+nsSVGAnimatedBoolean::GetValueString(nsAString& aValue)
>+{
>+  aValue.Truncate();
>+
>+  if (mBaseVal)
>+    aValue.AppendLiteral("true");
>+  else
>+    aValue.AppendLiteral("false");

How about aValue.AssignLiteral(mBaseVal ? "true" : "false");

>+/* attribute nsIDOMSVGNumber baseVal; */
>+NS_IMETHODIMP
>+nsSVGAnimatedBoolean::SetBaseVal(PRBool aBaseVal)
>+{
>+  WillModify();
>+  mBaseVal = aBaseVal;
>+  DidModify();
>+  return NS_OK;
>+}

Should you check for input validity?

>+protected:
>+  void ConvolvePixel(PRUint8 *aSourceData,
>+                     PRUint8 *aTargetData,
>+                     PRInt32 aWidth, PRInt32 aHeight,
>+                     PRInt32 aStride,
>+                     PRInt32 aX, PRInt32 aY,
>+                     PRUint16 aEdgeMode, float *aKernel,
>+                     float aDivisor, float aBias, PRBool aPreserveAlpha,
>+                     PRInt32 aOrderX, PRInt32 aOrderY,
>+                     PRInt32 aTargetX, PRInt32 aTargetY);
>+

aSourceData and aKernel should be const

>+    nsCOMPtr<nsISVGEnum> edgeMode;
>+    rv = NS_NewSVGEnum(getter_AddRefs(edgeMode),
>+                       nsSVGFEConvolveMatrixElement::SVG_EDGEMODE_DUPLICATE,
>+                       gEdgeModes);

I don't see a default in the spec. Have you confirmed this is the right value?

>+
>+#define BOUND(val, min, max) \
>+  PR_MIN(PR_MAX(val, min), max)
>+
>+#define WRAP(val, min, max) \
>+  (val < 0 ? (val + val % max) : val % max)
>+

>+{
>+  float sum[4] = {0, 0, 0, 0};
>+  aBias *= 255;
>+  PRInt32 offsets[4] = {GFX_ARGB32_OFFSET_R,
>+                        GFX_ARGB32_OFFSET_G,
>+                        GFX_ARGB32_OFFSET_B,
>+                        GFX_ARGB32_OFFSET_A } ;
>+
>+  for (PRInt32 y = 0; y < aOrderY; y++)
>+    for (PRInt32 x = 0; x < aOrderX; x++) {
>+      PRInt32 sampleX = aX + x - aTargetX;
>+      PRInt32 sampleY = aY + y - aTargetY;

Put sampleY calculation in outer y loop.

>+      for (PRInt32 i = 0; i < (aPreserveAlpha ? 3 : 4); i++) {

Create a variable to store (aPreserveAlpha ? 3 : 4)  to prevent needless repetetive calculation.

>+        if (sampleX < 0 || sampleY < 0 ||
>+            sampleX >= aWidth || sampleY >= aHeight) {

SampleY tests don't depend on X so you could recode this part of the test outside the x loop.

>+          switch (aEdgeMode) {
>+            case SVG_EDGEMODE_DUPLICATE:
>+              sum[i] +=
>+                aSourceData[BOUND(sampleY, 0, aHeight - 1) * aStride +
>+                            BOUND(sampleX, 0, aWidth - 1) * 4 + offsets[i]];
>+              break;
>+            case SVG_EDGEMODE_WRAP:
>+              sum[i] +=
>+                aSourceData[WRAP(sampleY, 0, aHeight) * aStride +
>+                            WRAP(sampleX, 0, aWidth) * 4 + offsets[i]];
>+              break;
>+            default:
>+              sum[i] += 0;

rather pointless.

>+              break;
>+          }
>+        } else {
>+          sum[i] +=
>+            aSourceData[sampleY * aStride + 4 * sampleX + offsets[i]] *
>+            aKernel[aOrderX * y + x];
>+        }
>+      }
>+    }

>+  if (aPreserveAlpha) {
>+    aTargetData[aY * aStride + 4 * aX + offsets[3]] =
>+      aSourceData[aY * aStride + 4 * aX + offsets[3]];
>+  }

Simpler just to use GFX_ARGB32_OFFSET_A rather than offsets[3]

>+  PRBool rescaling = PR_FALSE;
>+  float kernelX, kernelY;
>+  PRInt32 scaledWidth, scaledHeight;
>+  if (HasAttr(kNameSpaceID_None, nsGkAtoms::kernelUnitLength)) {
>+    rescaling = PR_TRUE;
>+    nsSVGLength2 val;
>+    val.Init(nsSVGUtils::X, 0xff,
>+             mNumberAttributes[KERNEL_UNIT_LENGTH_X].GetAnimValue(),
>+             nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>+    kernelX = instance->GetPrimitiveLength(&val);
>+    val.Init(nsSVGUtils::Y, 0xff,
>+             mNumberAttributes[KERNEL_UNIT_LENGTH_Y].GetAnimValue(),
>+             nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER);
>+    kernelY = instance->GetPrimitiveLength(&val);
>+#ifdef DEBUG_tor
>+    fprintf(stderr, "convolve kernelX/Y %f %f\n", kernelX, kernelY);
>+#endif
>+    if (kernelX <= 0 || kernelY <= 0)
>+      return NS_ERROR_FAILURE;
>+
>+    scaledWidth = NS_STATIC_CAST(PRInt32, fr.GetWidth() / kernelX);
>+    scaledHeight = NS_STATIC_CAST(PRInt32, fr.GetHeight() / kernelY);
>+#ifdef DEBUG_tor
>+    fprintf(stderr, "scaled size %d %d\n", scaledWidth, scaledHeight);
>+#endif

call nsSVGUtils::ConvertToSurfaceSize rather than the above.

>+    scaledSource = new gfxImageSurface(gfxIntSize(scaledWidth, scaledHeight),
>+                                       gfxASurface::ImageFormatARGB32);
>+    scaledTarget = new gfxImageSurface(gfxIntSize(scaledWidth, scaledHeight),
>+                                       gfxASurface::ImageFormatARGB32);
>+

>+  PRInt32 orderX, orderY;
>+  mOrderX->GetBaseVal(&orderX);
>+  mOrderY->GetBaseVal(&orderY);

Shouldn't these be GetAnimVal?

>+  nsAutoPtr<float> kernel(new float[orderX * orderY]);

Are we going to place any limits on orderX or orderY to prevent allocating lots of memory?
Also what about checking for failure to allocate the memory?

>+  if (HasAttr(kNameSpaceID_None, nsGkAtoms::divisor)) {
>+    divisor = mNumberAttributes[DIVISOR].GetBaseValue();

GetAnimValue? and in target and bias code which follows?

>+  PRInt32 stride, width, height;
>+  if (rescaling) {
>+    rect.x = rect.x * scaledWidth / fr.GetWidth();
>+    rect.y = rect.y * scaledHeight / fr.GetHeight();
>+    rect.width = rect.width * scaledWidth / fr.GetWidth();
>+    rect.height = rect.height * scaledHeight / fr.GetHeight();

Use *= above

Consider ordering of code in this function so that simple tests for failure come earlier e.g. num != orderX * orderY is fairly late on and could go higher up before the imagesurfaces are created.

Comment 5

[tor]

(In reply to comment #4)
> (From update of attachment 268504 [details] [diff] [review])
> >Index: dom/public/idl/svg/nsIDOMSVGAnimatedBoolean.idl
> >===================================================================
> >RCS file: dom/public/idl/svg/nsIDOMSVGAnimatedBoolean.idl
> >diff -N dom/public/idl/svg/nsIDOMSVGAnimatedBoolean.idl
> >--- /dev/null	1 Jan 1970 00:00:00 -0000
> 
> >+ * The Initial Developer of the Original Code is IBM Corporation.
> >+ * Portions created by the Initial Developer are Copyright (C) 2004
> >+ * the Initial Developer. All Rights Reserved.
> 
> s/2004/2007/
> 
> Also in content/svg/content/src/nsSVGAnimatedBoolean.cpp,
>         content/svg/content/src/nsSVGAnimatedBoolean.h

Actually the 2004 date is correct, in that I've had that class implemented and kicking around on my disk for a few years now as part of another patch that hasn't made it to prime time yet.  I'll fix it and update the date, though.

Comment 6

[tor]

> >+    nsCOMPtr<nsISVGEnum> edgeMode;
> >+    rv = NS_NewSVGEnum(getter_AddRefs(edgeMode),
> >+                       nsSVGFEConvolveMatrixElement::SVG_EDGEMODE_DUPLICATE,
> >+                       gEdgeModes);
> 
> I don't see a default in the spec. Have you confirmed this is the right value?

I pulled the default value from the DTD fragment in section 15.13:

    edgeMode ( duplicate | wrap | none ) 'duplicate'

Comment 7

[tor]

Attachment: update per comments, plus a couple spec changes

Applied most of the comments, except for the one about *= - avoiding that prevents the need to add a large number of casts.

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 268953 [details] [diff] [review]
update per comments, plus a couple spec changes

>+class nsSVGAnimatedBoolean : public nsIDOMSVGAnimatedBoolean,
>+                             public nsSVGValue
>+{

>+
>+protected:
>+  PRBool mBaseVal;

PRPackedBool perhaps. Makes no difference here though.

>+NS_IMETHODIMP
>+nsSVGAnimatedBoolean::GetValueString(nsAString& aValue)
>+{
>+  aValue.Truncate();

Truncate is unnecessary and should be removed.

>+
>+  aValue.Assign(mBaseVal
>+                ? NS_LITERAL_STRING("true")
>+                : NS_LITERAL_STRING("false"));
>+
>+  return NS_OK;
>+}

>+    // svg specification flips the kernel from one might expect

??? "from that which one might expect" maybe

>+
>+    PRBool overflow = PR_FALSE;
>+    scaledSize =
>+      nsSVGUtils::ConvertToSurfaceSize(gfxSize(fr.GetWidth() / kernelX,
>+                                               fr.GetHeight() / kernelY),
>+                                       &overflow);
>+    // if the requested size based on the kernel unit is too big,
>+    // we need to bail because the effect is pixel size dependent
>+    if (overflow)
>+      return NS_ERROR_FAILURE;

Do you need to check for scaledSize.width <= 0 or scaledSize.height <= 0? If fr.GetWidth() is small and kernelX is very large for instance could it round to 0?

>+    scaledSource = new gfxImageSurface(scaledSize,
>+                                       gfxASurface::ImageFormatARGB32);
>+    scaledTarget = new gfxImageSurface(scaledSize,
>+                                       gfxASurface::ImageFormatARGB32);
>+    if (!scaledSource || !scaledTarget)
>+      return NS_ERROR_FAILURE;

I'd be happier with belt and braces...

    if (!scaledSource || !scaledSource->Data() ||
        !scaledTarget || !scaledTarget->Data())
      return NS_ERROR_FAILURE;

Comment 9

[tor]

Attachment: update per comments

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 269430 [details] [diff] [review]
update per comments

>+NS_IMETHODIMP
>+nsSVGFEConvolveMatrixElement::Filter(nsSVGFilterInstance *instance)
>+{
>+  PRBool preserveAlpha;
>+  mPreserveAlpha->GetBaseVal(&preserveAlpha);

Should be GetAnimVal

And a couple of nits...

>+    // if the requested size based on the kernel unit is too big, we

s/if/If/

>+    // need to bail because the effect is pixel size dependent.  also

s/.  also/. Also/

r=longsonr with those changes made

Comment 11

[tor]

Attachment: address comments

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    rect.x = rect.x * scaledSize.width / fr.GetWidth();
+    rect.y = rect.y * scaledSize.height / fr.GetHeight();
+    rect.width = rect.width * scaledSize.width / fr.GetWidth();
+    rect.height = rect.height * scaledSize.height / fr.GetHeight();

I thought you were going to use *= here?

Can ConvolvePixel be a static helper function (non-member) here? That might give it a better chance of being inlined...

This looks like the slowest convolution matrix code ever, but I guess we should get this in as the general solution and then worry about optimizing cases, if that becomes a priority.

Comment 13

[Robert Longson [:longsonr]]

(In reply to comment #12)
> This looks like the slowest convolution matrix code ever, but I guess we should
> get this in as the general solution and then worry about optimizing cases, if
> that becomes a priority.
> 

I'd still bet on Gaussian Blur being keeping its current crown as king of slowness ;-)

Comment 14

[tor]

(In reply to comment #12)
> +    rect.x = rect.x * scaledSize.width / fr.GetWidth();
> +    rect.y = rect.y * scaledSize.height / fr.GetHeight();
> +    rect.width = rect.width * scaledSize.width / fr.GetWidth();
> +    rect.height = rect.height * scaledSize.height / fr.GetHeight();
> 
> I thought you were going to use *= here?

As I mentioned in comment 7, trying to use *= gets messy fast - you need to do casting to get a float result for the scaling factor, then *= gives warnings about the type of the operands.

> This looks like the slowest convolution matrix code ever, but I guess we should
> get this in as the general solution and then worry about optimizing cases, if
> that becomes a priority.

Considering this sort of thing should really be done using pixel shaders, I don't see much point in stressing over the software version unless it shows high on profiles or is part of a general purpose library, like cairo.

Comment 15

[tor]

Attachment: version for checkin - ConvolvePixel made static function

Comment 16

[tor]

Checked in.