Upcoming patch implements the feMorphology filter

Comment on attachment 241074 [details] [diff] [review] Patch >+nsresult >+nsSVGFEMorphologyElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, >+ nsIAtom* aPrefix, const nsAString& aValue, >+ PRBool aNotify) >+{ >+ nsresult rv = nsSVGFEMorphologyElementBase::SetAttr(aNameSpaceID, aName, aPrefix, >+ aValue, aNotify); >+ >+ if (aName == nsSVGAtoms::radius && aNameSpaceID == kNameSpaceID_None) { >+ float rx = 0.0f, ry = 0.0f; >+ char *str; >+ str = ToNewCString(aValue); >+ int num = sscanf(str, "%f %f\n", &rx, &ry); >+ if (num == 1) >+ ry = rx; >+ mNumberAttributes[RADIUS_X].SetBaseValue(rx, this, PR_FALSE); >+ mNumberAttributes[RADIUS_Y].SetBaseValue(ry, this, PR_FALSE); >+ nsMemory::Free(str); >+ } >+ >+ return rv; >+} The gauss, turbulence, and morphology filters all have this identical code - turn into a helper function? >+NS_IMETHODIMP >+nsSVGFEMorphologyElement::Filter(nsSVGFilterInstance *instance) >+{ >+ nsresult rv; >+ PRUint8 *sourceData, *targetData; >+ nsSVGFilterResource fr(instance); >+ >+ rv = fr.AcquireSourceImage(mIn1, this, &sourceData); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = fr.AcquireTargetImage(mResult, &targetData); >+ NS_ENSURE_SUCCESS(rv, rv); >+ nsRect rect = fr.GetRect(); >+ >+#ifdef DEBUG_tor >+ fprintf(stderr, "FILTER MORPH rect: %d,%d %dx%d\n", >+ rect.x, rect.y, rect.width, rect.height); >+#endif >+ >+ float rx, ry; >+ nsSVGLength2 val; >+ >+ GetAnimatedNumberValues(&rx, &ry, nsnull); >+ val.Init(nsSVGUtils::X, 0xff, rx, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER); >+ rx = instance->GetPrimitiveLength(&val); >+ >+ val.Init(nsSVGUtils::Y, 0xff, ry, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER); >+ ry = instance->GetPrimitiveLength(&val); >+ PRInt32 stride = fr.GetDataStride(); >+ PRInt32 x, y; >+ PRUint32 x1, y1, i; >+ PRUint32 xExt[4], yExt[4]; // X, Y indices of RGBA extrema >+ PRUint16 op; >+ mOperator->GetAnimVal(&op); >+ >+ if (rx == 0 && ry == 0) { >+ memcpy(targetData, sourceData, rect.height * stride); >+ return NS_OK; >+ } Should have a comment here explaining the general strategy the following code is trying to accomplish. >+ for (y = rect.y; y < rect.y + rect.height; y++) { >+ for (x = rect.x; x < rect.x + rect.width; x++) { >+ PRUint32 startX = PR_MAX(0, (int)(x - rx)); >+ PRUint32 endX = PR_MIN((int)(x + rx + 1), rect.x + rect.width); >+ PRUint32 startY = PR_MAX(0, (int)(y - ry)); >+ PRUint32 endY = PR_MIN((int)(y + ry + 1), rect.y + rect.height); >+ >+ // We need to scan the entire kernel >+ if (x == rect.x || xExt[0] < startX || xExt[1] < startX || >+ xExt[2] < startX || xExt[3] < startX) { >+ xExt[0] = xExt[1] = xExt[2] = xExt[3] = x; >+ yExt[0] = yExt[1] = yExt[2] = yExt[3] = y; Would having a extrema[4] array simplify the following code? >+ for (y1 = startY; y1 < endY; y1++) { >+ for (x1 = startX; x1 < endX; x1++) { >+ if (op == nsSVGFEMorphologyElement::SVG_OPERATOR_ERODE) { >+ for (i = 0; i < 4; i++) >+ if (sourceData[yExt[i] * stride + 4 * xExt[i] + i] >= >+ sourceData[y1 * stride + 4 * x1 + i]) { >+ xExt[i] = x1; >+ yExt[i] = y1; >+ } >+ } else if (op == nsSVGFEMorphologyElement::SVG_OPERATOR_DILATE) { >+ for (i = 0; i < 4; i++) >+ if (sourceData[yExt[i] * stride + 4 * xExt[i] + i] <= >+ sourceData[y1 * stride + 4 * x1 + i]) { >+ xExt[i] = x1; >+ yExt[i] = y1; >+ } >+ } These two cases duplicate code. Why not do something like this: for (i = 0; i < 4; i++) { PRUint8 pixel = sourceData[y1 * stride + 4 * x1 + i]; if ((op == ...ERODE && extrema[i] >= pixel) || (op == ...DILATE && extrema[i] <= pixel) { extrema[i] = pixel; xExt[i] = x1; yExt[i] = y1; } } >+ } >+ } >+ } else { // We only need to look at the newest column >+ for (y1 = startY; y1 < endY; y1++) { >+ if (op == nsSVGFEMorphologyElement::SVG_OPERATOR_ERODE) { >+ for (i = 0; i < 4; i++) >+ if (sourceData[yExt[i] * stride + 4 * xExt[i] + i] >= >+ sourceData[y1 * stride + 4 * endX + i]) { >+ xExt[i] = endX; >+ yExt[i] = y1; >+ } >+ } else if (op == nsSVGFEMorphologyElement::SVG_OPERATOR_DILATE) { >+ for (i = 0; i < 4; i++) >+ if (sourceData[yExt[i] * stride + 4 * xExt[i] + i] <= >+ sourceData[y1 * stride + 4 * endX + i]) { >+ xExt[i] = endX; >+ yExt[i] = y1; >+ } >+ } Ditto. >+ } >+ } >+ PRUint32 targIndex = y * stride + 4 * x; >+ targetData[targIndex ] = sourceData[yExt[0] * stride + 4 * xExt[0] ]; >+ targetData[targIndex+1] = sourceData[yExt[1] * stride + 4 * xExt[1] + 1]; >+ targetData[targIndex+2] = sourceData[yExt[2] * stride + 4 * xExt[2] + 2]; >+ targetData[targIndex+3] = sourceData[yExt[3] * stride + 4 * xExt[3] + 3]; >+ } >+ } >+ return NS_OK; >+}

Comment on attachment 241964 [details] [diff] [review] Patch3 >+static void >+SetAttrHelper(PRInt32 aNameSpaceID, nsIAtom* aName, >+ nsIAtom* aPrefix, const nsAString& aValue, >+ PRBool aNotify, nsIAtom* aAtom, nsSVGNumber2* aNum1, >+ nsSVGNumber2* aNum2, nsSVGFE* filter) >+{ >+ if (aName == aAtom && aNameSpaceID == kNameSpaceID_None) { >+ float x = 0.0f, y = 0.0f; >+ char *str; >+ str = ToNewCString(aValue); >+ int num = sscanf(str, "%f %f\n", &x, &y); >+ if (num == 1) >+ y = x; >+ aNum1->SetBaseValue(x, filter, PR_FALSE); >+ aNum2->SetBaseValue(y, filter, PR_FALSE); >+ nsMemory::Free(str); >+ } >+} Rename to something more indicative of what it does. You're not using the aPrefix, aNotify, or filter arguments - remove. Rename aName and aAtom to give a better clue as to what's what. >+NS_IMETHODIMP >+nsSVGFEMorphologyElement::Filter(nsSVGFilterInstance *instance) >+{ ... >+ PRInt32 x, y; >+ PRUint32 x1, y1, i; Move these down to the for loops.

Comment on attachment 242430 [details] [diff] [review] Patch 4 >+ for (PRInt32 y = rect.y; y < rect.y + rect.height; y++) { >+ for (PRInt32 x = rect.x; x < rect.x + rect.width; x++) { >+ PRUint32 startX = PR_MAX(0, (int)(x - rx)); >+ PRUint32 endX = PR_MIN((int)(x + rx + 1), rect.x + rect.width); >+ PRUint32 startY = PR_MAX(0, (int)(y - ry)); >+ PRUint32 endY = PR_MIN((int)(y + ry + 1), rect.y + rect.height); >+ PRUint32 targIndex = y * stride + 4 * x; startY and endY do not change in the x loop so could be hoisted out into the outer y loop. > DOM_CLASSINFO_MAP_BEGIN(SVGFEMergeElement, nsIDOMSVGFEMergeElement) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMSVGFEMergeElement) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMSVGFilterPrimitiveStandardAttributes) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMSVGStylable) > DOM_CLASSINFO_SVG_ELEMENT_MAP_ENTRIES > DOM_CLASSINFO_MAP_END > >+ DOM_CLASSINFO_MAP_BEGIN(SVGFEMorphologyElement, nsIDOMSVGFEMorphologyElement) >+ DOM_CLASSINFO_MAP_ENTRY(nsIDOMSVGFEMorphologyElement) >+ DOM_CLASSINFO_MAP_ENTRY(nsIDOMSVGFilterPrimitiveStandardAttributes) >+ DOM_CLASSINFO_MAP_ENTRY(nsIDOMSVGStylable) >+ DOM_CLASSINFO_SVG_ELEMENT_MAP_ENTRIES >+ DOM_CLASSINFO_MAP_END >+ Nit: Indenting seems inconsistent with other entries.

Comment on attachment 242508 [details] [diff] [review] Patch 5 >+static void >+ScanDualValueAttribute(const nsAString& aValue, nsSVGNumber2* aNum1, >+ nsSVGNumber2* aNum2, nsSVGFE* filter) >+{ >+ float x = 0.0f, y = 0.0f; >+ char *str; >+ str = ToNewCString(aValue); >+ int num = sscanf(str, "%f %f\n", &x, &y); >+ if (num == 1) >+ y = x; >+ aNum1->SetBaseValue(x, filter, PR_FALSE); >+ aNum2->SetBaseValue(y, filter, PR_FALSE); >+ nsMemory::Free(str); >+} We probably need to switch from using sscanf to PR_strtod to void problems with local specific decimal seperators (ala bug 356896).

Comment on attachment 243264 [details] [diff] [review] Patch 6 >+PRBool >+nsSVGFE::ScanDualValueAttribute(const nsAString& aValue, nsIAtom* aAttribute, >+ nsSVGNumber2* aNum1, nsSVGNumber2* aNum2, >+ NumberInfo* aInfo1, NumberInfo* aInfo2, >+ nsAttrValue& aResult) >+{ >+ float x = 0.0f, y = 0.0f; >+ char *str = ToNewCString(aValue); >+ char *rest; >+ PRBool parseError = PR_FALSE; >+ x = (float) PR_strtod(str, &rest); >+ >+ if (str == rest) { >+ //first value was not a number >+ parseError = PR_TRUE; >+ } >+ //consume white space >+ while (*rest && isspace(*rest)) { >+ ++rest; >+ } >+ >+ if (*rest == '\0') { >+ //second value was not supplied >+ y = x; >+ } else { >+ //second value exists >+ char* start = rest; >+ y = (float) PR_strtod(rest, &rest); >+ >+ //consume white space >+ while (*rest && isspace(*rest)) { >+ ++rest; >+ } >+ if (*rest != '\0') { >+ //second value was not a single number >+ parseError = PR_TRUE; >+ } >+ } Completely untested code which might simplify the parsing and memory alloc/dealloc slightly - keep in mind that strtod will swallow leading whitespace: NS_ConvertUTF16toUTF8 value(aValue); value.CompressWhitespace(PR_FALSE, PR_TRUE); char *str = value.get(); x = PR_strtod(str, &rest); if (str == rest) { parseError = PR_TRUE; } else { if (*rest == '\0') { y = x; } else { y = PR_strtod(rest, &rest); if (*rest != '\0') parseError = PR_TRUE; } }

Comment on attachment 243496 [details] [diff] [review] Patch 7 >+PRBool >+nsSVGFE::ScanDualValueAttribute(const nsAString& aValue, nsIAtom* aAttribute, >+ nsSVGNumber2* aNum1, nsSVGNumber2* aNum2, >+ NumberInfo* aInfo1, NumberInfo* aInfo2, >+ nsAttrValue& aResult) >+{ [munch] >+ } else { >+ y = NS_STATIC_CAST(float, PR_strtod(rest, &rest)); >+ if (*rest != '\0') { >+ //second value was illformed ... or trailing content. >+ parseError = PR_TRUE; >+ } >+ } >+ }

Now correctly renders: http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-filters-morph-01-f.html

Checked in for Alex.

The default value for the operator should be "erode" and not "dilate" http://www.w3.org/TR/SVG11/filters.html#feMorphologyElement Compare these both files: http://svg.tutorial.aptico.de/grafik_svg/kap13_10.svg http://svg.tutorial.aptico.de/grafik_svg/kap13_10.jpg Thank you for implementing SVG Filters!