[Webkit-unassigned] [Bug 52200] Small filter primitive renderer improvements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=52200





--- Comment #29 from Zoltan Herczeg <zherczeg at webkit.org>  2011-01-24 01:01:18 PST ---
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:151
> > -            return false;
> > +            return true;
> 
> This is difficult. If you return true, the filtered object continues drawing on to the context: the real context! Means we'll draw the target on to the context and on top of it we draw the filter result. Thats bad because of couple of reasons. At first, because we draw the result of the filter with conpositeOver to the context we may see both drawings. Second, the repaintRect of the target depends on the filter area. If the filter area is outside of the objectBoundingBox from the target, we won't clear the results on animations or what ever. Third, we do unnecessary drawings. The filtered target element might be the root element of thounds of childs, that is inefficient. Why do you have to return true here? Shouldn't it work with false as well? We would enter postApply() even it we return false here.

Is it? I think I misunderstood the return value. Hm, it looks like it is working with "return false". Great.

> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:300
> > +            filterData->builded = true;
> > +        }
> > +
> > +        if (!lastEffect->hasResult()) {
> > +            // Always true if filterData is just built.
> 
> Why did you move 'filterData->builded = true;' atop of 'if (!lastEffect->hasResult())' Isn't it build after we called apply() the first time? and the 'if' doesn't even check filterData->builded so it shouldn't matter here, but makes more sense in general to set it as true, if we really built it.

Ok I moved it after the apply(). Anyway, why it is "builded" instead of "built" ?

> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:363
> > +        repaintRectangle(filterData->absoluteDrawingRegion);
> 
> Interesting, but you couldn't check it on your mac?

Yeah, our mac has been recently updated and now pixel tests fail with 0.49% difference. Since the image is the same, I think the font was changed. After a clean build, I will check this again, but I don't expect this will change sincs non-filter related images are fail as well.

> what if you have another element that is drawn on top of the filtered object? will it get repainted as well? (Unsure right now, a test case would help) Also isn't the parent renderer hierarchy RenderSVGResourceContainer-> RenderSVGHiddenContainer? Does it even affect any redrawings? I'd move this to another patch an invalidate like we did before. So you cann add more tests to check the correct behavior of the code.

It is ok for me, but how should the code look like? Keeping the functions, but not modify the DiffuseLighting? Or removing the code? Without repainting, the patch does nothing useful, so I think disabling the call in DiffuseLighting would make sense.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.