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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1246893 - Fix boundary values of nsSMILKeySpline::GetTForX. r?birtles

Review commit: https://reviewboard.mozilla.org/r/34155/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34155/

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I pushed a try with attachment 8717365 [details].  The patch seems harmless.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f793f79b3cad

Comment 3

[Robert Longson [:longsonr]]

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Thank you, Robert.  Then we should limit the values in animation codes.

Comment 5

[Robert Longson [:longsonr]]

Maybe Brian might consider changing the SVG animation specification.

Comment 6

[Brian Birtles (:birtles)]

(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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 8

[Brian Birtles (:birtles)]

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.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(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 10

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

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

Comment 12

[Brian Birtles (:birtles)]

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?

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 14

[Brian Birtles (:birtles)]

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 15

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a5415e5110

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34fa562d20dc

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34fa562d20dc