907503 - SVG Animation is broken in SVG-as-image

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

In the test URL, the character does not animate unless a repaint is triggered by, e.g. resizing the window, scrolling, using Alt+Tab etc.

The character is an SVG image, animated with SVG animation, and included in the page using an <img> element.

When opening the SVG file directly (http://parapara.mozlabs.jp/characters/792.svg) the character animates correctly.

This appears to be broken from Firefox 24 onwards.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: (svg for use in testcase)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: original testcase

Comment 3

[Daniel Holbert [:dholbert]]

Last good nightly: 2013-05-25
First bad nightly: 2013-05-26

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7a2f7a45819a&tochange=0fed3377c839

Looks like a regression from either Bug 875175 or Bug 855221.

Comment 4

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ebb4eec0258d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130524 Firefox/24.0 ID:20130524102057
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e3e70237a47a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130524 Firefox/24.0 ID:20130524103344
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ebb4eec0258d&tochange=e3e70237a47a

Suspected: Bug 854765

Comment 5

[Daniel Holbert [:dholbert]]

Thanks Alice0775! (Note: that range is a (much tighter!) subset of the range that I got in comment 3, so our results are consistent.)

Comment 7

[Daniel Holbert [:dholbert]]

[aside:]

So, in theory, test_animSVGImage.html is supposed to be testing this:
http://mxr.mozilla.org/mozilla-central/source/image/test/mochitest/test_animSVGImage.html?force=1

It registers a "frame-complete" image decoder observer, which gets called "every time a frame has completed decoding" in the animated-GIF model.  (For animated SVG, that ends up meaning every time the animation has changed and requests a repaint -- with the upper-limit being the refresh driver frequency, I think.)

Each time the observer gets invoked, it checks to see if the image is green yet. (It's originally red, but it has an <animate> tag to move a green rect into view, covering up the red, after 0.1 sec.)

Anyway -- I can't dig too much into what's broken right now, as I'm on vacation, but I can take a look when I'm back. Alternately, if any other SVG folks want to dive on this, be my guest! :)

Comment 8

[bhavana bajaj [:bajaj]]

(In reply to Daniel Holbert [:dholbert] (vacation through 8/31) from comment #7)
> [aside:]
> 
> So, in theory, test_animSVGImage.html is supposed to be testing this:
> http://mxr.mozilla.org/mozilla-central/source/image/test/mochitest/
> test_animSVGImage.html?force=1
> 
> It registers a "frame-complete" image decoder observer, which gets called
> "every time a frame has completed decoding" in the animated-GIF model.  (For
> animated SVG, that ends up meaning every time the animation has changed and
> requests a repaint -- with the upper-limit being the refresh driver
> frequency, I think.)
> 
> Each time the observer gets invoked, it checks to see if the image is green
> yet. (It's originally red, but it has an <animate> tag to move a green rect
> into view, covering up the red, after 0.1 sec.)
> 
> Anyway -- I can't dig too much into what's broken right now, as I'm on
> vacation, but I can take a look when I'm back. Alternately, if any other SVG
> folks want to dive on this, be my guest! :)

If the issue may have a user impact outside the test url and we think this may have a serious user impact on real world websites using SVG we should prioritize this while you are away. Given we have only a week or two so to take low risk fixes of Fx24.

Could you please help with input on user impact and the commonality of the scenario here ?

Comment 9

[Alice0775 White]

Affected
http://parapara.mozlabs.jp/wall-maker/ 
And the page created SVG http://parapara.mozlabs.jp/characters/1809

Comment 10

[Alice0775 White]

Oops same page...

Comment 11

[Robert Longson [:longsonr]]

We should probably back out bug 901955 and then bug 854765 on beta (and maybe Aurora too). Need to back out the first as it landed on top and depends on it.

What do you think Jonathan, do you know what's going on here? If I call InvalidateFrame() unconditionally in  nsSVGPathGeometryFrame::DidSetStyleContext the animation starts working again so something about that is required for SVG as an image to trigger a repaint which ordinary SVG doesn't need.

Comment 12

[bhavana bajaj [:bajaj]]

needinfoi'ing :jwatt to get input on comment #8 and help with next steps here and if we are going with the backout option in comment #11?

Regarding timelines, we are in the fourth week of beta's already and obviously will prefer the lowest risk option here as we do not have many beta's left to get any changes well tested.

Comment 13

[Stanislas Signoud]

Please note that this regression prevent us to use SVG in some components like spinners. We had to revert back on gifs, and that's unfortunate.

If you need another example or test case, here we go: http://jsfiddle.net/zBudu/

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Backout bug 854765 and bug 901955 but leave test case

Possible backout patch which:
* Backs out bugs 854765 and bug 901955
* Leaves test cases added by bug 901955 so we don't regress that again when we fix bug 854765 properly
* Adds a couple of MOZ_OVERRIDEs that weren't there in the original for consistency with other changes that happened in those files in the meantime

A quick check of the test URL and the test case in comment #13 suggests this fixes the bug but I haven't created a regression reftest yet.

I'll wait to see the results of the try run before requesting review / approval:

https://tbpl.mozilla.org/?tree=Try&rev=d227fa79b5d1

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Stanislas Signoud from comment #13)
> Please note that this regression prevent us to use SVG in some components
> like spinners. We had to revert back on gifs, and that's unfortunate.
> 
> If you need another example or test case, here we go:
> http://jsfiddle.net/zBudu/

This test shows that CSS animations inside SVG-as-image are also broken.

I suggest the impact of this regression is fairly wide since it could potentially break any site using an <img> element to refer to an animated SVG asset such as a spinner.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment 796472 [details] [diff] is based off the beta branch but should apply to aurora and trunk with a little fuzz.

I'm on leave tomorrow so if try looks ok and anyone else thinks this is the right approach, please feel free to review the patch and request approval.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 796472 [details] [diff] [review]
Backout bug 854765 and bug 901955 but leave test case

Thanks for jumping on this, Brian!

I think that backing out bug 854765 (and bug 901955, basically a follow-up for it) seems like a sane course of action.

Ideally it'd be good to get jwatt's opinion before moving forward, since he wrote both of those patches and would be most likely to know any reasons *not* to backout.

But in the meantime, if he isn't able to get to this soon, I think we should go ahead with the backout since we really need to move forward to avoid shipping this regression.

So, I'll grant feedback+ on the idea of backing out those two patches, which you can consider upgraded to r=me in approximately a day and a half if jwatt hasn't been able to get to this yet. :)

