[Webkit-unassigned] [Bug 76527] Mask deformations when masked content is rotated

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Nikolas Zimmermann <zimmermann at kde.org>  2012-02-25 04:18:55 PST ---
Note the attachment "Reduced test" can be reduced even more:
the spinner1 setAttribute('transform') is not needed at all to trigger the bug, just makes it more visible.
Anyhow, you've said:
<quote>
The problem is that in this case the resource container is not an ancestor of RenderSVGRect, but a sibling:
        RenderSVGContainer {g}
          [masker="mask"] RenderSVGResourceMasker {mask}
          RenderSVGRect {rect}
</quote>

I don't see this in your testcase:
<defs>
  <mask id="mask">
    <circle cx="50" r="50" fill="white"/>
  </mask>
</defs>
...
<g mask="url(#mask)">
      <rect id="spinner2" y="-50" width="100" height="200" fill="green" onmousedown="rotate()"/>
</g>

..
        function rotate() {
                angle = angle + 5;
                document.getElementById('spinner2').setAttribute('transform', 'rotate(' + angle + ')');
...


When the <rect id="spinner2" transform changes, it should mark itself for layout and parent resource invalidation, from SVGStyledTransformableElement::svgAttributeChanged:
    if (attrName == SVGNames::transformAttr) {
        object->setNeedsTransformUpdate();
        RenderSVGResource::markForLayoutAndParentResourceInvalidation(object);
...

void RenderSVGResource::markForLayoutAndParentResourceInvalidation(RenderObject* object, bool needsLayout)
{
...
    removeFromFilterCacheAndInvalidateDependencies(object, needsLayout);
...
    RenderObject* current = object->parent();
    while (current) {
...
        if (current->isSVGResourceContainer()) {
            // This will process the rest of the ancestors.
            current->toRenderSVGResourceContainer()->removeAllClientsFromCache();
....

So what does markForLayoutAndParentResourceInvalidation do:
It deregisters all clients of elements in the ancestor chain, that are resources. eg: 
<linearGradient><stop/> ... if the <stop> 'offset' attribute changes, RenderSVGResourceLinearGradients::removeAllClientsFromCache(), so the gradient will be rebuilt, once its used next time.

What this method doesn't do is to check whether any ancestor element itself references resources.
It only does that for filter resources.

static inline void removeFromFilterCacheAndInvalidateDependencies(RenderObject* object, bool needsLayout)
{
    ASSERT(object);
#if ENABLE(FILTERS)
    if (SVGResources* resources = SVGResourcesCache::cachedResourcesForRenderObject(object)) {
        if (RenderSVGResourceFilter* filter = resources->filter())
            filter->removeClientFromCache(object);
    }
#endif

I think the correct fix is to extend this for clip/mask, like its done in SVGResources::removeClientFromCache(). We could refactor this to
removeClipperFilterMaskerDataFomCache(), and use that both from SVGResources::removeClientFromCache and from removeFromFilterCacheAndInvalidateDeps, which should then of course be renamed to something correct :-)

For fun, I tried to change the testcase to switch between fill="red" and fill="green" every 50ms. It turns out that this works fine and properly invalidates the masker resource. This is because clientStyleDidChange, calls clientUpdatedFromElement, which _rebuilds_ the SVGResources, even though the resources themselves haven't changed (aka. you didn't add or remove a mask/clip-path attribute). So it only works because we unncessaryly rebuild the SVGResources (removeResourcedFromRenderObject/addResourcesFromRenderObject) even though they didn't change.

If we fix that bug (change clientUpdatedFromElement to only rebuild the SVGResources* when necessary, CSS changes would suffer from the same mask/clip bug that you've found, erm honyk found).

To summarize: I'd advice to add mask/clip invalidation in removeFromFilterCacheAndInvalidateDependencies, as described above, that should fix all these bugs. We should then investigate cases where we do needless invalidations, instead of avoding them with the cache and compare target repaint rect sledgehammer. Needless invalidations have to be stopped right from the beginning, they shoulnd't happen at all - the whole resources concept is based on that.

Ouch, what a long post, I hope you're still with me :-)

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