301234 - Implement a subset of SVG filters

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: filter framework plus three test filters (feGaussianBlur, feComponentTransfer, feMerge)

Comment 2

[Jonathan Watt [:jwatt]]

*** Bug 296988 has been marked as a duplicate of this bug. ***

Comment 3

[tor]

Attachment: remove rendundant svg observer, <image> tweak, move UserSpace/ObjectSpace to nsSVGUtils

Comment 4

[Scooter Morris]

Comment on attachment 190344 [details] [diff] [review]
remove rendundant svg observer, <image> tweak, move UserSpace/ObjectSpace to nsSVGUtils


In:
>Index: content/svg/content/src/nsSVGAnimatedInteger.cpp

You should be the initial developer, and the contributer...


Same here....
>Index: content/svg/content/src/nsSVGAnimatedInteger.h

In...
>Index: content/svg/content/src/nsSVGFilterElement.cpp

These should be filterUnits
>+
>+  // DOM property: gradientUnits ,  #IMPLIED attrib: gradientUnits

And here
>+
>+  // DOM property: gradientUnits ,  #IMPLIED attrib: gradientUnits

In dom/public/idl/svg/nsIDOMSVGAnimatedInteger.idl:

>+ * The Initial Developer of the Original Code is Scooter Morris.
...should be you
>+ * Portions created by the Initial Developer are Copyright (C) 2004
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *    Scooter Morris <[e-mail]>
...should be you

I'll look at layout, next.

Comment 5

[Scooter Morris]

Comment on attachment 190344 [details] [diff] [review]
remove rendundant svg observer, <image> tweak, move UserSpace/ObjectSpace to nsSVGUtils

In NS_GetSVGFilterFrame:
Shouldn't we make this bug dependent on getRefFrame and change
this to use it?