Comment 18

[bhavana bajaj [:bajaj]]

We have a Beta going to build tomorrow morning PT, will be great if we get patches nominated and landed by then.

Is the plan to perform a backout everywhere including m-c ?

Comment 19

[Robert Longson [:longsonr]]

Definately Beta, probably Aurora, probably not m-c.

Comment 20

[Robert Longson [:longsonr]]

Comment on attachment 796472 [details] [diff] [review]
Backout bug 854765 and bug 901955 but leave test case

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 854765
User impact if declined: animation in SVG images is broken
Testing completed (on m-c, etc.): just a backout. Passes on try
Risk to taking this patch (and alternatives if risky): no real alternatives other than broken animation in SVG images.
String or IDL/UUID changes made by this patch: none

Comment 21

[Robert Longson [:longsonr]]

Absent some other plan from jwatt this is what we're left with. I'd go for an Aurora backout too personally.

Comment 22

[Daniel Holbert [:dholbert]]

I agree we should land the backout on aurora + beta.

(Maybe on trunk as well, but I think we can leave some time (maybe a week?) for someone to figure out a "real" regression-fix before we resort to a backout for trunk.)

I can land this tonight (pacific time), to be fixed in tomorrow morning's beta build, if approval is granted in time.

Comment 23

[Daniel Holbert [:dholbert]]

