Bug 76559 – SVG animation repaint issue with image and dynamic clipPath

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Kelly Norton]

Created attachment 122971 [details]
Reproducing Case

When a clip-path affecting an image is updated dynamically, the bounds of the image are not properly updated causing incorrect damage rectangle calculation on the next repaint.

[Dirk Schulze]

It would be great if you can use red and green boxes. If you can see red somewhere  you know that the test fails. Right now I can see two different results between Safari and WebKit trunk and I am still not sure what I should see.

[Kelly Norton]

Created attachment 123310 [details]
Reproducing Case w/ Red+Green

I'm also seeing slightly different repaint patterns. However, each should show some red with this one.

[Kelly Norton]

Created attachment 123368 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 123368 [details]
Patch

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

r- because of the test. Thanks for tackling this problem!

> Source/WebCore/rendering/svg/RenderSVGImage.h:44
> +    virtual void setNeedsBoundariesUpdate() { m_needsBoundariesUpdate = true; }

Who is calling this? It's not clear from the patch, that this is the actual bug fix :-)

> ChangeLog:8
> +        * ManualTests/svg-repaint-image-clip-path.svg: Added.

A manual test should really be avoided here, it's too important to not automatize it.
Can you look at creating a dynamic testcase?
I'd imagine doing this via a <script>, and a setTimeout(..., 0) - that should be enough to trigger it, no?

[Nikolas Zimmermann]

Created attachment 123488 [details]
Patch

[Nikolas Zimmermann]

It turns out that Kellys fix was not 100% correct - I hit a smiler bug that can also be fixed, when changing this patch a little, so I uploaded a new version based on Kellys, including the layout test that I asked for (which uses layoutTestController.display() to check repainting).

[WebKit Review Bot]

Attachment 123488 [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

Last 3072 characters of output:
utTests/platform/chromium/test_expectations.txt:3927:  Path does not exist. svg/filters/feImage-preserveAspectRatio.svg  [test/expectations] [2]
ERROR: FAILURES FOR <lucid, x86, debug, cpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg
LayoutTests/platform/chromium/test_expectations.txt:3927:  Path does not exist. svg/filters/feImage-preserveAspectRatio.svg  [test/expectations] [2]
ERROR: FAILURES FOR <lucid, x86, debug, gpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg
LayoutTests/platform/chromium/test_expectations.txt:3927:  Path does not exist. svg/filters/feImage-preserveAspectRatio.svg  [test/expectations] [2]
ERROR: FAILURES FOR <lucid, x86, release, cpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg
LayoutTests/platform/chromium/test_expectations.txt:3927:  Path does not exist. svg/filters/feImage-preserveAspectRatio.svg  [test/expectations] [2]
ERROR: FAILURES FOR <lucid, x86, release, gpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg
LayoutTests/platform/chromium/test_expectations.txt:3927:  Path does not exist. svg/filters/feImage-preserveAspectRatio.svg  [test/expectations] [2]
ERROR: FAILURES FOR <lucid, x86_64, debug, cpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg
LayoutTests/platform/chromium/test_expectations.txt:3927:  Path does not exist. svg/filters/feImage-preserveAspectRatio.svg  [test/expectations] [2]
ERROR: FAILURES FOR <lucid, x86_64, debug, gpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg
LayoutTests/platform/chromium/test_expectations.txt:3927:  Path does not exist. svg/filters/feImage-preserveAspectRatio.svg  [test/expectations] [2]
ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg
LayoutTests/platform/chromium/test_expectations.txt:3927:  Path does not exist. svg/filters/feImage-preserveAspectRatio.svg  [test/expectations] [2]
ERROR: FAILURES FOR <lucid, x86_64, release, gpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg
LayoutTests/platform/chromium/test_expectations.txt:3927:  Path does not exist. svg/filters/feImage-preserveAspectRatio.svg  [test/expectations] [2]
Total errors found: 32 in 12 files


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

[WebKit Review Bot]

Comment on attachment 123488 [details]
Patch

Attachment 123488 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11179067

New failing tests:
svg/repaint/image-with-clip-path.svg
svg/custom/relative-sized-image.xhtml

[Nikolas Zimmermann]

Created attachment 123499 [details]
Patch

[Zoltan Herczeg]

Comment on attachment 123499 [details]
Patch

Some comments:

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

> Source/WebCore/ChangeLog:14
> +        The logic is also inconsistent, compared to the other renderes.

renderers

> Source/WebCore/rendering/svg/RenderSVGImage.cpp:78
> +    if (oldBoundaries != m_objectBoundingBox) {
> +        m_imageResource->setContainerSizeForRenderer(enclosingIntRect(m_objectBoundingBox).size());
> +        m_needsBoundariesUpdate = true;
> +        return true;
> +    }
> +
> +    return false;

WebKit prefers early return, I would swap these two branches.

> Source/WebCore/rendering/svg/RenderSVGImage.cpp:86
> +    updateImageViewport();

This function is defined as returning with bool but its return value is unused. Why?

> LayoutTests/svg/repaint/image-with-clip-path-expected.txt:1
> +Bug 76559: After 200ms, A single green 50x50 rectangle should appear with center at 200, 50. All red should disappear when the green square appears.

What is the reason of the gray background?

[Nikolas Zimmermann]

(In reply to comment #10)
> renderers
Fixed.

> WebKit prefers early return, I would swap these two branches.
Fixed.

> > Source/WebCore/rendering/svg/RenderSVGImage.cpp:86
> > +    updateImageViewport();
> 
> This function is defined as returning with bool but its return value is unused. Why?
The bool is onlly used from SVGImageElement, when it determines if the bounds changed, and if so, it'll call setNeedsLayout(true) on the renderer. It serves two purposes.
 
> > LayoutTests/svg/repaint/image-with-clip-path-expected.txt:1
> > +Bug 76559: After 200ms, A single green 50x50 rectangle should appear with center at 200, 50. All red should disappear when the green square appears.
> 
> What is the reason of the gray background?
layoutTestController.display() to test the repainting - I'll change the text color to make it more readable.

[Nikolas Zimmermann]

Created attachment 123533 [details]
Patch v2

[Zoltan Herczeg]

> layoutTestController.display() to test the repainting - I'll change the text color to make it more readable.

I don't see any text anymore...

[Zoltan Herczeg]

> I don't see any text anymore...
Ok, so it helps to keep the png platform independent.

[Zoltan Herczeg]

Comment on attachment 123533 [details]
Patch v2

r=me

nice patch

[Nikolas Zimmermann]

Committed r105613: <http://trac.webkit.org/changeset/105613>

[Nikolas Zimmermann]

(In reply to comment #16)
> Committed r105613: <http://trac.webkit.org/changeset/105613>
The new test is flaky under guard malloc, filed bug 77103 for this.