Closed Bug 216462 Opened 21 years ago Closed 16 years ago

Implement SVG (SMIL) Animation

Categories

(Core :: SVG, defect, P2)

Product:
Core
Component:
SVG
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: darryl, Assigned: dholbert)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(7 files, 38 obsolete files)

diff attachment 316765 vs attachment 331581
103.41 KB, patch
Details | Diff | Splinter Review
type management patch v1
13.00 KB, patch
Details | Diff | Splinter Review
getOverrideStyle WIP v1
34.57 KB, patch
Details | Diff | Splinter Review
SMIL for CSS Integration WIP v1
42.14 KB, patch
Details | Diff | Splinter Review
diff attachment 331581 vs attachment 332186
1.84 KB, patch
Details | Diff | Splinter Review
Show a bug with rotation angle values greater than 180°
2.77 KB, image/svg+xml
Details
patch v5k
470.12 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030809 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030809 This is a tracking bug for implimenting the SVG (SMIL) animation feature. Note that SVG (SMIL) animation is part of 1.1 full, basic, and tiny. Only the new printing profile doesn't include it. Reproducible: Always Steps to Reproduce:
*** This bug has been marked as a duplicate of 93321 ***
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago
Resolution: --- → DUPLICATE
Reopening this bug because it tracks SVG-specific SMIL, not generic SMIL. We *will* implement this at some stage, so it's a useful tracker.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Summary: Impliment SVG (SMIL) Animation → Implement SVG (SMIL) Animation
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Mass reassign of SVG bugs that aren't currently being worked on by Alex to general@svg.bugs. If you think someone should be assigned to your bug you can join the #svg channel on mozilla.org's IRC server ( irc://irc.mozilla.org/svg ) where you can try to convince one of the SVG hackers to work on it. We aren't always there, so if you don't get a response straight away please try again later.
Assignee: alex → general
I'm working on this for a university project and have spoken to Schepers about it. Hopefully we'll work on it together. Sorry for the spam, I just want to make sure no-one else is working on this already. I'll wait to hear from Schepers before asking for it to be re-assigned.
*** Bug 287054 has been marked as a duplicate of this bug. ***
Adding my vote for this. Hopefully the SMIL tag <audio src=".."> can also be implemented into native SVG, allowing audio to be synched with animation. Lookout Flash, here we come!
By my opininion, it is very important to implement complete (!) SVG Tiny 1.1 at first.
SVG (SMIL) Animation is required for a complete implementation of SVG tiny.
Some discussion about implementing animation in SVG is on the Mozilla wiki at: http://wiki.mozilla.org/SVGDev::Animation Also, I'm documenting my progess at: http://brian.sol1.net/svg/
Attached patch Progress snapshot of SMIL work (obsolete) — — Splinter Review
This is my progress to date on implementing SMIL. The features covered are listed at: http://brian.sol1.net/svg/status.php Changes to configure are not included in the patch, only those to configure.in. Changes nsSVGLength and nsSVGAnimatedLength shouldn't be committed. Apart from fixing bugs and any issues raised this is all the work I plan to do on SMIL for the time being. Please let me know your thoughts so I can make any changes needed before this is ready for review. Please also see: http://brian.sol1.net/svg/2005/11/05/smil-animation-patch/ as I have listed a couple of outstanding issues here.
Brian wrote a good PDF-report about SMIL, see <http://brian.sol1.net/svg/>
Severity: enhancement → normal
Brian's TODO list that accompanied the patch: [ ] deCOMtamination - every SMIL class These three deserve special attention [ ] nsSMILInstanceTime [ ] nsSMILInterval [ ] nsSMILTimeValueSpec Perhaps for instance time we can 1. Change nsISMILTimedElement::AddInstanceTime to take a const ref to an instance time and then just copy the object. -- Does this method need to be in the public API? 2. Make nsSMILTimedElement store instance times in an nsTArray of objects by value (not by pointer) 3. Make nsSMILInterval just keep a raw pointer to its dependents when we come to doing this 4. Add a comment in the dtor for instance time to say that it has to remove itself from its timebase And for interval 1. Just make it reference counted not XPCOM And for time value spec? [ ] nsSMILAnimationController [ ] nsSMILAnimationFunction [ ] nsSMILAnimationRegistry [ ] nsSMILCompositor [ ] nsSMILTimedDocumentRoot [ ] nsSMILTimedElement [ ] roc: "The model seems to be that a periodic timer causes the animation controller (one per document) to fire, which updates all animatable properties, which will cause a later paint. There may be some issues with this approach ... the animation update and painting of different subdocuments could happen at different times, and you don't know how much the painting could lag behind the animation properties update. We might need a meta-animation-controller associated with the top level window, with a timer on that, which when fired updates all the animations in all subdocuments and then does a synchronous reflow (if necessary) and paint." [ ] roc: "an animation controller should be per-document-tree, not per-document, so ultimately there will only be one per top-level window." [ ] jwatt: + virtual void ToActive(const PRInt64& aBeginTime)=0; + virtual void ToInactive(PRBool aIsFrozen)=0; The "To" here indicates to me that some sort of conversion is going on. Would these be better named Activate and Deactivate? Or Activated and Deactivated? [ ] jwatt: + virtual nsresult ParentPaused()=0; + virtual nsresult ParentResumed()=0; How about HandleParentPaused and HandleParentResume? And changing the start of their comments from "Inform..." to "Used to inform..."? [ ] Remaining XXX's [ ] Review docs [ ] Fully #ifdef MOZ_SMIL everything -- test undefined builds [ ] Test on linux [ ] Update copyright dates [ ] Update wiki -- remove obsolete details
The animation controller is exactly the "compositor" that I talked about and have been working on. I suggest you just go with some simple nsITimer-based approach for now; if the compositor gets done, then we can easily integrate SMIL into it.
(In reply to comment #12) > Created an attachment (id=252826) [details] > Brian's latest patch (2006-10-11-2009) updated to trunk > I failed attempt to build with the patch. I added above to |mozilla/content/svg/content/src/Makefile.in|. #ifdef MOZ_SMIL INCLUDES += -I$(srcdir)/../../../smil/public #endif But even with it, I failed too. c++ -o nsSVGElement.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -DOSTYPE=\"Linux2.6\" -DOSARCH=\"Linux\" -DBUILD_ID=2007012621 -D_IMPL_NS_LAYOUT -I../../../../dist/include/xpcom -I../../../../dist/include/string -I../../../../dist/include/layout -I../../../../dist/include/content -I../../../../dist/include/widget -I../../../../dist/include/gfx -I../../../../dist/include/dom -I../../../../dist/include/js -I../../../../dist/include/locale -I../../../../dist/include/htmlparser -I../../../../dist/include/view -I../../../../dist/include/pref -I../../../../dist/include/necko -I../../../../dist/include/xpconnect -I../../../../dist/include/webshell -I../../../../dist/include/unicharutil -I../../../../dist/include/imglib2 -I../../../../dist/include/docshell -I../../../../dist/include -I../../../../dist/include/content -I../../../../dist/include/nspr -DMOZ_PNG_READ -DMOZ_PNG_WRITE -I../../../../dist/sdk/include -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../shared/public -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../html/base/src -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../xml/content/src -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../../dom -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../base/src -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../../layout/svg/base/src -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../../layout/style -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../events/src -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../html/content/src -I/home/taken/mozilla/works/smil/mozilla/content/xbl/src -I/home/taken/mozilla/works/smil/mozilla/content/svg/content/src/../../../smil/public -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -O -I../../../../dist/include/cairo -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -Wp,-MD,.deps/nsSVGElement.pp /home/taken/mozilla/works/smil/mozilla/content/svg/content/src/nsSVGElement.cpp /home/taken/mozilla/works/smil/mozilla/content/svg/content/src/nsSVGElement.cpp: In member function 'virtual nsresult nsSVGElement::DidModifySVGObservable(nsISVGValue*, nsISVGValue::modificationType)': /home/taken/mozilla/works/smil/mozilla/content/svg/content/src/nsSVGElement.cpp:667: warning: unused variable 'document' /home/taken/mozilla/works/smil/mozilla/content/svg/content/src/nsSVGElement.cpp: In member function 'void nsSVGElement::DidAnimateLength(PRUint8)': /home/taken/mozilla/works/smil/mozilla/content/svg/content/src/nsSVGElement.cpp:933: error: invalid use of undefined type 'struct nsIFrame' ../../../../dist/include/content/nsPropertyTable.h:77: error: forward declaration of 'struct nsIFrame' make[7]: *** [nsSVGElement.o] Error 1 make[7]: Leaving directory `/home/taken/mozilla/works/smil/mozilla/mybuild/content/svg/content/src' make[6]: *** [libs] Error 2 make[6]: Leaving directory `/home/taken/mozilla/works/smil/mozilla/mybuild/content/svg/content' make[5]: *** [libs] Error 2 make[5]: Leaving directory `/home/taken/mozilla/works/smil/mozilla/mybuild/content/svg' make[4]: *** [libs] Error 2 make[4]: Leaving directory `/home/taken/mozilla/works/smil/mozilla/mybuild/content' make[3]: *** [libs_tier_gecko] Error 2 make[3]: Leaving directory `/home/taken/mozilla/works/smil/mozilla/mybuild' make[2]: *** [tier_gecko] Error 2 make[2]: Leaving directory `/home/taken/mozilla/works/smil/mozilla/mybuild' make[1]: *** [default] Error 2 make[1]: Leaving directory `/home/taken/mozilla/works/smil/mozilla/mybuild' make: *** [build] Error 2
(In reply to comment #15) > I failed attempt to build with the patch. on Linux/cairo-gtk2 with |--enable-smil| # without the patch, I succeeded build firefox.
Attached patch Same, including all files (obsolete) — — Splinter Review
Previous diff missed a few files. You'll need to run autoconf-2.13 after applying the patch to regenerate configure. Patch only tested with --enable-smil.
Attachment #252826 - Attachment is obsolete: true
Attached patch update to tip (obsolete) — — Splinter Review
Attached patch update to tip (obsolete) — — Splinter Review
Attachment #252915 - Attachment is obsolete: true
Attachment #253187 - Attachment is obsolete: true
Attached patch update to tip (obsolete) — — Splinter Review
Attachment #255707 - Attachment is obsolete: true
Comment on attachment 256178 [details] [diff] [review] update to tip >+++ layout/build/Makefile.in 23 Feb 2007 17:33:33 -0000 >@@ -189,16 +189,22 @@ endif ... >+ifdef MOZ_SMIL >+SHARED_LIBRARY_LIBS += \ >+ $(DIST)/lib/$(LIB_PREFIX)gkconsmil_s.$(LIB_SUFFIX) \ >+ $(NULL) >+endif This should be > $(DEPTH)/content/smil/src/$(LIB_PREFIX)gkconsmil_s.$(LIB_SUFFIX) \ at least on my environment (Ubuntu Linux 6.10/cairo-gtk2)
Attached patch update to tip, fix makefile (obsolete) — — Splinter Review
Attachment #256178 - Attachment is obsolete: true
Attached patch make non-smil builds work (obsolete) — — Splinter Review
Attachment #256647 - Attachment is obsolete: true
Alias: smil
I think this is reasonable to land. I think we should unconditionally build the animation controller stuff in nsPresContext. This will reduce the number of #ifdefs floating around.
Attached patch update to tip again (obsolete) — — Splinter Review
Attachment #256680 - Attachment is obsolete: true
With regards to the change to nsXMLContentSync, I'm not sure it's all that bad to unconditionally send SVGLoad events to all <svg> elements, as long as it's not to all _SVG_ elements. The number of <svg> tags in a document should usually be low compared to others. If we are worried about this though, we could add a mHasSMILContent bit flag to nsDocument. With ContainsSMIL/SetContainsSMIL members added to nsIDocument we could then optimize to dispatch SVGLoad events to <svg> elements only when the document contains SMIL. So we can work on SMIL together I think we could for the moment #ifdef MOZ_SMIL <new code> #else <old code> in nsXMLContentSync though.
Either approach seems ok to me but don't forget the case when a previously static document becomes animated via DOM manipulation (e.g. adding an <animate> element as a child node via script). In this case I think we still need to record the time when the load event was fired so we can start the animation as if it was there from the beginning. See this test case: http://brian.sol1.net/svg/tests/deferred-animation.xhtml The more difficult case is when the entire SVG subtree is added programmatically as in: http://brian.sol1.net/svg/tests/deferred-tree.xhtml I never got this second case to work. I seem to remember discussing some of this with roc and we had some question for the WG but I can't remember what it is now.
Good point. Do you see any problem with forgetting about getting each <svg> to keep it's load time and just using the document load time? Would SVG content authors ever know the difference?
Good point. I'm not sure. I'll have to check the specs. We should probably take this offline huh, perhaps to usenet. I'll see if I get a chance in the next few days to have a look at the specs, otherwise I'm leave it up to you.
#24 "reasonable to land" when can we expect animation to be included in the nightly builds? I've a project that may well require rigorous testing of this module ~:" with reduced test cases...
Attached patch update to tip (obsolete) — — Splinter Review
Attachment #262785 - Attachment is obsolete: true
Attached patch update to tip (obsolete) — — Splinter Review
Attachment #269135 - Attachment is obsolete: true
Attached patch update to tip (obsolete) — — Splinter Review
Attachment #270185 - Attachment is obsolete: true
#33 tor just to mention as per #30 I am regularly, even daily, submitting bugreports to opera regarding SMIL animation. it would avoid duplication of my effort, if mozilla could enable this module. also, as you will know bugzilla is easier to search and more open than some other private bug reporting methodologies. I get confused about which bugs I filed where, unless I can find them easily ~:"
For anyone interested there is now an extension that gets declarative animation working for Firefox 2.x (and for 3.0 while we wait for this patch to land): http://schepers.cc/grafox
Attached patch update to tip (obsolete) — — Splinter Review
Attachment #282285 - Attachment is obsolete: true
The latest patch in 2007.11.8 has a problem when executing svg files that include 'animateTransform'. Some files that are well-executed in the previous patch applied in Mozilla 3.0 alpha, have a crash in the latest patch.
tor, it would be really helpful for me to understand the status of this. The original patch was submitted over a year ago, pretty soon after that we were at "reasonable to land", but at some point it became "not in Firefox 3". When will the Mozilla 2 "split" happen? Can this patch be updated and applied to the Mozilla 2 branch as soon possible so that we can start beating on the SMIL implementation? Sadly these days, I don't have time to build Mozilla and manually apply this patch. Just a little communication is all I ask :)
Blocks: acid3
Regarding comment 37, it looks like this is due to nsSVGAnimateTransformElement not having a virtual method for GetEnumInfo(). Add this to the class definition in nsSVGAnimateTransformElement.cpp: virtual EnumAttributesInfo GetEnumInfo(); and the following implementation: nsSVGElement::EnumAttributesInfo nsSVGAnimateTransformElement::GetEnumInfo() { return EnumAttributesInfo(mEnumAttributes, sEnumInfo, NS_ARRAY_LENGTH(sEnumInfo)); } This results in sites that crashed for me working. For example, http://www.codedread.com/menu.svg crashes with the current patch, but runs with this change. There are still crashes visiting other parts of Jeff's site though.
I should learn not to try a patch just to see if a video demo would work... I've found the issue causing the other crash. nsSVGAnimationElement::Init() is not calling the base class. This results in various nsSVGEnum values not being initialized. Fix: nsresult nsSVGAnimationElement::Init() { + nsresult rv = nsSVGAnimationElementBase::Init(); + NS_ENSURE_SUCCESS(rv, rv); +
Please upload your improved patch when you're done :-)
Updates to apply to trunk and fixes problems mentioned in my last couple of comments.
Attachment #287854 - Attachment is obsolete: true
I attacked some low hanging fruit for my own selfish purposes of getting something I'm writing to work. This patch: - Implements SVGSetElement. So <set ...> should work. - Fixes 'discrete' calcMode - Implements SeekToTime, allowing setCurrentTime to work The Acid 3 tests that use SMIL (75 and 76) work with this patch applied.
Attachment #316205 - Attachment is obsolete: true
Most of comment #13 still needs to be addressed. However the comments from me do not need to be addressed right now, I'll address them myself via Compositor and this could land without them. Some additional comments, though. -- Some documentation for what all these interfaces and classes are *for* would be very helpful -- Instead of storing these document positions, it would be a lot simpler to just make the CompareTo function compare positions in the content tree directly, using say nsContentUtils::PositionIsBefore. -- It would be great to get Jeff Schiller's SMIL tests to pass before we land this. Depends on how much extra work that would be. -- Whatever test automation we can get, preferably based on Jeff's tests plus our own. We should be able to write mochitests with *short* animations and check animVals before, after and during the animation --- the 'during' values won't be completely accurate but we can do sanity checks. + virtual PRBool EqualTarget(const nsISMILAttr &aOther) const; Call it EqualsTarget #ifdef MOZ_SVG - if (content->GetNameSpaceID() == kNameSpaceID_SVG && - content->HasAttr(kNameSpaceID_None, nsGkAtoms::onload)) { + if (content->GetNameSpaceID() == kNameSpaceID_SVG +#ifndef MOZ_SMIL + && content->HasAttr(kNameSpaceID_None, nsGkAtoms::onload) +#endif + ) { FlushTags(); As discussed in comment #26 this needs to change to check for onload OR the element is <svg>.
Hi. Just a point of clarification. "My" tests are nothing more than the tests from the W3C SVG 1.1 Full test suite that deal with animation (SMIL). I don't have any tests of my own, sorry. And fwiw, neither WebKit nor even Opera pass all such tests atm.
Attachment #316597 - Attachment is obsolete: true
Just getting back to a couple of roc's comments: -- There is some documentation of the interfaces at: http://wiki.mozilla.org/SVGDev:SMIL_Model . As for what the interfaces are *for*, some of them may seem superfluous because they anticipate unimplemented functionality. This functionality is outlined in the design on which this work is based: http://www.ludicrum.org/plsWork/papers/BatikSMILsupport.htm . That said, there might still be some genuinely superfluous interfaces. -- I'm not sure why we're not using nsContentUtils::PositionIsBefore. I can only guess that either (a) I didn't know it was there or it wasn't there when I wrote it, or (b) it doesn't provide the behaviour required by SMIL. Just checking the spec, I think this relates to calculating the animation priority under certain circumstances (SMILANIM 3.5) and it simply says, "the element that appears first in the document has lower priority". So I guess PositionIsBefore would work for that. -- Regarding automated tests, I have a whole stack of unit tests covering all sorts of edge cases but I couldn't work out how to set them up so that others can run them as well. I'm not sure if there are tools in place for this now. I've also fixed my website which has a few extra tests there (and apparently the website was hacked recently but I hadn't noticed) http://brian.sol1.net/svg/tests.php
Thanks Brian! It would be helpful to have some information about each interface/class in comments in the header files, because that's where people will probably look first. I don't know when nsContentUtils::PositionIsBefore was added, maybe it postdates your work. But anyway, the important thing is that we can simplify the patch by using it. Our test frameworks also largely postdate your work. Mochitests are a good framework for testing JS APIs.
Attached patch patch, updated for current trunk (obsolete) — — Splinter Review
Here's the patch again, taken from attachment 316765 [details] [diff] [review] and updated to apply to current trunk. I just fixed one chunk of the patch in nsXMLContentSink.cpp -- bug 431703 modified one of the patched lines (adding a null check for mDocument), so that needed updating. It also seems that the "MOZ_SMIL" status isn't getting propagated everywhere that it needs to be. In order to get the patch to successfully build (with --enable-smil), I had to manually "#define MOZ_SMIL" in these two files: content/base/src/nsGkAtomList.h dom/public/nsDOMClassInfoID.h
Attachment #316765 - Attachment is obsolete: true
In addition to Brian's tests, one handy test / demo page for this is: http://www.kevlindev.com/tutorials/basics/animation/svg_smil/index.htm This patch works on all of the demos there except for the last one, which uses 'animateMotion'.
Did you run autoconf2.13 after applying the patch? (For the MOZ_SMIL problem)
I just ran "make -f client.mk build", which I'd thought runs autoconf as part of the build process. But maybe it doesn't? ('autoconf --version' shows that my version is 2.13, btw)
Just to be clear "autoconf" (or "autoconf213") generates 'configure' from 'configure.in'. "make -f client.mk build" re-runs 'configure' but doesn't run autoconf.
Ah, thanks for the clarification. Ok, I'm trying that, and I rescind this bit of comment #50: > It also seems that the "MOZ_SMIL" status isn't getting propagated everywhere > that it needs to be. In order to get the patch to successfully build (with > --enable-smil), I had to manually "#define MOZ_SMIL" in these two files: > content/base/src/nsGkAtomList.h > dom/public/nsDOMClassInfoID.h (Note that I didn't actually include those added #defines in the updated patch that I posted, so the patch is still good.)
(In reply to comment #52) > Did you run autoconf2.13 after applying the patch? (For the MOZ_SMIL problem) (In reply to comment #56) > Ok, I'm trying that Great, that does indeed fix my build. (no extra #define MOZ_SMIL's needed.) Thanks!
Assignee: general → dholbert
I've published a repository of smil patches here: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/ Currently, it just has the most recent patch from this bug (attachment 320264 [details] [diff] [review]). To use this repo, do the following: 1. Install mercurial, per http://developer.mozilla.org/en/docs/Mercurial 2. Check out mozilla-central, per http://developer.mozilla.org/en/docs/Mercurial_FAQ#How_can_I_check_out_Mozilla.3F 3. Enable Mq in your .hgrc file, and get familiar with Mq, per http://www.selenic.com/mercurial/wiki/index.cgi/MqExtension 4. Check out my repo into your /mozilla/.hg directory, and name it "patches" (or make a symlink called "patches") 5. In your /mozilla directory, run "hg qpush smil_main" This should apply the current smil patch on top of your mozilla-central checkout. Then you can run "make -f client.mk build" (using a .mozconfig that includes "ac_add_options --enable-smil"), and you'll get a mozilla build with SMIL support. As I (and others?) commit changes to the smil-patches repository, I'll probably still post updated patch versions here, so they don't get lost accidentally. But in the meantime, we can file separate bugs for broken / missing SMIL behavior, using "Component: SVG" and "Version: Other Branch". (and with the URL of that hg repo in the bug description.)
Status: NEW → ASSIGNED
(In reply to comment #58) > 4. Check out my repo into your /mozilla/.hg directory, and name it "patches" > (or make a symlink called "patches") Since I didn't provide a URL with more info on step 4, the commands for this are: cd mozilla/.hg hg clone http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/ patches or, using a symlink: cd mozilla/.hg hg clone http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/ ln -s smil-patches patches (where "mozilla/" is a clone of the mozilla-central repository)
Blocks: 436422
Blocks: animateMotion
Blocks: 436276
Blocks: 436296
Marking wanted1.9.1? to get this on the triage queue.
Flags: wanted1.9.1?
Priority: -- → P2
FWIW: The FakeSMIL greasemonkey script looks really handy for testing / comparison. http://leunen.d.free.fr/fakesmile/status.html
wanted1.9.1+ to get on the list for 1.9.1.
Flags: wanted1.9.1? → wanted1.9.1+
Blocks: 446654
No longer blocks: 446654
Attached patch main smil patch v1 (obsolete) — — Splinter Review
Here's the SMIL patch with some updates from my patch queue at http://hg.mozilla.org/users/dholbert_mozilla.com/index.cgi/smil-patches/ This attached patch is a combination of "smil_main" (the carried-forward original patch, with some small modifications), "smil_tests" (a chunk of reftests to start with), and "smil_paced" (a patch that adds support for calcMode="paced"). I'll post a more detailed description of the changes (together with a diff of this patch vs. the last patch version) shortly.
Attachment #320264 - Attachment is obsolete: true
This is a diff of the older patch revision, attachment 316765 [details] [diff] [review] ("Fix discrete calcMode issue found by Jeff"), vs the just-posted attachment 331578 [details] [diff] [review] ("main smil patch v1"). I'm not comparing against the more recent "patch, updated for current trunk" because that version uses CVS-style file ordering, whereas the other two use hg/git-style file ordering, and that makes them easier to diff against each other. Also, I would've posted an interdiff, but I haven't been able to make one because the contextual code has changed enough that I can't get both patches to apply cleanly to the same tree, and so interdiff fails. :) So, the next-best thing is to just diff the patches. To minimize useless differences, I modified the old patch slightly to match the style of my newer patch, by replacing "a/mozilla/XXX/foo.cpp" with "a/XXX/foo.cpp" and by removing all header lines of the form "index 6431a1b..9c43666 100644". I'll post a summary of the changes in my next comment.
Attached patch main smil patch v1b (obsolete) — — Splinter Review
This patch fixes one thing vs."main smil patch v1" -- this one checks for "onload OR the element is <svg>" in nsXMLContentSink.cpp, per the end of comment 45. Another patch-diff is coming up, with a summary of changes.
Attachment #331578 - Attachment is obsolete: true
Here's a diff like the last one, this time between attachment 316765 [details] [diff] [review] vs attachment 331581 [details] [diff] [review]. Summary of changes since attachment 316765 [details] [diff] [review] ========================================== General changes: * s/ToActive/Activate/, s/ToInactive/Inactivate/ per comment 13 * s/ParentPaused/HandleParentPaused/, s/ParentPaused/HandleParentResumed/, "Used to inform" per comment 13 * s/EqualTarget/EqualsTarget/ per comment 45 nsSMILAnimationFunction.cpp: * Added calcMode="paced" implementation * Improve variable & function naming (lots of s/distance/progress/, to make a clear distinction between "progress" and "distance", per these definitions: - progress: values from 0.0 - 1.0, showing how far we are thru simple duration - distance: "distance" between nsSMILValues, as returned by ComputeDistance() * Fix calcMode="discrete" calculation -- the old implementation would never reach the final element in values list, when really that element is supposed to be visited for the final 1/Nth of the time, where N = # of values. nsSMILTimedDocumentRoot.cpp: * At first call to Sample(), when we set mStartTime, we now also clear mAccumulatedOffset. (The "accumulated offset" is supposed to be the amount of time we've been paused *since mStartTime*. So when we first set mStartTime, we need to clear the accumulated offset.) nsSVGAnimationElement.cpp: * Add a XXX comment in nsSVGAnimationElement::UpdateTargetElement saying that we need to clear animation effects. (if an <animate> element is reparented / retargeted mid-animation, it shouldn't leave animation effects on its old parent / target.) nsSVGSVGElement.cpp: * Only call SuspendRedraw / UnsuspendRedraw if we have a non-null GetPrimaryFrame(). If we don't have a frame, we shouldn't have to worry about redrawing. (And in fact SuspendRedraw will complain with some assertion failures if we try to suspend redrawing without a primary frame. This change fixes continuously-failing assertions with the new Ctrl-Tab UI, which uses some hidden SVG content.) * QUESTIONS: - Is my assumption above correct? (no frame --> no need to worry about redrawing?) - Is it possible that GetPrimaryFrame()'s output would change mid-compositing, so that we wouldn't call SuspendRedraw, but we would call UnsuspendRedraw()? nsXMLContentSink.cpp: * This includes the change mentioned in Comment 50, to accomodate changed contextual code, plus the "onload OR the element is <svg>" suggestion at the end of Comment 45. layout/reftests: * Added a variety of reftests **Note: some of the reftests (in layout/reftests/svg/smil/timed) include short setTimeout calls (totaling at most 2 seconds per testcase) to allow animations to proceed / complete. These timeouts may be too long / unpredictable for use in reftests -- that's why I've separated such reftests into their own "timed" directory, where they can be easily removed / disabled as a group. I should probably convert these into mochitests.
Attachment #331579 - Attachment is obsolete: true
(In reply to comment #66) > Created an attachment (id=331582) [details] > diff attachment 316765 [details] [diff] [review] vs attachment 331581 [details] [diff] [review] Note that bugzilla's patch-viewer hides lines starting with ++ and -- (bug 338486 / bug 233695), and thus, it's pretty broken for viewing diffs-of-patches. I recommend viewing attachment 316765 [details] [diff] [review] either as a raw patch file, or using a patch-viewer tool like KDE's kompare.
This extra patch (taken from its current version in my patch queue at http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/ ) modifies the way that a nsSMILValue keeps track of its type, so as to make it easier to know when mU.mPtr is valid. (I make use of this behavior in my upcoming "smil_css" patch) Basically, the idea with this patch is that nsSMILValue should only set its type *after* it has the necessary memory allocated in mU.mPtr (or after it has an otherwise meaningful value stored in mU, for those types that don't use mU.mPtr). Following this strategy, when DestroyValue is called to clean out the nsSMILType, the value should clear its mType pointer (to nsSMILNullType::sSingleton), because it no longer has the requisite memory allocated for its old type. This way, for SMIL types that use mU.mPtr, we can easily know whether or not the pointer is valid -- we just check mType. This is particularly important in the SetValueIdentity, AssignValue, and DestroyValue functions, which could potentially take initialized or non-initialized nsSMILValues. In these functions, we need a way to know whether the pointer points to something that needs cleaning up before we overwrite / destroy it.
This WIP patch contains a simple nsIDOMDocumentCSS::getOverrideStyle() implementation, for use in performing SMIL animation on CSS properties. (Taken from the current version of "getoverridestyle.patch" in my patch queue at http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/ -- file-listing here: http://tinyurl.com/5959fv).
Here's the current SMIL/CSS Integration WIP, snapshotted from http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/ file-listing here: http://tinyurl.com/5tpwt2 Limitations: - currently supports animating nsCSSValues. (Should be somewhat straightforward to extending to nsCSSRect etc, though that'll need a little bit more parsing infrastructure) - Interpolating only currently works with absolute-length units (pt/cm/in/etc). I need to figure out how best to interconvert between dpi-dependent and font-metric-dependent lengths like 'px' and 'em' in the place where I need to. (in nsSMILCSSValueType::Interpolate)
(In reply to comment #71) > Created an attachment (id=331660) [details] > reftest: prev.begin and ID.end animation trigger events This testcase doesn't work with any of the WIP patches yet, for several reasons... - The WIP patches don't yet support event-values for begin/end. (which includes the "prev.begin" and "[id].end" values in the testcase). I think that event-values-support would be a good candidate for a follow-up patch (possibly with its own separate bug). - The testcase makes extensive use of animating the text/tspan "rotate" attribute, which isn't yet implemented at all in Firefox (that's bug 433345). So, we can't animate that attribute until we support it. :)
Comment on attachment 331581 [details] [diff] [review] main smil patch v1b Tentatively r?-ing roc and tor on the main SMIL patch. :)
Attachment #331581 - Flags: superreview?(roc)
Attachment #331581 - Flags: review?(tor)
Attached patch main smil patch v1c (obsolete) — — Splinter Review
This updated patch has just a few minor changes wrt the last version (attachment 331581 [details] [diff] [review]) - Fixed nsSMILAnimationRegistry::SetCurrentTime() to convert sec --> msec _before_ converting to an integer value (so as not to lose precision) - Minor variable-renaming in nsSMILAnimationRegistry::GetCurrentTime() - Corrected the NS_NOTYETIMPLEMENTED call in nsSMILTimedDocumentRoot::WallclockToDocumentTime (which previously printed the wrong method name.)
Attachment #331581 - Attachment is obsolete: true
Attachment #332186 - Flags: superreview?(roc)
Attachment #332186 - Flags: review?(tor)
Attachment #331581 - Flags: superreview?(roc)
Attachment #331581 - Flags: review?(tor)
This diff-of-diffs shows the minimal changes between the last two "main smil patch" revisions (v1b and v1c), as described in the previous comment. (As mentioned previously, bugzilla doesn't display some lines in diffs-of-diffs, so it's better to view this as raw text or in a real patch-viewer, rather than in bugzilla's diff viewer)
Starting review - a few initial notes: Minor thing: at the code cleanup session it was mentioned that we should remove the /public and /src directory structure. Since you're adding content/smil, you could do that at the start. In nsXMLContentSink, you've changed the nature of the test. Without SMIL configured it will send the event to any element with an "onload" attribute, not just SVG elements. nsISMILAttr reimplements AddRef/Release inline. Is this really time critical enough that we need this, or could it be implemented with NS_IMPL_ADDREF/NS_IMPL_RELEASE. Additionally, since this isn't a moz-traditional queryable interface but rather a base class for the animatable attribute types, should it be renamed to nsSMILAttr?
(In reply to comment #45) > -- Instead of storing these document positions, it would be a lot simpler to > just make the CompareTo function compare positions in the content tree > directly, using say nsContentUtils::PositionIsBefore. This hasn't been addressed yet? Also the deCOMtamination of comment #13 has not been done ...
(In reply to comment #76) > Minor thing: at the code cleanup session it was mentioned that we should remove > the /public and /src directory structure. Since you're adding content/smil, > you could do that at the start. Sounds good. > In nsXMLContentSink, you've changed the nature of the test. Without SMIL > configured it will send the event to any element with an "onload" attribute, > not just SVG elements. Ah, right -- I'd misunderstood the end of Comment 45. I think I've fixed it in the patch-snippet below -- let me know if it looks ok. #ifdef MOZ_SVG if (mDocument && - content->GetNameSpaceID() == kNameSpaceID_SVG && + content->GetNameSpaceID() == kNameSpaceID_SVG) { +#ifdef MOZ_SMIL + // Check if content is <svg> -- if so, we need to send it an SVGLoad + // event regardless of whether it has 'onload', in case it's got some + // SMIL-animated content. + nsCOMPtr<nsIDOMSVGSVGElement> svgElem(do_QueryInterface(content)); + if (svgElem || +#else + if ( +#endif content->HasAttr(kNameSpaceID_None, nsGkAtoms::onload)) { FlushTags(); nsEvent event(PR_TRUE, NS_SVG_LOAD); event.eventStructType = NS_SVG_EVENT; event.flags |= NS_EVENT_FLAG_CANT_BUBBLE; // Do we care about forcing presshell creation if it hasn't happened yet? // That is, should this code flush or something? Does it really matter? // For that matter, do we really want to try getting the prescontext? Does // this event ever want one? nsRefPtr<nsPresContext> ctx; nsCOMPtr<nsIPresShell> shell = mDocument->GetPrimaryShell(); if (shell) { ctx = shell->GetPresContext(); } nsEventDispatcher::Dispatch(content, ctx, &event); } + } #endif > nsISMILAttr reimplements AddRef/Release inline. Is this really time critical > enough that we need this, or could it be implemented with > NS_IMPL_ADDREF/NS_IMPL_RELEASE. I don't think we _need_ inline implementations, but I also don't think we can use those macros -- when I tried them, I got compiler errors about '_mOwningThread', a member variable that the macros apparently expect to be present. (I'm guessing the macros expect a nsISupports object, which would have that member variable). So, AFAIK, we can't use those macros to implement AddRef/Release on nsISMILAttr. (though maybe there's a similar alternative, or something else I can do to make the macros work?) > Additionally, since this isn't a > moz-traditional queryable interface but rather a base class for the animatable > attribute types, should it be renamed to nsSMILAttr? Is it a general Mozilla rule that nsI*.h is reserved for queryable interfaces? There are a number of examples in the codebase where we go against this -- e.g. nsIReflowCallback.h ( http://tinyurl.com/5zzzet ), nsITableLayoutStrategy.h ( http://tinyurl.com/5nw92v ), and nsIMenuParent.h ( http://tinyurl.com/5bseko ). But if that is indeed a rule, I'm happy to rename the offending SMIL-related classes. (though we should probably get those other ones renamed too, at some point)
(In reply to comment #77) > (In reply to comment #45) > > -- Instead of storing these document positions, it would be a lot simpler to > > just make the CompareTo function compare positions in the content tree > > directly, using say nsContentUtils::PositionIsBefore. > > This hasn't been addressed yet? > > Also the deCOMtamination of comment #13 has not been done ... Correct. I haven't looked into those changes in too much detail -- I was hoping they could be addressed in follow-up patches, per the "I think this is reasonable to land" bit of comment 24. Do you think those need fixing before this lands?
(In reply to comment #78) > (In reply to comment #76) > Ah, right -- I'd misunderstood the end of Comment 45. I think I've fixed it in > the patch-snippet below -- let me know if it looks ok. QI is slow, just check content->Tag() == nsGkAtoms::svg
Ah -- thanks, Robert. That makes the updated nsXMLContentSink.cpp patch snippet just this, then: #ifdef MOZ_SVG if (mDocument && content->GetNameSpaceID() == kNameSpaceID_SVG && - content->HasAttr(kNameSpaceID_None, nsGkAtoms::onload)) { + ( +#ifdef MOZ_SMIL + content->Tag() == nsGkAtoms::svg || +#endif + content->HasAttr(kNameSpaceID_None, nsGkAtoms::onload))) { FlushTags();
(In reply to comment #79) > (In reply to comment #77) > > (In reply to comment #45) > > > -- Instead of storing these document positions, it would be a lot simpler to > > > just make the CompareTo function compare positions in the content tree > > > directly, using say nsContentUtils::PositionIsBefore. > > > > This hasn't been addressed yet? > > > > Also the deCOMtamination of comment #13 has not been done ... > > Correct. I haven't looked into those changes in too much detail -- I was > hoping they could be addressed in follow-up patches, per the "I think this is > reasonable to land" bit of comment 24. Do you think those need fixing before > this lands? Well ... I think I'd prefer to land with minimal complexity and add complexity later. I'm not too happy about landing something that's more complex than needed. Have you got time to bang away on those issues for a couple of days?
(In reply to comment #82) > Well ... I think I'd prefer to land with minimal complexity and add complexity > later. I'm not too happy about landing something that's more complex than > needed. Fair enough. > Have you got time to bang away on those issues for a couple of days? Yup, starting Monday. (out of town right now)
Hey Daniel. Can you share any update on this? It's on the list of wanted1.9.1, and it seems the patch is there, yet no activity here for almost a month now.
Attached patch main smil patch v2a (obsolete) — — Splinter Review
Here's an updated patch, which includes all of the unguarded patches from my repo at http://hg.mozilla.org/users/dholbert_mozilla.com/index.cgi/smil-patches/ (the equivalent of doing an "hg qpush -a", using that repo as your patches directory) I've also pushed this patch to the try server, so builds should be appearing in https://build.mozilla.org/tryserver-builds/?C=M;O=D in the next hour or so. I have to run right now, but I'll post a diff and a description of the updates shortly.
Attachment #332186 - Attachment is obsolete: true
Attachment #332186 - Flags: superreview?(roc)
Attachment #332186 - Flags: review?(tor)
Acid3 test 75 and test 76 are still failed. And testcase of this bug isn't display.
(In reply to comment #87) > Acid3 test 75 and test 76 are still failed. Yeah, with an error-console message saying "anim.beginElement is not a function", or something like that. I've seen that before -- it's a really weird issue, because for some reason i only can reproduce it in TryServer builds, but never in my own local debug or optimized builds using the exact same code as the TryServer. In any case, I'm 99% sure it's just happening because I need to add a chunk for nsIDOMSVGSetElement to nsDOMClassInfo.h / h, next to the existing chunks for nsIDOMSVGAnimateElement and nsIDOMSVGTransformElement. I'll post a new patch & kick off new TryServer builds with that fixed.
dholbert: are you creating new interfaces? If so, did you add the .xpt files to the packaging manifests? If not, then that could be why things fail on packaged builds but not your local builds.
Attached patch main smil patch v2b (obsolete) — — Splinter Review
Ted: Thanks for the tip! There is indeed one interface that's in its own "smil" folder, nsIDOMElementTimeControl.idl, and that one apparently wasn't making it in. This updated patch fixes the nsDOMClassInfo issue from comment 88 and also adds dom_smil.xpt to the packaging manifests. I've kicked off two new tryserver builds -- the first just has the nsDOMClassInfo fixes, and the second adds the manifest fix to that. (I expect that the second one should pass the acid 3 tests)
Attachment #335986 - Attachment is obsolete: true
Comment on attachment 331660 [details] reftest: prev.begin and ID.end animation trigger events (In reply to comment #87) > And testcase of this bug isn't display. If you're referring to attachment 331660 [details], that's because it depends on a number of animation and SVG features that we don't yet support and are outside the scope of this bug (see comment 72). --> Obsoleting the attachment to avoid further confusion.
Attachment #331660 - Attachment is obsolete: true
hi, i tried your build dholbert@mozilla.com-try-07ae6f925a9 and can confirm that Acid 3 "Test 75 failed: anim.beginElement is not a function" & "Test 76 failed: expected '0' but got '100' - Incorrect animVal value after svg animation" are history :). moreover i can confirm that all but one testcases from Brian's site pass. the testcase http://brian.sol1.net/svg/tests/enveloped-tree.xhtml (Enveloped tree) fails the "horizontal position at all times" condition. see: http://img295.imageshack.us/img295/9904/smilenvelopedtree01compdr1.png (Gecko vs. Webkit r35806). as shown Webkit produces gfx defects instead.
I tried the build 07ae6f925a9 too but acid tests don't failed. I use WinXP.
Thanks XtC4UaLL and Gabriele - good to know that those tests are passing. (In reply to comment #92) > moreover i can confirm that all but one testcases from Brian's site pass. > the testcase http://brian.sol1.net/svg/tests/enveloped-tree.xhtml (Enveloped > tree) fails the "horizontal position at all times" condition. Thanks for reporting that! I can confirm that here as well -- the blue circle is slightly ahead of the green circle. I'll look into that.
QA Contact: general
Attached patch main smil patch v3a (obsolete) — — Splinter Review
Attachment #336084 - Attachment is obsolete: true
Comment on attachment 339235 [details] [diff] [review] main smil patch v3a List of Notable Changes: ======================== - deCOMtamination of comment 13 - All numbered items in that comment (excluding #3, since that relates to syncbase timing, which isn't supported yet) - Various changes of nsCOMPtr to nsRefPtr and nsCOMArray to nsTArray<nsRefPtr<> > - nsSMILInstanceTime no longer extends nsISupports, because it doesn't need ref-counting. - It's possible there's some more that could be done, but I think that's a lot of what remained. - nsSMILAnimationFunction::CompareTo to use nsContentUtils::PositionIsBefore per roc's suggestion in comment 45. - nsISMILAttr now inherits from nsISupports. This... * ... lets it use nsISupports' ref-counting methods (as suggested by tor in comment 76), so nsISMILAttr no longer has to inline its own implementation * ... fixes a leak that shows up at application exit, from a dangling reference. - about:config preference added, called "svg.smil.enabled". This pref is used to initialize a flag on the nsPresContext. If you change the pref in about:config, the updated value will take effect in SVG documents that you (re)load after the change (when a new nsPresContext is initialized). This matches the way image.animationMode works. * Note: If the pref image.animationMode is "none", SMIL is considered to be disabled, regardless of the svg.smil.enabled value. * Note: I intentionally avoided using #ifdefs around this pref-related code in nsPresContext, to avoid excess #ifdef-complexity in that file (per the sentiment in comment 24). I'm happy to add #ifdefs there if you think that'd be better, though. - beginElement / endElement now have meaningful return values -- they return false if they fail due to 'restart' restrictions. (There's a mochitest to regression-test this behavior.) - js function-calls that should trigger immediate changes in the animation (e.g. setCurrentTime, beginElement, endElement) will now trigger an updated sample to be taken, earlier than it otherwise would have been, via a call to nsSVGElement::RequestSample(). This basically queues a nsRunnable, which forces a synchronous sample when it runs. This functionality lets an author do something like: elem.endElement(); setTimeout("nextStep()", 0); where nextStep() is something that depends on endElement having already taken effect. This yields nice predictable behavior, and it also matches Opera's behavior. (NOTE: This feature was also present in patches v2a and v2b, but I didn't describe it at the time, so I'm mentioning it here.) KNOWN ISSUES: ============= - Animations continue during print / print-preview, but they shouldn't. (I think I just need to mirror what animated GIFs do during printing) - Each subdocument still has its own animation controller & timer, as roc pointed out in comment 13. This means that subdocuemnts' will be updated at slightly different times, which can make them appear slightly out of sync. (This causes the issue on "enveloped-tree.xhtml" that XtC4UaLL mentioned in comment 92) WISH LIST / TODO's: =================== - Before landing, I should make the "svg.smil.enabled" pref default to "false", if we want to disable SMIL by default. (It's default-enabled in the patch for now, for easier testing.) - roc suggests that nsSVGAnimationElement::mAnimation should probably be a nsAutoPtr<nsAnimationFunction>, instead of a nsRefPtr, to emphasize that it doesn't live longer than the nsSVGAnimationElement itself. This should be fine, but it'll require a bit of tweaking, because it'd mean nsSMILAnimationFunctions can *never* be used in a nsRefPtr / nsCOMPtr. - I've got a folder of time-dependent SMIL reftests (in reftests/svg/smil/timed), but right now I've disabled them in the manifest reftests/svg/smil/reftest.list because the timeouts eat up time and could potentially introduce nondeterminism on the tinderboxes. I'd like to convert as many of them as possible into non-time-sensitive tests. (and the rest should probably be ditched) - I still need to merge smil/public and smil/src into one directory, as suggested by tor in comment 76.
Attachment #339235 - Flags: superreview?(roc)
Attachment #339235 - Flags: review?(tor)
Attachment #339235 - Attachment description: mail smil patch v3a → main smil patch v3a
Looks great Daniel, thanks again. I think the nsSMILInstanceTime may need to be ref-counted when we implement syncbase timing but for now it's probably fine to get rid of it. Also, I think nsISMILAttr didn't inherit from nsISupports because I (probably quite mistakenly) assumed that QI-support had some sort of cost associated with it, when only ref-counting was required.
+ bool IsNull() const PRBool + PRBool mSMILEnabled; PRPackedBool and put it next to some other PRPackedBools +class nsISMILAnimationController : public nsIAnimationController Does this need to be refcounted? Why not just let the prescontext own it? Seems to me we could just have nsSMILAnimationController, no abstract interfaces, and let the prescontext have an "nsAutoPtr<nsSMILAnimationController> mAnimationController". +class nsSMILAnimationFunction : public nsISMILAnimationFunction, Why do we need the nsISMILAnimationFunction base class? We could just use nsSMILAnimationFunction directly. And it doesn't need to inherit from nsISupports. In fact, as far as I can tell, this doesn't need to be heap-allocated or refcounted, it could just be a direct member of nsSVGAnimationElement. The methods can all be devirtualized. Similar for nsSMILTimedElement/nsISMILTimedElement. + mTimeControl = do_QueryInterface(mTimedElement); Why do we need mTimeControl if it's always just the QI of mTimedElement? If we get rid of nsISMILTimedElement and make mTimedElement a direct member, we definitely won't need mTimeControl. Also, don't make mTimeControl implement nsIDOMElementTimeControl itself, there's no need and it just makes functions virtual that don't need to be. You should be able to remove all the weak-ref stuff by having animation elements notify the animation controller(s) on all the document's presshells when they go away. In nsISMILAnimationObserver, StartSample/EndSample don't seem to be used at all. Can we get rid of them? StartCompositing/EndCompositing seem to just trigger SuspendRedraw/UnsuspendRedraw, which aren't really needed at all, so why not just get rid of the whole nsISMILAnimationObserver interface? +// nsISMILAnimationRegistry: Entry point for SMIL animated documents My understanding is that there's one of these for each <svg> subtree, so that in an XHTML document, say, you might have multiple registries but just one animation controller. OK, but why can't we get rid of the abstract interface and just have nsSMILAnimationRegistry which is not refcounted and owned by the root <svg> element for the subtree? Why does nsSMILTimedDocumentRoot have to be a separate object from nsSMILAnimationRegistry? nsSMILAnimationFunction is the only nsISMILTimedClient and nsISMILComposable, so can we get rid of those interfaces and just use nsSMILAnimationFunction directly? nsSMILCompositor::SortCompositors should be SortComposables, right? Storing the CompositorEntries in an array in the registry seems like a really bad idea. For example it seems that destroying an SVG subtree with N animated attributes in it will be O(N^2) as we look up each function in the array to remove it. So I urge a rethink of how compositors and composables (really, animation functions) are managed. This rethink might be able to simplify nsISMILAttr so that we don't have to wrap so much and worry about cycles due to holding a strong reference to the underlying element there. Another huge problem with the way this works now is that IDs are resolved to elements and then the element references cached in nsISMILAttr's subclasses, and this totally ignores the possibility that the element associated with a given ID can change for various reasons, such as an element with the same ID being inserted earlier in the document. It seems that all we're trying to achieve here is an efficient way to get the sorted list of composables for every animated attribute for use in each sample step. OK, so assuming animation elements in a document can only animate other elements in the same document, how about nsDocument just keeps a hash-set of all animation elements in the document. That's easy and cheap to maintain, in hooking nsSVGAnimationElement::BindToTree/UnbindToTree. Then, when nsSMILAnimationController wants to do a sample step, it can traverse all the document's animation elements, resolve each target animated element, and construct a hashtable mapping (element reference, attribute name) pairs to lists of nsSMILAnimationFunctions. The functions for each target element can then be sorted into the right order and applied. Then the nsSMILAnimationFunction won't need to store an nsISMILAttr or a reference to the animated element. In fact we might be able to get rid of nsISMILAttr and its subclasses completely. We would probably need to keep an auxiliary set of animated elements around so that when an element stops being animated (for whatever reason) we can detect that and reset its animated attribute values. + if (!SMILEnabled()) { + NS_NOTREACHED("Shouldn't be making/setting an animation controller " + "if SMIL is disabled..."); + return; + } It seems wrong for this to happen based on what prefs are set up. Prefs can change at any time. In fact, I would suggest not supporting a svg.smil.enabled pref. I do think we should extend the MOZ_SMIL #ifdefs to guard the code added to nsPresContext. I definitely haven't gone through the whole patch yet, but I think we've got a lot of simplification to do here. We should definitely not count on this landing for Gecko 1.9.1.
Hi roc, thanks very much for looking at this. There is definitely a lot of simplification that could be done. I'd like to just explain some of the reasons why things are in the state they are: 1) The code is based on a based on a design for a complete SMIL implementation whereas the one we have at the moment is lacking many features. Most notable of these missing features is syncbase-timing which complicates things considerably. Therefore the code is necessarily not, "the simplest thing that could possibly work." This is perhaps the reason for some of the seemingly unnecessary ref-counting as well as some of the extra interfaces. So there's definitely a lot of simplification that could be done but I don't think all of it should be done because some of it will have to be undone once we come to implement these other features. And I assume we are going to implement these other features because they are part of SVG 1.1. 2) There is some architectural basis for dividing the code up in this way. This is particularly the case when it comes to keeping the timing and animation functionality at arm's length. This division simplifies the code (or at least avoids Blob classes) and aids unit testing. (Unfortunately I haven't been able to find a way to get my unit tests into a shape able to be submitted here. I'm referring to real unit tests here which test just one class, as opposed to more integrative tests done using javascript.) The separation of the animation and timing model is the reason for separating nsSMILTimedDocumentRoot (a strictly timing-model-related class) and nsSMILAnimationRegistry (which belongs to neither the animation nor timing model but instead drives them both). Matters of architecture are also the reason for some of the unnecessary interfaces. For example... > nsSMILAnimationFunction is the only nsISMILTimedClient and nsISMILComposable, so can we get rid of those interfaces and just use nsSMILAnimationFunction directly? Yes, that's right. The interfaces just offer a separation of concerns (e.g. the compositor shouldn't be able to see methods like "SetBy" and so on). So you can drop it if that would help. As far as performance goes, from my profiling nearly all the time is spent in redrawing. So I'd say it's much more important to add smarts to reduce the amount of redrawing than it is to speed up the animation code itself. 3) I wasn't sure how certain things got destroyed. That is to say, it should be possible to remove some ref-counting particularly at the interface between SMIL and the rest of the Moz code (i.e. nsPresContext, the document tree etc.). Probably when I came to this I wasn't sure when different objects might disappear so to play it safe I made some of those objects ref-counted so at least pointers wouldn't dangle. Other misc issues: > [Regarding nsISMILAnimationRegistry] My understanding is that there's one of these for each <svg> subtree, so that in an XHTML document... Your understanding of the relationship between animation controller (one per pres-context) and animation registry (one per svg sub-tree) matches mine. > nsSMILCompositor::SortCompositors should be SortComposables, right? Yep. My bad. > Storing the CompositorEntries in an array in the registry seems like a really bad idea... Yeah, good point regarding performance. > Another huge problem with the way this works now is that IDs are resolved to elements... We don't support ID lookups yet (using xlink:href). Or at least I never implemented it. Implementing this also requires implementing attributeType. Instead, at the moment only parent elements can be targeted. > Then, when nsSMILAnimationController wants to do a sample step, it can traverse all the document's animation elements... I thought this would be expensive as samples are performed many times a second? I know we can maintain the mapping but then you still have the problem of IDs changing. I think I've misunderstood. Thanks again roc! I hope that helps clarify *why* the code isn't as simple as it could be. I don't mean to dissuade anyone from simplifying it, but just bear in mind the work that is yet to be done on SMIL.
(In reply to comment #99) > Hi roc, thanks very much for looking at this. There is definitely a lot of > simplification that could be done. I'd like to just explain some of the > reasons why things are in the state they are: Thanks very much for your comments! > 1) The code is based on a based on a design for a complete SMIL implementation > whereas the one we have at the moment is lacking many features. > > Most notable of these missing features is syncbase-timing which complicates > things considerably. Therefore the code is necessarily not, "the simplest > thing that could possibly work." > > This is perhaps the reason for some of the seemingly unnecessary ref-counting > as well as some of the extra interfaces. > > So there's definitely a lot of simplification that could be done but I don't > think all of it should be done because some of it will have to be undone once > we come to implement these other features. And I assume we are going to > implement these other features because they are part of SVG 1.1. OK. There is a significant risk of "premature generalization". That is, when we come to implement those other extra features, we may find that the best design is not exactly what we guessed today. In fact, by starting from a more complicated base, we may find it harder to get to the best design. I have learned this the hard way. I would be happy to have the "simplest thing that could possibly work" for the current feature set, as long as we think it's cleanly extensible for the rest of the features, i.e. without having to rewrite a lot of code. Right now I don't see how syncbase timing is likely to require undoing of the simplifications I described above. Another obvious extension we should think about is the extension to animate CSS properties. > 2) There is some architectural basis for dividing the code up in this way. > > This is particularly the case when it comes to keeping the timing and animation > functionality at arm's length. This division simplifies the code (or at least > avoids Blob classes) and aids unit testing. (Unfortunately I haven't been able > to find a way to get my unit tests into a shape able to be submitted here. I'm > referring to real unit tests here which test just one class, as opposed to more > integrative tests done using javascript.) > > The separation of the animation and timing model is the reason for separating > nsSMILTimedDocumentRoot (a strictly timing-model-related class) and > nsSMILAnimationRegistry (which belongs to neither the animation nor timing > model but instead drives them both). I appreciate that. In the situations where we have two classes with a one-to-one relationship, we should try to embed one object in the other directly to make the relationship clear and avoid run-time overhead, when that can be done without too much pain. But we have to recognize that separating classes usually imposes complexity costs of its own. Good comments to separate the different kinds of functionality supported in a class go a long way to providing the separation of concerns you want, while not adding complexity, LOC and runtime overhead. > Matters of architecture are also the reason for some of the unnecessary > interfaces. For example... > > > nsSMILAnimationFunction is the only nsISMILTimedClient and nsISMILComposable, so can we get rid of those interfaces and just use nsSMILAnimationFunction directly? > > Yes, that's right. The interfaces just offer a separation of concerns (e.g. the > compositor shouldn't be able to see methods like "SetBy" and so on). So you can > drop it if that would help. This is a situation where good comments should give us 90% of the benefit and none of the cost. > As far as performance goes, from my profiling nearly all the time is spent in > redrawing. So I'd say it's much more important to add smarts to reduce the > amount of redrawing than it is to speed up the animation code itself. Yes. The most important thing is to reduce the size of the source code and the number of "moving parts". But we do have to watch memory footprint and the number of allocated objects. A long time ago, those got out of control in our SVG implementation and we took a while to recover from that. > 3) I wasn't sure how certain things got destroyed. > > That is to say, it should be possible to remove some ref-counting particularly > at the interface between SMIL and the rest of the Moz code (i.e. nsPresContext, > the document tree etc.). Probably when I came to this I wasn't sure when > different objects might disappear so to play it safe I made some of those > objects ref-counted so at least pointers wouldn't dangle. OK. > > Another huge problem with the way this works now is that IDs are resolved to elements... > > We don't support ID lookups yet (using xlink:href). Or at least I never > implemented it. Implementing this also requires implementing attributeType. > > Instead, at the moment only parent elements can be targeted. Indeed, good point. Still, it's something we will need to handle and it just so happens that my scheme would handle it, while still being simple right now :-). > > Then, when nsSMILAnimationController wants to do a sample step, it can traverse all the document's animation elements... > > I thought this would be expensive as samples are performed many times a second? At each sample step, we already have to do work proportional to the number of animation elements (evaluate the nsSMILAnimationFunction for each one). So the scheme I propose would increase the constant factor but not the algorithmic complexity. If there's a really large number of animation elements and the cost of resolving the target elements and attributes is significant (compared to the cost of redrawing), then we could start optimizing in various ways. For example: -- store only the active animation elements in the document's list of animation elements -- cache the target element for each animation element if the ID-lookups get expensive (but they probably won't!) -- store the list of active animation elements in content order, say by using a balanced tree structure. Then when we process the list, we can just keep a hash-map of (target element, attribute) pairs to actual animated values, and as we process each animation element we can directly update the animated value in the hash-map. Then we don't have to build lists of animation functions and sort them. (Does that make sense?) > Thanks again roc! I hope that helps clarify *why* the code isn't as simple as > it could be. I don't mean to dissuade anyone from simplifying it, but just > bear in mind the work that is yet to be done on SMIL. We should keep that in mind, but not too much. If we have to err, we should err on the side of being simpler now. Reviewing for simplicity is hard for everyone. Some simplifications are hard to see without understanding the code and the spec inside and out, which is very difficult. I may feel there's a simpler way to do things, but it's hard to prove without just writing the code myself. So it's unsatisfying for everyone. But we have to deal with it.
Thanks roc! What you've said makes a lot of sense. (In reply to comment #100) > There is a significant risk of "premature generalization". Yes, ok, I agree. (By way of explanation, I normally follow the approach of keeping things as simple as possible but I followed a different pattern here because the design is based on Patrick Schmitz's implementation experience.) > But we have to recognize that separating classes usually imposes complexity > costs of its own. Good comments to separate the different kinds of > functionality supported in a class go a long way to providing the separation of > concerns you want, while not adding complexity, LOC and runtime overhead. Good call. If it hasn't already been done, each of the classes is summarised in my report (http://brian.sol1.net/svg/report/report.pdf) and these descriptions could be pasted into the appropriate header files. > Yes. The most important thing is to reduce the size of the source code and the > number of "moving parts". But we do have to watch memory footprint and the > number of allocated objects. A long time ago, those got out of control in our > SVG implementation and we took a while to recover from that. Yeah, good point about the number of small allocated objects. I hadn't considered that. That could make a significant difference in performance. > At each sample step, we already have to do work proportional to the number of > animation elements (evaluate the nsSMILAnimationFunction for each one). So the > scheme I propose would increase the constant factor but not the algorithmic > complexity. > > If there's a really large number of animation elements and the cost of > resolving the target elements and attributes is significant (compared to the > cost of redrawing), then we could start optimizing in various ways. For > example: > -- store only the active animation elements in the document's list of animation > elements I'm not sure about the coupling that this would create. > -- cache the target element for each animation element if the ID-lookups get > expensive (but they probably won't!) > -- store the list of active animation elements in content order, say by using a > balanced tree structure. Then when we process the list, we can just keep a > hash-map of (target element, attribute) pairs to actual animated values, and as > we process each animation element we can directly update the animated value in > the hash-map. Then we don't have to build lists of animation functions and sort > them. (Does that make sense?) Yes, I think the caching idea is good. But like you say, it might not be necessary. I wonder if you could just use bind/unbind events to update the cached mapping. > We should keep that in mind, but not too much. If we have to err, we should err > on the side of being simpler now. Yep, fair enough. Thanks again roc.
Thanks very much to roc for the suggested changes, and thanks very much to Brian for his perspectives on those wrt the original design / architecture! (In reply to comment #98) > + bool IsNull() const > PRBool > > nsSMILCompositor::SortCompositors should be SortComposables, right? Cool, thanks for catching those -- those are both fixed in my smil-patches patch queue now. > + PRBool mSMILEnabled; > PRPackedBool and put it next to some other PRPackedBools Ok -- though actually, there aren't any other PRPackedBools inside of nsPresContext. I guess this is a moot point anyway, though, if we're getting rid of the 'svg.smil.enabled' pref altogether, per your later comment. :) I'm happy to get rid of it and just use the image.animation_mode pref for now. (In reply to comment #101) > > Good comments to separate the different kinds of > > functionality supported in a class go a long way to providing the separation of > > concerns you want, while not adding complexity, LOC and runtime overhead. > > Good call. If it hasn't already been done, each of the classes is summarised in > my report (http://brian.sol1.net/svg/report/report.pdf) and these descriptions > could be pasted into the appropriate header files. Ok - I'll update / add those comments, based on the material in the report. (In reply to roc's misc simplification suggestions) Cool, I'll look into implementing these & posting a follow-up patch ASAP.
I tried to qpush smil_base, but I got these errors: http://martijn.martijn.googlepages.com/qpush_smil_base.txt I guess I'm doing something wrong here, right? (the latest patch applies just fine, thanks for that)
Just an update -- I'm finishing up a refactored version of the patch, addressing the review comments & related irc discussions. The good news is that I think the refactored version is much cleaner and makes a lot more sense. :) But the bad news is that today's the beta freeze date, and this definitely won't be done & reviewed in time for that. So, per the discussion in today's Platform meeting, it looks like this is going to go into Firefox 3.2 / 3.next, rather than Firefox 3.1. While I would've liked to see this in 3.1, I think that it's probably better to have it wait one release -- now, this can land towards the beginning of a release cycle, and it can get more comprehensive review / real-world testing / added features before it makes it to a final release.
(In reply to comment #103) > I tried to qpush smil_base, but I got these errors: (btw, we addressed this on IRC shortly after this comment appeared -- he'd just checked out the "patches" dir to the wrong spot. It needs to be located at mozilla-central/.hg/patches)
hm - it is too bad that SMIL can't make it into FF 3.1. This means that we have to rely on FakeSMIL (which is great but not as complete and efficient as a native SMIL implementation) for another several months ... SVG fonts and SMIL are the two major SVG 1.1 features missing in FF3. This is also where the competition (Opera, Webkit) is ahead of Firefox. Please make SMIL and SVG fonts a top priority in the Firefox 3.2 release plans. Thank you for your efforts!
I don't mean to bug spam, but sometimes ago, I read there would be a 3.1 beta 2... Can't this be landed for the second beta?
(In reply to comment #107) > I don't mean to bug spam, but sometimes ago, I read there would be a 3.1 beta > 2... Can't this be landed for the second beta? No, it can't -- beta 1 is the feature freeze. A very small set of features were pre-approved to land for b2 instead of b1, and in yesterday's platform meeting we briefly discussed adding SMIL to that list. However, we decided that that wouldn't make sense, for a number of reasons. Basically, it'd benefit a lot from having more time for review / testing / implementation of additional features.
For a while now, I have been making builds available that include pending patches (including the base SMIL patch) that fix issues reported by the Acid3 test. My intention in doing this was to provide some way to provide earlier testing of these patches to uncover issues earlier. This part seems to be working. What would be even better would be if it would provide a way to say we have done an adequate amount of testing on patches such as this one to land it. I am struggling with a method of determining a way to do this. So far I have about 25 people who seem to be downloading these builds each day and testing to determine if they find anything broken. I have had some reports about things that people found different about these builds and have added comments to the relevant bugs. So far, there have been no issues at all relative to this patch. In any event, these builds are available at http://www.wg9s.com/mozilla/firefox/ These builds include only the base patch because I could not figure out which other patches in the bug were required. But, It would be nice to determine if these builds cause any regressions. That was kind of the entire idea of me making them available before the patches actually landed. Sorry if you consider thsis SPAM. I am just trying to figure out a way that I can help here.
(In reply to comment #109) > These builds include only the base patch because I could not figure out which > other patches in the bug were required. Cool -- I see the latest one includes "main smil patch v3a", which should be all you need (as long as you've got "ac_add_options --enable-smil" in your mozconfig).
Attachment #339235 - Flags: superreview?(roc)
Attachment #339235 - Flags: review?(tor)
(In reply to comment #110) > (In reply to comment #109) > > These builds include only the base patch because I could not figure out which > > other patches in the bug were required. > > Cool -- I see the latest one includes "main smil patch v3a", which should be > all you need (as long as you've got "ac_add_options --enable-smil" in your > mozconfig). I do. These builds get 97/100 out of acid3 because they have all the pending acid3 related patches included, and build with --enable-smil. What I don't have nay idea of doing is figuring out how to determine how many people are doing enough significant testing against these to improve the chances of getting the patch landed at all. I try to get new builds each day, but can't always do that because of work constraints. I also do not have access to a MAC so can not do any MAC builds.
(In reply to comment #111) > What I don't have nay idea of doing is figuring out how to determine how many > people are doing enough significant testing against these to improve the > chances of getting the patch landed at all. I wouldn't worry about that so much at this point, now that landing has been postponed to post-3.1. The plan now is to get the patch cleaned up a bit, add a bunch of automated tests to ensure correct functionality (which I think Martijn is working on), reviewed, and then landed post-3.1. At that point, people will be able to test it in nightly builds. (or in unofficial builds, if we don't initially --enable-smil by default)
Actually I think there's still a good case to land this on trunk soon, but leave it disabled until after 3.1 is done. That would make it easier for people to test and contribute.
Well, in any event, I intend to continue to produce my builds. I just have no real way to quantify how much testing this of this patch that actually provides.
(In reply to comment #115) > dholbert, any chance you'll do a tryserver build of what Bill's doing, for Mac > users? Links to the other patches are: I did that: <http://hg.mozilla.org/try/rev/f0b3fc7de4eb> <http://hg.mozilla.org/try/rev/b420224ce878> <http://hg.mozilla.org/try/rev/1e9d26acfc8d> <http://hg.mozilla.org/try/rev/f81881f9385f> <http://hg.mozilla.org/try/rev/1297979b6a2c> Some of the patches were bit-rotted, so I hope that I've corrected them in the right way. Watch here <https://build.mozilla.org/tryserver-builds/?C=M;O=D> for the builds when they are available.
Try server builds should appear in <https://build.mozilla.org/tryserver-builds/2008-10-02_00:11-ehsan.akhgari@gmail.com-try-1297979b6a2/>. The Mac version is already there.
Ehsan, those builds score 95/100 passing the tests for all the patches except this SMIL one :( Was it to do with what was mentioned in comment 90 or the need to build with --enable-smil?
(In reply to comment #113) > Actually I think there's still a good case to land this on trunk soon, but > leave it disabled until after 3.1 is done. In my opinion, it would be a great idea! ;-) This has already happened for TraceMonkey [1] - which is still fairly unstable code as far as I was able to check... > That would make it easier for people to test and contribute. Yes, fully agreed. I'd say that, generally, being able to test a feature by simply changing a preference would surely increase changes of the community investing time in it. In my particular case, the overhead of downloading and maintaining (even) more browser builds, added to the panoply of browser versions I already have, makes SMIL testing somehow unattractive... Also, shipping support (even if disabled by default through a preference after 3.1) will start allowing usage in controlled environments (Intranets, Internet users instructed to consciously change their configuration, etc.). This would then (potentially) increase demand for SMIL content which, it turn, would overall help increasing deployment of SVG content. :-) [1] https://bugzilla.mozilla.org/show_bug.cgi?id=landtm
(In reply to comment #113) yes, I think this would be a good compromise for FF 3.1. It would allow easier testing. If you decide to go this route, please announce it at www.svg.org and the svg-developers@yahoogroups.com so that the SVG developers are aware of the option. This would definitely increase the amount of testers. > Actually I think there's still a good case to land this on trunk soon, but > leave it disabled until after 3.1 is done. That would make it easier for people > to test and contribute.
(In reply to comment #118) > Ehsan, those builds score 95/100 passing the tests for all the patches except > this SMIL one :( > Was it to do with what was mentioned in comment 90 or the need to build with > --enable-smil? I tried to enable building SMIL by default in this revision: <http://hg.mozilla.org/try/rev/b786c2a8726f> but the try server build produced <https://build.mozilla.org/tryserver-builds/2008-10-02_03:19-ehsan.akhgari@gmail.com-try-b786c2a8726/> still doesn't seem to have SMIL enabled. It scores 95/100, and neither of the SMIL tests at <http://brian.sol1.net/svg/tests.php> seem to pass. Dan: what am I missing? Is there any way to produce try server builds including the SMIL patch and enabling SMIL by default?
ehsan: Thanks very much for your efforts here! To answer your question, I don't know too much about our config system, but this "force_smil" patch worked for me the last time I tried it. (It's what I've used in generating try-server builds in the past) http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/f2febd04ac60/force_smil On the surface, it doesn't look much different from your patch, but I think it's worth a shot. (I think I tried something like your attempted smil-enabled-by-default patch in the past, and it didn't reliably work until I added an unconditional MOZ_SMIL=1 as in the "force_smil" patch I linked above) Would you mind giving the multiple-acid3-patches build one more shot, using that "force_smil" patch? (FWIW, here's a link to the last try-server build I generated, which I'm pretty sure just used patch v3a + force_smil. I just re-verified that the acid3 tests pass in that one. https://build.mozilla.org/tryserver-builds/2008-09-22_12:01-dholbert@mozilla.com-try-3dac82ae197/ )
OK, your patch did the trick! Try server builds including SMIL support and the rest of the ACID3 patches not landed yet are available at <https://build.mozilla.org/tryserver-builds/2008-10-03_00:03-ehsan.akhgari@gmail.com-try-836bb85189e/>. I'm not sure if this is related to this bug, but the Talos tests for Mac resulted in a crash: <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1223020852.1223042702.11336.gz>. Might be worth investigating.
(In reply to comment #123) > I'm not sure if this is related to this bug, but the Talos tests for Mac > resulted in a crash: > <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1223020852.1223042702.11336.gz>. > Might be worth investigating. Talos crashing is almost certainly bug 458440
Browsing through the SMIL planning [1] wiki section I just stumbled across the cleanup section [2]. Although it seems rather outdated documentation and most of the items (even some not marked with [DONE]) seem to already have been addressed, it sounded like a good idea to ping for that particular section [2] status, as most (all?) of the items referenced seem pretty relevant for a world-class implementation. :-) Sorry if this may sound like bug-spam, but I'm not yet familiar with the code to take my own conclusions (regarding both the status of the planned clean up nor the SMIL implementation in general). :-| [1] https://wiki.mozilla.org/SVG:Home_Page#Planning_for_SMIL_Animation [2] https://wiki.mozilla.org/SVG:Cleanup
(In reply to comment #96) > (From update of attachment 339235 [details] [diff] [review]) This patch has bitrotted and no longer applies without errors.
Attached patch patch v4a (obsolete) — — Splinter Review
Here's an updated version of the patch. It's got all the unguarded patches from my patch queue as of now (revision 63e7e68e652b). This has bitrot fixed & has some restructuring done, & it applies cleanly to mozilla-central.
Attachment #339235 - Attachment is obsolete: true
From http://blog.mozilla.com/bhearsum/archives/50 "* mozilla-central will NOT be used for trunk development until after we tag for Firefox 3.1b2" which should show up at https://wiki.mozilla.org/Releases/Firefox_3.1b2#Release_Tracking_.26_Schedule " Once that happens we will bump mozilla-central versions to 1.9.2a1pre/3.2a1pre" which is the green light for all systems go on this bug! :) Daniel, do you intend to do a tryserver build before 3.1b2 is tagged? wgianopoulos has updated builds with the v4a patch at http://www.wg9s.com/mozilla/firefox/
Just pushed one now -- builds should show up at https://build.mozilla.org/tryserver-builds/?C=M;O=D in the next couple of hours.
the latest builds fail on test http://brian.sol1.net/svg/tests/keysplines.html - instead of repainting the circle in the rectangle, the circle is drawing kind of path of artifacts after last position of the circle. (not sure how to explain that). MacOS 10.5
> - instead of repainting the circle in the rectangle, the circle is drawing kind > of path of artifacts after last position of the circle. Thanks for spotting that -- I see it too, on Linux. It seems to be some sort of invalidation issue; if I change focus away from Firefox (or back to firefox), the existing artifacts are cleared.
(In reply to comment #130) > - instead of repainting the circle in the rectangle, the circle is drawing kind > of path of artifacts after last position of the circle. (not sure how to > explain that). Also seeing this in nightly builds (Windows), even without SMIL enabled - there seem to be general repaint issues which unfortunately I haven't been able to track down and report yet... :-| Version details: OS - Windows XP SP3 Fx - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081120 Minefield/3.1b2pre
Helder: where do you see this without the patch enabled?
(In reply to comment #132) > Also seeing this in nightly builds (Windows), even without SMIL enabled I reproduced the exact same problem using a normal nightly build with the FakeSmile Greasemonkey userscript, so I think the blame lies elsewhere (somewhere in mozilla-central). I filed bug 465996 on this issue -- let's direct any further discussion on these visual artifacts to there.
I just tested the patch with some mixed markup: http://www.maths-informatique-jeux.com/international/mathml_with_other_standards/mathml_svg.xml ... and discovered that values of rotation angle are misinterpreted when they are greater than 180°. See attachment.
Yeah, that's a known issue -- there's a XXXdholbert comment about it in nsSVGTransformSMILAttr.cpp in the patch.
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Attached patch patch v4b (obsolete) — — Splinter Review
Attachment #348839 - Attachment is obsolete: true
Comment on attachment 350900 [details] [diff] [review] patch v4b Requesting review on patch v4b. It has a few outstanding TODO's (most of which are marked with XXXdholbert comments), but I think it'd be ready to land disabled-by-default.
Attachment #350900 - Flags: superreview?(roc)
Attachment #350900 - Flags: review?(birtles)
Looks good Daniel! I have a few questions and some comments. smil4b.patch:173:nsIContent.h: +#ifdef MOZ_SMIL + /* + * Returns a nsISMILAttr that allows the caller to animate the given + * attribute on this element. The 'aIsCSS' parameter indicates whether the + * requested attribute is a CSS property or just a standard XML attribute. + * + * XXXdholbert aIsCSS needs to be more generic + */ + virtual nsISMILAttr* GetAnimatedAttr(const nsIAtom* aName, + PRBool aIsCSS) = 0; +#endif // MOZ_SMIL (brb-1) This is only used in nsSMILCompositor::ComposeSample. I wonder if it's necessary to put this in nsIContent? (brb-2) Also, is this ok to return newly allocated memory like this? Shouldn't we at least document that that's what we're doing? (brb-3) I'm also wondering about the number of allocations this requires. Previously the compositor used to store a ref to the attribute. Now it stores a ref to the element and requests this attribute at every sample meaning a lot of little nsISMILAttr objects are created and destroyed. Is this right? I think it's reasonable to expect documents with 100 or more little circles (think bubbles or balloons) moving around so that seems like a lot of attributes being animated and a lot of little allocations for every frame. What do you think? I think it would be preferable if we could keep the attributes in memory rather than returning new ones every time. Another cost of destroying these attr objects all the time seems to be the extra complexity now in passing around opaque pointers to store extra state such as the transform type used when parsing. Can we do something simpler and safer? I understand however that creating and destroying these attributes at every sample simplifies lifecycles and ownership, particularly within implementations of nsISMILAttr. Regarding aIsCSS see brb-37. smil4b.patch:228:nsIDocument.h: +#ifdef MOZ_SMIL + // Adds a new animation element to be tracked in this document. + virtual void AddToAnimationSpecSet(nsISMILAnimationSpec* aAnimationSpec) = 0; + virtual void RemoveFromAnimationSpecSet(nsISMILAnimationSpec* aAnimationSpec) = 0; + + typedef PLDHashOperator(*PR_CALLBACK nsAnimationSpecEnumFunc) + (nsVoidPtrHashKey* aKey, void* aData); + virtual PRUint32 EnumerateAnimationSpecs(nsAnimationSpecEnumFunc aFunc, + void *aData) = 0; +#endif // MOZ_SMIL (brb-4) Should this be AddAnimationSpec and RemoveAnimationSpec? Later on we have EnumerateAnimationSpecs rather than EnumerateAnimationSpecSet. smil4b.patch:371:nsDocument.h: +#ifdef MOZ_SMIL + nsTHashtable<nsVoidPtrHashKey> mAnimationSpecSet; +#endif // MOZ_SMIL (brb-5) Just curious, is there any way to make this type-safe? So we don't do casting when we use these? smil4b.patch:723:nsISMILAnimationSpec.h: +////////////////////////////////////////////////////////////////////////////// +// nsISMILAnimationSpec: This is an interface used to request specific +// information about an animation of one attribute on one element. (brb-6) I think I don't quite understand this interface. Without the by, from, to, parts it makes sense as a kind of high-level management interface for tracking animations in a document, but the by, from, to getters seem out of place to me. (brb-7) Also, I'm not sure about the name. Is this interface supposed to cover SMIL content and CSS transitions? If so should we drop the SMIL? Or is that ok because everyone knows that the implementation is SMIL-based anyway. Spec is better than wrapper but I'm still not sure about spec either. I even wonder if just 'animation' would be ok, i.e. 'nsISMILAnimation' as this is the highest-level representation of an animation isn't it? smil4b.patch:761:nsISMILAnimationSpec.h: + /* + * Returns the name of the target (animated) attribute or property. + * + * @param aResult outparam -- will be set to the name of the target + * attribute or property. + * + * @returns PR_TRUE on success, or PR_FALSE if the animation doesn't have a + * specified target attribute/property. + */ + virtual PRBool GetTargetAttributeNameStr(nsAString& aResult) const = 0; (brb-8) Why would this ever be false? Is it possible for CSS transitions for this to be false? For SMIL/SVG it's a required attribute so if it's not there it's not valid. Would we return an empty string in that case? smil4b.patch:771:nsISMILAnimationSpec.h: + /* + * Returns the type of the target (animated) attribute or property. + * In SVG Animation, the valid return values are "XML", "CSS", and "auto". + * + * @param aResult outparam -- will be set to the type of the target + * attribute or property. + * + * @returns PR_TRUE on success, or PR_FALSE if the animation doesn't specify + * a type for its target attribute/property. + */ + virtual PRBool GetTargetAttributeTypeStr(nsAString& aResult) const = 0; (brb-9) Since the default for this attribute is "auto", should we return that instead of false? I think this should this return an enum instead of a string. smil4b.patch:771:nsISMILAnimationSpec.h: + /* + * Returns a string representation of the animation's "by" attribute. + * + * @param aResult outparam -- will be set to the animation's "by" attribute. + * + * @returns PR_TRUE on success, or PR_FALSE if the animation doesn't specify + * a "by" attribute. + */ + virtual PRBool GetByStr(nsAString& aResult) const = 0; See brb-6. I'm not sure that this belongs here. It seems like we could get this from the animation element or the animation function and that would be more fitting too if this interface is supposed to be used for CSS transitions too (which don't have by, from, to etc. as far as I know). Likewise for GetFromStr, GetToStr, GetValuesStr smil4b.patch:983:nsISMILTimeContainer.h: +/** + * A SMIL time container. + * + * When implementing SMIL 2.0 time containers, this interface will most likely + * inherit from nsISMILTimedElement, amongst other changes + */ +class nsISMILTimeContainer : public nsISupports (brb-10) This interface is implemented by nsTimedDocumentRoot. It could possibly be removed and the animation controller could deal with TimedDocumentRoots instead. At the same time the AnimationRegistry could be folded in. smil4b.patch:1122:nsISMILTimedElement.h: +class nsISMILTimedElement : public nsISupports +{ (brb-11) I think this interface can be removed as well. It's only implemented by nsSMILTimedElement as far as I know. (But please copy the documentation across to nsSMILTimedElement). smil4b.patch:1472:nsSMILAnimationController.h: + // XXXdholbert No other classes call this method right now -- should it + // really be public? + nsresult Reset(); It doesn't have to be but I don't think there's any harm in leaving it public. It's conceivable that this may be useful elsewhere and it should be safe to do so. smil4b.patch:1501:nsSMILAnimationController.h: + // XXXdholbert Why does this need weak references? + nsCOMArray<nsIWeakReference> mTimeContainers; There was potential cyclic ownership between the animation controller, animation registry and timed document root (via time container interface) but this is quite possibly no longer the case. smil4b.patch:1949:nsSMILAnimationRegistry.h: +// XXXdholbert Possibly remove this class, and have SVG elements +// talk directly to the TimedDocumentRoot instead? (This is mostly +// just a wrapper for TimedDocumentRoot, at this point) Ok, yes, that sounds reasonable (see brb-10). Previously this class also contained all the compositors but that appears to be no longer the case so it can be merged with TimedDocumentRoot. smil4b.patch:2541:nsSMILTimedDocumentRoot.h: + PRBool mNeedsSampleDuringPause; (brb-12) Just curious, is this so when we're paused the screen shows the state at the instant pause was requested? I guess that makes sense although could it lead to a lag effect if there's a subsequent update after pause is fired? Anyway, I'm just curious. smil4b.patch:2825:nsSMILAnimationController.cpp: +#define CSS_TYPE_LABEL NS_LITERAL_STRING("CSS") See brb-37. I think we should use an enum for CSS | XML | auto smil4b.patch:3074:nsSMILAnimationController.cpp: +void +nsSMILAnimationController::FireForceSampleEvent() +{ + if (!mIsForceSampleEventQueued) { + nsCOMPtr<nsIRunnable> event = new nsForceSampleEvent(this); (brb-13) This is very minor but would it be possible to pass ourselves by ref to the nsForceSampleEvent ctor so we don't have to do the null-pointer check there (although we'd still store a pointer). smil4b.patch:3087:nsSMILAnimationController.cpp: +//---------------------------------------------------------------------- +// nsIDOMPageTransitionListener methods (brb-14) Is this accurate? It looks like we don't implement nsIDOMPageTransitionListener anymore. So should this be nsIDOMEventListener? smil4b.patch:3171:nsSMILAnimationController.cpp: +AddAnimationToCompositorTable(nsVoidPtrHashKey *aKey, void *aData) +{ + // STEP 0: Cast args (key & data) to correct types. + // The animation spec (specifies a particular animation on an elem & attr) (brb-15) I wonder if the STEP 0, 1, 2 thing suggests this method should be broken into smaller pieces? There are three or four places like this. See also brb-18 and brb-24. smil4b.patch:3175:nsSMILAnimationController.cpp:AddAnimationToCompositorTable: + nsISMILAnimationSpec *animSpec = + static_cast<nsISMILAnimationSpec*>(const_cast<void*>(aKey->GetKey())); + NS_ASSERTION(animSpec, "Null content pointer in animation spec set"); ... + animSpec->GetTargetElement(getter_AddRefs(targetElem)); (brb-16) Sometimes after a NULL pointer assertion we seem to test for NULL anyway and bail, but here we just go ahead and dereference. Not sure which way is preferable--or is it that we are pretty confident we'll never get a null anim spec here? smil4b.patch:3202:nsSMILAnimationController.cpp:AddAnimationToCompositorTable: + // XXXdholbert The variable "PRBool isCSS" actually should be an enum, which + // could be {css, xml, auto} {more...?} Yes, I think so. See brb-37. smil4b.patch:3209:nsSMILAnimationController.cpp:AddAnimationToCompositorTable: + // STEP 3: Create compositor for elem & attrib, & insert it into table + // (if it's not already there) + nsSMILCompositor *probe = new nsSMILCompositor(targetElem, attributeName, isCSS); + nsHashableHashKey* result = compositorTable->PutEntry(probe); + nsSMILCompositor* entry = static_cast<nsSMILCompositor*>(result->GetKey()); + if (entry != probe) { + // The key was already in the hashtable. + delete probe; + } // else, probe is now stored in the hashtable. + // XXXdholbert make sure we delete it later on. (brb-17) Is there a safer way of ensuring this is deleted? smil4b.patch:3255:nsSMILAnimationController.cpp:SampleChildren: + // STEP 2: Create & populate a table of compositors for this sample, using + // the document's list of animation elements. (brb-18) Can we separate this out? This method (nsSMILAnimationController::SampleChildren) seems long enough already. See brb-15 and brb-24. smil4b.patch:3283:nsSMILAnimationController.cpp:SampleChildren: + // STEP 4: Do composition & animation on currently-animated elements/attrs. + rv = nsSMILCompositor::ComposeHashtableEntries(currentCompositorTable); (brb-19) This call is from nsSMILAnimationController (timing model) to nsSMILCompositor (animation model). I wonder if there's any way to preserve this architectural distinction? Maybe it's ok here? smil4b.patch:3486:nsSMILAnimationFunction.cpp:ReparseSMILValue: + switch(aBitfieldIndex) { + case BF_BY: + gotString = mAnimationSpec->GetByStr(resultStr); (brb-20) I don't think the animation spec is the place to store these attribute values. If anywhere, I think we should store the strings here (in nsSMILAnimationFunction), or otherwise ask the DOM element. smil4b.patch:3839:nsSMILAnimationFunction.cpp:InterpolateResult: +#ifdef ALLOW_BAD_KEYTIMES + if (GET_FLAG(mSetFlags, BF_KEY_TIMES)) { ... + } +#endif (brb-21) I think we can just get rid of this ALLOW_BAD_KEYTIMES define now and always allow bad keytimes. See: http://brian.sol1.net/svg/range-for-keytimes/ for an explanation. smil4b.patch:3868:nsSMILAnimationFunction.cpp:InterpolateResult: + // XXXdholbert -- maybe from and to should be pointers or references, + // so we don't have to do memory copying during the "from = [nsSMILValue]" + // assignment? (esp. for complex types with storage in mU.mPtr) (brb-22) See my comment in nsSMILValue's assignment operator that suggests adding a Swap method that could be used here to alleviate extra allocations. smil4b.patch:3952:nsSMILAnimationFunction.cpp:ComputePacedPosition: + // We found the right spot -- an interpolated position between + // values i and i+1. + aFrom = mValues[i]; + aTo = mValues[i+1]; + aIntervalProgress = remainingDist / curIntervalDist; (brb-23) Couldn't curIntervalDistance potentially be 0 here? smil4b.patch:4679:nsSMILCompositor.cpp: +void +nsSMILCompositor::ComposeSample() +{ + // FIRST: Sort the animationFunctions, to prepare for compositing. + nsSMILAnimationFunction::Comparator comparator; + mAnimationFunctions.Sort(comparator); (brb-24) The first, second, third thing suggests the method should be broken into smaller piece unless there's a performance reason for not doing so. Also, elsewhere instead of FIRST, SECOND, etc. we have STEP 1, STEP 2. See brb-15 and brb-18. smil4b.patch:4712:nsSMILCompositor.cpp:ComposeSample: + if (curAnimFunc) { + // XXXdholbert we need to add another function + // nsSMILAnimationFunction::HasChangedTarget(elem, smilAttr, isCSS) that + // we call here (in addition to HasChanged(), because even if function + // value hasn't changed, its target might have. + // For this to work, the nsSMILAnimationFunction needs to cache its last + // elem/smilAttr/isCSS values, and then check them against the new values + // here. + /* + if (!changed && curAnimFunc->HasChanged()) { + changed = PR_TRUE; + } + */ (brb-25) Is there any need to leave this disabled until HasChangedTarget is implemented? (brb-26) Also, when you say "its target might have [changed]" do you mean (a) that this function might now target a different attribute or (b) that the same target attribute might have been changed by something else? If it's (a) then fair enough. That used to be taken care of by SetTargetAttribute but I guess the approach to that has changed. If it's (b) then then I think it's ok that a targeted attribute is changed by script and not overwritten by an animation that otherwise would not be making a visible change. smil4b.patch:4737:nsSMILCompositor.cpp:ComposeSample: + // if (!changed) // XXXdholbert removing until we have HasChangedTarget + // return; As with brb-25. smil4b.patch:4750:nsSMILCompositor.cpp:ComposeSample: + // FOURTH: Set the animated value to the final composited result. + // XXXdholbert Check return value of SetAnimVal? (what could go wrong?) + smilAttr->SetAnimValue(resultValue); (brb-27) Maybe at least an assertion or warning? smil4b.patch:5412:nsSMILInterval.h: +class nsSMILInterval : public nsISupports +{ (brb-28) We probably don't need this to be an XPCOM object as we could explicitly disassociate with dependent instance times when we're destroyed. But perhaps this kind of simplification is best done when we come to implement syncbase timing--then we'll really know what we need. smil4b.patch:6822:nsSMILTimedDocumentRoot.cpp: +nsresult +nsSMILTimedDocumentRoot::SeekToTime(PRInt64 aSeekTo) +{ + PRInt64 now; + LL_DIV(now, PR_Now(), PR_USEC_PER_MSEC); + LL_SUB(mStartTime, now, aSeekTo); + mAccumulatedOffset = LL_Zero(); + if (mParentPaused || mContainerPaused) { + // Update mPauseStart to be now (= mStartTime + aSeekTo) + // and request a sample + mPauseStart = now; + mNeedsSampleDuringPause = PR_TRUE; + } + + ResetChildren(PR_FALSE); + return NS_OK; +} (brb-29) I think there's a potential problem here. ResetChildren clears certain instance times (namely instance times created by events and DOM calls). So suppose an animation is currently active in an interval which is defined by such instance times and then you seek the animation forward just a fraction I imagine the result would be unexpected. The interval you were animating in would be destroyed or defined by different endpoints so the animation might stop unexpectedly or jump to an unanticipated position. Could we adjust the accumulated offset instead? That might allow us to keep the currently set up instance times? I think this is the approach Schmitz prefers when he says: "note that an external mechanism determines when to sample the timegraph, and what the document time should be. This allows the timegraph to be completely abstract and independent of application (runtime, editing tool, etc.). The timegraph implementation must be stateless with respect to the sampling frequency and the time delta between samples." (http://www.ludicrum.org/plsWork/papers/BatikSMILsupport.htm#TimingClassesandInterfaces) Actually, just re-reading "SMILANIM 3.6.5 Hyperlinks and Timing" (http://www.w3.org/TR/smil-animation/#HyperlinkSemantics) we definitely can't clear these instance times. Regarding backwards seeking the spec says, "Note that resolved begin times (e.g. a begin associated with an event) are not cleared or lost by seeking to an earlier time." smil4b.patch:6876:nsSMILTimedDocumentRoot.cpp: +nsresult +nsSMILTimedDocumentRoot::HandleParentResumed() +{ + if (!mContainerPaused && mParentPaused) { + PRInt64 extraOffset; + PRInt64 now; + LL_DIV(now, PR_Now(), PR_USEC_PER_MSEC); + LL_SUB(extraOffset, now, mPauseStart); + LL_ADD(mAccumulatedOffset, extraOffset, mAccumulatedOffset); + } + mParentPaused = PR_FALSE; + return NS_OK; +} (brb-30) In Resume() we reset mNeedsSampleDuringPause to false. Is this in case we get calls to Pause() and Resume() in rapid succession such that there is no time to sample in between? From my understanding, even if that happened it wouldn't matter right? But in any case, if we reset mNeedsSampleDuringPause in Resume(), we should also reset it here right? smil4b.patch:7356:nsSMILTimedElement.cpp:BeginElementAt: + nsSMILInstanceTime *instanceTime = + new nsSMILInstanceTime(timeVal, nsnull, PR_TRUE); + NS_ENSURE_TRUE(instanceTime, NS_ERROR_OUT_OF_MEMORY); + AddInstanceTime(*instanceTime, PR_TRUE); + delete instanceTime; (brb-31) Can we put the instance time on the stack instead? It appears to be copied anyway. Likewise for EndElementAt. smil4b.patch:7982:nsSMILTimedElement.cpp:GetNextInterval: + // + // This is a little counter-intuitive but according to SMILANIM, if + // all the end's are after the begin, we _don't_ just assume an + // infinite end, it's actually a bad interval. ASV however will just + // use an infinite end. + // (brb-32) I'm pretty sure this comment is wrong. I think it should say: "This is a little counter-intuitive but according to SMILANIM, if all the ends are BEFORE the begin..." See the pseudocode at http://www.w3.org/TR/smil-animation/#Timing-BeginEnd-LC-Start smil4b.patch:9084:nsSVGAnimationElement.cpp:GetTargetElement: + } else { + // No "xlink:href" attribute --> target is my parent. + NS_IF_ADDREF(*aTarget = GetParentElement()); (brb-33) Do we need to do an NS_IF_RELEASE first on aTarget? smil4b.patch:9401:nsSVGAnimationElement.h: + // XXXdholbert should mTimedElement and mTimeControl be incorporated + // into the animationSpec object? Non-SVG animations will need access + // to them or something like them, so they shouldn't live here. (brb-34) I wonder if the animation spec object would then be in danger of becoming a blob? The original design was basically: wrap up the timing related information and processing in one object, wrap up the animation parts in another, and then give them to the animation element in the host language. There's already some overlap between animationSpec and animationFunction and animationFunction should definitely be kept separate because it is already large and represents a coherent logical unit. smil4b.patch:9619:nsSVGElement.cpp: +#ifdef MOZ_SMIL +#include "nsIDOMSVGTransformable.h" +#include "nsSVGTransformSMILAttr.h" +#endif // MOZ_SMIL (brb-35) I'm not sure if this really belongs in nsSVGElement? Particularly because this is likely to grow as we introduce more animated types. Previously it used to be in nsSVGAnimateTransformElement which seems to make more sense. Is there some way we can move this SMIL-related functionality into the animation elements? Perhaps we can look at this when we come to fix animateTransform. smil4b.patch:9672:nsSVGElement.cpp: +nsSMILAnimationController* +nsSVGElement::GetAnimationController(nsIDocument* aDocument, + PRBool aCreate /* = PR_FALSE */) +{ (brb-36) We can get rid of the aCreate parameter because we're not using it. smil4b.patch:9713:nsSVGElement.cpp: +nsISMILAttr* +nsSVGElement::GetAnimatedAttr(const nsIAtom* aName, PRBool aIsCSS) +{ + // Non-CSS: + // ------- + // Transforms: + // (Note: there is no CSS "transform" property for SVG, so this one + // is non-CSS-only.) (brb-37) I think aIsCSS should really be the enum: CSS | XML | auto and if we're requesting 'transform' with an attributeType of CSS we should fail. smil4b.patch:10536:nsSVGSetElement.cpp: +nsresult +nsSVGSetElement::Init() +{ + nsresult rv = nsSVGSetElementBase::Init(); + NS_ENSURE_SUCCESS(rv, rv); + + mAnimationSpec->GetAnimationFunction()->SetAttr(nsGkAtoms::calcMode, + NS_LITERAL_STRING("discrete"), + &rv); (brb-38) It seems like it would still be possible to set a calcMode other than 'discrete' via an attribute or the DOM. Do we need to override SetAttr to handle this (i.e. force discrete 'interpolation')? smil4b.patch:10686:nsSVGTransformSMILAttr.cpp:GetBaseValue: + // XXXdholbert Where does this get NS_RELEASE'd? + // (Add a note here indicating where, for clarity) + NS_ADDREF(raw_matrix); (brb-39) This would be good to address before we land right? smil4b.patch:12875:smil/reftest.list: +# Tests related to SVG Animation (using SMIL) +# XXXdholbert Partial list of features that still need reftests: ... +# - DOM interfaces (Section 6.2 of http://www.w3.org/TR/smil-animation/ ) Also SVG 1.1 section 5.17: http://www.w3.org/TR/SVG/struct.html#DOMInterfaces Particularly the SVGSVGElement interface. smil4b.patch:13186:anim-css-fontsize-px-1c.svg: diff --git a/layout/reftests/svg/smil/style/anim-css-fontsize-px-1c.svg b/layout/reftests/svg/smil/style/anim-css-fontsize-px-1c.svg (brb-40) While we're at it with these font-size tests we might as well add one to test animating between different units, e.g. 3em to 50px. (Also, in the future when we implement this I guess we will need to test that when we have a relative font size such as "3em" or "5ex" and the context font-size changes that the animation updates accordingly.)
Blocks: 468996
> smil4b.patch:3209:nsSMILAnimationController.cpp:AddAnimationToCompositorTable: > ... > + if (entry != probe) { > + // The key was already in the hashtable. > + delete probe; > + } // else, probe is now stored in the hashtable. > + // XXXdholbert make sure we delete it later on. > > (brb-17) Is there a safer way of ensuring this is deleted? I getting reports in nsStringStats of leaks and it seems to be coming from here (that's as far as I've traced it anyway). It would be good to fix this and confirm it fixes the leaks before landing. I can have a look tomorrow if you like.
Brian -- sorry for the delay in replying -- here's a first chunk of responses to your comments. (I'm separating my reply into two posts to make it more manageable. :)) (In reply to comment #139) > + virtual nsISMILAttr* GetAnimatedAttr(const nsIAtom* aName, > (brb-1) This is only used in nsSMILCompositor::ComposeSample. I wonder if it's > necessary to put this in nsIContent? Well, the idea here is that a generalized animation (not just SVG animations) could be targeted at *any* attribute on *any* DOM element, and only the content object itself can really give us an animatable handle on that attribute (if it wants to). AFAIK, nsIContent is the most straightforward place to put such a method, but I could be wrong. (This method also lets us keep ComposeSample relatively simple -- given the nsIContent* for the target element, we can simply request the right to animate the target attribute.) > (brb-2) Also, is this ok to return newly allocated memory like this? Shouldn't > we at least document that that's what we're doing? Mm, good point. Maybe it'd be better to pass in a reference to a to-be-populated nsISMILAttr, and then return success/failure via a PRBool, similar to how nsIContent::GetAttr works. However, that'd be tricky right now, because nsISMILAttr is just an interface without a concrete implementation. (i.e. we can't actually declare a local nsISMILAttr variable, to pass it in by reference) Using nsCOMPtrs could work around that, but then nsISMILAttr would need to inherit from nsISupports again, which it doesn't currently.... This needs a bit more thought. But you're right -- at the very least, we should document the current behavior better. > (brb-3) > Now [the compositor] stores a > ref to the element and requests this attribute at every sample meaning a lot of > little nsISMILAttr objects are created and destroyed. Is this right? Yes, that's basically right, with the clarification that the compositors *themselves* are now created & destroyed during each sample -- one compositor for each element/attribute that's being animated. (Technically, the compositors last for two samples -- they're kept around until the next sample in mLastCompositorTable so we can remove effects of retargeted animations etc, per the XXX comment at the bottom of nsSMILAnimationController.cpp) > I think it would be preferable if we could keep the attributes in > memory rather than returning new ones every time. Yeah, I agree that this would probably be preferable, though as you pointed out, the associated lifetime / ownership issues get complex. For now, I tend to think the current scheme for this is okay as a simple & working baseline, and then perhaps we can optimize from there. As roc mentioned in comment 100, we're already doing work proportional to the number of animation elements, so this doesn't make us asymptotically slower. > ... opaque pointers to store extra state > such as the transform type used when parsing. Can we do something simpler and > safer? Yeah -- maybe as a slight safety improvement, we can nix the pointer and replace it with an integer value, to represent an opaque type-dependent enum, which is less scary than a pointer... Right now the pointer is actually only being used as an integer, anyway (literally just cast to an integer) -- I just made it a generic pointer in case we needed different opaque data for other types. But now I'm pretty confident we only need it for transforms. (For any other animated attribute, we have enough information to parse & set the value, given {target element + attributeName + attributeType (CSS vs XML vs auto)}. But for animateTransform, we need the auxiliary "type" attribute as well.) > + virtual void AddToAnimationSpecSet(nsISMILAnimationSpec* aAnimationSpec) = > + virtual void RemoveFromAnimationSpecSet(nsISMILAnimationSpec* > (brb-4) Should this be AddAnimationSpec and RemoveAnimationSpec? Later on we > have EnumerateAnimationSpecs rather than EnumerateAnimationSpecSet. Sure, that makes sense -- or even just AddAnimation() / RemoveAnimation(), given (brb-7). > smil4b.patch:371:nsDocument.h: > +#ifdef MOZ_SMIL > + nsTHashtable<nsVoidPtrHashKey> mAnimationSpecSet; > +#endif // MOZ_SMIL > > (brb-5) Just curious, is there any way to make this type-safe? So we don't do > casting when we use these? Yeah, there is, and I'll change that. (when I wrote it with nsVoidPtrHashKey, I was going off of some guide or comments or somtehing that recommended using that for hash sets. But now that I search MXR I see a number of usages like nsTHashtable<ns[CustomType]> declarations that look cleaner.) > smil4b.patch:723:nsISMILAnimationSpec.h: > (brb-6) I think I don't quite understand this interface. It's intended as a layer of abstraction around the animation source (e.g. <animate>), that is available at sample-time and which can be queried for animation specifics. (e.g. for the animation's target & its from/to values.) > (brb-7) Also, I'm not sure about the name. [snip] > I even > wonder if just 'animation' would be ok, i.e. 'nsISMILAnimation' as this is the > highest-level representation of an animation isn't it? Yeah -- I agree that "nsISMILAnimation" is probably a better name for it. And I tend to like leaving SMIL in the name, for consistency and to indicate that it's backed by our SMIL code. > + virtual PRBool GetTargetAttributeNameStr(nsAString& aResult) const = 0; > (brb-8) Why would this ever be false? Mmm, good point... I made that return PRBool to mirror the behavior of nsGenericElement::GetAttr, since that's what GetTargetAttributeNameStr uses under the hood. But you're right -- we don't really need a return value -- we can just return the empty string (and thereby fail) if there's no target attribute. > + virtual PRBool GetTargetAttributeTypeStr(nsAString& aResult) const = 0; > > (brb-9) Since the default for this attribute is "auto", should we return that > instead of false? Sure. > I think this should this return an enum instead of a string. Cool - me, too. > + virtual PRBool GetByStr(nsAString& aResult) const = 0; > See brb-6. I'm not sure that this belongs here. It seems like we could get this > from the animation element or the animation function Actually, this method is to get it *from* the animation element, *for* the animation function. It's abstracted as a method on this interface, though, so it can be backed by a CSS transition or another non-SVG animation. > if this interface is supposed to be used for CSS transitions too > (which don't have by, from, to etc. as far as I know). No -- CSS Transitions *do* have a concept of from/to -- they animate _from_ one value _to_ another. They don't need by/values, but I don't see that as a big issue -- they can just have those methods fail. (return the empty string) The bottom line here is that nsSMILAnimationFunction needs *some* generic way to query its "animation source" about its specifics. > +class nsISMILTimeContainer : public nsISupports > (brb-10) This interface is implemented by nsTimedDocumentRoot. It could > possibly be removed and the animation controller could deal with > TimedDocumentRoots instead. At the same time the AnimationRegistry could be > folded in. Ok. > +class nsISMILTimedElement : public nsISupports > (brb-11) I think this interface can be removed as well. It's only implemented > by nsSMILTimedElement as far as I know. (But please copy the documentation > across to nsSMILTimedElement). Cool, ok. > + // XXXdholbert No other classes call this method right now -- should it > + // really be public? > + nsresult Reset(); > > It doesn't have to be but I don't think there's any harm in leaving it public. Fair enough. Comment removed in http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/3dcabb816bf8 > + // XXXdholbert Why does this need weak references? > + nsCOMArray<nsIWeakReference> mTimeContainers; > There was potential cyclic ownership between the animation controller, > animation registry and timed document root (via time container interface) but > this is quite possibly no longer the case. Ok - we should look into that. > smil4b.patch:1949:nsSMILAnimationRegistry.h: > +// XXXdholbert Possibly remove this class > Ok, yes, that sounds reasonable (see brb-10) Cool. > smil4b.patch:2541:nsSMILTimedDocumentRoot.h: > + PRBool mNeedsSampleDuringPause; > > (brb-12) Just curious, is this so when we're paused the screen shows the state > at the instant pause was requested? I guess that makes sense although could it > lead to a lag effect if there's a subsequent update after pause is fired? > Anyway, I'm just curious. This is primarily to fix our behavior in situations like this: svg.setCurrentTime(5000); svg.pauseAnimations(); or this: svg.pauseAnimations(); svg.setCurrentTime(5000); or even this: svg.setCurrentTime(5000); setTimeout("svg.pauseAnimations()", 5); Previously, in all the above examples, the effects of the "setCurrentTime" call wouldn't be visible until after we unpause (if that ever happens). This behavior was unintuitive, didn't match other browsers' behaviors, and prevented reliable reftesting. The mNeedsSampleDuringPause flag lets us work around this by forcing one sample *after* we pause. Furthermore, this shouldn't introduce any "lag", because nsSMILTimedDocumentRoot::Pause() sets mPauseStart, and that's used as the "now" time-value while we're paused. So regardless of the amount of delay between Pause() and the next sample, the outcome should be the same -- we'll paint the sample as of the time at which we paused. > +#define CSS_TYPE_LABEL NS_LITERAL_STRING("CSS") > See brb-37. I think we should use an enum for CSS | XML | auto Yeah, me too. > smil4b.patch:3074:nsSMILAnimationController.cpp: > +void > +nsSMILAnimationController::FireForceSampleEvent() > +{ > + if (!mIsForceSampleEventQueued) { > + nsCOMPtr<nsIRunnable> event = new nsForceSampleEvent(this); > > (brb-13) This is very minor but would it be possible to pass ourselves by ref > to the nsForceSampleEvent ctor so we don't have to do the null-pointer check > there (although we'd still store a pointer). Sure, I suppose so.
(In reply to comment #141) > (I'm separating my reply into two posts to make it more > manageable. :)) Make that three posts. Here's the 2nd of those. (In reply to comment #139) > +// nsIDOMPageTransitionListener methods > (brb-14) Is this accurate? It looks like we don't implement > nsIDOMPageTransitionListener anymore. So should this be nsIDOMEventListener? Good catch -- fixed in http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1065bddbb279 > + // STEP 0: Cast args (key & data) to correct types. > (brb-15) I wonder if the STEP 0, 1, 2 thing suggests this method should be > broken into smaller pieces? There are three or four places like this. See also > brb-18 and brb-24. Possibly, but I'm not sure I'd break it down much further in most of those cases... most of the steps there are 2-3 lines each, and they'd be somewhat silly as individual functions. Mostly I was trying to be very clear with the commenting there, and perhaps I overcommented. :) > smil4b.patch:3175:nsSMILAnimationController.cpp:AddAnimationToCompositorTable: > + nsISMILAnimationSpec *animSpec = > + static_cast<nsISMILAnimationSpec*>(const_cast<void*>(aKey->GetKey())); > + NS_ASSERTION(animSpec, "Null content pointer in animation spec set"); > ... > + animSpec->GetTargetElement(getter_AddRefs(targetElem)); > > (brb-16) Sometimes after a NULL pointer assertion we seem to test for NULL > anyway and bail, but here we just go ahead and dereference. Not sure which way > is preferable--or is it that we are pretty confident we'll never get a null > anim spec here? Well, if we have a null anim spec here, that means something is very wrong... but I suppose it's wise to be graceful in that situation. Changed in: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/d2c8fb3cee2d > smil4b.patch:3209:nsSMILAnimationController.cpp:AddAnimationToCompositorTable: > + if (entry != probe) { > + // The key was already in the hashtable. > + delete probe; > + } // else, probe is now stored in the hashtable. > + // XXXdholbert make sure we delete it later on. > (brb-17) Is there a safer way of ensuring this is deleted? If we iterate through the hashtable "mLastCompositorTable" when we're done with it, & delete all the entries, we should be fine, I think. (we don't do that yet, though -- I didn't add the chunk for using mLastCompositorTable yet) > + // STEP 2: Create & populate a table of compositors for this sample, using > (brb-18) Can we separate this out? This method > (nsSMILAnimationController::SampleChildren) seems long enough already. See > brb-15 and brb-24. Sure, I guess we could. > smil4b.patch:3283:nsSMILAnimationController.cpp:SampleChildren: > + // STEP 4: Do composition & animation on currently-animated elements/attrs. > + rv = nsSMILCompositor::ComposeHashtableEntries(currentCompositorTable); > > (brb-19) This call is from nsSMILAnimationController (timing model) to > nsSMILCompositor (animation model). I wonder if there's any way to preserve > this architectural distinction? Maybe it's ok here? Well, I don't see the current behavior as differing much from the original patch in this regard -- originally, nsSMILTimedDocumentRoot (timing model) called "EndSample" on nsSMILAnimationRegistry (animation model), which called ComposeSample on each of the compositors (animation model). The current behavior is pretty much the same as that, except we call a function called "ComposeHashtableEntries" instead of "EndSample"... I don't see much of an architectural difference, though. Basically, the timing model needs *some* way to notify the animations model that it needs to do a sample. > + gotString = mAnimationSpec->GetByStr(resultStr); > (brb-20) I don't think the animation spec is the place to store these attribute > values. If anywhere, I think we should store the strings here (in > nsSMILAnimationFunction), or otherwise ask the DOM element. Agreed -- in this case, the AnimationSpec actually *does* ask the DOM element, under the hood -- it doesn't store any values. I'm just using a method on the AnimationSpec method so as to abstract the DOM element away, to support non-<animate>-backed animations. (e.g. CSS Transitions) > smil4b.patch:3839:nsSMILAnimationFunction.cpp:InterpolateResult: > +#ifdef ALLOW_BAD_KEYTIMES > (brb-21) I think we can just get rid of this ALLOW_BAD_KEYTIMES define now and > always allow bad keytimes. > See: http://brian.sol1.net/svg/range-for-keytimes/ for an explanation. Ok. Fixed in http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/9f04368ad2ec > smil4b.patch:3868:nsSMILAnimationFunction.cpp:InterpolateResult: > + // XXXdholbert -- maybe from and to should be pointers or references, > + // so we don't have to do memory copying during the "from = [nsSMILValue]" > + // assignment? (esp. for complex types with storage in mU.mPtr) > > (brb-22) See my comment in nsSMILValue's assignment operator that suggests > adding a Swap method that could be used here to alleviate extra allocations. Yeah, that would definitely help here. Even with that, though, I still think it makes sense to use (const) pointers/references here, because AFAIK we shouldn't be modifying the values of from/to there, right? I don't see any reason to copy them. > + aIntervalProgress = remainingDist / curIntervalDist; > (brb-23) Couldn't curIntervalDistance potentially be 0 here? Great question -- but no, AFAIK, it can't be zero there. We only hit that clause when remainingDist < curIntervalDistance, and so if we were there with a zero-valued curIntervalDistance, that would imply that remainingDist would have to be negative. And I don't think remainingDist can ever become negative, because it starts out as a non-negative value (by definition, as a distance), and then we chip away at it, but never subtract more than its current value off of it. I've added some assertions & comments, plus statements to clamp ComputeDistance()'s output to 0 (just in case it's evil), in this changeset: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/eb8fbd4b05f8 > + // FIRST: Sort the animationFunctions, to prepare for compositing. > > (brb-24) The first, second, third thing suggests the method should be broken > into smaller piece unless there's a performance reason for not doing so. Also, > elsewhere instead of FIRST, SECOND, etc. we have STEP 1, STEP 2. See brb-15 and > brb-18. Yeah, that method could probably be broken down a bit... Though, without the explanatory comments, it's actually pretty short. :) > smil4b.patch:4712:nsSMILCompositor.cpp:ComposeSample: > + /* > + if (!changed && curAnimFunc->HasChanged()) { > (brb-25) Is there any need to leave this disabled until HasChangedTarget is > implemented? > + // if (!changed) // XXXdholbert removing until we have HasChangedTarget > + // return; > As with brb-25. Well, the second one there *definitely* needs to stay commented out, because otherwise we might return early, assuming that we haven't changed, when in reality we've been retargeted. The first one could stay I suppose, though it's not useful right now. I basically decided to comment out all "changed" usage, because it's not a reliable optimization, until we have HasChangedTarget. > (brb-26) Also, when you say "its target might have [changed]" do you mean (a) > that this function might now target a different attribute or (b) that the same > target attribute might have been changed by something else? I mean that this function could now target a completely different attribute on a completely different element. (e.g. consider an frozen animation that uses xlink:href for targeting, and suppose we swap around IDs in the document, causing the animation to target a new element. This function would return false to HasChanged(), because its values are the same, but it should return true to HasChangedTarget().) > smil4b.patch:4750:nsSMILCompositor.cpp:ComposeSample: > + // XXXdholbert Check return value of SetAnimVal? (what could go wrong?) > + smilAttr->SetAnimValue(resultValue); > (brb-27) Maybe at least an assertion or warning? Fair enough. :) Added warning in: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/d2c044f023ee > smil4b.patch:5412:nsSMILInterval.h: > +class nsSMILInterval : public nsISupports > +{ > > (brb-28) We probably don't need this to be an XPCOM object as we could > explicitly disassociate with dependent instance times when we're destroyed. But > perhaps this kind of simplification is best done when we come to implement > syncbase timing--then we'll really know what we need. Ok. I haven't worked much with nsSMILInterval, but if it's an easy change to make it non-XPCOM (and then optionally re-add that later when we implement syncbase timing), we should do that.
Using attachment 339235 [details] [diff] [review] (which is somewhat old) I see a leak of content nodes in layout/reftests/svg/smil/anim-targethref-1.svg . (I haven't tested with the newer patch to see if it's fixed.)
(In reply to comment #142) > (In reply to comment #141) > > (I'm separating my reply into two posts to make it more > > manageable. :)) > > Make that three posts. Here's the 2nd of those. Here's the 3rd & final chunk of responses to Brian's notes in Comment 139. :) Thanks again for all the review comments! > +nsSMILTimedDocumentRoot::SeekToTime(PRInt64 aSeekTo) [snip] > + ResetChildren(PR_FALSE); [snip] > (brb-29) I think there's a potential problem here. ResetChildren clears certain > instance times (namely instance times created by events and DOM calls). [snip] > Could we adjust the accumulated offset instead? That might allow us to keep the > currently set up instance times? Ah, good catch -- yeah, we should definitely address this. I think adjusting the accumulated offset should work, though that would open up the possibility of negative accumulated offsets, which I don't think (?) were possible before. So, we should make sure to test those for any potential issues. > +nsSMILTimedDocumentRoot::HandleParentResumed() > (brb-30) In Resume() we reset mNeedsSampleDuringPause to false. Is that in case > we get calls to Pause() and Resume() in rapid succession such that there is no > time to sample in between? Correct. > From my understanding, even if that happened it > wouldn't matter right? Correct -- this is just for sanity; hence the comment that was there about it probably not being necessary. > But in any case, if we reset mNeedsSampleDuringPause in > Resume(), we should also reset it here right? Right, good point. I've now added that (with comments in both places, making this more clear) in http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/6cedc6c9b9e8 > smil4b.patch:7356:nsSMILTimedElement.cpp:BeginElementAt: > + nsSMILInstanceTime *instanceTime = > (brb-31) Can we put the instance time on the stack instead? It appears to be > copied anyway. Likewise for EndElementAt. Yep. It looks like you already fixed this (thanks! :)) as part of smil_decemberFixesBrian, in http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/4f69bcab9ecd > (brb-32) I'm pretty sure this comment is wrong. I think it should say: > "This is a little counter-intuitive but according to SMILANIM, if all the > ends are BEFORE the begin..." Ah, that makes more sense. :) Fixed in http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/d44136f37d49 > + // No "xlink:href" attribute --> target is my parent. > + NS_IF_ADDREF(*aTarget = GetParentElement()); > (brb-33) Do we need to do an NS_IF_RELEASE first on aTarget? Yeah -- thanks for catching that. Fixed in http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/98096bdca291 > smil4b.patch:9401:nsSVGAnimationElement.h: > (brb-34) I wonder if the animation spec object would then be in danger of > becoming a blob? [snip] Yeah -- per today's conversation in #svg, I think we'll be getting rid of nsSMILAnimationSpec anyway, and having the document store a list of SVG Animation nodes again. > smil4b.patch:9619:nsSVGElement.cpp: > +#ifdef MOZ_SMIL > +#include "nsIDOMSVGTransformable.h" > +#include "nsSVGTransformSMILAttr.h" > +#endif // MOZ_SMIL > (brb-35) I'm not sure if this really belongs in nsSVGElement? Particularly > because this is likely to grow as we introduce more animated types. Previously > it used to be in nsSVGAnimateTransformElement which seems to make more sense. I disagree -- I think this belongs on the nsSVGElement, just as the nsISMILAttr for SVG lengths is in nsSVGLength2.cpp/h. I think it makes sense to ask the *target* for a handle (the nsISMILAttr) to animate one of its attributes. Conceptually, the *target element* -- not the animation element -- owns its attributes & controls their implementations, so it makes sense that we need to ask the target element itself when we want to animate one of its attribute. IIRC, the "create a transform-nsISMILAttr" code was *able* to originally live in nsSVGAnimateTransformElement because, conveniently, we *can* animate the transform attribute without direct access to the guts of nsSVGElement. But that's not the case generally, and I don't think we should depend on that being the case. This conceptual division also lets us separate things fairly nicely during compositing, I think. For example, in nsSMILCompositor::ComposeSample(), we have the animated element & its attr name & the compositors, and we can just query the animated element for a nsISMILAttr, and then use that for all the composition & for setting the final value. > smil4b.patch:9672:nsSVGElement.cpp: > +nsSMILAnimationController* > +nsSVGElement::GetAnimationController(nsIDocument* aDocument, > + PRBool aCreate /* = PR_FALSE */) > +{ > > (brb-36) We can get rid of the aCreate parameter because we're not using it. Cool. Fixed in http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/d8eec6292f37 > smil4b.patch:9713:nsSVGElement.cpp: > +nsISMILAttr* > +nsSVGElement::GetAnimatedAttr(const nsIAtom* aName, PRBool aIsCSS) > +{ > + // Non-CSS: > + // ------- > + // Transforms: > + // (Note: there is no CSS "transform" property for SVG, so this one > + // is non-CSS-only.) > > (brb-37) I think aIsCSS should really be the enum: CSS | XML | auto and if > we're requesting 'transform' with an attributeType of CSS we should fail. Agreed -- I'll fix this shortly. > smil4b.patch:10536:nsSVGSetElement.cpp: > +nsresult > +nsSVGSetElement::Init() > +{ > + nsresult rv = nsSVGSetElementBase::Init(); > + NS_ENSURE_SUCCESS(rv, rv); > + > + mAnimationSpec->GetAnimationFunction()->SetAttr(nsGkAtoms::calcMode, > + > NS_LITERAL_STRING("discrete"), > + &rv); > > (brb-38) It seems like it would still be possible to set a calcMode other than > 'discrete' via an attribute or the DOM. Do we need to override SetAttr to > handle this (i.e. force discrete 'interpolation')? Ooh, you're right -- good catch. This probably doesn't need to happen before the initial landing, though, as it's not a security issue AFAICT -- it's just a "you can make your <set> behave like <animate> if you want to" issue. > smil4b.patch:10686:nsSVGTransformSMILAttr.cpp:GetBaseValue: > + // XXXdholbert Where does this get NS_RELEASE'd? > + // (Add a note here indicating where, for clarity) > + NS_ADDREF(raw_matrix); > > (brb-39) This would be good to address before we land right? I don't think we need to, given the following: (a) I think you're fixing (or replacing) this as part of the the animateTransform rework/fixes in bug 468996, right? (b) This leaks, but it's not scary security-wise (c) This will be landing #ifdef'd-out by default, and we'll want to fix bug 468996 (fixing this) before we turn SMIL support on by default. > smil4b.patch:12875:smil/reftest.list: > Also SVG 1.1 section 5.17: http://www.w3.org/TR/SVG/struct.html#DOMInterface [snip] > (brb-40) While we're at it with these font-size tests we might as well add one > to test animating between different units, e.g. 3em to 50px. [snip] > when we have a relative font size such as "3em" or "5ex" and the context > font-size changes that the animation updates accordingly.) Cool -- I added these as notes in reftest.list http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/b38fe9be9305
(In reply to comment #143) > Using attachment 339235 [details] [diff] [review] (which is somewhat old) I see a leak of content nodes > in layout/reftests/svg/smil/anim-targethref-1.svg . (I haven't tested with the > newer patch to see if it's fixed.) Thanks for the heads-up! I don't currently see any leak-warnings when loading that testcase in a debug build & quitting, but I'm not using any fancy profiling tools. Birtles is looking into some other leaks, which might be related. Just in case this is still a problem & isn't related to those leaks, though, I'll put this on my to-do list to verify as being fixed.
Attachment #350900 - Flags: superreview?(roc)
Attachment #350900 - Flags: review?(birtles)
This version incorporates the changes suggested by my review as well as addressing other issues raised in the process. Known issues: * There is still further optimisations and simplifications required in the use of nsSMILValue and especially in conjunction with nsSVGTransformSMILType. These are addressed in bug 468996 (implement animateTransform). * There is also further simplification possible in the lower level parts of the timing model, specifically, nsSMILInterval, nsSMILTimeValueSpec, and nsSMILTimedElement. We propose to address this more fully when implementing syncbase timing. Other outstanding issues that we would like to address in separate bugs: * merging smil/public and smil/src * brb-24 -- refactor nsSMILCompositor::ComposeAttribute (possibly no longer necessary) * brb-29 -- fix behaviour of SeekToTime() so it doesn't call Reset() * brb-38 -- lock the calcMode of <set> * Fix image.animation_mode handling -- Setting image.animation_mode="none" and then doing back&fwd should end up w/ paused animations, to match GIF & APNG behavior but currently it doesn't. * Clear animation effects on no-longer-targeted attributes (use mLastCompositorTable) -- related to this--implement nsSMILCompositor::HasChangedTarget
Attachment #353748 - Flags: review+
Attachment #353748 - Flags: superreview?(roc)
Attachment #201905 - Attachment is obsolete: true
OK, here are some partial comments. In the time available I've focused on the integration of SMIL with the rest of the universe. So far the design looks all good, my comments are mostly stylistic. +function setTimeAndSnapshot(timeInSeconds, pauseFlag) { + var svg = document.documentElement; + if (pauseFlag) { + svg.pauseAnimations(); + } Why would we ever pass false for pauseFlag? If the animation's actually still running and we don't pause it, surely the reftest will give unpredictable results. Why do we need nsIDOMSVGSetElement.idl and nsIDOMSVGAnimateTransformElement.idl and nsIDOMSVGAnimateElement.idl? + float getStartTime ( ); + float getCurrentTime ( ); + float getSimpleDuration ( ); getStartTime(); etc ... this is all over the place in your IDL files content/svg/content/src/nsSVGTransformSMILType.h: + virtual nsresult Interpolate(const nsSMILValue& aStartVal, + const nsSMILValue& aEndVal, + float aUnitDistance, + nsSMILValue& aResult); Fix indent. + + PRUint16 mTransformType; Here too. content/svg/content/src/nsSVGSVGElement.h: + virtual void UnbindFromTree(PRBool aDeep = PR_TRUE, + PRBool aNullParent = PR_TRUE); Don't put the default params in here. nsIContent sets them. +#ifdef MOZ_SMIL + PRBool WillBeOutermostSVG(nsIContent* aParent, nsIContent* aBindingParent) + const; +#endif // MOZ_SMIL What's that const hanging there for? Break the line somewhere else. And document this function. + // animation + nsAutoPtr<nsSMILTimedDocumentRoot> mTimedDocumentRoot; This is only set for the outermost SVG right? Better mention that here. Also mention that every outermost SVG has one. And move it up next to one of the other pointer fields to avoid alignment overhead. + PRBool mAnimationNeedsKickStart; PRPackedBool for all bools in structs/classes. Maybe "mStartAnimationOnBindToTree" would be a better name? content/svg/content/src/nsSVGSVGElement.cpp: + *_retval = (root) ? root->IsPaused() : PR_FALSE; root && root->IsPaused() If PR_TRUE or PR_FALSE appear as part of a ? : operator, something's wrong :-) + if (mTimedDocumentRoot && smilController) + rv = mTimedDocumentRoot->SetController(smilController); + + if (mTimedDocumentRoot && mAnimationNeedsKickStart) + mTimedDocumentRoot->Resume(); {} around all non-break/return/continue "if" statements. + if (mTimedDocumentRoot && smilController) + rv = mTimedDocumentRoot->SetController(smilController); + + if (mTimedDocumentRoot && mAnimationNeedsKickStart) + mTimedDocumentRoot->Resume(); Group these inside "if (mTimedDocumentRoot)" + nsIContent* parent = (aBindingParent) ? aBindingParent : aParent; Unnecessary parens content/svg/content/src/nsSVGLength2.h: + nsISMILAttr* ToSMILAttr(nsSVGElement* aSVGElement); Document that this returns a new object the caller must delete. + virtual nsresult ValueFromString(const nsAString& aStr, const void* aData, + nsSMILValue &aValue); + virtual nsresult GetBaseValue(nsSMILValue& aValue); + virtual nsresult SetAnimValue(const nsSMILValue& aValue); Unnecessary space after nsresult. This happens a lot and they should all be fixed. content/svg/content/src/nsSVGLength2.cpp: + if (aValue.mType == &nsSMILFloatType::sSingleton) + mVal->SetAnimValue((float)aValue.mU.mDouble, mSVGElement); {} content/smil/public/nsISMILAttr.h: +//////////////////////////////////////////////////////////////////////// +// nsISMILAttr: Interfaces for data types that can have a base and +// animated value. This comment isn't very helpful. This isn't a type. You should say that it represents a variable that can be animated by SMIL, for example, a particular attribute of a particular SVG element. You should also say something about memory management; it's not ref-counted, and at all times there is a single owner responsible for eventually freeing it. You should also mention that these objects only exist during the compositing phase of SMIL animation calculations. + * @param aData Optional parameter for passing in additional data, + * if needed. This comment isn't very helful. What's aData used for? + virtual nsresult ValueFromString(const nsAString& aStr, const void* aData, + nsSMILValue& aValue) = 0; + + /** + * Gets the underlying value of this attribute. + * + * @param aValue The value to fill. + * @return NS_OK on success or an error code if getting failed. + */ + virtual nsresult GetBaseValue(nsSMILValue& aValue) = 0; Why not just return an nsSMILValue as a direct result? For ValueFromString, document that this needs to be associated with a particular attribute (and not just a generic type) because computing values for units like ems depends on the actual element you're doing it for. I guess you need to make it clear that this isn't just parsing, we're actually resolving units to convert to some kind of primitive computed value. I didn't say that well but I hope you can phrase it better :-). content/base/public/nsIContent.h + virtual nsresult GetAnimatedAttr(const nsIAtom* aName, PRBool aIsCSS, + nsISMILAttr** aOutAttr) = 0; I think we should probably just return aOutAttr as a direct result. Just document clearly that the caller owns the object and must delete it. We should really have something like already_AddRefed that forces the caller to store the result in an nsAutoPtr. content/base/public/nsIDocument.h + virtual void SetAnimationController(nsSMILAnimationController* aController) + = 0; Horrid line breaking. +nsDocument::RegisterAnimationElement(nsISMILAnimationElement* + aAnimationElement) +nsDocument::UnregisterAnimationElement(nsISMILAnimationElement* + aAnimationElement) Ditto + nsTHashtable<nsISupportsHashKey> mAnimationElementSet; I think you're going to need some cycle collector magic here to traverse the elements in this set so that cycles involving these references don't cause leaks. content/smil/public/nsISMILAnimationElement.h +////////////////////////////////////////////////////////////////////////////// +// nsISMILAnimationElement: interface implemented by animation elements Document that an animation element is an element that controls the animation of some property of another element. + /* + * Returns the target (animated) element, by reference. + */ + virtual already_AddRefed<nsIContent> GetTargetElementContent() = 0; You shouldn't need to return already_AddRefed here. Just return a raw (not-addrefed) pointer. content/smil/public/nsISMILType.h +#ifndef __NS_ISMILTYPE_H__ There's some crazy rule about identifiers with two consecutive underscores being reserved for the compiler and platform, or something like that. So use, say, _NS_ISMILTYPE_H_. Fix that all over the place. content/svg/content/src/nsSVGElement.cpp: + return (result) ? NS_OK : NS_ERROR_FAILURE; unnecessary parens
Everywhere you use PRFloat64 you should just use 'double'. Although you should be consistent and use 'double' instead of 'float' in some places. nsISMILType needs a comment at the declaration of the class explaining what this is for. It should explain why we don't just have subclases of nsSVGValue (because that would prevent declarations of plain 'nsSVGValue' and direct assignment, thus requiring a lot of heap allocation). Is nsISMILType supposed to be used directly or should it always be used through an nsSMILValue instance? It seems like the latter, but that should be documented clearly. + virtual nsresult Repeat(nsSMILValue& aDest, + PRUint32 aCount, + const nsSMILValue* aRepeatValue = nsnull) = 0; The optional-ness of the last parameter is not used, just remove it. (And remove the optional-ness from nsSMILValue::Repeat too.) The method names in nsISMILType don't need "Value" in them. So they can just be "SetIdentity", "Destroy", "Assign". Calling the operation "Add" could be misleading. The operation need not be commutative. (We have here a monoid: http://en.wikipedia.org/wiki/Monoid ). But I guess the SMIL spec uses this terminology already, so never mind :-(. In any case, there should be documentation for nsISMILType which is quite specific. It seems there are two kinds of types: additive and non-additive. The comments should say which methods always fail for non-additive types. It should say that for additive types, we have a monoid: namely, an identity element and an associative (but not necessarily commutative) binary "add" operator; the methods for additive types can be defined in terms of the identity element and the add operator. + * Add and Interpolate should be defined in such a way that Add followed by + * Interpolate produces the same result as Interpolate followed by Add. What exactly does this mean? Define it using equations and variables. The documentation for Repeat should say that it applies the Add operator aCount times. It's a little strange really; it would be more common mathematically to make it a "Power" operator where aCount=0 would return the identity, aCount=1 would return the input value aRepeatValue, aCount=2 would return aRepeatValue + aRepeatValue, etc. Is there any reason not to do it that way? The invariants on the types of values should be more strictly defined. We kind of require that the types of values used in binary operators be equal, but we don't really. For example it would be natural to require that the types of two nsSMILValues passed to Add should be equal, but we can't do that because there are five different types of transforms that have to be mixable. It might make more sense to have only one transform nsISMILType object --- so all transform nsSMILValue mTypes would be the same pointer --- and distinguish the transform matrix types via an extra field in the allocated data for transform nsSMILValues. Actually I haven't yet figured out what those transform types are for. Where do we need to explicitly classify the transform matrices?
(In reply to comment #147) > content/smil/public/nsISMILType.h > > +#ifndef __NS_ISMILTYPE_H__ > > There's some crazy rule about identifiers with two consecutive underscores > being reserved for the compiler and platform, or something like that. So use, > say, _NS_ISMILTYPE_H_. Fix that all over the place. Both ISO C and C++ reserve any names beginning with an underscore followed by a capital letter, names beginning with two underscores (and in C++ names merely *containing* two underscores), and even names beginning with a single underscore in the global scope of names. I *think* C++ extends these restrictions to macro names (and C99 does not), but 17.4.3.1.1 paragraph 2 of ISO C++ does it at best in a very indirect and convoluted manner. Thus: * __NS_ISMILTYPE_H__ is wrong * _NS_ISMILTYPE_H_ is wrong * NS_ISMILTYPE_H__ is wrong * NS_ISMILTYPE_H_ is correct Reserved name rules are great fun, aren't they?
> OK, here are some partial comments. In the time available I've focused on the > integration of SMIL with the rest of the universe. So far the design looks all > good, my comments are mostly stylistic. Great, thanks roc! I've updated the patch now to address most of these issues. I only have a few queries which I've noted below. > +function setTimeAndSnapshot(timeInSeconds, pauseFlag) { > + var svg = document.documentElement; > + if (pauseFlag) { > + svg.pauseAnimations(); > + } > > Why would we ever pass false for pauseFlag? If the animation's actually still > running and we don't pause it, surely the reftest will give unpredictable > results. There are some situations where the animation might be running but be still (e.g. frozen animation where the animation has finished animating but the final animation effect is still applied because fill="freeze"; or a discrete animation with long steps and where we take a snapshot within one of those steps; or a test animation that is designed to have no effect during correct operation). It seems reasonable to me not to pause in such cases, because when we pause we force a sample, so by not pausing we test that samples are still being generated during the course of regular animation. At least, I guess that's the reasoning. Daniel, is that right? > Why do we need nsIDOMSVGSetElement.idl and nsIDOMSVGAnimateTransformElement.idl > and nsIDOMSVGAnimateElement.idl? Probably because I don't really understand how XPCOM and XPConnect and all that work. These are simply the interfaces specified by SVG 1.1 which for example includes specifications like: > interface SVGAnimateTransformElement : SVGAnimationElement {}; Are there any users of XPCOM that might actually ask for an SVGAnimateTransformElement interface? > + nsTHashtable<nsISupportsHashKey> mAnimationElementSet; > > I think you're going to need some cycle collector magic here to traverse the > elements in this set so that cycles involving these references don't cause > leaks. Ok, where do we get this magic from? ;) > content/smil/public/nsISMILType.h > > +#ifndef __NS_ISMILTYPE_H__ > > There's some crazy rule about identifiers with two consecutive underscores > being reserved for the compiler and platform, or something like that. So use, > say, _NS_ISMILTYPE_H_. Fix that all over the place. I'll follow Jeff's suggestion (NS_ISMILTYPE_H_) if that's ok. I note we have problems like this all over the SVG code (e.g. __NS_SVGSVGELEMENT_H__) and even nsGenericElement (nsGenericElement_h___) violates the no-double-underscore rule. For now, I've just modified the new header files that this patch adds. > In any case, there should be documentation for nsISMILType which is quite > specific. It seems there are two kinds of types: additive and non-additive. > The comments should say which methods always fail for non-additive types. > It should say that for additive types, we have a monoid: namely, an identity > element and an associative (but not necessarily commutative) binary "add" > operator; the methods for additive types can be defined in terms of the > identity element and the add operator. Yes, this is something which I'm still not sure about. I'm not sure how many categories of types there are. The SVG spec talks as if there is just additive and non-additive but there seem to be many other situations too. For example, colour is sometimes additive, sometimes not (and yet it will almost certainly be represented by the one nsISMILType). The spec also says that for some data types there might not be a notion of distance and so you can't do paced animation. Furthermore, some types are not even able to be interpolated (e.g. strings). I haven't gone through all the possible cases in SVG to develop the categories. Is it necessary to spell out the categories ? Especially at this stage where they aren't really known to us. Or can we simply say, "If the type can't be interpolated, Interpolate will fail" etc. Likewise for ComputeDistance, Add, etc.? I've tried to develop some categories but I'm really not too confident in it yet. It will take quite some digging through specs and testing various implementations to determine if path data is additive, or if color keywords are able to be interpolated. At this stage I'm just not sure. This is as far as I've come: +---------------------+---------------+-------------+------------------+ | CATEGORY: | DISCRETE | LINEAR | ADDITIVE | +---------------------+---------------+-------------+------------------+ | Example: | strings, | path data? | lengths, | | | color k/words?| | RGB color values | | | | | | | -- Assign? | X | X | X | | -- Add? | | | | | -- simple | - | X? | X | | -- w/ underlying | - | -? | X | | -- ComputeDistance? | - | - | X? | | -- Interpolate? | - | X | X | | -- Repeat? | - | X? | X | +---------------------+---------------+-------------+------------------+ > The documentation for Repeat should say that it applies the Add operator > aCount times. It's a little strange really; it would be more common > mathematically to make it a "Power" operator where aCount=0 would return > the identity, aCount=1 would return the input value aRepeatValue, > aCount=2 would return aRepeatValue + aRepeatValue, etc. Is there any > reason not to do it that way? I don't really know anything about maths, but I thought "Power" normally meant multiplication by itself? i.e. Power() with aCount=3 --> aRepeatValue * aRepeatValue * aRepeatValue but here we have: Repeat() with aCount=3 --> aRepeatValue + aRepeatValue + aRepeatValue + aRepeatValue And actually, it turns out to be a bit confusing here in that there are two types of addition. This is something I've only just discovered. It's not really obvious in the spec, but in the SVGT1.2 Test Suite a certain interpretation has been taken of SMIL3 with regards to animateTransform that regards the addition you do in accumulating repeat values differently to the addition you do in combining the result with underlying values. When we Repeat here, it's doing simple addition. In Add, we actually need an extra flag to indicate if we're doing simple addition or if we're combining with an underlying value. I've introduced this flag in the animateTransform patch. This means that it's actually possible to have a type that can be repeated but not added (except for simple addition). This needs to be documented, but in the animateTransform patch where this differentiation is made. But back to the original question, like I said, I don't really know anything about maths and so the name 'Power' doesn't make sense to me. It's really multiplication by (aCount + 1). > The invariants on the types of values should be more strictly defined. > We kind of require that the types of values used in binary operators > be equal, but we don't really. For example it would be natural to > require that the types of two nsSMILValues passed to Add should be > equal, but we can't do that because there are five different types > of transforms that have to be mixable. It might make more sense > to have only one transform nsISMILType object --- so all > transform nsSMILValue mTypes would be the same pointer --- and > distinguish the transform matrix types via an extra field in the > allocated data for transform nsSMILValues. Actually I haven't yet > figured out what those transform types are for. Where do we > need to explicitly classify the transform matrices? That's been taken care of in the animateTransform patch.
> > Why would we ever pass false for pauseFlag? If the animation's actually still > > running and we don't pause it, surely the reftest will give unpredictable > > results. > > There are some situations where the animation might be running but be still > (e.g. frozen animation where the animation has finished animating but the final > animation effect is still applied because fill="freeze"; or a discrete > animation with long steps and where we take a snapshot within one of those > steps; or a test animation that is designed to have no effect during correct > operation). > > It seems reasonable to me not to pause in such cases, because when we pause we > force a sample, so by not pausing we test that samples are still being > generated during the course of regular animation. At least, I guess that's the > reasoning. Daniel, is that right? Yep, that's it exactly. See for example the anim-discrete-* reftests (which triggers the snapshot between discrete samples of an ongoing animation) and the anim-height-done-* reftests (which triggers the snapshot at the end of a frozen animation). > > The documentation for Repeat should say that it applies the Add operator > > aCount times. It's a little strange really; it would be more common > > mathematically to make it a "Power" operator where aCount=0 would return > > the identity, aCount=1 would return the input value aRepeatValue, > > aCount=2 would return aRepeatValue + aRepeatValue, etc. Is there any > > reason not to do it that way? > > I don't really know anything about maths, but I thought "Power" normally meant > multiplication by itself? I think roc was talking about the semantics of how many times the operator is applied, not the actual operation itself. Also, FWIW, the smil-animation spec seems to match roc's suggested "Power" semantics with its definition of the "repeatCount" attribute. See the "Computing the active duration" table here... http://www.w3.org/TR/smil-animation/#ComputingActiveDur ...and note that a repeatCount of 'd' produces a time value of "d * activeDur", not (d+1)*activeDur. I think for sanity's & consistency's sake, we should match those "Power" semantics in this Repeat() operator as well. (i.e. aCount+1 should be replaced by aCount in the Repeat implementation)
(In reply to comment #152) > I think roc was talking about the semantics of how many times the operator is > applied, not the actual operation itself. Oh, so it's just a naming issue. I guess my query was not so much about whether it's 0-based or 1-based, but what the actual operation was (addition vs multiplication, multiplication vs exponentiation). > I think for sanity's & consistency's sake, we should match those "Power" > semantics in this Repeat() operator as well. (i.e. aCount+1 should be replaced > by aCount in the Repeat implementation) That's fine. I think that makes sense too, it's a bit confusing to be thinking aCount + 1 all the time and we're sure to get that wrong somewhere. So it's a naming issue. Because if you have a method called "Repeat" and you tell an object to "Repeat" itself 0 times then it should be unchanged. If you tell it to "Repeat" itself once (aCount = 1), then its value should be double. That's the reason we use aCount + 1. However, now that we've got rid of the possibility of repeating yourself (aRepeatValue can no longer be null) that no longer makes sense. So the operation is really "set the value to the simple addition of this other value aCount times". Repeat? SetToMultiple? Or we could make a Multiply method that takes an unsigned int as its operand and returns a temporary nsSMILValue object but I think this might cause an extra allocation for some types even with the return-by-value-optimisation taken into account.
(In reply to comment #151) > > Why would we ever pass false for pauseFlag? If the animation's actually still > > running and we don't pause it, surely the reftest will give unpredictable > > results. > > There are some situations where the animation might be running but be still > (e.g. frozen animation where the animation has finished animating but the final > animation effect is still applied because fill="freeze"; or a discrete > animation with long steps and where we take a snapshot within one of those > steps; or a test animation that is designed to have no effect during correct > operation). OK. > > Why do we need nsIDOMSVGSetElement.idl and nsIDOMSVGAnimateTransformElement.idl > > and nsIDOMSVGAnimateElement.idl? > > Probably because I don't really understand how XPCOM and XPConnect and all that > work. These are simply the interfaces specified by SVG 1.1 which for example > includes specifications like: > > > interface SVGAnimateTransformElement : SVGAnimationElement {}; > > Are there any users of XPCOM that might actually ask for an > SVGAnimateTransformElement interface? The only difference it could make, AFAIK, is that "elem instanceof SVGSetElement" will succeed if we define that (empty) interface, but it will fail otherwise. I don't know if that's required by the spec, but I guess it's a good enough reason to leave them in. > > + nsTHashtable<nsISupportsHashKey> mAnimationElementSet; > > > > I think you're going to need some cycle collector magic here to traverse the > > elements in this set so that cycles involving these references don't cause > > leaks. > > Ok, where do we get this magic from? ;) Look at the other cycle-collection related stuff in nsDocument. > Is it necessary to spell out the categories ? Especially at this stage where > they aren't really known to us. Or can we simply say, "If the type can't be > interpolated, Interpolate will fail" etc. Likewise for ComputeDistance, Add, > etc.? I think the latter is OK. However, I hope we can at least promise (and deliver!) that, for a given type, a method will either *always* fail or *always* succeed (apart from OOM errors) --- i.e. success or failure never depends on the particular values type. And then in each type we should clearly document which methods are supported. > And actually, it turns out to be a bit confusing here in that there are two > types of addition. This is something I've only just discovered. It's not > really obvious in the spec, but in the SVGT1.2 Test Suite a certain > interpretation has been taken of SMIL3 with regards to animateTransform that > regards the addition you do in accumulating repeat values differently to the > addition you do in combining the result with underlying values. Ugh, that sounds horrible. > When we Repeat here, it's doing simple addition. In Add, we actually need an > extra flag to indicate if we're doing simple addition or if we're combining > with an underlying value. I've introduced this flag in the animateTransform > patch. Rather than introducing a flag, we should simply treat it as a new operator. If we need it to be available as a binary operator, let's create a new method for that. The default implementation can just delegate to Add. > But back to the original question, like I said, I don't really know anything > about maths and so the name 'Power' doesn't make sense to me. It's really > multiplication by (aCount + 1). Yeah. We can carry on calling it Repeat, but we should make the index zero-based and make it clear that Repeat(V,N) combines N copies of V using the RepeatAdd operator (which is usually but not always the same as Add). The important part is that it's N copies of V and N-1 applications of the operator, not N applications of the operator. I think returning a temporary nsSMILValue here should be fine. > > The invariants on the types of values should be more strictly defined. > > We kind of require that the types of values used in binary operators > > be equal, but we don't really. For example it would be natural to > > require that the types of two nsSMILValues passed to Add should be > > equal, but we can't do that because there are five different types > > of transforms that have to be mixable. It might make more sense > > to have only one transform nsISMILType object --- so all > > transform nsSMILValue mTypes would be the same pointer --- and > > distinguish the transform matrix types via an extra field in the > > allocated data for transform nsSMILValues. Actually I haven't yet > > figured out what those transform types are for. Where do we > > need to explicitly classify the transform matrices? > > That's been taken care of in the animateTransform patch. Perhaps we should actually remove all transform-animation changes from this patch, then. That would reduce the size of this patch (a good thing) and avoid passing through an intermediate state where the transform mTypes are messed up.
Attached patch patch v5c (obsolete) — — Splinter Review
* Address changes regarding add and repeat behaviour * Removed <animateTransform> code from this patch (and moved to patch for bug 468996) * Migrated changes to nsSMILValue from patch for bug 468996 to this patch Remaining issues: * Add cycle-collection magic and address potential memory leaks * Whatever roc digs up in the next stage of his review
Attachment #354265 - Attachment is obsolete: true
Thanks for the fixes, Brian! I've updated the patch queue to include the changes. The only difference is that my patch queue also has a bitrot-fix for a trivial whitespace change that I split out of the smil patch last night as http://hg.mozilla.org/mozilla-central/rev/cfa7563faecb Here are a few things I noticed, while looking over the latest fixes: (In reply to comment #155) > * Removed <animateTransform> code from this patch (and moved to patch for bug > 468996) We should probably revert the changes to nsSVGAnimatedTransformList.h/cpp, and move those to the other patch too, right? (currently those are still included in the patch on this bug) > +nsSVGAnimatedTransformList::GetAnimVal(nsIDOMSVGTransformList** aAnimVal) > { > - *aAnimVal = mBaseVal; > + *aAnimVal = (mAnimVal) ? mAnimVal : mBaseVal; Unnecessary parens there. (though, per my above comment, this change probably belongs in the transform-specific patch anyway) > nsISMILType.h: + /** + * Adds two values. + * + * The count parameter facilitates repetition. + * + * By equation, + * + * aDest += aAddValue * aCount It might be worth explicitly mentioning "If aCount is 0, aDest will remain unaltered." Maybe that's obvious given the above equation, though. Also: it looks like "aAddValue" is a typo there -- I think you mean "aSrc". (Or, maybe we should rename the "aSrc" parameter to be called "aAddValue"... that seems clearer.) + * @pre (aDest.mType == aSrc.mType == this) + */ + virtual nsresult Assign(nsSMILValue& aDest, + const nsSMILValue& aSrc) const = 0; I noticed that in this latest patch, you added parens to that "@pre" statement there, but not elsewhere in the same file -- we should either add them around the other @pre statements as well, or remove the newly-added ones, for consistency. > nsSMILAnimationFunction.cpp: In the s/Repeat/Add/ change (with the 1-based / 0-based distinction), we've changed one call that previously had aCount="mRepeatIteration - 1" to use "mRepeatIteration". But another call used "mRepeatIteration" before and *still* uses "mRepeatIteration" (instead of, say, mRepeatIteration+1). Did we have an off-by-one error before, in one of those cases? Or, do we have an off-by-one error now? > nsSMILValue.h: + nsSMILValue(nsISMILType const * aType); + nsSMILValue(const nsSMILValue& aVal); This should be "const nsISMILType* aType" in the first line there, for consistency with its neighborhood. Same thing in function definition in nsSMILValue.cpp.
Attached patch patch v5d (obsolete) — — Splinter Review
* Fixed bitrot * Moved changes to nsSVGAnimatedTransformList to the animateTransform patch (bug 468996) * Addressed changes suggested in comment #156 * Added cycle collection magic and noticed that this fixes a leak when <use> and <animate> appear in the same document (yay!) The cycle collection code (see nsSMILAnimationController and nsSMILCompositor) needs review. I don't really know what I'm doing here.
Attachment #354747 - Attachment is obsolete: true
(In reply to comment #156) > Thanks for the fixes, Brian! I've updated the patch queue to include the > changes. The only difference is that my patch queue also has a bitrot-fix for > a trivial whitespace change that I split out of the smil patch last night as > http://hg.mozilla.org/mozilla-central/rev/cfa7563faecb Ok, great! Sorry, I should have checked for that. I've integrated it now into the new patch (v5d). > > nsSMILAnimationFunction.cpp: > > In the s/Repeat/Add/ change (with the 1-based / 0-based distinction), we've > changed one call that previously had aCount="mRepeatIteration - 1" to use > "mRepeatIteration". But another call used "mRepeatIteration" before and > *still* uses "mRepeatIteration" (instead of, say, mRepeatIteration+1). Did we > have an off-by-one error before, in one of those cases? Or, do we have an > off-by-one error now? I think this was right both before and after. The reason is that I also changed some of the code before the call to Add/Repeat. So in the first occurrence, before calling Add we now set "result" to "last" (then if Add fails we don't need to do anything). So we only need to Add() mRepeatIteration times, not mRepeatIteration + 1 times. Before that patch, result was initialised to "zero". So it's a composite change hence mRepeatIteration doesn't need to change here. I've run all the tests as well as some manual tests and I'm pretty sure if that was off-by-one the tests (certainly the manual ones) would have picked that up.
Attached patch patch v5e (obsolete) — — Splinter Review
* Fixed bitrot in nsSVGLength2 * Added test case for intervals that end at t=0 * s/true/PR_TRUE/ and s/false/PR_FALSE/ in nsSMILTimedElement
Attachment #354777 - Attachment is obsolete: true
OK, here are comments about your fixes to my previous comments. + if (mTimedDocumentRoot) + mTimedDocumentRoot->Resume(); {} around statement + virtual nsISMILAttr* GetAnimatedAttr(const nsIAtom* aName, + PRBool aIsCSS) = 0; Do we really need aIsCSS here? Seems to me that animated CSS properties should not need to call an element-specific API. Any element (or at least nsStyledElement) should be able to support animation of its CSS properties, and that should happen through the style system in a way that the element doesn't need to know about. In any case, it doesn't really make sense to have a boolean parameter like this, we can just introduce a new method if needed. + * @param aData Optional parameter that may contain additional context data. + * This context data is set by calling SetSMILAttrData on the + * nsSMILAnimationFunction that targets this attribute. This still isn't very helpful. If the type of the attribute depends on aData, shouldn't aData just be a field of some subclass of nsISMILAttr --- the type of an nsISMILAttr can't *change*, can it? If this is only used by the transform animation stuff, then either please explain exactly how it is used by transforms, or don't include aData stuff in this patch and introduce it in the next patch. + virtual void + SetAnimationController(nsSMILAnimationController* aController) = 0; + + // Adds a new animation element to be tracked in this document. + virtual void + RegisterAnimationElement(nsISMILAnimationElement* aAnimationElement) = 0; + virtual void + UnregisterAnimationElement(nsISMILAnimationElement* aAnimationElement) = 0; Still horrid. Try virtual void SetAnimationController( nsSMILAnimatioNController* aController) = 0; etc + * Returns the target (animated) element, by reference. "by reference" should be dropped, there is no way to return an element by value + nsresult ComputeDistance(const nsSMILValue& aTo, PRFloat64& aDistance) const; aDistance should be 'double'
+// XXXdholbert maybe this should live in the SVGSVGElement, so that the +// SVGElement is cleaner. (e.g. then SVGElement wouldn't need +// GetAnimationController) Sounds good... nsSMILAnimationController.cpp: + return (mTimer) ? NS_OK : NS_ERROR_OUT_OF_MEMORY; Unnecessary parens + receiver->AddEventListener(NS_LITERAL_STRING("pageshow"), this, + PR_FALSE); + receiver->AddEventListener(NS_LITERAL_STRING("pagehide"), this, + PR_FALSE); I think instead of using actual DOM event listeners here, which could potentially be confused by content firing its own pageshow/pagehide named events, we should hook things up directly so that the code that fires pageshow/pagehide directly notifies the animation controller if there is one. + PRBool mHidden; + PRBool mIsForceSampleEventQueued; PRPackedBool +class nsSMILAnimationController : public nsIDOMEventListener +{ Hopefully you won't have to inherit from nsIDOMEventListener. But can you please explain in a comment what this class is for and who manages it? (e.g. mentioning that there's zero or one per document). +class nsSMILAnimationFunction +{ Same here. It looks like we go to some trouble to keep cached stuff in nsSMILAnimationFunction around between samples. Is there any reason to not just strip nsSMILAnimationFunction to the bare minimum state that needs to be kept around between iterations? I think we probably need to split this into two objects: the bare minimum state that needs to be kept around between iterations, and a fleshed-out object with cached parsed attribute values that only needs to live during composition passes. + * @param aSimpleTime The sample time for this timed element expressed in + * simple time. Somewhere you should document that all your PRInt64 timestamps are in milliseconds (relative to the animation start time or whatever the SMIL technical term is). In fact, it's probably worth introducing a typedef for that. + enum nsSMILCalcMode + { + calc_linear, + calc_discrete, + calc_paced, + calc_spline + }; Enum values should be in CAPS +class nsSMILEnum +{ What is this class needed for? Do we really need to have the enum value carry around an nsSMILEnumMapping*? Don't we know where the enum is used what type of value it is and hence, statically, which nsSMILEnumMapping* we need? Maybe we need a class or typedef per SMIL enum type? + nsSMILKeySpline(const double& aX1, const double& aY1, + const double& aX2, const double& aY2); "const double&" should just be "double". + double GetSplineValue(const double& x) const; Same here. Also you need documentation explanining what all these parameter values mean. +inline PRBool +nsSMILTimeValue::IsIndefinite() const +{ + return mIndefinite; +} + +inline PRBool +nsSMILTimeValue::IsResolved() const +{ + return mResolved; +} It's easier to write these directly in the class. content/smil/public/nsSMILTimeValue.h: + * Objects of this class may be in one of three orthogonal states: + * + * 1) The time is resolved and has a millisecond value + * 2) The time is indefinite + * 3) The time in unresolved There are in fact three *non-orthogonal* states, right? Resolved, indefinite, unresolved. In other words, mIndefinite and mResolved cannot both be true. In that case, there should probably be only one state field, an enum with three values. content/smil/public/nsSMILInterval.h: + nsSMILTimeValue mBegin; + nsSMILTimeValue mEnd; Unnecessary whitespace. There's a lot of unnecessarily wide indenting in the patch. + /** + * The nsSMILTimeValueSpec that created this instance time if any. This will + * be NULL for instance times created via DOM calls etc. + */ + nsWeakPtr mCreator; Why does it need to reference its creator? This seems weird. mCreator isn't even used. Can we get rid of it? + /* + * This will only be used for for identifying the instance times associated + * with a deleting interval. We will never de-reference this pointer, but only + * use it for pointer comparisons. Therefore it's not necessary for instances + * of nsSMILInterval to be reference-counted. + */ + // nsSMILInterval *mTimebase -- NOT YET IMPLEMENTED Take this out. It sounds dodgy. When we have to implement this, we can re-add it. +class nsSMILTimeValueSpec : public nsSupportsWeakReference +{ Once again, this needs to be documented to explain what this class is for. It looks to me like we could just reparse time values when necessary, in which case we wouldn't need this class, it could just be a helper function that produces nsSMILTimeValues. + nsSMILAnimationController* mController; // owned by nsDocument + PRInt64 mStartTime; + PRInt64 mAccumulatedOffset; + nsTArray<nsSMILTimedElement*> mTimedElements; + PRPackedBool mParentPaused; + PRPackedBool mContainerPaused; + PRInt64 mPauseStart; + // Should generally update whenever mPauseStart is updated + PRPackedBool mNeedsSampleDuringPause; Reorder these fields to minimise space usage. Put the PRInt64s first, then the pointers and the array, then the PRPackedBools. +nsSVGAnimationElement::Init() +{ + nsresult rv = nsSVGAnimationElementBase::Init(); + NS_ENSURE_SUCCESS(rv, rv); + + mAnimation = new nsSMILAnimationFunction(this); + NS_ENSURE_TRUE(mAnimation, NS_ERROR_FAILURE); + + mTimedElement = NS_NewSMILTimedElement(); + NS_ENSURE_TRUE(mTimedElement, NS_ERROR_FAILURE); + + mTimedElement->SetTimeClient(mAnimation); Wouldn't it be simpler to put nsSMILTimedElement as a direct member of nsSVGAnimationElement, instead of having a pointer to a heap-allocated one? And wouldn't it be simpler for mAnimation to just be a field of mTimedElement, so there's no need to heap-allocate the nsSMILAnimationFunction, and no need to give mTimedElement a pointer to it? +class nsSMILTimedElement +{ Again, you need a comment explaining that this is reusable animation element functionality. In this class it looks like there's a lot of members that are just caching attribute values. This is not really necessary --- you can store various types --- enum values, integers, floats, atoms, strings --- directly in the nsAttrValue --- see various ParseAttribute implementations. You could do that instead of copying values into member fields; it can save us memory usage and can give you reasonably efficient access to those attribute values when you want to use them. Apart from that, like nsSMILAnimationFunction, I think we should store the bare minimum in nsSMILTimedElement between iterations and if necessary have an extra object that gets created for the duration of a composition containing more stuff. I need to sleep on this and think a bit more about what can be done to slim this class down.
This patch addresses the issues raised by roc's review. Forthcoming patch also merges smil/src and smil/public.
Attachment #350900 - Attachment is obsolete: true
Attachment #353748 - Attachment is obsolete: true
Attachment #354894 - Attachment is obsolete: true
Attachment #353748 - Flags: superreview?(roc)
This patch addresses the issue raised by roc's review. It is identical to v5f except that in this patch smil/src and smil/public have been merged into just smil (as recommended by tor in comment 76).
Attachment #355712 - Attachment is obsolete: true
Attachment #355713 - Flags: superreview?(roc)
+ // Store raw ptr to mDocument. It owns the controller, so controller + // shouldn't outlive it + nsIDocument *mDocument; But it can now, because the controller is refcounted. I think it would be best if the controller was not refcounted. Instead of having mIsForceSampleEventQueued, store an actual (strong ref) pointer to the event that you've queued. Then when the controller goes away you can have it null out the event's pointer to the controller. The event's firing can test that pointer and silently do nothing if the controller has gone away. + void SampleAt(const nsSMILTime& aSampleTime, + const nsSMILTimeValue& aSimpleDuration, + const PRUint32& aRepeatIteration); Last parameter should just be PRUint32. Also I think we should just pass nsSMILTimes by value as well. + void ComposeResult(const nsISMILAttr& aSMILAttr, nsSMILValue &aResult); Be consistent with & placing. I think we prefer "T&". + const nsSMILTime& GetBeginTime() const; Return nsSMILTimes by value too. Lots of parameters in nsSMILKeySpline need the 'a' prefix. Somewhere it would be helpful to have an essay describing what all the various time types mean and why we need them all. + // + // We need to distinguish between attempting to set the begin spec and failing + // (in which case the mBeginSpecs array will be empty) and not attempting to + // set the begin spec at all. In the first case, we should act as if the begin + // was indefinite, and in the second, we should act as if begin was 0s. + // + PRPackedBool mBeginSpecSet; + Move this down near the other PRPackedBool.
+ nsresult rv = + nsSMILParserUtils::GetClockValue(aStringSpec, + &clockTime, + true, // allow + or - + true); // allow 'indefinite' PR_TRUE + if (!mDocumentRoot) { + NS_ERROR("Attempting to end but there is no document root."); + return PR_FALSE; + } + + nsSMILTime offsetVal; + LL_D2L(offsetVal, offset); + nsSMILTime timeWithOffset; + LL_ADD(timeWithOffset, mDocumentRoot->GetDocumentTime(), offsetVal); + nsSMILTimeValue timeVal; + timeVal.SetMillis(timeWithOffset); + + nsSMILInstanceTime instanceTime(timeVal, nsnull, PR_TRUE); + AddInstanceTime(instanceTime, PR_FALSE); This chunk of code can be shared between BeginElementAt and EndElementAt. + // XXX Cache previous sample time and if this time is less then perform + // backwards seeking behaviour (see SMILANIM 3.6.5 Hyperlinks and timing) "Need to cache" + PRInt32 count = mBeginInstances.Length(); + + for (PRInt32 i = 0; i < count; ++i) { + nsSMILInstanceTime &instance = mBeginInstances[0]; + if (instance.ClearOnReset()) { + mBeginInstances.RemoveElementAt(0); + } + } This must be wrong. You're trying to remove all instances which are ClearOnReset? But you only look at the first instance. Is there a testcase which could show this bug? + /** + * Reset the element's internal state. This is useful for repeating time + * containers and so that the timing model can be cached. + * + * @param aHardReset Perform a hard reset such that all instance times are + * cleared. For a soft reset only instance times created by + * DOM calls and events are cleared. A hard reset is needed + * in the case of a cached timing model whilst for + * repeating only a soft reset is required. + */ + void Reset(PRBool aHardReset = PR_FALSE); It'd be better to define a separate HardReset method. +PRBool +nsSMILTimedElement::UnsetAttr(nsIAtom* aAttribute) +{ + PRBool rv = PR_TRUE; Don't use 'rv' for anything but an nsresult. + PRBool rv = temp.ParseEnumValue(aRestartSpec, sRestartModeTable, PR_TRUE); Same here + ? (nsSMILFillMode)temp.GetEnumValue() Constructor style cast; nsSMILFillMode(temp.GetEnumValue()) + nsRefPtr<nsSMILTimeValueSpec> spec; + SMILTimeValueSpecList& timeSpecsList = (aIsBegin) + ? mBeginSpecs + : mEndSpecs; + nsTArray<nsSMILInstanceTime>& instancesList = (aIsBegin) + ? mBeginInstances + : mEndInstances; There's too much indenting here and elsewhere in the spec. The variable names do not need to be indented. Put the expressions on one line. You also don't need to parenthesize simple booleans in ?:. For example nsTArray<nsSMILInstanceTime>& instancesList = aIsBegin ? mBeginInstances : mEndInstances; + start = end + 1; + end = aSpec.FindChar(';', start); + length = (end == -1) ? -1 : end - start; This indenting is strange. Making local variables and statements line up somehow is a waste of time, but here you're not even doing that. +{ + PRBool found = PR_FALSE; + PRInt32 count = aList.Length(); + + for (; aPosition < count && !found; ++aPosition) { + const nsSMILInstanceTime &val = aList[aPosition]; + if (val.Time().CompareTo(aBase) >= 0) { + aResult = val.Time(); + found = PR_TRUE; + } + } + + return found; +} Here the whole function body is indented by 4 instead of 2. Plus the local variables are indented for no reason. + // To get an active duration we need to: + // - multiply the repeat count by 1000 (ok 1024) + // - convert the repeat count to a 64-bit integer + // - multiply the repeat count by the simple duration + // - divide the result by 1000 (1024) + // + // This should give us 3 decimal places of precision for the repeat count + // at the cost of some range for the 64-bit int. I don't get it. Do we really care about the range for the 64-bit int? When would converting the duration to a double lose anything? All the LL_ stuff must go. Please just use regular operators. + // Despite its name, GetNextGreater actually gets the next instance time that + // is greater than _or_equal_to_ the reference time so we have to loop to make + // sure we're getting an instance time that is actually _after_ the interval + // begin time So why don't we rename GetNextGreater to GetNextGreaterOrEqual right now? + static nsresult GetClockValue(const nsAString& aSpec, + nsSMILTimeValue* aResult, + PRBool aAllowSign = false, + PRBool aAllowIndefinite = false, + PRBool aAllowMedia = false, + PRBool* aIsMedia = nsnull); These three PRBool parameters should be combined into a single PRUint32 flags word. It's easier to read callers that way, as well as being more efficient.
(In reply to comment #163) > Created an attachment (id=355713) [details] > patch v5g -- after merging smil/src and smil/public This does not compile under Windows. I get the following errors: c:/mozilla/mozilla2/content/smil/nsSMILAnimationController.cpp(315) : error C2373: 'nsSMILAnimationController::AddRef' : redefinition; different type modifiers c:\mozilla\mozilla2\content\smil\nsSMILAnimationController.h(93) : see declaration of 'nsSMILAnimationController::AddRef' c:/mozilla/mozilla2/content/smil/nsSMILAnimationController.cpp(316) : error C2373: 'nsSMILAnimationController::Release' : redefinition; different type modifiers c:\mozilla\mozilla2\content\smil\nsSMILAnimationController.h(94) : see declaration of 'nsSMILAnimationController::Release'
+ virtual nsresult Init(nsSMILValue&) const { return NS_OK; } + virtual void Destroy(nsSMILValue&) const {} + virtual nsresult Assign(nsSMILValue&, const nsSMILValue&) const; + + // The remaining methods should never be called, so although they're very + // simple they don't need to be inline. + virtual nsresult Add(nsSMILValue&, const nsSMILValue&, PRUint32) const; + virtual nsresult ComputeDistance(const nsSMILValue&, + const nsSMILValue&, + double&) const; + virtual nsresult Interpolate(const nsSMILValue&, + const nsSMILValue&, + double, + nsSMILValue&) const; Put the parameter names in here. +INCLUDES += \ + -I$(srcdir)/../base/src \ + -I$(topsrcdir)/gfx/public \ + $(NULL) I think you should add gfx to REQUIRES instead of needing this extra include here. +//////////////////////////////////////////////////////////////////////// +// nsSMILTimedDocumentRoot: Timed document root Can you add a comment explaining what this means, for the casual reader who may not be familiar with SMIL terminology? In nsSVGAnimationElement: + // nsISVGContent specializations + virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent, + nsIContent* aBindingParent, + PRBool aCompileEventHandlers); + virtual void UnbindFromTree(PRBool aDeep = PR_TRUE, + PRBool aNullParent = PR_TRUE); These are nsIContent methods, so fix the comment. Also, don't use optional parameters here, the base class handles that. + nsSMILTimedDocumentRoot* mTimedDocumentRoot; Can this be a direct member? Also, fix the overindenting of these members. + return nsnull; + } else { + // No "xlink:href" attribute --> target is my parent. + return GetParentElement(); Don't need else after return, take it out. +nsSVGAnimationElement::GetTargetAttributeName() const +{ + nsAutoString nameStr; + GetAttr(kNameSpaceID_None, nsGkAtoms::attributeName, nameStr); + + return NS_NewAtom(nameStr); If you parse attributeName using ParseAtom in ParseAttribute, then you should be able to just check here that the nsAttrValue exists, then you can call GetAtomValue on it without needing to create a string or a new atom. + nsAutoString typeStr; + GetAttr(kNameSpaceID_None, nsGkAtoms::attributeType, typeStr); + + nsCOMPtr<nsIAtom> typeAtom = do_GetAtom(typeStr); + if (typeAtom == nsGkAtoms::css) + return eSMILTargetAttrType_CSS; + if (typeAtom == nsGkAtoms::XML) + return eSMILTargetAttrType_XML; + + return eSMILTargetAttrType_auto; This can be written more cleanly and efficiently using nsIContent::FindAttrValueIn. + nsCOMPtr<nsIContent> targetContent = GetTargetElementContent(); targetContent can just be an nsIContent* here. + NS_IF_RELEASE(*aTarget); + NS_IF_ADDREF(*aTarget = targetSVG); You don't need the NS_IF_RELEASE. + (void)retval; Don't do this. We don't do it elsewhere. If you feel the need to suppress the warning, just do "*retval = 0.0f;" + if (!mTimedDocumentRoot) + // Timed document root hasn't been created yet. This will be created when + // the SVG parent is bound. + return NS_OK; Is this actually possible? I would have thought that if there's an ownerSVG element, there must be a timed document root associated with it or one of its ancestors. Either this should be an assertion? +nsSMILAnimationController::OnPageShow() +{ + if (!mHidden) + return; + + mHidden = PR_FALSE; + + Reset(); + Resume(); +} We shoudn't reset in OnPageShow, we should retain the existing animation state. But be careful here because we don't want to resume animation in a document where animation was paused via script or by the user. As we discussed, the mTimedElements array in nsSMILTimedDocumentRoot is a problem. As I mentioned in comment #98, it can lead to O(N^2) behaviour during UnbindFromTree; back then I expected it to go away. I think we can still get rid of it completely. Basically, at sample time, instead of iterating through the timed document roots and then iterating through their descendant timed elements, we can first compute the document time for each timed document root and then iterate through all animation elements asking them to sample, passing in the document time for each timed element's timed document root (or else we can store the current document time in each timed document root and the timed element can get it from there). ResetChildren could be fixed the same way, or maybe we can just get rid of it. + if (aNamespaceID == kNameSpaceID_None) { + + nsresult rv = NS_ERROR_FAILURE; Unnecessary blank line. + return PR_TRUE; + } else { + return nsSMILAnimationFunction::SetAttr(aAttribute, aValue, + aResult, aParseResult); + } Get rid of else after return. + return PR_TRUE; + } else { + return nsSMILAnimationFunction::UnsetAttr(aAttribute); + } Also here.
In nsSMILFloatValue.cpp: + aDistance = to - from; + if (aDistance < 0) + aDistance = -aDistance; Just call libc "abs". + for (int i = 0; i < kSplineTableSize; ++i) + mSampleValues[i] = CalcBezier((double)i * kSampleStepSize, mX1, mX2); {} around the statement +inline PRBool +nsSMILParserUtils::IsSpace(const char c) +{ + return (c == 0x9 || c == 0xA || c == 0xD || c == 0x20); +} + +inline PRBool +nsSMILParserUtils::IsSpace(const PRUnichar c) +{ You only need one of those overloads, surely? + while (aIter != aIterEnd && IsSpace(*aIter)) + ++aIter; {} around the statement. That's needed all over the place in this file. +nsSMILParserUtils::GetKeySplines(const nsAString& aSpec, + nsTArray<double> &aSplineArray) +{ + nsresult rv = NS_OK; + + nsCAutoString spec; + LossyCopyUTF16toASCII(aSpec, spec); Slightly better as NS_ConvertUTF16toUTF8 spec(aSpec); IMHO In nsSMILParserUtils, instead of GetKeyTimes/GetRepeatCount/etc I'd call them ParseKeyTimes/ParseRepeatCount/etc. + // No more processing required if the value was "indefinite" or "media". + if (isIndefinite || (aIsMedia && *aIsMedia)) + return NS_OK; So if aAllowMedia is true but aIsMedia is false, we carry on processing? That seems bad. Instead of aAllowMedia why not just say that "media" is allowed iff aIsMedia is non-null? Seems like we could use a lot of parser tests, both for valid values and invalid values. Mochitest might be the best way to do it. + nsSMILTime now; + LL_DIV(now, PR_Now(), PR_USEC_PER_MSEC); + + nsSMILTime currentTime = (mParentPaused || mContainerPaused) + ? mPauseStart + : now; Instead of calling PR_Now and getting different results for GetDocumentTime() each time you call it, wouldn't it be better to have nsSMILAnimationControll::Sample read PR_Now once and then pass that around to every timed document root to compute its document time? That would ensure that multiple animation trees stay in sync. (In fact, we should eventually have a global "now" value for all the documents rendering in a window, but that's compositor-land.) + (void)aWallclockSpec; Don't do this... + ResetChildren(PR_FALSE); Did you say that this (in nsSMILTimedDocumentRoot::SeekToTime) is going to go away? + if (mController) + rv = mController->AddTimedDocumentRoot(*this); {} around statement. +nsSMILTimedDocumentRoot::Resume() +{ + if (mContainerPaused && !mParentPaused) { + nsSMILTime extraOffset; + nsSMILTime now; + LL_DIV(now, PR_Now(), PR_USEC_PER_MSEC); Again, I think for sanity PR_Now should be gathered once in the animation controller and passed into the Resume() methods as a parameter. +void +nsSMILTimedDocumentRoot::HandleParentPaused() +{ + if (!mContainerPaused && !mParentPaused) { + LL_DIV(mPauseStart, PR_Now(), PR_USEC_PER_MSEC); And here. +nsSMILTimedDocumentRoot::HandleParentResumed() +{ + if (!mContainerPaused && mParentPaused) { + nsSMILTime extraOffset; + nsSMILTime now; + LL_DIV(now, PR_Now(), PR_USEC_PER_MSEC); And here. +nsSMILTimedDocumentRoot::Sample() +{ + nsSMILTime now; + if (mParentPaused || mContainerPaused) { + if (mNeedsSampleDuringPause) { + // Perform requested sample, for pause-start-time + now = mPauseStart; + mNeedsSampleDuringPause = PR_FALSE; + } else { + // Paused, and don't need a sample. Nothing to do. + return; + } + } else { + // Not paused -- need a sample at time = now. + LL_DIV(now, PR_Now(), PR_USEC_PER_MSEC); And here In nsSMILAnimationController.h: + nsTArray<nsSMILTimedDocumentRoot *> mTimedDocumentRoots; This really should be a hashset. Otherwise a document with N mTImedDocumentRoots is going to be O(N^2) to take down. + // Check if an 'auto' attributeType refers to a CSS property or XML attribute. + // Note that SMIL requires we search for CSS properties first. So if they + // overlap, 'auto' = 'CSS'. (SMILANIM 3.1) + if (attributeType == eSMILTargetAttrType_auto) { + attributeType = (targetElem->IsAttributeMapped(attributeName)) + ? eSMILTargetAttrType_CSS + : eSMILTargetAttrType_XML; I don't think this is quite right --- there are CSS properties not corresponding to mapped attributes --- but I guess it doesn't matter until we support animation of CSS... + if (entry != probe) { + // A copy of this compositor was already in the compositorTable. + // So, we don't need the probe-copy. + delete probe; Heap-allocating the probe and then deleting it if there's already one in the table is a bit yucky. A better way to deal with this is to use nsTHashtable and make nsSMILCompositor an entry like the keys in nsHashKeys.h. You can make they KeyType be a struct containing nsIContent*, nsIAtom* and a PRPackedBool. + mLastCompositorTable = currentCompositorTable; I'd prefer this to be written as "... currentCompositorTable.forget();" to make it clear there's a transfer of ownership here. +NS_IMPL_THREADSAFE_ISUPPORTS1(nsSMILCompositor, + nsIHashable) Not THREADSAFE. + NS_ASSERTION(aFunc, "NULL animation function passed."); + if (aFunc) { + mAnimationFunctions.AppendElement(aFunc); + } Don't need "if (aFunc)". If you're asserting, then it shouldn't happen. + if (!smilAttr) { + // Target attribute not found + return; + } Should have an NS_WARNING here. + nsCOMPtr<nsIContent> animatedElem(do_QueryInterface(mElement)); I'm confused. mElement is already an nsIContent*. That's it! I've reviewed the entire patch!
Attached patch patch v5h (obsolete) — — Splinter Review
Addressed issues from roc's review.
Attachment #355713 - Attachment is obsolete: true
Attachment #356894 - Flags: superreview?(roc)
Attachment #355713 - Flags: superreview?(roc)
(In reply to comment #167) > + if (!mTimedDocumentRoot) > + // Timed document root hasn't been created yet. This will be created when > + // the SVG parent is bound. > + return NS_OK; > > Is this actually possible? I would have thought that if there's an ownerSVG > element, there must be a timed document root associated with it or one of its > ancestors. Either this should be an assertion? As discussed, this situation does actually arise when entire SVG trees are created by script such as this test case: http://brian.sol1.net/svg/tests/deferred-tree.xhtml (A similar test is included in the reftests for this patch: smil/container/deferred-tree-1.xhtml) However, we still need to work out exactly why. > That's it! I've reviewed the entire patch! Thanks Robert!
+ nsCOMPtr<nsIRunnable> mForceSampleEvent; If you make nsForceSampleEvent an inner class of nsSMILAnimationController (call it just ForceSampleEvent), then I think you can make this nsRefPtr<ForceSampleEvent> and avoid some casts. +nsSMILTime +nsSMILAnimationFunction::GetBeginTime() const +{ + return mBeginTime; +} Make this inline in the class +PRBool +nsSMILAnimationFunction::IsActive() const +{ + /* Notes: + * - Frozen animations should be considered active for the purposes of + * compositing. + * - This function does not assume that our nsSMILValues (by/from/to/values) + * have already been parsed. + */ + return (mIsActive || mIsFrozen); +} This might as well be inline too +inline PRBool +nsSMILParserUtils::IsSpace(const char c) +{ + return IsSpace((PRUnichar)c); +} Why do we need this version of IsSpace? + /** + * Restores the element to its initial state. As with Reset() this involves + * clearing certain instance times as well as discarding previously + * constructed intervals and removing previous animation effects. + */ + void HardReset(); This comment is a little confusing. Make it clear what HardReset does that Reset doesn't do. + nsSMILTime lMilliseconds = (PRInt64)NS_round(fMilliseconds); There are a few (PRInt64) casts that we'd prefer to write as PRInt64(...). Same for (double). Love the essay in nsSMILTimeValue.h! +#ifdef MOZ_SMIL + nsTHashtable<nsISupportsHashKey> mAnimationElementSet; + nsAutoPtr<nsSMILAnimationController> mAnimationController; +#endif // MOZ_SMIL I should have mentioned this earlier, sorry, but I think mAnimationElementSet would be better as part of the animation controller. The animation elements can get the controller and add themselves there. + nsSMILTimeContainerHashtable* activeContainers + = (nsSMILTimeContainerHashtable*)aData; static_cast + nsSMILTimeContainer* container = aKey->GetKey(); + if (container && container->NeedsSample()) { + container->Sample(); + activeContainers->PutEntry(container); + } + + aKey->GetKey()->Sample(); How can GetKey return null? Shouldn't we just assert that it's non-null? and shouldn't we just call container->Sample()? But why would we call sample twice? + /* + * Per-sample operations to be performed whenever Sample() is called and + * NeedsSample() is true. Called after updating mCurrentTime; + */ + virtual void DoSample() { } + + /* + * Adding and removing child containers is not implemented in the base class + * because not all subclasses need this. + */ + + /* + * Adds a child time container. + */ + virtual nsresult AddChild(nsSMILTimeContainer& aChild) + { + return NS_ERROR_FAILURE; + } + + /* + * Removes a child time container. + */ + virtual void RemoveChild(nsSMILTimeContainer& aChild) { } Lets get rid of these until we need them. Will we need them for SVG? + if (!animElem) { + NS_NOTREACHED("NULL animation-element pointer in animation element set"); + return PL_DHASH_NEXT; + } This should just be an NS_ASSERTION. + nsSMILTimeContainerHashtable* activeContainers + = (nsSMILTimeContainerHashtable*)aData; static_cast + if (!animElem) { // sanity-check + NS_NOTREACHED("Null animation-element pointer in animation element set"); + return PL_DHASH_NEXT; + } just make it an NS_ASSERTION + if (!compositorTable) { // sanity-check + NS_NOTREACHED("Null data pointer (to compositor table) while iterating " + "through animation element set!"); + return PL_DHASH_NEXT; + } This one too Is there a reason we can't fuse the loops over all animation elements for steps 2 and 3? + nsCOMPtr<nsIContent> targetElem = aAnimElem->GetTargetElementContent(); This can be nsIContent* + nsCOMPtr<nsIAtom> attributeName = aAnimElem->GetTargetAttributeName(); This can be nsIAtom*, if we make GetTargetAttributeName return an nsIAtom*, which we can. + nsRefPtr<nsIContent> mElement; + nsRefPtr<nsIAtom> mAttributeName; // XXX need to consider namespaces here These can be nsIContent* and nsIAtom* too. You've reduced calls to PR_Now to one per time container per sample, right? That's an improvement although still not ideal. But I guess we can leave it for now, since we really need to get this under the control of a global composition manager. We're converging fast now!
(In reply to comment #171) > + nsRefPtr<nsIContent> mElement; > + nsRefPtr<nsIAtom> mAttributeName; // XXX need to consider namespaces here > > These can be nsIContent* and nsIAtom* too. Never mind, I understand why they have to be nsRefPtrs. (Because we keep the table around in mLastCompositorTable.)
(In reply to comment #169) > Created an attachment (id=356894) [details] > patch v5h > > Addressed issues from roc's review. This patch also fails to build under windows. The last patch attached to this bug that compiles successfully under windows is smil5e.patch. Here are the errors: c:/mozilla/mozilla2/content/svg/content/src/nsSVGAnimateTransformElement.cpp(84) : error C2259: 'nsSVGAnimateTransformElement' : cannot instantiate abstract class due to following members: 'nsSMILAnimationFunction &nsISMILAnimationElement::AnimationFunction(void)' : is abstract ../../../../dist/include/content\nsISMILAnimationElement.h(134) : see declaration of 'nsISMILAnimationElement::AnimationFunction' c:/mozilla/mozilla2/content/svg/content/src/nsSVGAnimateTransformElement.cpp(137) : error C2259: 'nsSVGAnimateTransformElement' : cannot instantiate abstract class due to following members: 'nsSMILAnimationFunction &nsISMILAnimationElement::AnimationFunction(void)' : is abstract ../../../../dist/include/content\nsISMILAnimationElement.h(134) : see declaration of 'nsISMILAnimationElement::AnimationFunction' make[7]: *** [nsSVGAnimateTransformElement.obj] Error 2 make[7]: Leaving directory `/c/mozilla/mozilla2/fx-obj/content/svg/content/src' make[6]: *** [src_libs] Error 2 make[6]: Leaving directory `/c/mozilla/mozilla2/fx-obj/content/svg/content' make[5]: *** [content_libs] Error 2 make[5]: Leaving directory `/c/mozilla/mozilla2/fx-obj/content/svg' make[4]: *** [svg_libs] Error 2 make[4]: Leaving directory `/c/mozilla/mozilla2/fx-obj/content' make[3]: *** [libs_tier_gecko] Error 2 make[3]: Leaving directory `/c/mozilla/mozilla2/fx-obj' make[2]: *** [tier_gecko] Error 2 make[2]: Leaving directory `/c/mozilla/mozilla2/fx-obj' make[1]: *** [default] Error 2 make[1]: Leaving directory `/c/mozilla/mozilla2/fx-obj' make: *** [build] Error 2
Turns out this fails under Linux as well: /home/wag/mozilla/mozilla2/content/svg/content/src/nsSVGAnimateTransformElement.cpp: In function ‘nsresult NS_NewSVGAnimateTransformElement(nsIContent**, nsINodeInfo*)’: /home/wag/mozilla/mozilla2/content/svg/content/src/nsSVGAnimateTransformElement.cpp:84: error: cannot allocate an object of abstract type ‘nsSVGAnimateTransformElement’ /home/wag/mozilla/mozilla2/content/svg/content/src/nsSVGAnimateTransformElement.cpp:53: note: because the following virtual functions are pure within ‘nsSVGAnimateTransformElement’: ../../../../dist/include/content/nsISMILAnimationElement.h:134: note: virtual nsSMILAnimationFunction& nsISMILAnimationElement::AnimationFunction() /home/wag/mozilla/mozilla2/content/svg/content/src/nsSVGAnimateTransformElement.cpp: In member function ‘virtual nsresult nsSVGAnimateTransformElement::Clone(nsINodeInfo*, nsINode**) const’: /home/wag/mozilla/mozilla2/content/svg/content/src/nsSVGAnimateTransformElement.cpp:137: error: cannot allocate an object of abstract type ‘nsSVGAnimateTransformElement’ /home/wag/mozilla/mozilla2/content/svg/content/src/nsSVGAnimateTransformElement.cpp:53: note: since type ‘nsSVGAnimateTransformElement’ has pure virtual functions gmake[7]: *** [nsSVGAnimateTransformElement.o] Error 1 gmake[7]: Leaving directory `/home/wag/mozilla/mozilla2/fx32-obj/content/svg/content/src' gmake[6]: *** [src_libs] Error 2 gmake[6]: Leaving directory `/home/wag/mozilla/mozilla2/fx32-obj/content/svg/content' gmake[5]: *** [content_libs] Error 2 gmake[5]: Leaving directory `/home/wag/mozilla/mozilla2/fx32-obj/content/svg' gmake[4]: *** [svg_libs] Error 2 gmake[4]: Leaving directory `/home/wag/mozilla/mozilla2/fx32-obj/content' gmake[3]: *** [libs_tier_gecko] Error 2 gmake[3]: Leaving directory `/home/wag/mozilla/mozilla2/fx32-obj' gmake[2]: *** [tier_gecko] Error 2 gmake[2]: Leaving directory `/home/wag/mozilla/mozilla2/fx32-obj' make[1]: *** [default] Error 2 make[1]: Leaving directory `/home/wag/mozilla/mozilla2/fx32-obj' make: *** [build] Error 2
Ooops. Please ignore my previous 2 comments. There is no issue in building with just this patch alone applied. The issue occurs when you also try to apply the latest patch for bug 468996, so evidently this patch requires an update to that patch as well. Sorry for my confusion and the resultant bugspam.
Attached patch patch v5i (obsolete) — — Splinter Review
Address previous round of roc's review. Thanks again Robert!
Attachment #356894 - Attachment is obsolete: true
Attachment #357066 - Flags: superreview?(roc)
Attachment #356894 - Flags: superreview?(roc)
Attachment #357066 - Flags: superreview?(roc) → superreview+
Blocks: 470868
Attached patch patch v5j (obsolete) — — Splinter Review
Here's an updated patch with some minor cleanup: - removed whitespace on 2 blank lines - tweaked placement of */&/const in a few places, to be consistent - s/nonnegative/non-negative/ in comments within nsSMILAnimationFunction.cpp, for consistency - s/nsSMILAnimationFunction/nsSMILAnimationController/ in comment header of nsSMILAnimationController.h - Mark enveloped-tree reftest as random, with a link to bug 470868. (see that bug for more info)
Attachment #357066 - Attachment is obsolete: true
Comment on attachment 357076 [details] [diff] [review] patch v5j One more review note: >+PRBool >+nsSMILAnimationFunction::GetValues(const nsISMILAttr& aSMILAttr, >+ nsSMILValueArray& aResult) This method is declared as returning PRBool, but it has a number of places where it tries to return an nsresult value (rv, NS_ERROR_FAILURE, NS_OK). It should either return PRBool or nsresult, not both. :) Brian -- would you mind fixing that? After that, I think we're good to land!
Attached patch patch v5k — — Splinter Review
Fix return type for nsSMILAnimationFunction::GetValues
Attachment #357076 - Attachment is obsolete: true
Comment on attachment 357078 [details] [diff] [review] patch v5k Carrying forward roc's r+
Attachment #357078 - Flags: review+
Blocks: 473702
Blocks: 473705
Pushed "patch v5k" to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/ed15cc897a16 Woot!
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 16 years ago
Resolution: --- → FIXED
Note: though this has landed, the SMIL code is disabled in builds by default, so it'll have no effect unless you enable it by adding "ac_add_options --enable-smil" to your .mozconfig. I've filed bug 473705 on enabling it by default.
(In reply to comment #182) > I've filed bug 473705 on enabling it by default. Transferring blocking/wanted requests to that bug.
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Scratch that, I think I didn't --enable-smil correctly in that tryserver build. I'll post a working one tomorrow morning. :)
(In reply to comment #185) > Scratch that, I think I didn't --enable-smil correctly in that tryserver build. d'oh -- turns out it works after all -- I just mistyped when I was testing it and confused myself. :) So, that try-server build link is indeed good. (It just uses mozilla-central from right after the smil patch landed, plus the "force_smil" patch from my patch queue (mentioned in comment 122) to enable SMIL in the build.)
<animateColor>, <animateMotion>, <set> coming in soon?
<set> is in this patch and landed today. The others need more work. I'm not sure how hard they'll be or when they'll be done.
Mr Holbert, I using the tryserver build listed here https://build.mozilla.org/tryserver-builds/2009-01-14_20:59-dholbert@mozilla.com-try-6f0b198ab0f/ But it wont run this <?xml version="1.0" encoding="UTF-8" standalone="no"?> <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"> <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> <rect id="rec" x="300" y="100" width="300" height="100" fill="red"> <animate attributeName="fill" begin="2s" from="red" to="green" dur="10"/> </rect> </svg>
We don't support animation of colors yet, only lengths.
Woo-hoo! Fantastic work! :-)
Blocks: 473904
Blocks: 474049
#190 ROC is the expectation that bugs be filed for broken or unimplemented parts? eg visibility="hidden" is broken...
peepo: That's my understanding - see all the bugs that this bug blocks at the top (Bug 410460, etc)
(In reply to comment #192) > eg visibility="hidden" is broken... Yeah, that's not implemented yet. The current implementation only supports lengths, and other attributes that are implemented as lengths (e.g. x, y, rx, ry). You can file a bug on that if you'd like -- though it'll probably end up being addressed in bug 436276 ("support animating font-size", i.e. support other types of attributes).
I've changed the title of bug 436265 so that it covers animating css properties to try to avoid ending up with dozens almost identical "cannot animate <x> bugs".
(In reply to comment #195) > I've changed the title of bug 436265 (you mean bug bug 436276) > so that it covers animating css properties Bug 474049 is actually already filed on animating CSS properties. So, I've now retargeted bug 436276 to support animating *non-nsSVGLength2-backed SVG attributes* (like the "font-size" and "visibility" attributes).
Adding bug 485157 as dependant, but.. Daniel, I think that bug would more depend on: "event-values-support would be a good candidate for a follow-up patch (possibly with its own separate bug)" quote from comment 72 Did that ever get filed?
Blocks: 485157
(In reply to comment #197) > Adding bug 485157 as dependant, but.. Thanks -- I just renamed that bug to make it about supporting event values.
Blocks: 474742
Blocks: 474743
Blocks: 474739
Blocks: 487450
Blocks: 491080
I noticed bug in current implementation of animations. Animations don't apply to text elements (even if working for rects, circles, etc.). For interested: bug #491268
Blocks: 492081
(In reply to comment #199) Just to follow up on this -- per bug 491268 comment 3, that's because text element x and y attributes aren't lengths -- they're lists of lengths (which we can't animate yet)
Blocks: 492458
Doesn't appear to be any docs on MDC about SMIL and in particular this page needs updated: https://developer.mozilla.org/en/SVG_in_Firefox
Keywords: dev-doc-needed
That's the Firefox 2.0 page, this is the trunk page https://developer.mozilla.org/En/Mozilla_SVG_Status It still needs updating but somewhat less so.
Confusing that the Firefox 2 page mentions things being implemented in Firefox 3. Why are we keeping two separate pages anyway?
(In reply to comment #203) > Why are we keeping two separate pages anyway? The release version pages contain snapshots of the included features documentation so one can target a specific Firefox version, determine if a specific features is available in that release (before choosing whether to use it), etc. The other page reflect the current (trunk) status and will likely be snapshotted whenever a new release including those features (Firefox 3.5 or Firefox.next) is released.
There is no separate Firefox 2 page; the https://developer.mozilla.org/en/SVG_in_Firefox article is the official SVG document for all versions of Firefox. It needs updating; preferably by someone that knows SMIL.
I can update it, in the next day or two.
Blocks: 506096
I started this page, but it's on hold for now since I'm told SVG animation has been bumped to 1.9.3: https://developer.mozilla.org/en/SVG_animation_%28SMIL%29_in_Firefox
Whiteboard: [doc-waiting-1.9.3]
Alias: smil
Target Milestone: --- → mozilla1.9.2a1
Whiteboard: [doc-waiting-1.9.3]
We now have as much documentation for SVG animation as we will have for a while. The entire SVG section of the documentation is tentatively slated for a total redo once Firefox 4 documentation starts winding down, so for now, we mention that we support it, and this page: https://developer.mozilla.org/en/SVG/SVG_animation_with_SMIL Offers a simple example and some very basic information, as well as links to specifications. Additionally, the main SVG page links to that, and the SVG in Firefox article now links to it as well. I've filed bug 602022 for that.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: