Bug 76559 - SVG animation repaint issue with image and dynamic clipPath
Summary: SVG animation repaint issue with image and dynamic clipPath
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kelly Norton
URL:
Keywords:
Depends on:
Blocks: 10403
  Show dependency treegraph
 
Reported: 2012-01-18 11:41 PST by Kelly Norton
Modified: 2012-01-26 07:50 PST (History)
7 users (show)

See Also:


Attachments
Reproducing Case (1.10 KB, image/svg+xml)
2012-01-18 11:41 PST, Kelly Norton
no flags Details
Reproducing Case w/ Red+Green (1.46 KB, image/svg+xml)
2012-01-20 07:02 PST, Kelly Norton
no flags Details
Patch (9.24 KB, patch)
2012-01-20 13:23 PST, Kelly Norton
no flags Details | Formatted Diff | Diff
Patch (155.95 KB, patch)
2012-01-22 10:57 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (156.18 KB, patch)
2012-01-22 11:56 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (147.62 KB, patch)
2012-01-23 02:33 PST, Nikolas Zimmermann
zherczeg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly Norton 2012-01-18 11:41:27 PST
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.
Comment 1 Dirk Schulze 2012-01-19 08:35:48 PST
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.
Comment 2 Kelly Norton 2012-01-20 07:02:21 PST
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.
Comment 3 Kelly Norton 2012-01-20 13:23:18 PST
Created attachment 123368 [details]
Patch
Comment 4 Nikolas Zimmermann 2012-01-21 00:38:28 PST
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?
Comment 5 Nikolas Zimmermann 2012-01-22 10:57:27 PST
Created attachment 123488 [details]
Patch
Comment 6 Nikolas Zimmermann 2012-01-22 10:59:11 PST
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).
Comment 7 WebKit Review Bot 2012-01-22 10:59:50 PST
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.
Comment 8 WebKit Review Bot 2012-01-22 11:49:45 PST
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
Comment 9 Nikolas Zimmermann 2012-01-22 11:56:03 PST
Created attachment 123499 [details]
Patch
Comment 10 Zoltan Herczeg 2012-01-23 02:08:05 PST
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?
Comment 11 Nikolas Zimmermann 2012-01-23 02:19:39 PST
(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.
Comment 12 Nikolas Zimmermann 2012-01-23 02:33:44 PST
Created attachment 123533 [details]
Patch v2
Comment 13 Zoltan Herczeg 2012-01-23 03:59:25 PST
> layoutTestController.display() to test the repainting - I'll change the text color to make it more readable.

I don't see any text anymore...
Comment 14 Zoltan Herczeg 2012-01-23 04:29:59 PST
> I don't see any text anymore...
Ok, so it helps to keep the png platform independent.
Comment 15 Zoltan Herczeg 2012-01-23 04:31:38 PST
Comment on attachment 123533 [details]
Patch v2

r=me

nice patch
Comment 16 Nikolas Zimmermann 2012-01-23 04:48:47 PST
Committed r105613: <http://trac.webkit.org/changeset/105613>
Comment 17 Nikolas Zimmermann 2012-01-26 07:50:39 PST
(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.