1425246 - timestampOffset leads to currentTime-frame mismatch since Firefox 57

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[mkleinschmidt]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171206182557

Steps to reproduce:

* load video data into MediaSource
* use a negative timestampOffset e.g. "-1.0s" to move the video on timeline
* set the currentTime to e.g. 0.40s

Example code available at http://www.digamailboxdepot.net:8090/ff57-mse-timestampoffset.html


Actual results:

* the displayed frame is frame "9" instead of frame "10" (0.4s = 0.04s * 10)
* the buffer state of timeline displays a gap between 0 and 0.000001s



Expected results:

* the corrent frame should be displayed
* there should be no 0.000001s gap
* timestampOffset should behave similar to Chrome, Edge and Firefox 56

Comment 1

[mkleinschmidt]

Attachment: ff57.zip

Example code attached

Comment 2

[mkleinschmidt]

The wrong frame seems to be displayed sporadically. 
Please refresh the example page multiple times to reproduce the error.

Comment 3

[Ralph Giles (:rillian)]

Jean-Yves, could this be another reordering bug? What do you think?

Comment 4

[Jean-Yves Avenard [:jya]]

It's a rounding error following bug 1222866

so our -1.0 becomes -99999us

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1425246 - Don't round time when negative.

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

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8953398 [details]
Bug 1425246 - Don't round time when negative.

https://reviewboard.mozilla.org/r/222676/#review228694

::: dom/media/TimeUnits.h:55
(Diff revision 1)
>      if (mozilla::IsInfinite<double>(aValue)) {
>        return FromInfinity();
>      }
>      // Due to internal double representation, this
>      // operation is not commutative, do not attempt to simplify.
> -    double val = (aValue + .0000005) * USECS_PER_S;
> +    double val = aValue <= 0 ? aValue : ((aValue + .0000005) * USECS_PER_S);

Don't you still need to multiply by `USECS_PER_S` in the non-positive case?

I also wondered if it be more consistent to round negative numbers toward zero instead? e.g.
```
double half_usec = 5.0e-7;
double val = (aValue < 0 ? (aValue - half_usec) : (aValue + half_usec)) * USEC_PER_S;
```

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8953398 [details]
Bug 1425246 - Don't round time when negative.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222676/diff/1-2/

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8953398 [details]
Bug 1425246 - Don't round time when negative.

https://reviewboard.mozilla.org/r/222676/#review229612

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cc679e17f5a
Don't round time when negative. r=rillian

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/7cc679e17f5a