Closed Bug 1430654 Opened 6 years ago Closed 6 years ago

Intermittent /web-animations/interfaces/DocumentTimeline/constructor.html | A positive origin time makes the document timeline's current time lag behind the default document timeline - assert_approx_equals: expected -9906.34 +/- 0.0005 but got -9906.32

Categories

(Core :: DOM: Animation, defect, P5)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: intermittent-bug-filer, Assigned: hiro)

References

Details

(Keywords: intermittent-failure)

Attachments

(3 files)

Bug 1430654 - Introduce assert_time_equals_literal and use it.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1430654 - Double epsilon value for assert_times_equal.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1430654 - Use assert_times_equal for comparing timing values.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
We need to update TIME_PRECISION in testcommon.js to the latest value, 0.001s <https://drafts.csswg.org/web-animations/#precision-of-time-values>.   Also this test is affected by our timer precision value which has been changed recently?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> We need to update TIME_PRECISION in testcommon.js to the latest value,
> 0.001s <https://drafts.csswg.org/web-animations/#precision-of-time-values>. 
> Also this test is affected by our timer precision value which has been
> changed recently?

I think so. One option might be to just turn off privacy.reduceTimerPrecision. e.g.

  https://hg.mozilla.org/mozilla-central/rev/bb887872b8e5
Taking I am going to update the TIME_PRECISION values (in testing/web-platform/ and in dom/animation/test).
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
See Also: → 1432068
Let's see what happens without turning off the pref.
Keywords: leave-open
Comment on attachment 8944326 [details]
Bug 1430654 - Introduce assert_time_equals_literal and use it.

https://reviewboard.mozilla.org/r/214558/#review220132

::: testing/web-platform/tests/web-animations/testcommon.js:15
(Diff revision 1)
>  // The recommended minimum precision to use for time values[1].
>  //
>  // [1] https://drafts.csswg.org/web-animations/#precision-of-time-values
> -const TIME_PRECISION = 0.0005; // ms
> +const TIME_PRECISION = 0.001; // ms
>  
>  // Allow implementations to substitute an alternative method for comparing
>  // times based on their precision requirements.
>  if (!window.assert_times_equal) {
>    window.assert_times_equal = (actual, expected, description) => {
>      assert_approx_equals(actual, expected, TIME_PRECISION, description);
>    };
>  }

This isn't right (I thought there used to be a comment about this but I guess not).

The spec text is:

"it is RECOMMENDED that user agents be able to represent input time values with microsecond precision so that a time value (which nominally represents milliseconds) of 0.001 is distinguishable from 0.0"

To be able to distinguish 0.001 from 0.0 we need to be sure that values that *should* be 0.001 are between 0.0005 and 0.0015 and values that should be 0.0 are between -0.0005 and 0.0005.

The definition of assert_approx_equals (as used by assert_times_equal) is:

"asserts that actual is a number within ±epsilon of expected"

so the epsilon we want is 0.0005.

Either the spec text needs to change or we need to find a way to override assert_times_equal.
Attachment #8944326 - Flags: review?(bbirtles)
Comment on attachment 8944327 [details]
Bug 1430654 - Double epsilon value for assert_times_equal.

https://reviewboard.mozilla.org/r/214560/#review220134

::: commit-message-bdbfe:1
(Diff revision 1)
> +Bug 1430654 - Use assert_times_equal for compasiton time values. r?birtles

s/compasiton/comparison/

Or better still: "... for comparing timing values"
Attachment #8944327 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #8)
> Comment on attachment 8944326 [details]
> Bug 1430654 - Update TIME_PRECISION to the latest one.
> 
> https://reviewboard.mozilla.org/r/214558/#review220132
> 
> ::: testing/web-platform/tests/web-animations/testcommon.js:15
> (Diff revision 1)
> >  // The recommended minimum precision to use for time values[1].
> >  //
> >  // [1] https://drafts.csswg.org/web-animations/#precision-of-time-values
> > -const TIME_PRECISION = 0.0005; // ms
> > +const TIME_PRECISION = 0.001; // ms
> >  
> >  // Allow implementations to substitute an alternative method for comparing
> >  // times based on their precision requirements.
> >  if (!window.assert_times_equal) {
> >    window.assert_times_equal = (actual, expected, description) => {
> >      assert_approx_equals(actual, expected, TIME_PRECISION, description);
> >    };
> >  }
> 
> This isn't right (I thought there used to be a comment about this but I
> guess not).
> 
> The spec text is:
> 
> "it is RECOMMENDED that user agents be able to represent input time values
> with microsecond precision so that a time value (which nominally represents
> milliseconds) of 0.001 is distinguishable from 0.0"
> 
> To be able to distinguish 0.001 from 0.0 we need to be sure that values that
> *should* be 0.001 are between 0.0005 and 0.0015 and values that should be
> 0.0 are between -0.0005 and 0.0005.

