Bug 99996

Summary:

SVG Filter Updates do not trigger repaint

Product:

WebKit

Reporter:

Dominik Röttsches (drott) <d-r>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

NEW

Severity:

Normal

CC:

dino, eric, fmalita, gyuyoung.kim, krit, pdr, rakuco, schenney, simon.fraser, thorton, webkit.review.bot, xidorn-webkit, zimmermann

Priority:

Keywords:

CanvaBug

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
testResult_no_update.png	none
testResult_circle_update.png	none
Modification to the test case	none
Patch	krit: review-

Dominik Röttsches (drott)

Reported 2012-10-22 07:58:32 PDT

I believe there's something wrong with repaint triggering in the following test cases: svg/dynamic-updates/SVGFEDropShadowElement-dom-shadow-color-attr.html svg/dynamic-updates/SVGFEDropShadowElement-dom-shadow-opacity-attr.html svg/dynamic-updates/SVGFEDropShadowElement-svgdom-shadow-color-prop.html svg/dynamic-updates/SVGFEDropShadowElement-svgdom-shadow-opacity-prop.htm Taking a closer look at the first one: svg/dynamic-updates/SVGFEDropShadowElement-dom-shadow-color-attr.html In testResult_no_update.png you can see that the foreground circle is green, the shadow stays black and a repaint only happened to the debug log div at the bottom. In the debug log, it says that the flood color of the circle is green, which is not reflected in the SVG rendering. I think that's incorrect but I would like to hear some SVG experts' feedback. What I believe should happen is: Two repaint rects, one for the circle and its shadow, one for the debug log area. And the circle should have a green shadow. If I modify the test case to also modify the circle's color or change any other property of the foreground circle, its shadow gets repainted to the correct color, green. See attached testResult_circle_update.png So, I'd conclude that the filter update does not cause the required repaint of all elements that this filter is applied to.

Attachments
testResult_no_update.png (35.18 KB, image/png) 2012-10-22 08:00 PDT, Dominik Röttsches (drott)	no flags	Details
testResult_circle_update.png (36.06 KB, image/png) 2012-10-22 08:01 PDT, Dominik Röttsches (drott)	no flags	Details
Modification to the test case (775 bytes, patch) 2012-10-22 08:03 PDT, Dominik Röttsches (drott)	no flags	Details Formatted Diff Diff
Patch (712.03 KB, patch) 2012-10-24 06:20 PDT, Dominik Röttsches (drott)	krit: review-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Dominik Röttsches (drott)

Comment 1 2012-10-22 08:00:51 PDT

Created attachment 169910 [details] testResult_no_update.png

Dominik Röttsches (drott)

Comment 2 2012-10-22 08:01:12 PDT

Created attachment 169911 [details] testResult_circle_update.png

Dominik Röttsches (drott)

Comment 3 2012-10-22 08:03:22 PDT

Created attachment 169913 [details] Modification to the test case

Dirk Schulze

Comment 4 2012-10-22 11:01:51 PDT

Basically it means that a property update on a primitive does not trigger a repainting of the filtered element. Sounds interesting. I couldn't test it right now but this would be a regression.

Dominik Röttsches (drott)

Comment 5 2012-10-23 01:59:08 PDT

(In reply to comment #4) > Basically it means that a property update on a primitive does not trigger a repainting of the filtered element. Sounds interesting. I couldn't test it right now but this would be a regression. Thanks for your feedback - I am not sure it's a regression though. Even the initial expectations for the test case I am referring to did not show the green circle.

Dominik Röttsches (drott)

Comment 6 2012-10-23 01:59:54 PDT

(In reply to comment #5) > .. the green circle. The green shadow circle, that is.

Dominik Röttsches (drott)

Comment 7 2012-10-23 07:24:22 PDT

I have a fix for the first two cases, working on the remainder. The problem is that the flood-color and flood-opacity properties are not causing an invalidate.

Dominik Röttsches (drott)

Comment 8 2012-10-24 06:20:18 PDT

Created attachment 170378 [details] Patch

Dominik Röttsches (drott)

Comment 9 2012-10-24 06:23:04 PDT

Comment on attachment 170378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170378&action=review This is a bit of a new area for me, I have some questions. For case 2) mentioned in the ChangeLog I am pretty sure my change is correct, for case 1) some feedback would be helpful. > Source/WebCore/svg/SVGFEDropShadowElement.cpp:158 > + if (attrName != SVGNames::flood_colorAttr && attrName != SVGNames::flood_opacityAttr) Do we need to do something with the attribute values locally? > Source/WebCore/svg/SVGFEDropShadowElement.cpp:-150 > - ASSERT_NOT_REACHED(); I removed this assertion since it's only testing whether isSupportedAttribute is identical to those four.

Dominik Röttsches (drott)

Comment 10 2012-10-24 06:25:28 PDT

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

Dirk Schulze

Comment 11 2012-10-26 08:03:27 PDT

