SVG SMIL: Calls to setCurrentTime, beginElementAt etc. should update the DOM state immediately

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
SVG provides methods that directly manipulate animations. Particularly,

  SVGSVGElement.SetCurrentTime
  SVGAnimationElement.BeginElement
  SVGAnimationElement.BeginElementAt
  SVGAnimationElement.EndElement
  SVGAnimationElement.EndElementAt

If you call one of these methods and then immediately query the DOM it should reflect the new state of the animation. For example, a call to SetCurrentTime(100) followed by a query of an animVal should contain the updated animation state.

Currently we don't do that, and we just put an sample event on the queue. If script wants to see the updated DOM they need to do a setTimeout('nextbit()',0).

SVG/SMIL doesn't dictate how this should work (although SVG gives some guidelines about when error processing should occur in relation to redrawing) but our current arrangement is both counter-intuitive and really cumbersome, especially when we're writing mochitests and we have to litter them with setTimeouts.

Instead, we should force a resample whenever one of these methods is called.
(Assignee)

Comment 1

10 years ago
Created attachment 357736 [details] [diff] [review]
patch v1a

Proposed patch.

I've introduced the idea of a "resample" which does the same thing as a sample but doesn't advance the current time and doesn't check if the container is paused. So for beginElementAt etc. it will just do the sample again with the same sample time as last time. For setCurrentTime it will just do the sample with the newly set time.

I've left the asynchronous force sample methods in nsSMILAnimationController for now although they're no longer used. It seems they could be useful in the future. Or should I remove them?

I've run the SMIL reftests and SMIL mochitests on this.
Attachment #357736 - Flags: superreview?(roc)
Attachment #357736 - Flags: review?(roc)
I'm afraid what we really need here is lazy resampling. With your patch, if I have a big document with many animations and I call BeginElementAt on each one, we're going to have O(N^2) performance. What we really need is a flag that means "resampling needed!" which we check along all GetAnimVal paths and force the resampling before returning an animval. That might be a bit of a performance hit, but fortunately we do most of our getting of animated values for rendering via nsSVGElement::GetAnimated*Values, so we can do just one check while returning many values.
We also need to set that flag whenever anything affecting animations is changed (methods and attributes on animation elements), and whenever (instead of) we clear out mAnimVal in animated values when the base value is changed.

This will mean that you can do things like
animatedElement.setAttribute("x", ...);
var v = animatedElement.x.animVal;
and get the right animated result.
(Assignee)

Comment 4

10 years ago
Created attachment 357899 [details] [diff] [review]
patch v1b

Second attempt. Addressed issues raised in roc's review.

As discussed, I'm deferring making transform types fully-synchronised for now as we need to overhaul the transform type in the same way we've done in nsSVGLength2.
Attachment #357736 - Attachment is obsolete: true
Attachment #357899 - Flags: superreview?(roc)
Attachment #357899 - Flags: review?(roc)
Attachment #357736 - Flags: superreview?(roc)
Attachment #357736 - Flags: review?(roc)
I think nsSMILAnimationController::DoSample should assert !mResampleNeeded at the end, to catch errors where we accidentally trigger that during sampling. 

+  nsresult    StartTimer(PRBool aDoInitialSynchronousSample = PR_TRUE);

Don't use an optional parameter here. We generally try to avoid them.

   float GetMMPerPixel(nsIFrame *aNonSVGFrame) const;
   float GetAxisLength(nsIFrame *aNonSVGFrame) const;
   float GetEmLength(nsIFrame *aFrame) const
     { return nsSVGUtils::GetFontSize(aFrame); }
   float GetExLength(nsIFrame *aFrame) const
     { return nsSVGUtils::GetFontXHeight(aFrame); }