I verified that the attached patch qimports cleanly onto beta & aurora.

RyanVM said in IRC that he can land this later today [good man], pending approval. Tagging him for needinfo so he doesn't forget. ;)

Comment 24

[bhavana bajaj [:bajaj]]

Comment on attachment 796472 [details] [diff] [review]
Backout bug 854765 and bug 901955 but leave test case

Approving the backout for aurora/beta.

Comment 25

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #21)
> Absent some other plan from jwatt this is what we're left with.

I'm off sick and still not feeling on the ball enough to figure out what's wrong or make judgement calls about this. The backed out bugs are just perf improvements though, so I think backout is okay. Thanks for sorting this out for branches everyone.

Comment 26

[Jonathan Watt [:jwatt]]

Can we limit the backout to beta? This shouldn't be a hard issue to fix, and I'd rather get any extra bug reports for aurora to ensure we fix various permutations.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Landed on beta.
https://hg.mozilla.org/releases/mozilla-beta/rev/bba7e74f525f

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #26)
> Can we limit the backout to beta? This shouldn't be a hard issue to fix, and
> I'd rather get any extra bug reports for aurora

We should make sure we fix this on Aurora, some way or another, before train-switch-day, so that this doesn't become re-broken for our Beta users that day.

(Sorry to hear that you're sick - I hope you feel better soon!)

Comment 29

[Jonathan Watt [:jwatt]]

Okay...the problem is that we don't call nsIFrame::ClearInvalidationStateBits() on the SVG-as-image document's root frame under the VectorImage painting path to clear NS_FRAME_DESCENDANT_NEEDS_PAINT etc. from the VectorImage's document's frames when we paint. As a result, when DoApplyRenderingChangeToTree() calls InvalidateFrameSubtree() to process the nsChangeHint_RepaintFrame that nsStyleVisibility::CalcDifference() returned for the visibility change, we fail the parent->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT) check in InvalidateFrameInternal under the InvalidateFrameSubtree() call, which means that we never call nsSVGEffects::InvalidateDirectRenderingObservers() on the changed frame's ancestors, which means we never call SVGRootRenderingObserver::DoUpdate(), which means we never call VectorImage::InvalidateObserver(), which means we don't repaint for the change.

The painting stack for the SVG rendered directly is something like:

  nsIFrame::ClearInvalidationStateBits
  nsDisplayList::PaintForFrame // widgetTransaction == true
  nsDisplayList::PaintRoot
  nsLayoutUtils::PaintFrame
  PresShell::Paint
  nsViewManager::ProcessPendingUpdatesForView
  nsViewManager::ProcessPendingUpdates
  nsRefreshDriver::Tick

Whereas when it's rendered as an image it is something like:

  nsDisplayList::PaintForFrame // widgetTransaction == false
  nsDisplayList::PaintRoot
  nsLayoutUtils::PaintFrame
  PresShell::RenderDocument
  mozilla::image::SVGDrawingCallback::operator()
  gfxCallbackDrawable::Draw
  gfxUtils::DrawPixelSnapped
  mozilla::image::VectorImage::Draw
  DrawImageInternal
  nsLayoutUtils::DrawSingleImage
  nsImageFrame::PaintImage
  nsDisplayImage::Paint

It's not clear to me why we don't call ClearInvalidationStateBits when widgetTransaction is false. If we did, this bug wouldn't occur. Maybe we can call it under mozilla::image::VectorImage::Draw or something. I'll dig into that.

Comment 30

[Jonathan Watt [:jwatt]]

Attachment: patch for m-c and aurora

Talking to mattwoodrow and roc, it's not clear that the "real" solution is simple. For now, here's a minimal backout of bug 854765 that I think is a good compromise between keeping the perf wins from bug while fixing the animation issues for SVG-as-image. This patch could be even more minimal, but to keep the risk low for aurora landing I've kept it fairly close to a backout of bug 854765.

