Pixel difference in FEMorphology effect

• Component: SVG
• Status: RESOLVED FIXED

[Andras Piroska]

Created attachment 99692 [details]
Screenshot from the bug in four browser (Opera, Firefox, QtTestBrowser, Chrome)

When an animated SVG uses the FEMorphology filter (erode or dilate) and the animation is done by JavaScript,
the Y-radius does not change in some cases. The changes related to X-radius works without any problem.
The issue should be somewhere around the event handling. Because if  the radius attribute is set once,
the svg does not update the value of Y-radius, but if the radius is set twice, the svg is updated first
time with a wrong Y-radius and second with the correct one.

for example:
	-if set the attribute of the filter twice the animation works as well
	 	example: (javascript)
		* dilate.setAttribute("radius",r);
		* dilate.setAttribute("radius",r);
	-but in this case not works:
		* dilate.setAttribute("radius", "1,7");
		* dilate.setAttribute("radius", r + ",1");
		the value of the X-radius at first is 1, after updated to 'r'
		and the value of the Y-radius at first is 7, but not updated to 1

[Andras Piroska]

Created attachment 99693 [details]
Animated FEMorphology SVG that show the bug on webkit-based browsers

Animated FEMorphology SVG that show the bug on webkit-based browsers.
The SVG looks different in Opera or Firefox browser.

[Andras Piroska]

I discovered the reason of this bug, I fix it soon.

[Dirk Schulze]

Great that you work on that!

[Gabor Loki]

I can confirm this bug with Chrome and QtTestBrowser as well.

The "Animated FEMorphology SVG that show the bug on webkit-based browsers" attachment ( https://bug-63930-attachments.webkit.org/attachment.cgi?id=99693 ) is not working at all, neighter in WebKit (here it results in this bug: "This page contains the following errors: error on line 2 at column 93: Space required after the Public Identifier Below is a rendering of the page up to the first error.") nor in FireFox 5 (here: XML-Verarbeitungsfehler: Syntax-Fehler
Adresse: https://bug-63930-attachments.webkit.org/attachment.cgi?id=99693
Zeile Nr. 2, Spalte 96:<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
-----------------------------------------------------------------------------------------------^ ).

I think the attachment is broken.

[Andras Piroska]

Created attachment 99811 [details]
Pixel difference in FEMorphology effect

[Andras Piroska]

Thanks, really wrong, I will fix it.

[Zoltan Herczeg]

Nice catch! Can we add a Layout test for this?

[Andras Piroska]

Created attachment 99813 [details]
Animated FEMorphology SVG that show the bug on webkit-based browsers

[Gabor Loki]

Created attachment 100943 [details]
Pixel difference in FEMorphology effect

Andras is on holiday, so I have updated the patch and the pixel test on Mac.

[Gabor Loki]

> Andras is on holiday, so I have updated the patch and the pixel test on Mac.

We have to update this tests on the following platforms as well:
./chromium-linux/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png
./chromium-mac-leopard/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png
./chromium-mac/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png
./chromium-win/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png
./gtk/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png
./mac-leopard/svg/dynamic-updates/SVGFEMorphologyElement-dom-radius-attr-expected.png

I am CC'ing several people to do the update on their platforms as well.

[Zoltan Herczeg]

Comment on attachment 100943 [details]
Pixel difference in FEMorphology effect

View in context: https://bugs.webkit.org/attachment.cgi?id=100943&action=review

> Source/WebCore/svg/SVGFEMorphologyElement.cpp:132
> +    if (attrName == SVGNames::radiusAttr) {
> +        bool isRadiusXChanged = morphology->setRadiusX(radiusX());
> +        bool isRadiusYChanged = morphology->setRadiusY(radiusY());
> +        return isRadiusXChanged || isRadiusYChanged;
> +    }

Please put a comment here why this form is needed.

[Gabor Loki]

Created attachment 100960 [details]
Pixel difference in FEMorphology effect

A comment is added, and the ChangeLog is modified as well.

[Nikolas Zimmermann]

Comment on attachment 100943 [details]
Pixel difference in FEMorphology effect

View in context: https://bugs.webkit.org/attachment.cgi?id=100943&action=review

> Source/WebCore/ChangeLog:11
> +        If the X-radius attribute of FEMorphology filter is changed, the setRadiusX() will return true
> +        and the right side of || (OR) operator will be ignored. So right operand,
> +        the setRadiusY() won't be called and the Y-radius attribute won't updated neighter.
> +        To resolve this problem, we need enforce evaluate of both operand.

This alone makes no sense without having read "return (morphology->setRadiusX(radiusX()) || morphology->setRadiusY(radiusY()));".
I'd include this here, to make it easy understandable.

[Zoltan Herczeg]

> This alone makes no sense without having read "return (morphology->setRadiusX(radiusX()) || morphology->setRadiusY(radiusY()));".
> I'd include this here, to make it easy understandable.

This is the previous changelog from a previous patch :P

[WebKit Review Bot]

Comment on attachment 100960 [details]
Pixel difference in FEMorphology effect

Clearing flags on attachment: 100960

Committed r91067: <http://trac.webkit.org/changeset/91067>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.