Closed Bug 301234 Opened 17 years ago Closed 17 years ago

Implement a subset of SVG filters

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: tor, Assigned: tor)

References

(Blocks 1 open bug)

Details

Attachments

(19 files, 4 obsolete files)

262.17 KB, patch
scootermorris
: review+
Details | Diff | Splinter Review
1.30 KB, image/svg+xml
Details
2.88 KB, image/svg+xml
Details
3.17 KB, image/svg+xml
Details
1.84 KB, image/svg+xml
Details
5.52 KB, image/svg+xml
Details
1.10 KB, image/svg+xml
Details
2.95 KB, image/svg+xml
Details
1.42 KB, image/svg+xml
Details
1.27 KB, image/svg+xml
Details
2.34 KB, image/svg+xml
Details
1.83 KB, image/svg+xml
Details
1.93 KB, image/svg+xml
Details
25.20 KB, image/png
Details
20.60 KB, image/png
Details
22.40 KB, image/png
Details
7.77 KB, image/png
Details
29.36 KB, image/png
Details
33.04 KB, image/png
Details
 
*** Bug 296988 has been marked as a duplicate of this bug. ***
Attachment #189709 - Attachment is obsolete: true
Flags: blocking1.8b4?
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 <scootermorris@comcast.net>
...should be you

I'll look at layout, next.
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);
>+
Attachment #190344 - Attachment is obsolete: true
Flags: blocking1.8b4? → blocking1.8b4-
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 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 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
Attached patch merge to trunk tip (obsolete) — Splinter Review
Attachment #191640 - Attachment is obsolete: true
> 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.

Attachment #194368 - Attachment is obsolete: true
Attachment #194696 - Flags: review?(scootermorris)
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!
Attachment #194696 - Flags: review?(scootermorris) → review+
(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)".

Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
*** Bug 251414 has been marked as a duplicate of this bug. ***
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!
Attached image this works
Attached image feComponentTransfer.svg
works, but output differ from Adobe SVG Viewer6
Comment on attachment 195886 [details]
feComponentTransfer.svg

works, but output differ from Adobe SVG Viewer6
---
Also GRADIENT on text is INCORRECTLY SCALED
complete algorithm code is placed in SVG spec.!
Summary: Implement SVG filters → Implement a subset of SVG filters
Flags: blocking1.8.1?
Not going to block 1.8.1 for this bug.
Flags: blocking1.8.1? → blocking1.8.1-
Blocks: 311029
Duplicate of this bug: 251414
Depends on: 534156
You need to log in before you can comment on or make changes to this bug.