I've filed bug 911759 for getting rid of the DidSetStyleContext "hacks" that this patch introduces for invalidation for SMIL animation in SVG-as-image.

Comment 31

[Robert Longson [:longsonr]]

Can't you modify nsFrame::DidSetStyleContext instead so that we only need this change in one place. It will be easier to remove. Probably need to wrap it in IsSVG() and maybe IsLeaf() or something.

If we really need it this way then aren't we missing nsSVGTextFrame2 and nsSVGGlyphFrame?

Can we get a reftest to check that animation of SVG images doesn't regress?

Comment 32

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> So, in theory, test_animSVGImage.html is supposed to be testing this:
> http://mxr.mozilla.org/mozilla-central/source/image/test/mochitest/
> test_animSVGImage.html?force=1

The reason that that test passes is because it tests animation of the 'x' attribute, so it hits the nsSVGEffects::InvalidateRenderingObservers(this) call here:

http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGPathGeometryFrame.cpp?rev=1d64db569ebf&mark=117#104

Comment 33

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #31)
> If we really need it this way then aren't we missing nsSVGTextFrame2 and
> nsSVGGlyphFrame?

Ah yes, nsSVGTextFrame2 was added later - good catch! (That said, note that the bustage of SVG text in the new SVG text frame world is not a regression from bug 854765, but a regression caused by switching to the new SVG text frame world.)

I think that sways things towards handling this bug in nsFrame::DidSetStyleContext as you suggest, since that's the only place to handle the issue for SVG text in the new SVG text frame world. Okay, I'll do that.

> Can we get a reftest to check that animation of SVG images doesn't regress?

Yes, I'll add some more permutations of image/test/mochitest/test_animSVGImage.html

Comment 34

[Jonathan Watt [:jwatt]]

Attachment: patch for m-c and aurora

Comment 35

[Jonathan Watt [:jwatt]]

Comment on attachment 798579 [details] [diff] [review]
patch for m-c and aurora

For reasons I'm not clear about, this patch can cause us to reenter RestyleManager::ProcessPendingRestyles, triggering the assertion there. I'm working on something better, but it's taking some time to figure out and I'm not sure whether it will be suitable for aurora at this stage. We may need to take the backout patch on aurora.

Comment 36

[Jonathan Watt [:jwatt]]

Attachment: patch to call ClearInvalidationStateBits() in SVG-as-an-image documents

As discussed with roc and mattwoodrow. We should really be doing this, and it solves the problem described in comment 29.

Comment 37

[Jonathan Watt [:jwatt]]

To be taken with the tests already reviewed by longsonr.

Comment 38

[Robert Longson [:longsonr]]

