786894 - SVG in an active GFX layer is not invalidating properly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR: Load attached testcase.

EXPECTED RESULTS: Animation rapidly switching the rect from blue to yellow, along with subtle opacity animation.

ACTUAL RESULTS: Just the subtle opacity animation is visible, and the color only changes when you force an invalidate by clicking the titlebar or the rect.

I suspect this is related to the off-main-thread animation stuff that dzbarsky was doing, since that was targeted at a few properties including opacity.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: semi-reference case 1

Here's a reference case, with the subtle opacity animation removed. This shows the (rapid) color animation that should be visible in the testcase.

Comment 2

[Daniel Holbert [:dholbert]]

mozregression output (for m-c):
Last good nightly: 2012-08-02
First bad nightly: 2012-08-03
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=588424024294&tochange=89dcadd42ec4

And here's the pushlog for the regression on mozilla-inbound (found by hand using "mozilla-inbound-debug" nightlies):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=59d1993da7e1&tochange=cb017d051029

So -- looks like dzbarsky is innocent after all. :)

New suspect: jwatt, whose checkin for Bug 776054 (enable display-list-based SVG painting) is in both pushlogs

Comment 3

[Daniel Holbert [:dholbert]]

Flagging tracking-firefox17, since this is a functional regression in Firefox 17.

Comment 4

[Jonathan Watt [:jwatt]]

Yeah, flipping svg.display-lists.painting.enabled on/off breaks/fixes this.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: clearer testcase

What's happening here is that when an SVG element is being rendered into an active GFX layer, we aren't properly invalidating for changes to that element (or its descendants).

This is almost certainly due to the way that nsSVGUtils::InvalidateBounds works. It transforms the dirty area up the parent chain taking filters into account until it reaches an nsSVGOuterSVGFrame, and invalidates there. Unfortunately that isn't right now that SVG display lists are enabled, since enabling SDL means we can now get active layers for SVG content. With nsSVGUtils::InvalidateBounds as it is, we skip invalidation of any such layers along the parent chain on the way to the nsSVGOuterSVGFrame. :-/

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Jonathan - can you confirm your ability to prioritize work on this in the coming 5 weeks?  We're about to put 17 on to the Beta channel and so it's time to decide if this is still something to be tracked - SVG bugs have a higher prominance with webapps so please let us know if you can't work on this, who might be a good person to pass it to.

Comment 7

[Jonathan Watt [:jwatt]]

I'm pretty sure I should be able to have a fix for this for 17.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch for mozilla-beta (17)

The code on m-c is quite different to branches now that DLBI has landed. It doesn't seem like a good use of time to write a complete fix for branches and a completely different patch for m-c. I think for branches we should simply prevent active layers in SVG.

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 671371 [details] [diff] [review]
patch for mozilla-beta (17)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 776054
User impact if declined: broken rendering of SVG
Risk to taking this patch (and alternatives if risky): should be low risk
String or UUID changes made by this patch: none

Testing completed (on m-c, etc.): I've not landed it on m-c since the code is different there making any baking on m-c of minimal value. I propose landing this directly on beta.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 671371 [details] [diff] [review]
patch for mozilla-beta (17)

Since it's very early still in Beta, we can take this Beta-only fix as we'll have time to assess and backout if there are any regressions. Marking this tracking for 18 though and we'll need an equivalent patch (or if the same one works on Aurora too, great) so that we're covered for 18 while the code in 19 rides the trains.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Also setting tracking on FF19 so we ensure that the proper fix working with DLBI is landed in trunk.

Comment 12

[Scoobidiver (away)]

Pushed to Beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/7ab34cfe4869

Comment 13

[Alex Keybl [:akeybl]]

jwatt - do we need this for FF18?

Comment 14

[Jonathan Watt [:jwatt]]

Sorry, missed that question. Yeah, we need to fix this one in my opinion.

Comment 15

[Jonathan Watt [:jwatt]]

The fix involves getting rid of nsSVGUtils::InvalidateBounds and relying on the DLBI mechanisms for SVG invalidation, which is what bug 802628 is about. I have a partially written but buggy patch for this. I need to talk to mattwoodrow about a few things there.

Comment 16

[Jet Villegas (inactive)]

(In reply to Jonathan Watt [:jwatt] from comment #15)
> The fix involves getting rid of nsSVGUtils::InvalidateBounds and relying on
> the DLBI mechanisms for SVG invalidation, which is what bug 802628 is about.
> I have a partially written but buggy patch for this. I need to talk to
> mattwoodrow about a few things there.

I was looking for a r? patch here based on our last call. Any update?

Comment 17

[Jonathan Watt [:jwatt]]

I'm having a little trouble with some test failures. I'm giving up for the evening but should have patches up tomorrow.

Comment 18

[Jonathan Watt [:jwatt]]

My first and second patches from bug 802628 are sufficient to fix this. They've landed on beta, and I'll land them on aurora once it reopens.

Comment 19

[Alex Keybl [:akeybl]]

(In reply to Jonathan Watt [:jwatt] from comment #18)
> My first and second patches from bug 802628 are sufficient to fix this.
> They've landed on beta, and I'll land them on aurora once it reopens.

Firefox 19 still appears to be affected. Can we move forward with landing to mozilla-beta?

Comment 20

[Jonathan Watt [:jwatt]]

Both patches landed on m-a; see bug 802628 comment 11. I just forgot to update the status flags here. Thanks.

Comment 21

[Virgil Dicu [:virgil] [QA]]

Verified the issue with the testcases from comment 0, 1 and 5 in 2012-08-29 Nightly
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0
Build ID: 20120829030520

Verified the fix for Firefox 19.0 beta 3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130123083802