use monotonic clock for elapsed time

[thijstriemstra]

Description

Improve accuracy using performance.now()

See https://developers.google.com/web/updates/2012/08/When-milliseconds-are-not-enough-performance-now

Specific Changes proposed

All major browsers support this for ages, although IE supports it since v10, according to https://developer.mozilla.org/en-US/docs/Web/API/Performance/now

[thijstriemstra]

Not sure what this error is about: https://travis-ci.org/videojs/video.js/builds/507654239#L295 Some vjsstandard thing?

[brandonocasey]

To get around the vjsstandard linter issue you will want to use window.performance.now() rather than just performance.now(). Although based on the benchmarks that I see on jsperf you may want to do const performance = window.performance; at the top of the file and just keep the code how it is.

I also see that we use Date in two others places that may also need to be changed:
https://github.com/videojs/video.js/blob/master/src/js/utils/dom-data.js#L26
https://github.com/videojs/video.js/blob/master/src/js/tech/html5.js#L868

After looking into this a bit it seems like performance.now() is slower performance wise: https://jsperf.com/new-date-vs-date-now-vs-performance-now/6

It also looks like Google recommends using performance.now: https://developers.google.com/web/tools/lighthouse/audits/date-now

[gkatsev]

I doubt that anywhere we use it, we really have any performance implications to using Date.now() vs new Date().getTime() vs performance.now().
Looks like IE11 has performance.now()? Should be safe to update then.

[thijstriemstra]

we really have any performance implications

Agreed, the term performance(.now) is a little misleading to me, the actual improvement is a consistent clock (that does not adjust at any time, unlike Date.getTime).

I also see that we use Date in two others places that may also need to be changed:

Will take a look. Thanks for the review.

[thijstriemstra]

Another mysterious failure: https://travis-ci.org/videojs/video.js/builds/513188217#L432

[gkatsev]

Looks like the test that's failing is the node require test. Looks like one of the usages of window.performance is being run directly on require and it's crashing. We should either do null checks and fallback to Date.now() or potentially just not do anything if it doesn't exist. It should basically just not throw any errors in a node require rather than have to work.

[thijstriemstra]

Should I just revert the changes in src/js/tech/html5.js?

[brandonocasey]

OK, so I looked into the travis issue and it is related to the change in dom-data and the require in node test. It fails because we cannot use window.performance in node since node does not have a window and performance is not a global. Node exports performance in perf_hooks. Perhaps for now we should revert the change in that file, as there doesn't seem to be an easy way to deal with it.

EDIT:
Actually I found a way, we could do it for now.
Add a now definition at the top of dom-data and then use that instead of window.performance.now

EX:

const now = (window.performance && window.performance.now && window.performance.now()) || Date.now();

...
const elIdAttr = 'vdata' + Math.floor(now);

[thijstriemstra]

Add a now definition at the top of dom-data and then use that instead of window.performance.now

will do.

[thijstriemstra]

Made changes, up for review.

[gkatsev]

ooh, good call

[brandonocasey]

I don't think anyone should rely on vdata_ and if they do they should be using something like vdata_\d+ to find it. I don't have any concerns with this change.

[thijstriemstra]

with this sinon stub in place, a change in src/js/tech/html5.js should be testable as well right?

[thijstriemstra]

with this sinon stub in place, a change in src/js/tech/html5.js should be testable as well right?

it actually seems the stub isn't initialized properly when moving it to the global-shims module.. any idea?

[gkatsev]

the stub in global-shim may be happening to early, it may be necessarily to do after every usage of useFakeTimers.
Looks like we may want to update our version of sinon finally since it seems a newer version of lolex the underlying fake timers lib stubs out performance.

[thijstriemstra]

Looks like we may want to update our version of sinon

sounds good.

[thijstriemstra]

Can you update sinon in the master branch?

[gkatsev]

if you use npm 6, it should update the package-lock file as well, you'd want to check it in. But yeah, we can take a look at updating sinon in a separate PR as it's possible things will break.

[thijstriemstra]

updating sinon in a separate PR as it's possible things will break.

I've opened #5953 for this. We shouldn't add workarounds for testing in prod code, but simply make sure the test env is setup properly.

[gkatsev]

@thijstriemstra I updated sinon in #5954 which is now in master, if you rebase/merge in master, it should fix the build.

[gkatsev]

Not sure what's up with netlify, but it isn't actually required to pass.

[thijstriemstra]

Ready for review.

[gkatsev]

Thanks @thijstriemstra!

[fermarci]

Hi, The usage performance.now is not "secure" in all occurrences.

In html5.js and dom-data.js it is fine.
But in:
fn.js -
64. let last = window.performance.now();
67. const now = window.performance.now();
live-tracker.js -
96. const newTime = Number(window.performance.now().toFixed(4));
components.js -
1221. touchStart = window.performance.now();
1259. const touchTime = window.performance.now() - touchStart;

in systems where Performance API is not available it will generate an error.

would be nice to fix this.

[thijstriemstra]

in systems where Performance API is not available

Which ones?

[fermarci]

Hi, we had problems on older LG and Samsung TV sets.

[thijstriemstra]

@fermarci you can use a polyfill for these devices: https://gist.github.com/paulirish/5438650#gistcomment-2976364