Closed
Bug 1242256
Opened 9 years ago
Closed 9 years ago
A continuously animated GIF in an SVG pattern only animates briefly before stopping
Categories
(Core :: SVG, defect)
Tracking
()
VERIFIED
FIXED
mozilla48
People
(Reporter: marc, Assigned: longsonr)
References
Details
(Keywords: regression)
Attachments
(2 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.111 Safari/537.36
Steps to reproduce:
example: http://tympanus.net/Tutorials/AnimatedTextFills/index9.html
firefox 43 on osx el capitan
Actual results:
animated gif in svg stops playing after a few seconds
Expected results:
animated gif should play in continous loop (like in webkit browsers)
Comment 1•9 years ago
|
||
I can reproduce on Windows7.
There are 3 regressions window.
#1 Regression (not drawn anything)
#2 Regression (animation stops when scroll)
#3 Regression (animation stops after for a second)
#1 Regression window(not drawn anything):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4511d9ac1000&tochange=79624417d247
#1 Regressed by: Bug 994081
#2 Regression window(animation stops when scroll):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6419d2007c97&tochange=617e3d552045
#2 Regressed by: Bug 932198
#3 Regression window(animation stops after for a second):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=78a8270c230e&tochange=7a609d5e44a3
#3 Regressed by: Bug 1169880
Status: UNCONFIRMED → NEW
status-firefox42:
--- → wontfix
status-firefox43:
--- → wontfix
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox46:
--- → affected
status-firefox-esr38:
--- → wontfix
status-firefox-esr45:
--- → ?
Component: Untriaged → SVG
Ever confirmed: true
Flags: needinfo?(seth)
Flags: needinfo?(mwu)
Flags: needinfo?(jwatt)
Keywords: regression
OS: Mac OS X → All
Product: Firefox → Core
Comment 2•9 years ago
|
||
Comment 3•9 years ago
|
||
There's ongoing work in bug 1190881 to fix animations in svg-as-an-image. We should fix that first then reevaluate here.
Depends on: 1190881
Flags: needinfo?(jwatt)
Comment 4•9 years ago
|
||
Oh, wait, this is animation of a GIF in an SVG pattern used in HTML, not CSS animation used in SVG-as-an-image.
No longer depends on: 1190881
Comment 5•9 years ago
|
||
(In reply to Alice0775 White from comment #1)
> I can reproduce on Windows7.
> There are 3 regressions window.
Alice, I'm unclear of what you mean by this. The GIF either animates or it doesn't, no? Or did the animation stopping get progressively quicker? Or do you mean that the animation broke, got fixed, broke, got fixed, and now has broken again?
Flags: needinfo?(alice0775)
Comment 6•9 years ago
|
||
The animation was gradually broken.
Animated Fine > Bug 994081 > No image is drawn > Bug 932198 > Animated, but after scroll the page, the animation stops > Bug 1169880 > Animation stops after for a second.
Flags: needinfo?(alice0775)
Updated•9 years ago
|
Summary: animated svg pattern stops → A continuously animated GIF in an SVG pattern only animates briefly before stopping
Comment 7•9 years ago
|
||
Using the visibility debugging feature added in bug 1257315, it's immediately apparent that we don't consider this image visible.
The image actually comes from an SVG pattern fill.
A relatively simple fix would be to consider all images in SVG patterns visible. A somewhat more complicated one would be to detect which patterns are actually painted and consider only images in those patterns visible. I suspect that the complexity there is not worth it, at least for now.
Tim, do you concur? Should we just mark all images in SVG patterns visible?
Flags: needinfo?(tnikkel)
Flags: needinfo?(seth)
Flags: needinfo?(mwu)
Comment 8•9 years ago
|
||
(In reply to Seth Fowler [:seth] [:s2h] from comment #7)
> Tim, do you concur? Should we just mark all images in SVG patterns visible?
FWIW an SVGPatternElement maintains a list of other elements that observe it which can be obtained via:
element->GetProperty(nsGkAtoms::renderingobserverlist);
That will return a nsSVGRenderingObserverList. Currently that object doesn't expose the list of observing elements, but it could do. There are a couple of complications though.
One issue is that observers can form a chain of arbitrary length. For example, a pattern could be being used to fill an element that is itself part of a second pattern that is filling a visible element. Or a pattern could be filling an element used in a marker. Etc.
Another issue is that the rendering observers are removed whenever the observed element requires invalidation, then are re-added as each referencing element is repainted. (This mechanism is partly to reduce the expense of invalidation.) I'm not sure how poorly that mechanism would interact with one to figure out whether a pattern is currently visible.
BTW what's the API for marking an image as visible?
Comment 9•9 years ago
|
||
(I'd imagine that marker, mask and feImage are also broken, but let's focus on pattern first.)
Comment 10•9 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #9)
> (I'd imagine that marker, mask and feImage are also broken, but let's focus
> on pattern first.)
feImage at least just marks itself as always visible. I'm not sure offhand about marker and mask.
I'm in the process of changing the API for visibility; see bug 1157546 to get an idea of where we're going.
Given the complexity you just described, I'm inclined to indeed just mark all images in SVG patterns visible as a first cut. We can make it more precise later, but it sounds nontrivial.
Flags: needinfo?(tnikkel)
Comment 11•9 years ago
|
||
Bug 1223753 may be relevant here; it implements visibility tracking for -moz-element, which is implemented using rendering observers.
Assignee | ||
Comment 12•9 years ago
|
||
Assignee: nobody → longsonr
Attachment #8734254 -
Flags: review?(seth)
Comment 13•9 years ago
|
||
Comment on attachment 8734254 [details] [diff] [review]
implements comment 10 all images in patterns/clipPaths/masks are marked as visible
Review of attachment 8734254 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good to me! I'd appreciate a quick double-check from someone more familiar with the SVG code, specifically to verify that this covers everything we care about (other than feImage, which already works), and that this will work even for frames which are deeper in the frame tree than e.g. the root of the pattern. Not doubting you, Robert, more doubting myself, as I'm not nearly as familiar with SVG layout as I am HTML/XUL layout. =)
Attachment #8734254 -
Flags: review?(seth)
Attachment #8734254 -
Flags: review?(jwatt)
Attachment #8734254 -
Flags: review+
Comment 14•9 years ago
|
||
Comment on attachment 8734254 [details] [diff] [review]
implements comment 10 all images in patterns/clipPaths/masks are marked as visible
It seems like the increment should happen after the FrameCreated notification, and that the decrement before the FrameDestroyed call. Can you change that? And also change "patterns or clipPaths" to "patterns or mask" ('image' isn't so relevant to clipPath). Thanks, Robert!
Attachment #8734254 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 15•9 years ago
|
||
Seth, do you agree with the first sentence of comment 14? if so I need to fix the feImage code too as that uses the same order for increment and decrement.
Flags: needinfo?(seth)
Comment 16•9 years ago
|
||
(In reply to Robert Longson from comment #15)
> Seth, do you agree with the first sentence of comment 14? if so I need to
> fix the feImage code too as that uses the same order for increment and
> decrement.
So I'm based this answer on bug 1157546's version of nsImageLoadingContent, which I'm just about to land. I'm pretty sure this is accurate for the old version too, but the code is a bit more confusing.
The answer is that it should not matter, because nsImageLoadingContent::GetOurPrimaryFrame() will fail to return a frame for <feImage>, and so we aren't able to track the visibility counter on the frame itself. So FrameCreated() will mark the frame as visible regardless.
FrameCreated() will probably go away in the not-too-distant future, and this will end up not mattering.
For the time being, given that it kinda doesn't matter, I'd recommend using the same ordering as <feImage>, which is known to work, just in case there are unknown bugs with doing it the other way.
Also, since I'm landing bug 1157546 very soon, you'll have to rebase this patch. Take a look at what I did for <feImage> in part 1e of that bug.
Flags: needinfo?(seth)
Assignee | ||
Comment 17•9 years ago
|
||
Comment 18•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite-
Comment 19•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Updated•8 years ago
|
QA Whiteboard: [good first verify]
Comment 20•8 years ago
|
||
I have reproduced this bug with Nightly 46.0a1 (2016-01-24) on Windows 7, 64 Bit!
The bug's fix is verified on Latest Beta 48.0b2.
Build ID 20160630123429
User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Assignee | ||
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
QA Whiteboard: [good first verify] → [good first verify][bugday-20160706]
Comment 21•8 years ago
|
||
Gif runs continuously. Has been running for 15 minutes and still going.
Browser: Firefox 48.0b6
OS: OS X Yosemite 10.10.2
Comment 22•8 years ago
|
||
Thanks julesmyers2011 for helping us.
I managed to reproduce this issue on Firefox 47.0.1 RC and on Windows 10 x64.
The issue is no longer reproducible on Firefox 48.0b6, Firefox 49.0a2 (2016-07-10) and on Firefox 50.0a1 (2016-07-10).
The test were performed under Windows 10 x64, Ubuntu 16.04 x64, Mac OS X 10.11.1.
status-firefox49:
--- → verified
status-firefox50:
--- → verified
Comment 23•8 years ago
|
||
mboldan: You're welcome. :)
You need to log in
before you can comment on or make changes to this bug.
Description
•