Oh, both of the sentence in the spec and this clarification sentence are quite confusing to me.
It seems to me that the minimum possible value for '0.001' is '0.0005' and the maximum value for '0' is '0.0005'.  It can't be distinguished? 

Anyway, now I understand why the patch is not correct.
The problem is that the failure cases compare two different time values with the TIME_PRECISION.  When we compare the time value with a fixed value, the precision value works as expected.  For example,

assert_times_equals(animation.startTime, 0.01, ..);

This should be fine.  But if we compare two different time values, say

assert_times_equals(animation.startTime, animation.timeline.currentTime, ..);

In this case, we need to use 0.001 for the epsilon since the startTime might have errors between -0.005 and 0.005 and the currentTime might also have the same.   Anyway, I now understand what the problem is, so I will figure out how to solve this problem. 

Thanks for the clarification as usual!
(In reply to Brian Birtles (:birtles) from comment #8)
> The definition of assert_approx_equals (as used by assert_times_equal) is:
> 
> "asserts that actual is a number within ±epsilon of expected"
> 
> so the epsilon we want is 0.0005.

Oh, I did miss this part. :/  So.. Hmm we have no way other than turning off the pref?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> (In reply to Brian Birtles (:birtles) from comment #8)
> > The definition of assert_approx_equals (as used by assert_times_equal) is:
> > 
> > "asserts that actual is a number within ±epsilon of expected"
> > 
> > so the epsilon we want is 0.0005.
> 
> Oh, I did miss this part. :/  So.. Hmm we have no way other than turning off
> the pref?

Do'h, I am an idiot as usual, did not read assert_approx_equals().  assert_approx_equals() is;

 assert(Math.abs(actual - expected) <= epsilon,

so in the case where we comparison two time values (not the case comparing with fixed (literal) number, I am not sure what should be called), we need to use epsilon*2 there anyway.
As discussed with Brian on IRC, we'd introduce a new assert function named assert_time_equals_to_fixed_value() for the case where we compare a timing value with a fixed literal value (e.g. 0.5).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2c954fd0b5bc08d995ba9b88d4ca3877aa36db2
Comment on attachment 8944326 [details]
Bug 1430654 - Introduce assert_time_equals_literal and use it.

https://reviewboard.mozilla.org/r/214558/#review220140

::: commit-message-5faab:1
(Diff revision 2)
> +Bug 1430654 - Introduce assert_time_equals_to_fixed_value and use it. r?birtles

(As discussed below, we'll need to update this commit message too.)

::: dom/animation/test/testcommon.js:35
(Diff revision 2)
>  }
>  
>  /*
> + * Compare a time value based on its precision requirements with a fixed value.
> + */
> +function assert_time_equals_to_fixed_value(actual, expected, description) {

As discussed on IRC there's no need for the '_to' in this function name.

As further discussed, let's go with 'assert_time_equals_literal' after all. Sorry!

::: testing/web-platform/tests/web-animations/README.md:101
(Diff revision 2)
> -*   Use the `assert_times_equal` assertion for comparing calculated times.
> -    It tests times are equal using the precision recommended in the spec whilst
> -    allowing implementations to override the function to meet their own
> -    precision requirements.
> +*   Use the `assert_times_equal` assertion for comparing calculated times or
> +    use the `assert_time_equals_to_fixed_value` for comparing a calculated
> +    time value with a fixed value.
> +    They test times are equal or a time is equal to a fixed value using the
> +    precision recommended in the spec whilst allowing implementations to
> +    override the function to meet their own precision requirements.

Perhaps we can say:

"Use the `assert_times_equal` assertion for comparing times returned from the API. This asserts that the time values are equal using a tolerance based on the precision recommended in the spec. This tolerance is applied to *both* of the values being compared. That is, it effectively allows double the epsilon that is used when comparing with an absolute value.

For comparing a time value returned from the API to an absolute value, use `assert_time_equals_literal`. This tests that the actual value is equal to the expected value within the precision recommended in the spec.

Both `assert_times_equal` and `assert_time_equals_literal` are defined in a way that implementations can override them to meet their own precision requirements."
Attachment #8944326 - Flags: review?(bbirtles) → review+
Comment on attachment 8944341 [details]
Bug 1430654 - Use assert_times_equal for comparing timing values.

https://reviewboard.mozilla.org/r/214562/#review220142
Attachment #8944341 - Flags: review?(bbirtles) → review+
Thanks for the review!  I did fix the function name and also adjust some indentations there.

A final try just in case;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d72306b3f2aff3ce23516264ff8767a05fb7d3
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edbac2302f90
Introduce assert_time_equals_literal and use it. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a226a272784c
Double epsilon value for assert_times_equal. r=birtles
https://hg.mozilla.org/integration/autoland/rev/235797f7083a
Use assert_times_equal for comparing timing values. r=birtles
The leave-open keyword is there and there is no activity for 6 months.
:hiro, maybe it's time to close this bug?
Flags: needinfo?(hikezoe)
Ineed!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: needinfo?(hikezoe)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: