809181 - VectorImage::ExtractFrame produces rendering issues if multiple images with the same underlying SVG document end up displayed on the same page

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Comment 1

[Seth Fowler [:seth] [:s2h]]

VectorImage stores the last size it rendered at in |mLastRenderedSize|. This is used to determine whether to call mSVGDocumentWrapper::UpdateViewportBounds() when VectorImage::Draw() is called. If multiple VectorImages with the same underlying SVG documents but different sizes end up coexisting on the same page, which can happen through the use of ExtractFrame(), then after the first rendering (when none of them have a valid mLastRenderedSize, so they all call UpdateViewportBounds() and everything's OK), subsequent renderings will render all of them with the viewport size of the last image to be rendered. This produces obvious visual glitches.

The solution is for mSVGDocumentWrapper::UpdateViewportBounds() to decide internally whether the bounds have changed and action is needed, rather than leaving that decision to the caller.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 1) SVGDocumentWrapper::UpdateViewportBounds() should check if anything has changed instead of relying on callers.

Proposed patch.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 678990 [details] [diff] [review]
(part 1) SVGDocumentWrapper::UpdateViewportBounds() should check if anything has changed instead of relying on callers.

Looks good!

Per IRC, we should include a reftest for this if possible.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 2) Test.

Here's an attempt at a reftest for this. I've tried a lot of variations, and I just can't reproduce this in a reftest, although I can easily demonstrate it in a normal browser.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 2) Test.

Minor typo fix in the test.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 679004 [details] [diff] [review]
(part 2) Test.

Reftest works for me! :)  fails w/o patch, passes w/ patch.

Just a few nits:

* SVG-as-an-image reftests currently live in https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/as-image/ , so we should probably put this test there, instead of in a new /image/reftests/svg directory.

(The reftests do mostly test code that lives in /image, but I stuck them in /layout/reftests back when I added SVG-as-an-image support because that's where basically all reftests live, even for non-layout code, with a few exceptions. Might be worth migrating them to /image at some point, but for now, we should keep all the SVG-as-an-image reftests together.)

>@@ -0,0 +1,6 @@
>+<?xml version="1.0" encoding="UTF-8" ?>
>+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
>+<svg viewBox="0 0 40 20" version="1.1" xmlns="http://www.w3.org/2000/svg">

As of the semi-recent MPL 2 switchover, we generally prefer for testcase files to all have the public-domain header near the top, from here:
 https://www.mozilla.org/MPL/headers/

(This would go right before the <svg> tag, I think.  It'd also go in the HTML files, before your existing comment w/ the bug link there.)

>+<path fill="#000000" d=" M 0.00 0.00 L 20.00 0.00 C 20.00 6.67 20.00 13.33 20.00 20.00 C 13.33 20.00 6.67 20.00 0.00 20.00 L 0.00 0.00 Z" />
>+<path fill="#ff0000" d=" M 20.00 0.00 L 40.00 0.00 C 40.00 6.67 40.00 13.33 40.00 20.00 C 33.33 20.00 26.67 20.00 20.00 20.00 C 20.00 13.33 20.00 6.67 20.00 0.00 Z" />
>+</svg>
>diff --git a/image/test/reftest/svg/reftest.list b/image/test/reftest/svg/reftest.list
>new file mode 100644
>--- /dev/null
>+++ b/image/test/reftest/svg/reftest.list
>@@ -0,0 +1,3 @@
>+# SVG tests 

>+== svg-border-image-repaint.html svg-border-image-repaint-ref.html

Name these files ...-1.html and ...-1-ref.html.  (We try to do that for all reftests, so we can easily add -2, -3, etc. variants.  Even if it's not anticipated, it's good for consistency)

Also: "border-image.svg" might be a little too general of a name for the helper SVG file.  We may want to test other border-image bugs later on, and it'd look like this is the One Border-Image Helper SVG File To Rule Them All, which it likely isn't.  Maybe call it "svg-border-image-repaint-helper.svg"?

>diff --git a/image/test/reftest/svg/svg-border-image-repaint.html b/image/test/reftest/svg/svg-border-image-repaint.html
>new file mode 100644
>--- /dev/null
>+++ b/image/test/reftest/svg/svg-border-image-repaint.html
>@@ -0,0 +1,17 @@
>+<!DOCTYPE html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=809181
>+-->
>+<html id='root' class="reftest-wait">
>+  <head>
>+    <script>
>+      document.addEventListener('MozReftestInvalidate', function() {
>+        document.getElementById('test').style.opacity = '0.5';
>+        document.getElementById('root').removeAttribute('class');

document.documentElement.removeAttribute('class'); should work.  (That's what most reftest-wait tests do.)  And then the <html> element won't need an ID anymore.

r=me with those addressed.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: Demonstration of the problem in the browser. (Open and wait 2 seconds, until the background changes.)

This demonstrates the bug in the browser. (This also demonstrates other bugs... border-image doesn't work too well with SVGs. But it's the repainting glitch we care about here.)

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 2) Test.

Thanks Daniel. I made the requested changes. Carrying over r+.

Comment 9

[Seth Fowler [:seth] [:s2h]]

I've started an initial try job with just the test. This is expected to fail. Link here: https://tbpl.mozilla.org/?tree=Try&rev=ae2b208d5c03

Comment 10

[Seth Fowler [:seth] [:s2h]]

I've started a second try job with both the test and the patch. This is expected to pass. Link here: https://tbpl.mozilla.org/?tree=Try&rev=718c2c45a8e1

Comment 11

[Mozilla RelEng Bot]

Try run for 718c2c45a8e1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=718c2c45a8e1
Results (out of 257 total builds):
    success: 221
    warnings: 35
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-718c2c45a8e1

Comment 12

[Mozilla RelEng Bot]

Try run for ae2b208d5c03 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ae2b208d5c03
Results (out of 256 total builds):
    success: 212
    warnings: 43
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ae2b208d5c03

Comment 13

[Seth Fowler [:seth] [:s2h]]

Trying again. The first attempt appears to have failed because of small differences in the colors produced by "opacity: 0.5". This one is expected to fail: https://tbpl.mozilla.org/?tree=Try&rev=fda7ca7622c3

Comment 14

[Seth Fowler [:seth] [:s2h]]

This one is expected to succeed: https://tbpl.mozilla.org/?tree=Try&rev=52542b3ea4b9

Comment 15

[Seth Fowler [:seth] [:s2h]]

Attachment: (part 2) Test.

(Carrying forward r+) This is the updated version of the test. The final opacity is 1 instead of 0.5 now.

Comment 16

[Mozilla RelEng Bot]

Try run for 52542b3ea4b9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=52542b3ea4b9
Results (out of 257 total builds):
    success: 232
    warnings: 24
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-52542b3ea4b9

Comment 17

[Mozilla RelEng Bot]

Try run for fda7ca7622c3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fda7ca7622c3
Results (out of 257 total builds):
    success: 216
    warnings: 40
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fda7ca7622c3

Comment 18

[Seth Fowler [:seth] [:s2h]]

OK, we're good to go. The test fails without the patch and passes with it. Requesting checkin for both part 1 and part 2.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f2cbe6e161
https://hg.mozilla.org/integration/mozilla-inbound/rev/0346ce24bb4c

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a7f2cbe6e161
https://hg.mozilla.org/mozilla-central/rev/0346ce24bb4c