Closed Bug 978911 Opened 6 years ago Closed 6 years ago

Intermittent scale-1.svg | image comparison (==), max difference: 255, number of differing pixels: {3441,3456}

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox28 --- unaffected
firefox29 --- unaffected
firefox30 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: RyanVM, Assigned: mattwoodrow)

References

Details

(Keywords: intermittent-failure, regression)

Attachments

(7 files, 2 obsolete files)

Attached image test screenshot
Unfortunately, this started after a mega-merge, so there's a LOT of changes to sift through:
https://hg.mozilla.org/integration/fx-team/pushloghtml?startID=5389&endID=5390

But I do find it interesting that it's happening on multiple platforms. Also, bug 974643 looks maybe relevant?

https://tbpl.mozilla.org/php/getParsedLog.php?id=35542323&tree=Fx-Team

Windows XP 32-bit fx-team opt test reftest on 2014-03-03 10:41:43 PST for push d8033ff0e71a
slave: t-xp32-ix-106

10:56:51     INFO -  REFTEST TEST-START | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/smil/transform/scale-1.svg
10:56:52     INFO -  REFTEST TEST-LOAD | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/smil/transform/scale-1.svg | 8356 / 10596 (78%)
10:56:52     INFO -  REFTEST TEST-LOAD | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/smil/transform/scale-1-ref.svg | 8356 / 10596 (78%)
10:56:52     INFO -  REFTEST TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/smil/transform/scale-1.svg | image comparison (==), max difference: 255, number of differing pixels: 3441
10:56:52     INFO -  REFTEST   IMAGE 1 (TEST): 
10:56:52     INFO -  REFTEST   IMAGE 2 (REFERENCE): 
10:56:52     INFO -  REFTEST INFO | Loading a blank page
10:56:52     INFO -  REFTEST TEST-END | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/svg/smil/transform/scale-1.svg
Flags: needinfo?(tnikkel)
Bug 974643 should have no effect on winxp and there doesn't appear to be anything position: fixed in the failing test so bug 974643 causing this is very unlikely.

I took at look at the range but nothing popped out at me.
Flags: needinfo?(tnikkel)
Dan, I don't suppose you see anything interesting in that range?
The test is about various animations on the SVG "transform" attribute, which points me at:
 mozilla-central changeset 9a491cb7be31	(Bug 971720) for mentioning "transform"
 mozilla-central changeset 4e0303f0aebc	(Bug 976877) for mentioning "invalidation"

I suspect the latter cset in particular, since this smells like an invalidation bug.
Yeah, definitely looks like an invalidation / repaint bug.

In all of the failures, the testcase is rendering with the center smiley being too huge (though it seems to be clipped to the correct box for the smaller size that it *should* be rendering at)

When the test initially loads -- i.e. at animation time=0 -- it is correct for the center smiley  to be huge.  (all the other ones should be tiny)

But just before we take the snapshot, we seek the animation timeline to 101 sec, at which point all of the smileys should be the same size.

So the issue seems to be that we're not correctly repainting that center smiley.

Special things about the center smiley (as compared to the others), which might be involved in causing this issue:
 - It shrinks in-place (i.e. its final invalidation region is a subset of its initial invalidation region)
 - It's the only animated piece of content that *initially* has the transform attribute set, before the animation takes effect. (Maybe that makes it a layer up-front, whereas the others all get converted to layers after the animation takes effect?)

In any case, setting needinfo=mattwoodrow to take a look, since his csets seem to be implicated per comment 5.
Flags: needinfo?(matt.woodrow)
Interesting, that would suggest an existing SVG invalidation bug that was being hidden.

My change removed a region simplification, where we were frequently taking a set of invalid rects and simplifying to a single rect that covered the bounds of the region.

If we were failing to invalidate the right parts of the SVG, but they happened to get included because of this simplification, then it would make sense that this would regress the test.
Flags: needinfo?(matt.woodrow)
Component: Layout → SVG
Keywords: regression
Here's a patch that makes this test use MozReftestInvalidate, which makes it more deterministic (in that we'll now *definitely render* the t=0 state, and then advance to t=101, and then re-render).

This makes this test consistently fail (perma-orange) on my local machine, with the failure matching the attached test screenshot.

This means that this is a real, consistent invalidation issue, and for the TBPL runs where this test passes, it's only passing because we're skipping the initial rendering (from the onload firing quickly).
Just to confirm -- using that reftest-patch, I verified that this issue indeed started as of 4e0303f0aebc, too.

(A build from that cset fails the reftest reliably, with attachment 8385803 [details] [diff] [review] applied. The parent cset (1b4b7d198185) passes the reftest reliably, with attachment 8385803 [details] [diff] [review] applied.)

Per comment 7, this likely means that 4e0303f0aebc exposed an underlying invalidation bug, rather than introducing a new bug.
Hardware: ARM → All
Version: unspecified → Trunk
Bug 975757 is another svg invalidation bug involving <use>.
Attached image live testcase
Here's a live testcase to demonstrate the bug. In my nightly build, the circle leaves behind artifacts, after its post-load dynamic tweak.
This screenshot with paint flashing shows what seems to be the problem.

