Bug 5863 - feMorphology filter is not implemented
: feMorphology filter is not implemented
Status: RESOLVED FIXED
: WebKit
SVG
: 523.x (Safari 3)
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
:
:
:
: 5857 26389
  Show dependency treegraph
 
Reported: 2005-11-28 17:22 PST by
Modified: 2009-11-06 13:09 PST (History)


Attachments
feMorphologyElement (24.88 KB, patch)
2009-08-23 23:37 PST, Dirk Schulze
eric: review-
Review Patch | Details | Formatted Diff | Diff
feMorphology filter effect (3.49 KB, patch)
2009-08-24 02:37 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
feMorphologyElemnt (105.05 KB, patch)
2009-10-08 12:53 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
feMorphologyElemnt (33.91 KB, patch)
2009-10-08 14:32 PST, Dirk Schulze
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
feMorphologyElement (34.91 KB, patch)
2009-10-09 11:52 PST, Dirk Schulze
no flags Review Patch | Details | Formatted Diff | Diff
feMorphology filter (5.04 KB, patch)
2009-11-06 05:24 PST, Dirk Schulze
zimmermann: review-
Review Patch | Details | Formatted Diff | Diff
feMorphology filter (5.14 KB, patch)
2009-11-06 08:42 PST, Dirk Schulze
zimmermann: review+
eric: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2005-11-28 17:22:06 PST

    
------- Comment #1 From 2009-08-23 23:37:14 PST -------
Created an attachment (id=38470) [details]
feMorphologyElement

Added feMorphologyElement as preparation for FEMorphology filter effect.
------- Comment #2 From 2009-08-24 02:37:50 PST -------
Created an attachment (id=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.
------- Comment #3 From 2009-08-24 12:26:06 PST -------
(From update of attachment 38470 [details])
m__operator seems like a strange name.
------- Comment #4 From 2009-08-24 12:40:19 PST -------
(From update of attachment 38470 [details])
m__operator seems like a strange name.
------- Comment #5 From 2009-08-24 13:42:53 PST -------
(From update of attachment 38470 [details])
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;
------- Comment #6 From 2009-08-24 23:34:22 PST -------
(In reply to comment #5)
> (From update of attachment 38470 [details] [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.
------- Comment #7 From 2009-08-25 00:30:10 PST -------
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
------- Comment #8 From 2009-08-25 03:30:02 PST -------
(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...
------- Comment #9 From 2009-10-08 12:53:32 PST -------
Created an attachment (id=40904) [details]
feMorphologyElemnt

A preperation for the Effect. We need to add the Element first.
------- Comment #10 From 2009-10-08 14:32:21 PST -------
Created an attachment (id=40913) [details]
feMorphologyElemnt

Moved out XCode clean up. Commited by Nikolas.
------- Comment #11 From 2009-10-08 15:08:41 PST -------
(From update of attachment 40913 [details])
r-, the test is buggy, already discussed with krit on IRC.
------- Comment #12 From 2009-10-09 11:52:44 PST -------
Created an attachment (id=40956) [details]
feMorphologyElement

Same patch, better test.
------- Comment #13 From 2009-10-09 12:01:19 PST -------
(From update of attachment 40956 [details])
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.
------- Comment #14 From 2009-10-09 12:57:04 PST -------
(From update of attachment 40956 [details])
clearing review flag. Landed in r49400.
------- Comment #15 From 2009-11-06 05:24:06 PST -------
Created an attachment (id=42647) [details]
feMorphology filter

Implementation of the graphic filter.
------- Comment #16 From 2009-11-06 06:08:08 PST -------
(From update of attachment 42647 [details])
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+.
------- Comment #17 From 2009-11-06 08:42:35 PST -------
Created an attachment (id=42653) [details]
feMorphology filter

corrected the mentioned style issues.
------- Comment #18 From 2009-11-06 10:51:06 PST -------
Can you also update WebKitSite/projects/svg/status.xml as part of your patch? :)
------- Comment #19 From 2009-11-06 11:20:50 PST -------
(From update of attachment 42653 [details])
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.
------- Comment #20 From 2009-11-06 11:30:23 PST -------
(From update of attachment 42653 [details])
Marking cq- since there were changes requested.
------- Comment #21 From 2009-11-06 13:09:10 PST -------
landed in r50604. Closing bug.