-
Notifications
You must be signed in to change notification settings - Fork 424
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
feat: stats for timeToLoadedData, appendsToLoadedData, mainAppendsToLoadedData, audioAppendsToLoadedData, and mediaAppends #1106
Conversation
src/master-playlist-controller.js
Outdated
this.timeToFirstFrame__ = 0; | ||
this.appendsToFirstFrame__ = 0; | ||
|
||
this.tech_.one('canplay', () => { |
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.
once we 'canplay' a frame will be showing.
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.
Apparently this is close to a "time to first byte" since if we are paused, the video may not get rendered immediately. playing
or first timeupdate
is potentially closer to a time to first rendered frame if we are autoplaying/not paused.
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 this experimental API that can better represent the frame getting rendered https://wicg.github.io/video-rvfc/
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.
switched it to loadeddata
https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/loadeddata_event
as it specifically says: "The loadeddata event is fired when the frame at the current playback position of the media has finished loading; often the first frame."
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.
that seems more like time to first byte rather than time to first frame, the latter implies that the frame has rendered which loadeddata doesn't guarantee it.
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.
Looked even deeper and tested on a few browsers, It seems like listening to loadeddata
might be as close as we are going to get. For preload === 'none'
we will have to start our timer on play
but for all other values we should be able to start it on loadstart
. Using those values gives somewhat consistent timeToFirstFrame
for me.
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.
I guess what do you consider timeToFirstFrame
? Is it the data has been loaded into the video element or the frame has rendered visibly to the user?
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.
Since we don't have a good way to determine if the frame has been rendered visibly to the user (except for the experimental API), I think time to first frame should be when the video element reports that it has a buffered
region containing currentTime
. That might be a few ms before the frame is visible to the user, but I think it's the best cross browser solution.
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.
Makes sense. I would probably want to rename the field then. We should also make sure to document exactly what it means so that we don't get confused in the future and have a place to reference it.
Also, does live have anything special for this beyond being equivalent to preload=none?
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.
maybe timeToLoadedData
and appendsToLoadedData
? Live shouldn't be any different for VOD for these stats.
Codecov Report
@@ Coverage Diff @@
## main #1106 +/- ##
==========================================
+ Coverage 86.38% 86.43% +0.05%
==========================================
Files 39 39
Lines 9435 9465 +30
Branches 2174 2182 +8
==========================================
+ Hits 8150 8181 +31
+ Misses 1285 1284 -1
Continue to review full report at Codecov.
|
66eaf5f
to
4b2ecd0
Compare
src/segment-loader.js
Outdated
@@ -2968,6 +2969,8 @@ export default class SegmentLoader extends videojs.EventTarget { | |||
// used for testing | |||
this.trigger('appended'); | |||
|
|||
this.mediaAppends++; |
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.
This will show extra appends for:
- segments with no data to be appended
- sync requests that did not get appended
It will also under-report total number of appends, since we potentially append multiple times per segment.
Do we want to consider those cases and think about naming as "segmentAppends" since that may denote full segments?
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.
sync requests that do not get appended will return much earlier in the function. We could wrap mediaAppends
increment in segmentInfo.hasAppendedData
?
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.
the other problem with calling it segment appends is that we now append parts too.
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.
👍 on sync requests
On wrapping and part appends, what is our use-case for this data? Do we want to know actual appends, or are we more concerned about requests?
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.
I think we want to know actual appends, we already have stats for requests.
Description
Add the mentioned stats