Closed Bug 1242256 Opened 8 years ago Closed 8 years ago

A continuously animated GIF in an SVG pattern only animates briefly before stopping

Categories

(Core :: SVG, defect)

43 Branch
Unspecified
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- verified
firefox49 --- verified
firefox-esr38 --- wontfix
firefox-esr45 --- ?
firefox50 --- verified

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)
OS: Unspecified → Mac OS X
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
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
Attached file reduced html
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)
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
(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)
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)
Summary: animated svg pattern stops → A continuously animated GIF in an SVG pattern only animates briefly before stopping
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)
(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?
(I'd imagine that marker, mask and feImage are also broken, but let's focus on pattern first.)
(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)
Bug 1223753 may be relevant here; it implements visibility tracking for -moz-element, which is implemented using rendering observers.
See Also: → 1223753
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 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+
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)
(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)
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/603181ac9e84
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
No longer depends on: 1264341
QA Whiteboard: [good first verify]
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][bugday-20160706]
Gif runs continuously. Has been running for 15 minutes and still going.
Browser: Firefox 48.0b6
OS: OS X Yosemite 10.10.2
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.
mboldan: You're welcome. :)
You need to log in before you can comment on or make changes to this bug.