-  float GetUnitScaleFactor(nsIFrame *aFrame) const;
+  float GetUnitScaleFactor(nsIFrame *aFrame, PRUint8 aUnitType) const;
 
   float GetMMPerPixel(nsSVGSVGElement *aCtx) const;
   float GetAxisLength(nsSVGSVGElement *aCtx) const;
   float GetEmLength(nsSVGElement *aSVGElement) const
     { return nsSVGUtils::GetFontSize(aSVGElement); }
   float GetExLength(nsSVGElement *aSVGElement) const
     { return nsSVGUtils::GetFontXHeight(aSVGElement); }
-  float GetUnitScaleFactor(nsSVGElement *aSVGElement) const;
-  float GetUnitScaleFactor(nsSVGSVGElement *aCtx) const;
+  float GetUnitScaleFactor(nsSVGElement *aSVGElement, PRUint8 aUnitType) const;
+  float GetUnitScaleFactor(nsSVGSVGElement *aCtx, PRUint8 aUnitType) const;

Can some or all of these be static member functions?

In nsSVGSVGElement::SetCurrentTime, why can't we just call AnimationNeedsResample instead of forcing a sample right there? I presume it's because SetCurrentTime(A) followed by another SetCurrentTime(B) is not equivalent to SetCurrentTime(B), according to the SMIL spec? If so, better note that in a comment here :-( (and make sure it's tested).

Maybe I shouldn't worry about it, but I think nsSMILAnimationController::FlushResampleRequests should be inline to make nsSVGElement::FlushAnimations as fast as possible.

Otherwise great!
(Assignee)

Comment 6

10 years ago
Created attachment 358008 [details] [diff] [review]
patch v1c

Address review comments.
Attachment #357899 - Attachment is obsolete: true
Attachment #358008 - Flags: superreview?(roc)
Attachment #358008 - Flags: review?(roc)
Attachment #357899 - Flags: superreview?(roc)
Attachment #357899 - Flags: review?(roc)
(Assignee)

Comment 7

10 years ago
(In reply to comment #5)
> Can some or all of these be static member functions?

Some can, but not all. Originally I tried making them all static but some of them refer to mCtxType.

So for example:

 GetMMPerPixel(nsIFrame *aNonSVGFrame)
 
can be made static, but
 
 GetMMPerPixel(nsSVGSVGElement *aCtx)
 
can't.

So shall I convert half of them or just leave them all?

> In nsSVGSVGElement::SetCurrentTime, why can't we just call
> AnimationNeedsResample instead of forcing a sample right there? I presume it's
> because SetCurrentTime(A) followed by another SetCurrentTime(B) is not
> equivalent to SetCurrentTime(B), according to the SMIL spec? If so, better note
> that in a comment here :-( (and make sure it's tested).

Yeah, that's right, we really must force a sample. This is tested and documented in test_smilSync.xhtml:testSetCurrentTime() but I've added comments now as suggested.
(In reply to comment #7)
> So shall I convert half of them or just leave them all?

Convert the ones that can be static.

> > In nsSVGSVGElement::SetCurrentTime, why can't we just call
> > AnimationNeedsResample instead of forcing a sample right there? I presume it's
> > because SetCurrentTime(A) followed by another SetCurrentTime(B) is not
> > equivalent to SetCurrentTime(B), according to the SMIL spec? If so, better note
> > that in a comment here :-( (and make sure it's tested).
> 
> Yeah, that's right, we really must force a sample. This is tested and
> documented in test_smilSync.xhtml:testSetCurrentTime() but I've added comments
> now as suggested.

Thanks.
Other than the static issue, the patch looks good.
(Assignee)

Comment 10

10 years ago
Created attachment 358028 [details] [diff] [review]
patch v1d

Address review comments: make some methods static

Thanks Robert!
Attachment #358008 - Attachment is obsolete: true
Attachment #358028 - Flags: superreview?(roc)
Attachment #358028 - Flags: review?(roc)
Attachment #358008 - Flags: superreview?(roc)
Attachment #358008 - Flags: review?(roc)
Attachment #358028 - Flags: superreview?(roc)
Attachment #358028 - Flags: superreview+
Attachment #358028 - Flags: review?(roc)
Attachment #358028 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/97daab9778b0
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.