Bug 5863 – feMorphology filter is not implemented

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Oliver Hunt]

[Dirk Schulze]

Created attachment 38470 [details]
feMorphologyElement

Added feMorphologyElement as preparation for FEMorphology filter effect.

[Dirk Schulze]

Created attachment 38473 [details]
feMorphology filter effect

This is the filter effect feMorphology. I hope that I can make the algorithm more efficient. So this is just for testing atm.

[Eric Seidel (no email)]

Comment on attachment 38470 [details]
feMorphologyElement

m__operator seems like a strange name.

[Eric Seidel (no email)]

Comment on attachment 38470 [details]
feMorphologyElement

m__operator seems like a strange name.

[Eric Seidel (no email)]

Comment on attachment 38470 [details]
feMorphologyElement

I don't understand why these exist:
char SVGRadiusXAttrIdentifier[] = "SVGRadiusXAttr";
 33 char SVGRadiusYAttrIdentifier[] = "SVGRadiusYAttr";

Why the _?
set_operatorBaseValue
is this all to avoid "operator" in C++?

This seems wrong, no?
 36         readonly attribute SVGAnimatedEnumeration _operator;

[Dirk Schulze]

(In reply to comment #5)
> (From update of attachment 38470 [details])
> I don't understand why these exist:
> char SVGRadiusXAttrIdentifier[] = "SVGRadiusXAttr";
>  33 char SVGRadiusYAttrIdentifier[] = "SVGRadiusYAttr";
Radius is one attribute, that may have one or two arguments. These lines create the "virtual" attributes radiusX and radiusY to differ between the both values. See also SVGGaussianBlurElement. radiusX and radiusY are not part of svgattr.in, because they don't realy exist in the SVG Spec.

> Why the _?
> set_operatorBaseValue
> is this all to avoid "operator" in C++?
Yes, the set...BaseValue is gernerated by the attribute name and you can't name the attribute operator. The way with the underscore is also use in SVGFECompositeElement.

> 
> This seems wrong, no?
>  36         readonly attribute SVGAnimatedEnumeration _operator;
No, why should it be wrong? the attribute operator is an enumeration and animateable. Because of the underscore see comments above. 

Can you please review the patch again eric? I would like to set the r? again.

[Eric Seidel (no email)]

Why are the external strings needed?  Why not just "SVGRadiusXAttr" as a literal c string?

readonly attribute SVGAnimatedEnumeration _operator;
will expose "_operator" to javascript instead of "operator".  That seems bad.

Seems we would need to hack the js generator to work around this c++ keyword.  Or have some custom attribute for the attribute in the idl

[Nikolas Zimmermann]

(In reply to comment #7)
> Why are the external strings needed?  Why not just "SVGRadiusXAttr" as a
> literal c string?
The external string are perfectly fine. There are no "radiusX" / "radiusY" Attribute Strings that would be exported in SVGNames.h, there's only "radius", so Dirk needs to define these values. Exactly like it's done for ie. SVGKernelUnitLengthXIdentifier / SVGKernelUnitLengthYIdentifier and numerous other classes in svg/. If you remember, this stuff is needed for SVG<->DOM attribute synchronization.

> 
> readonly attribute SVGAnimatedEnumeration _operator;
> will expose "_operator" to javascript instead of "operator".  That seems bad. 
Yes, that's a real problem. Already persistent in other classes like SVGFECompositeElement, in trunk.
I'd rather suggest to fix all these problems in one-shot in a follow-up patch. Though if Dirk wants to fix it here, no problem as well...

[Dirk Schulze]

Created attachment 40904 [details]
feMorphologyElemnt

A preperation for the Effect. We need to add the Element first.

[Dirk Schulze]

Created attachment 40913 [details]
feMorphologyElemnt

Moved out XCode clean up. Commited by Nikolas.

[Nikolas Zimmermann]

Comment on attachment 40913 [details]
feMorphologyElemnt

r-, the test is buggy, already discussed with krit on IRC.

[Dirk Schulze]

Created attachment 40956 [details]
feMorphologyElement

Same patch, better test.

[Nikolas Zimmermann]

Comment on attachment 40956 [details]
feMorphologyElement

Wonderful patch, r=me.

Some small style issues:

> +SVGFEMorphologyElement::SVGFEMorphologyElement(const QualifiedName& tagName, Document* doc)
> +    : SVGFilterPrimitiveStandardAttributes(tagName, doc)

Use "document" instead of "doc", old legacy code uses abbrevations here.
> +    interface [Conditional=SVG&FILTERS, GenerateConstructor] SVGFEMorphologyElement : SVGElement,
> +                                                          SVGFilterPrimitiveStandardAttributes {
Please line up SVGElement & SVGFilterPrimitiveStandardAttributes.

> -        PassRefPtr<FEMorphology> create(FilterEffect*, MorphologyOperatorType, const float&, const float&);  
> +        static PassRefPtr<FEMorphology> create(FilterEffect*, MorphologyOperatorType, float, float);  
Please specify parameter names for the floats.

> -        FEMorphology(FilterEffect*, MorphologyOperatorType, const float&, const float&);
> +        FEMorphology(FilterEffect*, MorphologyOperatorType, float, float);
Ditto.

Please fix before landing.

[Dirk Schulze]

Comment on attachment 40956 [details]
feMorphologyElement

clearing review flag. Landed in r49400.

[Dirk Schulze]

Created attachment 42647 [details]
feMorphology filter

Implementation of the graphic filter.

[Nikolas Zimmermann]

Comment on attachment 42647 [details]
feMorphology filter

Nice job Dirk,

some style nitpicks:

> +    if (m_radiusX == 0 || m_radiusY == 0)
> +        return;

if (!m_radiusX || !m_radiusY)


> +    radiusX = std::min(effectDrawingRect.width() - 1, radiusX);

I suggest to use "using std::min" / "using std::max" in the beginning.

> +                    if ((m_type == 1 && pixel <= columnExtrema) || (m_type == 2 && pixel >= columnExtrema))
...

> +                    if ((m_type == 1 && extrema[kernelIndex] <= entireExtrema) ||
> +                        (m_type == 2 && extrema[kernelIndex] >= entireExtrema))
...

Don't we have any named constants for this? Didn't check the source...
Please clarify these things, almost ready to give r+.

[Dirk Schulze]

Created attachment 42653 [details]
feMorphology filter

corrected the mentioned style issues.

[Jeff Schiller]

Can you also update WebKitSite/projects/svg/status.xml as part of your patch? :)

[Nikolas Zimmermann]

Comment on attachment 42653 [details]
feMorphology filter

Looks good to me. r+.

Please change from "using namespace std" to an explicit "using std::min; using std::max;", before landing. I think that's rather common style in WebKit.

[Eric Seidel (no email)]

Comment on attachment 42653 [details]
feMorphology filter

Marking cq- since there were changes requested.

[Dirk Schulze]

landed in r50604. Closing bug.