Bug 76527 – Mask deformations when masked content is rotated

• Component: SVG
• Status: RESOLVED FIXED

[honyk]

Created attachment 122899 [details]
Test Case

1. Open the attached image in Chrome 18-dev or Safari
2. Drag and rotate that yellow arc
3. While FF, MSIE and Opera render that arc properly, Webkit based browsers deform the original mask shape

[Florin Malita]

Created attachment 123991 [details]
Reduced test.

[Florin Malita]

The problem appears to be aggressive caching of mask image data: it is generated based on the target clamped rect (RenderSVGResourceMasker::applyResource()), but never updated/invalidated if the target rect changes (transform updates, etc.). Then the old image buffer is applied as a mask to a different size rect and the graphics context scales it to fit -> hence the deformation.

The same issue also affects clip paths.

One solution is to force a mask/clip data update on target rect changes.

[Florin Malita]

Created attachment 124002 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 124002 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        Track the target rect used when creating the mask/clip image buffer, and re-generate the data
> +        on changes.

This seems like the wrong approach - we expect the resources to be invalidated, if they need to be updated. They shouldn't trigger invalidations on their own!
I'll look through the testcase, and see why it doesn't work atm.

[Nikolas Zimmermann]

Comment on attachment 124002 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        Track the target rect used when creating the mask/clip image buffer, and re-generate the data
> +        on changes.

This seems like the wrong approach - we expect the resources to be invalidated, if they need to be updated. They shouldn't trigger invalidations on their own!
I'll look through the testcase, and see why it doesn't work atm.

[Florin Malita]

(In reply to comment #5)
> (From update of attachment 124002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124002&action=review
> 
> > Source/WebCore/ChangeLog:24
> > +        Track the target rect used when creating the mask/clip image buffer, and re-generate the data
> > +        on changes.
> 
> This seems like the wrong approach - we expect the resources to be invalidated, if they need to be updated.

I've already looked at the invalidation path.

RenderSVGResource::markForLayoutAndParentResourceInvalidation walks the ancestor chain and invalidates any resource container it finds:

     while (current) {
        removeFromFilterCache(current);

        if (current->isSVGResourceContainer()) {
            // This will process the rest of the ancestors.
            current->toRenderSVGResourceContainer()->removeAllClientsFromCache();
            break;
        }

        current = current->parent();
    }


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}


So walking RenderSVGRect's ancestor chain doesn't hit any resource containers. We can tweak the implementation to query and invalidate the resources for each ancestor - let me know if you prefer this approach and I'll update the patch.

But do we really want to invalidate the mask data on target updates?

1) with the test submitted by honyk for example, there's only a narrow range of angles which cause variation in the target rect - so the current patch saves a bunch of mask redraws. I guess we could filter this on the invalidation path also, but...

2) the fact that the mask image buffer depends on its target repaint rect seems like a local implementation detail (optimization) which should not leak into the framework

Could we just size the mask image buffer independently of the target instead and avoid all this?

[honyk]

Any progress in this?

> the current patch saves a bunch of mask redraws
I don't know internals, but it sounds reasonable to me.

[Nikolas Zimmermann]

Comment on attachment 124002 [details]
Patch

I'd like to see the testcases using the repaint.js framework, instead of reftests. Also I'm still not sure I understand it yet:
<g mask="ur(#mask)">
 <rect id="masker"..>
If you change the transform attribute for masker, it has no effect on the mask in trunk?
If you'd use <rect id="masker" mask="..." /> and change the transform here, the mask updates properly in trunk?

I'm not at all against, revisiting the whole resource updating logic, but I wouldn't like it, if we had a mixture. Currently most resource updating logic is piped through markForLayoutAndParentResourceInvalidation, and it would be a new approach, if clippers/maskers/filters would track the target repaint rect instead to save redraws. The question is: how can we unify this?

In general, I think tracking the absolute repaint rect should be an optimization, and not a bug fix? Isn't it just a side-effect that this is now fixed.
Are there no other ways of invalidation that still won't work?

What about, eg:
<g mask="url(#mask)">
    <svg id="svgWithViewBox" viewBox="0 0 1 100">
        <rect id="masker" x="...
    </svg
</g>

Now change the 'viewBox' attribute through a script. Does that work in trunk, or with your patch? I don't fully understand it yet, it seems.

[Nikolas Zimmermann]

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 :-)

[Florin Malita]

Created attachment 141773 [details]
Patch

[WebKit Review Bot]

Attachment 141773 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:3780:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. fast/dom/Window/window-property-shadowing-name.html  [test/expectations] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Florin Malita]

(In reply to comment #11)
> Attachment 141773 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> LayoutTests/platform/chromium/test_expectations.txt:3780:  BUG\d+ is not allowed, must be one of BUGCR\d+, BUGWK\d+, BUGV8_\d+, or a non-numeric bug identifier. fast/dom/Window/window-property-shadowing-name.html  [test/expectations] [5]
> Total errors found: 1 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

That line is not part of this patch.

[Nikolas Zimmermann]

Comment on attachment 141773 [details]
Patch

Looks great, r=me.

[WebKit Review Bot]

Comment on attachment 141773 [details]
Patch

Clearing flags on attachment: 141773

Committed r117056: <http://trac.webkit.org/changeset/117056>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.