> +  if (widgetTransaction ||
> +      // SVG-as-an-image docs don't paint as part of the retained layer tree,
> +      // but they still needs the invalidation state bits cleared in order for

s/needs/need/

Comment 39

[Jonathan Watt [:jwatt]]

Comment on attachment 799201 [details] [diff] [review]
patch to call ClearInvalidationStateBits() in SVG-as-an-image documents

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8c2ce395a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c690d9998f7

I've landed this patch, plus the tests from the patch that longsonr reviewed except for the <tspan> test (test_animSVGImage3.html). This patch does fix what the <tspan> test tests, but subpixel differences between the inline SVG reference and the SVG-as-an-image mean that it fails. I'll work that out in a separate bug since I don't want to hold this up and risk missing aurora landing before the uplift.

Comment 40

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #39)
> I'll work that out in a separate bug

bug 912446

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8d8c2ce395a1
https://hg.mozilla.org/mozilla-central/rev/1c690d9998f7

Comment 42

[Manuela Muntean [Away]]

Verified as fixed with Firefox 24 beta 9 (build ID: 20130905180733) on Win 7 64 bit.

All these animations work as soon as they are opened:

http://parapara.mozlabs.jp/characters/792
http://parapara.mozlabs.jp/characters/792.svg
https://bug907503.bugzilla.mozilla.org/attachment.cgi?id=793246
https://bug907503.bugzilla.mozilla.org/attachment.cgi?id=793250

Comment 43

[Jonathan Watt [:jwatt]]

Comment on attachment 799201 [details] [diff] [review]
patch to call ClearInvalidationStateBits() in SVG-as-an-image documents

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 854765, plus fixes a bug 839955 regression too
User impact if declined: broken animation in SVG that's embedded using <html:img> or CSS
Testing completed (on m-c, etc.): landed on m-c a couple of days ago
Risk to taking this patch (and alternatives if risky): should be low, since it makes SVG-as-an-image behave a bit more like SVG-as-a-document
String or IDL/UUID changes made by this patch: none

This also depends on the fix I split out into bug 911862 to avoid reentry issues, and that bug was tweaked in bug 913247. These should both be very low risk. I'll request approval for them too on those bugs.

Comment 44

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/33d447b77903

Comment 45

[Petruta Horea [:phorea], Desktop QA]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Verified fixed on latest Aurora (20130908004001) on Windows 7, 64-bit.

Comment 46

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on latest Aurora 26.0a2 (buildID: 20131001004005).

Comment 47

[Daniel Holbert [:dholbert]]

FWIW, I just noticed that this bug's test had a link to the wrong bug in its mochitest boilerplate.

Fixed here: https://hg.mozilla.org/integration/mozilla-inbound/rev/edf4baac0f67

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/edf4baac0f67

Comment 49

[temaruk]

The marked duplicate (bug 908634) does not seem to be fixed, as of 33.0, on Windows 7.

UA string: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0

Check http://jsfiddle.net/zBudu/ for an example. The SVG embedded via the background-image property is not animated. When the SVG is opened directly, it is animated.

If this is not a bug, but the intended behaviour, then it might be a good idea to update the documentation somewhere around these parts:

- https://developer.mozilla.org/en-US/docs/Web/SVG
- https://developer.mozilla.org/en-US/docs/Web/SVG#Animation_and_interactions
- https://developer.mozilla.org/en-US/docs/Web/SVG/SVG_as_an_Image

Comment 50

[Daniel Holbert [:dholbert]]

Confirmed, that bug still seems to affect Nightly. I'll reopen the other bug.

Comment 51

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

I noticed that test_animSVGImage2.html has intermittent oranges, which led me to this bug. Looking at the code I can clearly see why it's happening, and it's due to a misunderstanding that I want to clear up for the benefit of people reading this bug in the future:

(In reply to Daniel Holbert [:dholbert] from comment #7)
> It registers a "frame-complete" image decoder observer, which gets called
> "every time a frame has completed decoding" in the animated-GIF model.

Unfortunately that is *not* true, though it certainly seems like it would be from the notification name! FRAME_COMPLETE should really be called FIRST_FRAME_COMPLETE; it only fires *once*, when the first frame is completely decoded. ProgressTracker enforces that the notification is sent only once; the FLAG_FRAME_COMPLETE notification in VectorImage::SendInvalidationNotifications doesn't get sent at all, since we've already sent that notification in VectorImage::OnSVGDocumentLoaded. It's misleading, and I'll create a patch to remove it.

VectorImage::SendInvalidationNotifications also sends a FRAME_UPDATE notification, and *that* is what indicates that the contents of the image have changed due to animation. So that's why things work when displaying animated SVG-as-image in the browser. test_animSVGImage2.html, though, listens for the wrong notification, and only happens to work most of the time due to coincidences of timing.

Naming is a big deal, and the misleading name of FRAME_COMPLETE has caused people to write buggy code a number of times. Unfortunately changing it is potentially an add-on compatibility issue, but I do plan to look at the possibility of changing it in the future.