It looks like we're neglecting to invalidate the intersection of the old bounding-box and the new bounding-box.
Here's the output from the attached testcase's dynamic tweak, when I turn on nglayout.debug.invalidation:
{
Starting ProcessPendingUpdates
---- PAINT START ----PresShell(0x7f8cef14c000), nsView(0x7f8cef2dccc0), nsIWidget(0x7f8cf05a5400)
Display item type nsDisplayTransform(0x7f8ce5162250) (in layer 0x7f8ceac1ac00) changed geometry!
Invalidating layer 0x7f8ceac1ac00: < (x=53, y=53, w=73, h=7); (x=53, y=60, w=7, h=66); (x=126, y=60, w=14, h=66); (x=60, y=126, w=80, h=14); >
Invalidating entire layer 0x7f8ceac1c800
Inactive LayerManager(0x7f8ce9450740) for display item nsDisplayTransform(0x7f8ce5162250) has an invalid region - invalidating layer 0x7f8ceac1ac00
Invalidating layer 0x7f8ceac1ac00: < (x=54, y=54, w=72, h=6); (x=54, y=60, w=6, h=66); (x=126, y=60, w=14, h=66); (x=60, y=126, w=80, h=14); >
---- PAINT END ----
Ending ProcessPendingUpdates
--COMPOSITE-- 0x7f8cef14c000
--ENDCOMPOSITE--
Starting ProcessPendingUpdates
---- PAINT START ----PresShell(0x7f8cef14c000), nsView(0x7f8cef2dccc0), nsIWidget(0x7f8cf05a5400)
---- PAINT END ----
Ending ProcessPendingUpdates
}

I'm not 100% sure, but I think it's the "Invalidating entire layer" step that's having trouble here, because if I step into that code (InvalidateEntireThebesLayer), it invalidates aLayer->GetValidRegion().GetBounds(), which in this case is an empty rect (0,0,0,0).

If I change that instead to invalidate GetVisibleRegion().GetBounds(), then the bug goes away (because unlike the Valid rect, the Visible rect is non-empty).

Not suggesting that that's the right fix, but hopefully that might shine some light on what's going on.
Matt, does this give you any additional ideas as to where the problem might lie? (What does it mean for mValidRegion to be empty?)
Attachment #8385966 - Flags: feedback?(matt.woodrow)
Attachment #8385966 - Attachment is obsolete: true
Attachment #8385966 - Flags: feedback?(matt.woodrow)
Attachment #8386395 - Flags: review?(roc)
Thanks dholbert! Testcase was especially useful :)

The reason mValidRegion is empty is because this was a non-retained thebes layer, so that's entirely expected.

The reason we're trying to invalidate the whole layer is because we want to redraw it at a different resolution (which makes no sense for inactive layers). This resolution change hid the transform change (since we fold the scale factors down the layer tree) from the layer tree comparison, and meant we missed invalidating forit.
Good catch. Guess we have to do this instead!
Attachment #8386395 - Attachment is obsolete: true
Attachment #8386395 - Flags: review?(roc)
Attachment #8386511 - Flags: review?(roc)
Thanks for the quick action on this, Matt!

BTW: I think it's probably worth including an automated test for this issue (simpler/more targeted than scale-1.svg) -- maybe just a MozReftestInvalidate-based version of my attached live testcase?

That way, if someone regresses this in the future, it'll be more immediately clear what the problem is.
Flags: in-testsuite?
Attachment #8386526 - Flags: review?(dholbert)
Comment on attachment 8386526 [details] [diff] [review]
Add test for this bug

Looks good to me! Thanks.

[note to self: I want to land attachment 8385803 [details] [diff] [review] once this is fixed, too, to improve scale-1.svg. needinfo=me to do that.]
Attachment #8386526 - Flags: review?(dholbert) → review+
Flags: needinfo?(dholbert)
Try run with a variant of my MozReftestInvalidate patch (optimistically removing the reftest.list fuzzy/random annotations for the test, in the hopes that they're either variants of this same underlying problem or have been fixed elsewhere):
  https://tbpl.mozilla.org/?tree=Try&rev=81a1d462c374
[Comment 52 is an instance of this on inbound, from the push right *before* this landed, BTW. So all is still well.]
Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)
Flags: in-testsuite?
Flags: in-testsuite+
Comment on attachment 8385803 [details] [diff] [review]
patch: make the test use MozReftestInvalidate, to make it more deterministic [and perma-fail] (spun off into bug 980783)

(Actually, I spun off bug 980783 for fixing scale-1.svg to use MozReftestInvalidate and cleaning up its reftest.list fuzzy/random annotations)
Attachment #8385803 - Attachment description: patch: make the test use MozReftestInvalidate, to make it more deterministic [and perma-fail] → patch: make the test use MozReftestInvalidate, to make it more deterministic [and perma-fail] (spun off into bug 980783)
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/147b95edf4b3
https://hg.mozilla.org/mozilla-central/rev/2ce2bb2668fd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Assignee: nobody → matt.woodrow
Ignore comment 56 and comment 57, they were unrelated and fixed by backout (83ca0a9eb83e).
You need to log in before you can comment on or make changes to this bug.