Comment on attachment 170378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170378&action=review > Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:54 > + if (node()->hasTagName(SVGNames::feFloodTag) || node()->hasTagName(SVGNames::feDropShadowTag)) { Looks reasonable. Wasn't that enough to trigger repainting? > Source/WebCore/svg/SVGFEDropShadowElement.cpp:86 > +bool SVGFEDropShadowElement::setFilterEffectAttribute(FilterEffect* effect, const QualifiedName& attrName) It is very strange that this is implemented in some SVGFE*Elements, but not in others. > Source/WebCore/svg/SVGFEDropShadowElement.cpp:101 > + if (attrName == SVGNames::flood_colorAttr) { > + dropShadow->setShadowColor(style->svgStyle()->floodColor()); > + return true; > + } > + if (attrName == SVGNames::flood_opacityAttr) { > + dropShadow->setShadowOpacity(style->svgStyle()->floodOpacity()); > + return true; > + } This is very strange that we have that here (and in other elements). I think I can remember why we added it. But it is still not the proper solution. Repainting should be handled by RenderStyle on CSS Properties. While this may work with attributes, what about inline styles? feDropShadoe.style.floodColor = 0.5; Does this repaint? Can you check that please? >> Source/WebCore/svg/SVGFEDropShadowElement.cpp:158 >> + if (attrName != SVGNames::flood_colorAttr && attrName != SVGNames::flood_opacityAttr) > > Do we need to do something with the attribute values locally? This is handled by CSS. You can remove this. >> Source/WebCore/svg/SVGFEDropShadowElement.cpp:-150 >> - ASSERT_NOT_REACHED(); > > I removed this assertion since it's only testing whether isSupportedAttribute is identical to those four. Why remove it? So we make sure that we don't miss something. > LayoutTests/ChangeLog:10 > + Rebaselining GTK and EFL svg filter tests after repainting issue has been fixed. > + For Chromium, Mac & Qt I am marking the tests as ImageOnlyFailure since pixel results on these ports > + need updates. Looking at your tests, I realized two things. First, some repaint areas are huge! Why is that the case? Second, CSS properties don't trigger repainting. That means the whole concept is wrong and needs to be removed. This is not your fault, but making the attribute changes working is not the right approach. We need to refactor the whole thing and let RenderStyle do it's job.

Dominik Röttsches (drott)

Comment 12 2012-10-26 08:23:11 PDT

(In reply to comment #11) > (From update of attachment 170378 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170378&action=review > > > Source/WebCore/rendering/svg/RenderSVGResourceFilterPrimitive.cpp:54 > > + if (node()->hasTagName(SVGNames::feFloodTag) || node()->hasTagName(SVGNames::feDropShadowTag)) { > > Looks reasonable. Wasn't that enough to trigger repainting? No, this calls the setFilterEffectAttribute ultimately. But drop shadow didn't have one, then it was asserting in the base class' method. > > > Source/WebCore/svg/SVGFEDropShadowElement.cpp:86 > > +bool SVGFEDropShadowElement::setFilterEffectAttribute(FilterEffect* effect, const QualifiedName& attrName) > > It is very strange that this is implemented in some SVGFE*Elements, but not in others. Yep, see above. > > Source/WebCore/svg/SVGFEDropShadowElement.cpp:101 > > + if (attrName == SVGNames::flood_colorAttr) { > > + dropShadow->setShadowColor(style->svgStyle()->floodColor()); > > + return true; > > + } > > + if (attrName == SVGNames::flood_opacityAttr) { > > + dropShadow->setShadowOpacity(style->svgStyle()->floodOpacity()); > > + return true; > > + } > > This is very strange that we have that here (and in other elements). I think I can remember why we added it. But it is still not the proper solution. Repainting should be handled by RenderStyle on CSS Properties. While this may work with attributes, what about inline styles? > > feDropShadoe.style.floodColor = 0.5; > > Does this repaint? Can you check that please? Will check and report back. > >> Source/WebCore/svg/SVGFEDropShadowElement.cpp:158 > >> + if (attrName != SVGNames::flood_colorAttr && attrName != SVGNames::flood_opacityAttr) > > > > Do we need to do something with the attribute values locally? > > This is handled by CSS. You can remove this. Okay. > >> Source/WebCore/svg/SVGFEDropShadowElement.cpp:-150 > >> - ASSERT_NOT_REACHED(); > > > > I removed this assertion since it's only testing whether isSupportedAttribute is identical to those four. > > Why remove it? So we make sure that we don't miss something. Well, in the other implementations the assertion in this place is really meant to check whether the function is called with any properties that are not handled. But we return early anyway, so the assertion doesn't make much sense. > > LayoutTests/ChangeLog:10 > > + Rebaselining GTK and EFL svg filter tests after repainting issue has been fixed. > > + For Chromium, Mac & Qt I am marking the tests as ImageOnlyFailure since pixel results on these ports > > + need updates. > > Looking at your tests, I realized two things. First, some repaint areas are huge! Why is that the case? That I don't know. This is really not my area, so your help with fixing this is certainly appreciate. I just had a go at fixing the basic case. > Second, CSS properties don't trigger repainting. That means the whole concept is wrong and needs to be removed. This is not your fault, but making the attribute changes working is not the right approach. We need to refactor the whole thing and let RenderStyle do it's job. Same here, your help or at least guidance is appreciated. I am not sure I have time and more importantly enough grasp of this architecture to refactor this. Should we still land this after reworking it according to your comments? Or should this be solved by the refactoring?

Dirk Schulze

Comment 13 2012-10-26 11:29:44 PDT

(In reply to comment #12) > Should we still land this after reworking it according to your comments? Or should this be solved by the refactoring? IMO it should be solved by the refactoring :/. This patch would just fix an edge case. In times of CSS Animations and CSS Transitions, the CSSOM gets more important.

Dominik Röttsches (drott)

Comment 14 2012-10-29 01:37:40 PDT

(In reply to comment #13) > (In reply to comment #12) > > > Should we still land this after reworking it according to your comments? Or should this be solved by the refactoring? > > IMO it should be solved by the refactoring :/. This patch would just fix an edge case. In times of CSS Animations and CSS Transitions, the CSSOM gets more important. Understand. I think my knowledge of this area and time to do look into it are limited at this point, I'd leave it up to the people more knowledgeable in this area to do this. Resetting assignee to default.

Note You need to log in before you can comment on or make changes to this bug.