Closed
Bug 907503
Opened 11 years ago
Closed 11 years ago
SVG Animation is broken in SVG-as-image
Categories
(Core :: SVG, defect)
Tracking
()
VERIFIED
FIXED
mozilla26
People
(Reporter: birtles, Assigned: jwatt)
References
()
Details
(Keywords: regression)
Attachments
(5 files, 1 obsolete file)
23.09 KB,
image/svg+xml
|
Details | |
133 bytes,
text/html
|
Details | |
15.78 KB,
patch
|
longsonr
:
review+
dholbert
:
feedback+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
12.58 KB,
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
2.35 KB,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
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.
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Comment 4•11 years ago
|
||
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
Blocks: 854765
Comment 5•11 years ago
|
||
Thanks Alice0775! (Note: that range is a (much tighter!) subset of the range that I got in comment 3, so our results are consistent.)
Updated•11 years ago
|
Comment 7•11 years ago
|
||
[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•11 years ago
|
||
(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•11 years ago
|
||
Affected http://parapara.mozlabs.jp/wall-maker/ And the page created SVG http://parapara.mozlabs.jp/characters/1809
Comment 10•11 years ago
|
||
Oops same page...
Comment 11•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee: nobody → jwatt
Flags: needinfo?(jwatt)
Updated•11 years ago
|
Comment 13•11 years ago
|
||
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/
Reporter | ||
Comment 14•11 years ago
|
||
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
Reporter | ||
Comment 15•11 years ago
|
||
(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.
Reporter | ||
Comment 16•11 years ago
|
||
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•11 years ago
|
||
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. :)
Attachment #796472 -
Flags: feedback+
Comment 18•11 years ago
|
||
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•11 years ago
|
||
Definately Beta, probably Aurora, probably not m-c.
Updated•11 years ago
|
Attachment #796472 -
Flags: review+
Comment 20•11 years ago
|
||
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
Attachment #796472 -
Flags: approval-mozilla-beta?
Attachment #796472 -
Flags: approval-mozilla-aurora?
Comment 21•11 years ago
|
||
Absent some other plan from jwatt this is what we're left with. I'd go for an Aurora backout too personally.
Comment 22•11 years ago
|
||
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•11 years ago
|
||
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. ;)
Flags: needinfo?(ryanvm)
Comment 24•11 years ago
|
||
Comment on attachment 796472 [details] [diff] [review] Backout bug 854765 and bug 901955 but leave test case Approving the backout for aurora/beta.
Attachment #796472 -
Flags: approval-mozilla-beta?
Attachment #796472 -
Flags: approval-mozilla-beta+
Attachment #796472 -
Flags: approval-mozilla-aurora?
Attachment #796472 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 25•11 years ago
|
||
(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.
Flags: needinfo?(jwatt)
Assignee | ||
Comment 26•11 years ago
|
||
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•11 years ago
|
||
Landed on beta. https://hg.mozilla.org/releases/mozilla-beta/rev/bba7e74f525f
Flags: needinfo?(ryanvm)
Comment 28•11 years ago
|
||
(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!)
Assignee | ||
Comment 29•11 years ago
|
||
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.
Assignee | ||
Comment 30•11 years ago
|
||
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.
Attachment #798516 -
Flags: review?(longsonr)
Comment 31•11 years ago
|
||
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?
Assignee | ||
Comment 32•11 years ago
|
||
(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
Assignee | ||
Comment 33•11 years ago
|
||
(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
Blocks: 839955
Updated•11 years ago
|
Attachment #798516 -
Flags: review?(longsonr) → review-
Assignee | ||
Comment 34•11 years ago
|
||
Attachment #798516 -
Attachment is obsolete: true
Attachment #798579 -
Flags: review?(longsonr)
Updated•11 years ago
|
Attachment #798579 -
Flags: review?(longsonr) → review+
Assignee | ||
Comment 35•11 years ago
|
||
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.
Assignee | ||
Comment 36•11 years ago
|
||
As discussed with roc and mattwoodrow. We should really be doing this, and it solves the problem described in comment 29.
Attachment #799201 -
Flags: review?(roc)
Assignee | ||
Comment 37•11 years ago
|
||
To be taken with the tests already reviewed by longsonr.
Attachment #799201 -
Flags: review?(roc) → review+
Comment 38•11 years ago
|
||
> + 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/
Assignee | ||
Comment 39•11 years ago
|
||
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.
Assignee | ||
Comment 40•11 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #39) > I'll work that out in a separate bug bug 912446
https://hg.mozilla.org/mozilla-central/rev/8d8c2ce395a1 https://hg.mozilla.org/mozilla-central/rev/1c690d9998f7
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 42•11 years ago
|
||
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
Assignee | ||
Comment 43•11 years ago
|
||
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.
Attachment #799201 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #799201 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 44•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/33d447b77903
Comment 45•11 years ago
|
||
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•11 years ago
|
||
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).
Updated•10 years ago
|
QA Contact: twalker
Comment 47•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/edf4baac0f67
Flags: in-testsuite+
Comment 49•10 years ago
|
||
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•10 years ago
|
||
Confirmed, that bug still seems to affect Nightly. I'll reopen the other bug.
Comment 51•10 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•