Closed Bug 1425246 Opened 5 years ago Closed 4 years ago

timestampOffset leads to currentTime-frame mismatch since Firefox 57

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mkleinschmidt, Assigned: jya)

Details

Attachments

(2 files)

4.20 MB, application/x-zip-compressed
Details
59 bytes, text/x-review-board-request
rillian
: review+
Details
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
Attached file ff57.zip
Example code attached
The wrong frame seems to be displayed sporadically. 
Please refresh the example page multiple times to reproduce the error.
Component: Untriaged → Audio/Video: MediaStreamGraph
Product: Firefox → Core
Component: Audio/Video: MediaStreamGraph → Audio/Video: Playback
Jean-Yves, could this be another reordering bug? What do you think?
Flags: needinfo?(jyavenard)
Priority: -- → P3
It's a rounding error following bug 1222866

so our -1.0 becomes -99999us
Flags: needinfo?(jyavenard)
Assignee: nobody → jyavenard
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;
```
Attachment #8953398 - Flags: review?(giles) → review-
Comment on attachment 8953398 [details]
Bug 1425246 - Don't round time when negative.

https://reviewboard.mozilla.org/r/222676/#review229612
Attachment #8953398 - Flags: review?(giles) → review+
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7cc679e17f5a
Don't round time when negative. r=rillian
https://hg.mozilla.org/mozilla-central/rev/7cc679e17f5a
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.