In...
>+NS_IMETHODIMP
>+nsSVGFilterFrame::FilterPaint(nsISVGRendererCanvas *aCanvas,
>+                              nsISVGChildFrame *aTarget)
>+{

SourceAlpha seems to be handled very separately, here.	In the future, would
there be more if statements to check for SourceGraphic, BackgroundAlpha,
BackgroundGraphic,
or is SourceAlpha special for some reason?  

>+  if (requirements & NS_FE_SOURCEALPHA) {


This seems generally useful.  Could this be moved to SVGUtils also?  The
Pattern
patch moves it there..

>+void
>+nsSVGFilterFrame::TransformPoint(nsIDOMSVGMatrix *ctm, 
>+                                        float& x, float& y)
>+{

In...

>Index: layout/svg/base/src/nsSVGFilterInstance.h

Remove commented section...

>+//  void GetFilterSubregion(nsIDOMSVGLength *aX, nsIDOMSVGLength *aY,
>+//                          nsIDOMSVGLength *aWidth, nsIDOMSVGLength *aHeight,
>+//                          nsRect *result);
>+  void GetFilterSubregion(nsIDOMSVGFilterPrimitiveStandardAttributes *aFilter,
>+                          nsRect defaultRegion,
>+                          nsRect *result);
>+

Comment 6

[tor]

Attachment: merge with getrefframe patch, address some comments, add feOffset

Comment 7

[Scooter Morris]

Beginning to test and review filters.  Just a cursory start, but should the
image in the test suite (filters-comptran-01-b) look like the SVG?  I checked
and the SVG is not using any additional filters (like many of the other filter
tests!).

Comment 8

[Scooter Morris]

Comment on attachment 191640 [details] [diff] [review]
merge with getrefframe patch, address some comments, add feOffset

Here are some comments on the content and dom
pieces of this.  Note that I left the Index lines
in for context...

Index: content/svg/content/src/Makefile.in
Index: content/svg/content/src/nsISVGFilter.h
>
Please include a comment as to what these values
are used for

>+
>+#define NS_FE_SOURCEGRAPHIC   0x01
>+#define NS_FE_SOURCEALPHA     0x02
>+#define NS_FE_BACKGROUNDIMAGE 0x04
>+#define NS_FE_BACKGROUNDALPHA 0x08
>+#define NS_FE_FILLPAINT       0x10
>+#define NS_FE_STROKEPAINT     0x20
>+
Index: content/svg/content/src/nsSVGAnimatedInteger.cpp

See bug 306131 -- shouldn't use ToNewCString for this?

>+  char *str = ToNewCString(aValue);
>+

Index: content/svg/content/src/nsSVGAnimatedInteger.h
Index: content/svg/content/src/nsSVGAtomList.h
--> conflicts on merge since textPath landing
Index: content/svg/content/src/nsSVGElementFactory.cpp
--> conflicts on merge since textPath landing
Index: content/svg/content/src/nsSVGFilterElement.cpp
In...
>+nsresult
>+nsSVGFilterElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                            nsIAtom* aPrefix, const nsAString& aValue,
>+                            PRBool aNotify)
>+{

See bug 306131 -- shouldn't use ToNewCString for this?

>+    str = ToNewCString(aValue);

Index: content/svg/content/src/nsSVGFilters.cpp

Wrong comment....
>+/* readonly attribute nsIDOMSVGAnimatedNumber stdDeviationX; */
>+NS_IMETHODIMP nsSVGFEGaussianBlurElement::GetStdDeviationY(nsIDOMSVGAnimatedNumber * *aY)
>+

In...
>+nsresult
>+nsSVGFEGaussianBlurElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                                    nsIAtom* aPrefix, const nsAString& aValue,
>+                                    PRBool aNotify)

...
>+      mStdDeviationX->SetBaseVal(stdX);
This looks wrong -- shouldn't it be SetBaseVal(stdY)?
>+      mStdDeviationY->SetBaseVal(stdX);

In...
>+NS_IMETHODIMP
>+nsSVGFEComponentTransferElement::Filter(nsSVGFilterInstance *instance)
>+{

Shouldn't there at least be a warning if the child doesn't support provide
these interfaces?
>+    nsCOMPtr<nsIDOMSVGFEFuncRElement> elementR = do_QueryInterface(child);
>+    nsCOMPtr<nsIDOMSVGFEFuncGElement> elementG = do_QueryInterface(child);
>+    nsCOMPtr<nsIDOMSVGFEFuncBElement> elementB = do_QueryInterface(child);
>+    nsCOMPtr<nsIDOMSVGFEFuncAElement> elementA = do_QueryInterface(child);
>+    if (elementR)
>+      elementR->GenerateLookupTable(tableR);
>+    if (elementG)
>+      elementG->GenerateLookupTable(tableG);
>+    if (elementB)
>+      elementB->GenerateLookupTable(tableB);
>+    if (elementA)
>+      elementA->GenerateLookupTable(tableA);

In...
>+NS_IMETHODIMP
>+nsSVGComponentTransferFunctionElement::GenerateLookupTable(PRUint8 *aTable)
>+{

Is GetItem guaranteed to return a number?  If not, this is a potential crasher.
>+      list->GetItem(k, getter_AddRefs(number));
>+      number->GetValue(&v1);
...same here...
>+      list->GetItem(PR_MIN(k + 1, num - 1), getter_AddRefs(number));
>+      number->GetValue(&v2);

Index: content/svg/content/src/nsSVGGraphicElement.cpp
Index: content/svg/content/src/nsSVGSVGElement.cpp
Index: dom/public/nsIDOMClassInfo.h

These break the alpha order, but probably make
more sense as is...
>+  eDOMClassInfo_SVGFEFuncRElement_id,
>+  eDOMClassInfo_SVGFEFuncGElement_id,
>+  eDOMClassInfo_SVGFEFuncBElement_id,
>+  eDOMClassInfo_SVGFEFuncAElement_id,

Index: dom/public/idl/svg/Makefile.in
Index: dom/public/idl/svg/nsIDOMSVGAnimatedInteger.idl
Index: dom/public/idl/svg/nsIDOMSVGFilterElement.idl
Index: dom/public/idl/svg/nsIDOMSVGFilters.idl
Index: dom/src/base/nsDOMClassInfo.cpp
...alpha order not right, but probably makes more sense as is...same below
>+  NS_DEFINE_CLASSINFO_DATA(SVGFEFuncRElement, nsElementSH,
>+                           ELEMENT_SCRIPTABLE_FLAGS)
>+  NS_DEFINE_CLASSINFO_DATA(SVGFEFuncGElement, nsElementSH,
>+                           ELEMENT_SCRIPTABLE_FLAGS)
>+  NS_DEFINE_CLASSINFO_DATA(SVGFEFuncBElement, nsElementSH,
>+                           ELEMENT_SCRIPTABLE_FLAGS)
>+  NS_DEFINE_CLASSINFO_DATA(SVGFEFuncAElement, nsElementSH,
>+                           ELEMENT_SCRIPTABLE_FLAGS)

Comment 9

[Scooter Morris]

Comment on attachment 191640 [details] [diff] [review]
merge with getrefframe patch, address some comments, add feOffset

OK, here are the comments on the layout section of the code...

Index: layout/base/nsCSSFrameConstructor.cpp
--> merge conflicts

processChildren not used anymore..
>     processChildren = PR_TRUE;
>     rv = NS_NewSVGClipPathFrame(mPresShell, aContent, &newFrame);
>   }
>+  else if (aTag == nsSVGAtoms::filter) {
>+    processChildren = PR_TRUE;
>+    rv = NS_NewSVGFilterFrame(mPresShell, aContent, &newFrame);

Index: layout/base/nsLayoutAtomList.h
Index: layout/style/nsCSSParser.cpp
Index: layout/style/nsCSSPropList.h
Index: layout/style/nsCSSStruct.cpp
Index: layout/style/nsCSSStruct.h
Index: layout/style/nsRuleNode.cpp
Index: layout/style/nsStyleContext.cpp
Index: layout/style/nsStyleStruct.cpp
Index: layout/style/nsStyleStruct.h
Index: layout/svg/base/src/Makefile.in
Index: layout/svg/base/src/nsISVGChildFrame.h
Index: layout/svg/base/src/nsSVGClipPathFrame.cpp
Index: layout/svg/base/src/nsSVGDefsFrame.cpp
Index: layout/svg/base/src/nsSVGDefsFrame.h
Index: layout/svg/base/src/nsSVGFilterFrame.cpp

Should we print out a warning for debug builds if the filter is unimplemented?
>+  // check for source requirements or filter elements that we don't support yet
>+  if (requirements & ~(NS_FE_SOURCEGRAPHIC | NS_FE_SOURCEALPHA) ||
>+      unimplementedFilter) {
>+    nsRect dummyRect;
>+    aTarget->PaintSVG(aCanvas, dummyRect, PR_TRUE);
>+    return NS_OK;
>+  }

Index: layout/svg/base/src/nsSVGFilterFrame.h
Index: layout/svg/base/src/nsSVGFilterInstance.h
Index: layout/svg/base/src/nsSVGForeignObjectFrame.cpp
Index: layout/svg/base/src/nsSVGGFrame.cpp
Index: layout/svg/base/src/nsSVGGFrame.h
Index: layout/svg/base/src/nsSVGGenericContainerFrame.cpp
Index: layout/svg/base/src/nsSVGGenericContainerFrame.h
Index: layout/svg/base/src/nsSVGGlyphFrame.cpp
Index: layout/svg/base/src/nsSVGGradientFrame.cpp
Index: layout/svg/base/src/nsSVGImageFrame.cpp
Index: layout/svg/base/src/nsSVGInnerSVGFrame.cpp
Index: layout/svg/base/src/nsSVGMarkerFrame.cpp
Index: layout/svg/base/src/nsSVGOuterSVGFrame.cpp
Index: layout/svg/base/src/nsSVGPathGeometryFrame.cpp
Index: layout/svg/base/src/nsSVGPathGeometryFrame.h
Index: layout/svg/base/src/nsSVGTSpanFrame.cpp
Index: layout/svg/base/src/nsSVGTextFrame.cpp
Index: layout/svg/base/src/nsSVGUseFrame.cpp
Index: layout/svg/base/src/nsSVGUtils.cpp
Index: layout/svg/base/src/nsSVGUtils.h

Comment 10

[tor]

Attachment: merge to trunk tip

Comment 11

[tor]

> Index: content/svg/content/src/nsSVGAnimatedInteger.cpp
> 
> See bug 306131 -- shouldn't use ToNewCString for this?
> 
> >+  char *str = ToNewCString(aValue);
> >+

> >+nsresult
> >+nsSVGFilterElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> >+                            nsIAtom* aPrefix, const nsAString& aValue,
> >+                            PRBool aNotify)
> >+{
> 
> See bug 306131 -- shouldn't use ToNewCString for this?
> 
> >+    str = ToNewCString(aValue);

Thinking of waiting until we know what the proper string technique is, then
changing everything.

> >+nsresult
> >+nsSVGFEGaussianBlurElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> >+                                    nsIAtom* aPrefix, const nsAString& aValue,
> >+                                    PRBool aNotify)
> 
> ...
> >+      mStdDeviationX->SetBaseVal(stdX);
> This looks wrong -- shouldn't it be SetBaseVal(stdY)?
> >+      mStdDeviationY->SetBaseVal(stdX);

Actually it's right - I've changed the code to make it clearer.


> In...
> >+NS_IMETHODIMP
> >+nsSVGFEComponentTransferElement::Filter(nsSVGFilterInstance *instance)
> >+{
> 
> Shouldn't there at least be a warning if the child doesn't support provide
> these interfaces?
> >+    nsCOMPtr<nsIDOMSVGFEFuncRElement> elementR = do_QueryInterface(child);
> >+    nsCOMPtr<nsIDOMSVGFEFuncGElement> elementG = do_QueryInterface(child);
> >+    nsCOMPtr<nsIDOMSVGFEFuncBElement> elementB = do_QueryInterface(child);
> >+    nsCOMPtr<nsIDOMSVGFEFuncAElement> elementA = do_QueryInterface(child);
> >+    if (elementR)
> >+      elementR->GenerateLookupTable(tableR);
> >+    if (elementG)
> >+      elementG->GenerateLookupTable(tableG);
> >+    if (elementB)
> >+      elementB->GenerateLookupTable(tableB);
> >+    if (elementA)
> >+      elementA->GenerateLookupTable(tableA);

Don't think so, as in compound documents you might legitimatally have a
non-FEFunc element as a child.

> In...
> >+NS_IMETHODIMP
> >+nsSVGComponentTransferFunctionElement::GenerateLookupTable(PRUint8 *aTable)
> >+{
> 
> Is GetItem guaranteed to return a number?  If not, this is a potential crasher.
> >+      list->GetItem(k, getter_AddRefs(number));
> >+      number->GetValue(&v1);
> ...same here...
> >+      list->GetItem(PR_MIN(k + 1, num - 1), getter_AddRefs(number));
> >+      number->GetValue(&v2);

Looks like it will aways returns a number.

Comment 12

[tor]

Attachment: incorporate scooter's comments

Comment 13

[Scooter Morris]

Comment on attachment 194696 [details] [diff] [review]
incorporate scooter's comments

In....
>+NS_IMETHODIMP
>+nsSVGFilterFrame::FilterPaint(nsISVGRendererCanvas *aCanvas,
>+                              nsISVGChildFrame *aTarget)

>+  // check for source requirements or filter elements that we don't support yet
>+  if (requirements & ~(NS_FE_SOURCEGRAPHIC | NS_FE_SOURCEALPHA) ||
>+      unimplementedFilter) {


>+#ifdef DEBUG_tor
>+    if (requirements & ~(NS_FE_SOURCEGRAPHIC | NS_FE_SOURCEALPHA))
>+      fprintf(stderr, "FilterFrame: unimplemented source requirement\n");
>+    if (requirements & ~(NS_FE_SOURCEGRAPHIC | NS_FE_SOURCEALPHA))
>+      fprintf(stderr, "FilterFrame: unimplemented filter element\n");
>+#endif

Should be:
+#ifdef DEBUG
+    if (requirements & ~(NS_FE_SOURCEGRAPHIC | NS_FE_SOURCEALPHA))
+      fprintf(stderr, "FilterFrame: unimplemented source requirement\n");
+    else
+      fprintf(stderr, "FilterFrame: unimplemented filter element\n");
+#endif

shouldn't it??	In any case, its just a DEBUG printf, so r+ from me!

Comment 14

[tor]

(In reply to comment #13)
> >+#ifdef DEBUG_tor
> >+    if (requirements & ~(NS_FE_SOURCEGRAPHIC | NS_FE_SOURCEALPHA))
> >+      fprintf(stderr, "FilterFrame: unimplemented source requirement\n");
> >+    if (requirements & ~(NS_FE_SOURCEGRAPHIC | NS_FE_SOURCEALPHA))
> >+      fprintf(stderr, "FilterFrame: unimplemented filter element\n");
> >+#endif
> 
> Should be:
> +#ifdef DEBUG
> +    if (requirements & ~(NS_FE_SOURCEGRAPHIC | NS_FE_SOURCEALPHA))
> +      fprintf(stderr, "FilterFrame: unimplemented source requirement\n");
> +    else
> +      fprintf(stderr, "FilterFrame: unimplemented filter element\n");
> +#endif
> 
> shouldn't it??	In any case, its just a DEBUG printf, so r+ from me!

Whoops, the latter test should have been "if (unimplementedFilter)".

Comment 15

[tor]

Checked in.

Comment 16

[Kevin Newman]

*** Bug 251414 has been marked as a duplicate of this bug. ***

Comment 17

[Martin Hejral]

Hello, many thanks for great work!!!

But there is more filter primitives to implement, in my Deer Park 1.6a1

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050912
Firefox/1.6a1

work ONLY Gaussian blur?!

THIS BUG IS STILL NOT FIXED!

Comment 18

[Martin Hejral]

Attachment: this works

Comment 19

[Martin Hejral]

Attachment: feComponentTransfer.svg

works, but output differ from Adobe SVG Viewer6

Comment 20

[Martin Hejral]

Comment on attachment 195886 [details]
feComponentTransfer.svg

works, but output differ from Adobe SVG Viewer6
---
Also GRADIENT on text is INCORRECTLY SCALED

Comment 21

[Martin Hejral]

Attachment: feColorMatrix.svg - WORKS NOT

Comment 22

[Martin Hejral]

Attachment: feBlend.svg - WORKS NOT

Comment 23

[Martin Hejral]

Attachment: feComposite.svg - WORKS NOT - very important filt. primitive

Comment 24

[Martin Hejral]

Attachment: feOffset.svg - WORKS NOT

Comment 25

[Martin Hejral]

Attachment: feTurbulence-1.svg - WORKS NOT

complete algorithm code is placed in SVG spec.!

Comment 26

[Martin Hejral]

Attachment: feMorphology.svg - WORKS NOT

Comment 27

[Martin Hejral]

Attachment: feConvolveMatrix.svg - WORKS NOT

Comment 28

[Martin Hejral]

Attachment: feDisplacementMap.svg - WORKS NOT

Comment 29

[Martin Hejral]

Attachment: feSpecularLighting+Point.svg - WORKS NOT

Comment 30

[Martin Hejral]

Attachment: feSpecularLighting+Spot.svg - WORKS NOT

Comment 31

[Martin Hejral]

Attachment: feColorMatrix.png - REFERENCE IMAGE

Comment 32

[Martin Hejral]

Attachment: feComponentTransfer.png - REFERENCE IMAGE

Comment 33

[Martin Hejral]

Attachment: feBlend.png - REFERENCE IMAGE

Comment 34

[Martin Hejral]

Attachment: feComposite.png - REFERENCE IMAGE

Comment 35

[Martin Hejral]

Attachment: feSpecularLighting+Point.png - REFERENCE IMAGE

Comment 36

[Martin Hejral]

Attachment: feSpecularLighting+Spot.png - REFERENCE IMAGE

Comment 37

[Darin Fisher]

Not going to block 1.8.1 for this bug.