Closed Bug 1246893 Opened 5 years ago Closed 5 years ago

nsSMILKeySpline::GetSplineValue should return 0 or 1 on its boundaries

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: hiro, Unassigned)

References

Details

Attachments

(1 file)

For example, cubic-bezier(0, 1.5, 1. 1.5), GetSplineValue returns 1.000146 on 1.0.  The error of the value is not acceptable to be used in automation tests.
That will make content that is invalid, valid. The SVG specification does not say to do that (although perhaps it should). Instead it says such splines are invalid. https://www.w3.org/TR/SVG/animate.html#KeySplinesAttribute
Thank you, Robert.  Then we should limit the values in animation codes.
Attachment #8717365 - Flags: review?(bbirtles)
Maybe Brian might consider changing the SVG animation specification.
(In reply to Robert Longson from comment #3)
> That will make content that is invalid, valid. The SVG specification does
> not say to do that (although perhaps it should). Instead it says such
> splines are invalid.
> https://www.w3.org/TR/SVG/animate.html#KeySplinesAttribute

We already allow y values outside the range [1,0] since this code is re-used by CSS animation which allows it. For SVG, I'm pretty sure we do the range checking when we parse the string:

  https://dxr.mozilla.org/mozilla-central/rev/2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/dom/smil/nsSMILParserUtils.cpp#529
Brian, thank you for pointing it out.

I found another use of GetSplineValue in AsyncScrollBase::PositionAt[1]
[1] https://dxr.mozilla.org/mozilla-central/rev/2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/layout/generic/AsyncScrollBase.cpp#103
And it seems that the function there also produces values greater than 1 if user sets SmoothScrollCurrentVelocityWeighting to a sufficiently small value, say 1/100000000.
[2] https://dxr.mozilla.org/mozilla-central/rev/2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/layout/generic/AsyncScrollBase.cpp#96


Although I don't think it becomes a problem in the real world, I think we should fix the errors on the both boundaries in GetSplineValue.
Attachment #8717365 - Flags: review?(bbirtles)
Comment on attachment 8717365 [details]
MozReview Request: Bug 1246893 - Fix boundary values of nsSMILKeySpline::GetTForX. r?birtles

https://reviewboard.mozilla.org/r/34155/#review31487

Looks good although I'm also a little curious as to the nature of the error. There is some approximation performed by nsSMILKeySpline but I thought it would be ok at the boundaries.
Attachment #8717365 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #8)
> Comment on attachment 8717365 [details]
> MozReview Request: Bug 1246893 - Fix boundary values of
> nsSMILKeySpline::GetSplineValue. r?birtles
> 
> https://reviewboard.mozilla.org/r/34155/#review31487
> 
> Looks good although I'm also a little curious as to the nature of the error.
> There is some approximation performed by nsSMILKeySpline but I thought it
> would be ok at the boundaries.

Thank you, Brian!  Your concerns and curious are always very keen.  I should have dug into the source of the errors.

The errors actually come from guessForT in nsSMILKeySpline::GetTForX [1] because |intervalStart + dist * kSampleStepSize| != 1.0 in case of aX == 1.0.

[1] https://dxr.mozilla.org/mozilla-central/rev/e355cacefc881ba360d412853b57e8e060e966f4/dom/smil/nsSMILKeySpline.cpp#92

I will post a revised patch soon.
Comment on attachment 8717365 [details]
MozReview Request: Bug 1246893 - Fix boundary values of nsSMILKeySpline::GetTForX. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34155/diff/1-2/
Attachment #8717365 - Attachment description: MozReview Request: Bug 1246893 - Fix boundary values of nsSMILKeySpline::GetSplineValue. r?birtles → MozReview Request: Bug 1246893 - Fix boundary values of nsSMILKeySpline::GetTForX. r?birtles
Comment on attachment 8717365 [details]
MozReview Request: Bug 1246893 - Fix boundary values of nsSMILKeySpline::GetTForX. r?birtles

I can't re-request review on MozReview.

A try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fc6e2840034
Attachment #8717365 - Flags: review+ → review?(bbirtles)
Comment on attachment 8717365 [details]
MozReview Request: Bug 1246893 - Fix boundary values of nsSMILKeySpline::GetTForX. r?birtles

https://reviewboard.mozilla.org/r/34155/#review31571

::: dom/smil/nsSMILKeySpline.cpp:80
(Diff revision 2)
> +  // so we return 1.0 here to avoid the errors.

Nit: s/calcuration/calculation/

Just curious, where does the error come from? Is it due to the values we store in mSampleValues?

i.e. If we make nsSMILKeySpline::CalcSampleValues simply set mSampleValues[kSplineTableSize-1] = 1.0, would that fix this?
Attachment #8717365 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #12)
> Comment on attachment 8717365 [details]
> MozReview Request: Bug 1246893 - Fix boundary values of
> nsSMILKeySpline::GetTForX. r?birtles
> 
> https://reviewboard.mozilla.org/r/34155/#review31571
> 
> ::: dom/smil/nsSMILKeySpline.cpp:80
> (Diff revision 2)
> > +  // so we return 1.0 here to avoid the errors.
> 
> Nit: s/calcuration/calculation/
> 
> Just curious, where does the error come from? Is it due to the values we
> store in mSampleValues?
> 
> i.e. If we make nsSMILKeySpline::CalcSampleValues simply set
> mSampleValues[kSplineTableSize-1] = 1.0, would that fix this?

Unfortunately, no.
The error comes from just a result of summation.

An example code:

int
main(int argc, char *argv[])
{
  int i;
  double sum = 0;
  for (i = 0; i < 10; i++) {
    sum += 0.1;
  }

  printf("sum = %e\n", sum);
  if (sum == 1.0) {
    printf("Equals!\n");
  }
  return 0;
}

I can not see 'Equals' with gcc-4.8.4.
Attachment #8717365 - Flags: review+
Comment on attachment 8717365 [details]
MozReview Request: Bug 1246893 - Fix boundary values of nsSMILKeySpline::GetTForX. r?birtles

https://reviewboard.mozilla.org/r/34155/#review31611

::: dom/smil/nsSMILKeySpline.cpp:80
(Diff revision 2)
> +  // so we return 1.0 here to avoid the errors.

// Early return when aX == 1.0 to avoid floating-point inaccuracies.
Comment on attachment 8717365 [details]
MozReview Request: Bug 1246893 - Fix boundary values of nsSMILKeySpline::GetTForX. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34155/diff/2-3/
https://hg.mozilla.org/mozilla-central/rev/34fa562d20dc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.