Bug 5863 - feMorphology filter is not implemented
: feMorphology filter is not implemented
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 523.x (Safari 3)
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 68469 5857 26389
  Show dependency treegraph
 
Reported: 2005-11-28 17:22 PST by Oliver Hunt
Modified: 2014-05-12 05:54 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2005-11-28 17:22:06 PST
 
Comment 1 Dirk Schulze 2009-08-23 23:37:14 PDT
Created attachment 38470 [details]
feMorphologyElement

Added feMorphologyElement as preparation for FEMorphology filter effect.
Comment 2 Dirk Schulze 2009-08-24 02:37:50 PDT
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.
Comment 3 Eric Seidel 2009-08-24 12:26:06 PDT
Comment on attachment 38470 [details]
feMorphologyElement

m__operator seems like a strange name.
Comment 4 Eric Seidel 2009-08-24 12:40:19 PDT
Comment on attachment 38470 [details]
feMorphologyElement

m__operator seems like a strange name.
Comment 5 Eric Seidel 2009-08-24 13:42:53 PDT
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;
Comment 6 Dirk Schulze 2009-08-24 23:34:22 PDT
(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.
Comment 7 Eric Seidel 2009-08-25 00:30:10 PDT
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 Nikolas Zimmermann 2009-08-25 03:30:02 PDT
(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 Dirk Schulze 2009-10-08 12:53:32 PDT
Created attachment 40904 [details]
feMorphologyElemnt

A preperation for the Effect. We need to add the Element first.
Comment 10 Dirk Schulze 2009-10-08 14:32:21 PDT
Created attachment 40913 [details]
feMorphologyElemnt

Moved out XCode clean up. Commited by Nikolas.
Comment 11 Nikolas Zimmermann 2009-10-08 15:08:41 PDT
Comment on attachment 40913 [details]
feMorphologyElemnt

r-, the test is buggy, already discussed with krit on IRC.
Comment 12 Dirk Schulze 2009-10-09 11:52:44 PDT
Created attachment 40956 [details]
feMorphologyElement

Same patch, better test.
Comment 13 Nikolas Zimmermann 2009-10-09 12:01:19 PDT
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.
Comment 14 Dirk Schulze 2009-10-09 12:57:04 PDT
Comment on attachment 40956 [details]
feMorphologyElement

clearing review flag. Landed in r49400.
Comment 15 Dirk Schulze 2009-11-06 05:24:06 PST
Created attachment 42647 [details]
feMorphology filter

Implementation of the graphic filter.
Comment 16 Nikolas Zimmermann 2009-11-06 06:08:08 PST
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+.
Comment 17 Dirk Schulze 2009-11-06 08:42:35 PST
Created attachment 42653 [details]
feMorphology filter

corrected the mentioned style issues.
Comment 18 Jeff Schiller 2009-11-06 10:51:06 PST
Can you also update WebKitSite/projects/svg/status.xml as part of your patch? :)
Comment 19 Nikolas Zimmermann 2009-11-06 11:20:50 PST
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.
Comment 20 Eric Seidel 2009-11-06 11:30:23 PST
Comment on attachment 42653 [details]
feMorphology filter

Marking cq- since there were changes requested.
Comment 21 Dirk Schulze 2009-11-06 13:09:10 PST
landed in r50604. Closing bug.