23.09 KB, image/svg+xml
133 bytes, text/html
15.78 KB, patch
|Details | Diff | Splinter Review|
12.58 KB, patch
|Details | Diff | Splinter Review|
2.35 KB, patch
|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.
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.
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
Thanks Alice0775! (Note: that range is a (much tighter!) subset of the range that I got in comment 3, so our results are consistent.)
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
tracking-firefox26: --- → ?
[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! :)
(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 ?
Affected http://parapara.mozlabs.jp/wall-maker/ And the page created SVG http://parapara.mozlabs.jp/characters/1809
Oops same page...
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.
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
tracking-firefox24: ? → +
tracking-firefox25: ? → +
tracking-firefox26: ? → +
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/
Created attachment 796472 [details] [diff] [review] 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
(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.
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 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+
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 ?
Definately Beta, probably Aurora, probably not m-c.
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
Absent some other plan from jwatt this is what we're left with. I'd go for an Aurora backout too personally.
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.
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 on attachment 796472 [details] [diff] [review] Backout bug 854765 and bug 901955 but leave test case Approving the backout for aurora/beta.
(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.
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.
Landed on beta. https://hg.mozilla.org/releases/mozilla-beta/rev/bba7e74f525f
status-firefox24: affected → fixed
(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!)
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.
Created attachment 798516 [details] [diff] [review] 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.
Attachment #798516 - Flags: review?(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?
(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
(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
Attachment #798516 - Flags: review?(longsonr) → review-
Created attachment 798579 [details] [diff] [review] patch for m-c and aurora
Attachment #798579 - Flags: review?(longsonr) → review+
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.
Created attachment 799201 [details] [diff] [review] 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.
Attachment #799201 - Flags: review?(roc)
To be taken with the tests already reviewed by longsonr.
Attachment #799201 - Flags: review?(roc) → review+
> + 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 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.
(In reply to Jonathan Watt [:jwatt] from comment #39) > I'll work that out in a separate bug bug 912446
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox26: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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
status-firefox24: fixed → verified
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?
Attachment #799201 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox25: affected → fixed
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.
status-firefox25: fixed → verified
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).
Status: RESOLVED → VERIFIED
status-firefox26: fixed → verified
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
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
Confirmed, that bug still seems to affect Nightly. I'll reopen the other bug.
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.