Bug 63930

Summary: Pixel difference in FEMorphology effect
Product: WebKit Reporter: Andras Piroska <pandras>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: dglazkov, krit, lars.sonchocky-helldorf, lforschler, loki, mihaip, mrowe, rniwa, webkit.review.bot, zherczeg, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 68469, 26389    
Attachments:
Description Flags
Screenshot from the bug in four browser (Opera, Firefox, QtTestBrowser, Chrome)
none
Animated FEMorphology SVG that show the bug on webkit-based browsers
none
Pixel difference in FEMorphology effect
none
Animated FEMorphology SVG that show the bug on webkit-based browsers
none
Pixel difference in FEMorphology effect
zherczeg: review-, zherczeg: commit-queue-
Pixel difference in FEMorphology effect none

Andras Piroska
Reported 2011-07-05 03:43:15 PDT
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
Attachments
Screenshot from the bug in four browser (Opera, Firefox, QtTestBrowser, Chrome) (184.03 KB, image/png)
2011-07-05 03:43 PDT, Andras Piroska 		no flags
Details
Animated FEMorphology SVG that show the bug on webkit-based browsers (1.74 KB, image/svg+xml)
2011-07-05 03:52 PDT, Andras Piroska 		no flags
Details
Pixel difference in FEMorphology effect (2.27 KB, patch)
2011-07-06 03:59 PDT, Andras Piroska 		no flags
DetailsFormatted DiffDiff
Animated FEMorphology SVG that show the bug on webkit-based browsers (1.75 KB, image/svg+xml)
2011-07-06 04:06 PDT, Andras Piroska 		no flags
Details
Pixel difference in FEMorphology effect (97.00 KB, patch)
2011-07-15 00:39 PDT, Gabor Loki 		zherczeg: review-
zherczeg: commit-queue-
DetailsFormatted DiffDiff
Pixel difference in FEMorphology effect (96.77 KB, patch)
2011-07-15 05:54 PDT, Gabor Loki 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Andras Piroska
Comment 1 2011-07-05 03:52:39 PDT
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
Comment 2 2011-07-05 04:00:28 PDT
I discovered the reason of this bug, I fix it soon.
Dirk Schulze
Comment 3 2011-07-05 04:03:00 PDT
Great that you work on that!
Gabor Loki
Comment 4 2011-07-05 04:28:58 PDT
I can confirm this bug with Chrome and QtTestBrowser as well.
lars.sonchocky-helldorf
Comment 5 2011-07-05 10:47:44 PDT
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
Comment 6 2011-07-06 03:59:21 PDT
Created attachment 99811 [details] Pixel difference in FEMorphology effect
Andras Piroska
Comment 7 2011-07-06 04:04:00 PDT
Thanks, really wrong, I will fix it.
Zoltan Herczeg
Comment 8 2011-07-06 04:05:58 PDT
Nice catch! Can we add a Layout test for this?
Andras Piroska
Comment 9 2011-07-06 04:06:49 PDT
Created attachment 99813 [details] Animated FEMorphology SVG that show the bug on webkit-based browsers
Gabor Loki
Comment 10 2011-07-15 00:39:30 PDT
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
Comment 11 2011-07-15 00:47:36 PDT
> 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 12 2011-07-15 05:33:01 PDT
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
Comment 13 2011-07-15 05:54:12 PDT
Created attachment 100960 [details] Pixel difference in FEMorphology effect A comment is added, and the ChangeLog is modified as well.
Nikolas Zimmermann
Comment 14 2011-07-15 06:38:16 PDT
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
Comment 15 2011-07-15 06:45:24 PDT
> 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 16 2011-07-15 07:38:26 PDT
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
Comment 17 2011-07-15 07:38:34 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.