-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add getVideoPlaybackQuality API #4286
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also make an "@abstract" method in tech.js
that returns an empty object or something.
src/js/player.js
Outdated
* is no tech or the tech does not support it. | ||
*/ | ||
getVideoPlaybackQuality() { | ||
if (this.tech_ && this.tech_.getVideoPlaybackQuality) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's the techGet_
helper that you should use instead.
Updated to use |
Added abstract method |
@gkatsev PWEASE? |
src/js/player.js
Outdated
* Gets available media playback quality metrics as specified by the W3C's Media | ||
* Playback Quality API. | ||
* | ||
* @see https://wicg.github.io/media-playback-quality/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elsewhere we use something like:
/*
* @see [Spec]{@link https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-play}
*/
And in the other comments.
window.performance.timing && | ||
typeof window.performance.timing.navigationStart === 'number') { | ||
videoPlaybackQuality.creationTime = | ||
window.Date.now() - window.performance.timing.navigationStart; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we decide to have no value for creationTime
if timing.navigationStart
isn't available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By looking at the tests, I guess the answer is yes.
|
||
const videoPlaybackQuality = {}; | ||
|
||
if (typeof this.el().webkitDroppedFrameCount !== 'undefined' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible for only one to be available but not the other?
Also, do we want similar thing for the firefox mozilla prefixed items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Judging by https://bugs.webkit.org/attachment.cgi?id=80515&action=diff (which I think is the right patch), the webkit prefixed properties were added at the same time.
As for the mozilla prefixed properties, there do seem to be some:mozParsedFrames
, mozDecodedFrames
, mozPresentedFrames
, mozPaintedFrames
, and mozFrameDelay
, but since they aren't 1:1 matches, I'm not sure it would be worth trying to use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I assume a PR against master will be coming soon now?
Created Video.js 6 PRs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty straightforward. I didn't look too deeply at the tests, though.
Description
NOTE: This requires videojs/video-js-swf#225
Provides a tech getter for getVideoPlaybackQuality as specified by the W3C's Media Playback Quality API: https://wicg.github.io/media-playback-quality/
Requirements Checklist