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
Fixes demo graphs for improved bandwidth estimation #4904
Fixes demo graphs for improved bandwidth estimation #4904
Conversation
demo/main.js
Outdated
if (data.frag.type !== 'main' || data.frag.sn === 'initSegment') { | ||
return; | ||
} |
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 assume this check was based on this code.
// Only count non-alt-audio frags which were actually buffered in our BW calculations
if (frag.type !== PlaylistLevelType.MAIN || frag.sn === 'initSegment') {
return;
}
This is to only use the larger segments to get a more accurate line speed in our EWMA sample. However, this data should have all segments so I'm not sure this is necessary.
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 assume this check was based on this code.
Exactly.
This is to only use the larger segments to get a more accurate line speed in our EWMA sample. However, this data should have all segments so I'm not sure this is necessary.
That's a good question. I used the demo to verify the changes made to the bw estimation and found it counterintuitive that the graph shows different data than the bw estimator uses internally. Adding console.logs instead to get a better understanding of what is happening.
However I can see that the graph might show the bandwidth of every request regarding what is being selected for estimation.
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 making the change so the bitrate is calculated the same is a good change and should definitely be made.
What do you think about simply grabbing the calculated EWMA and showing it on the metrics page? That way you would get the exact data, and we wouldn't need to re-calculate the demo page if we ever changed the way the metric was calculated.
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 believe I understand that you want the bandwidth (bw
) value calculated in the event to accurate represent what hls.js would have sampled into the EWMA class, is that accurate?
If that's true, then I think the direction you have with the calculation for processingMs and using stats.loaded
is on track.
Some of the early exits are changing which fragments will be stored as data on the demo page, which I don't think is ideal.
Thanks for the review i will work through your feedback and update the PR next week. |
Agree. What do you think would be good solution? Adding a new graph comes to my mind. |
…ents and higher TTFB Fixes video-dev#3578 (special thanks to @Oleksandr0xB for submitting video-dev#4283) Fixes video-dev#3563 and Closes video-dev#3595 (special thanks to @kanongil)
fe62282
to
b4b3bfd
Compare
@silltho I think you might need to rebase against |
Yeah, rather than an entirely new graph, what do you think about directly getting the EWMA value and add it directly on the bitrate graph as a new value? Leaving the bitrate graph showing all requests, and the moving bitrate. This would allow you to easily see the difference between the raw bitrate/s average required, versus what the EWMA is reporting in code for selection. |
713664d
to
ec2d213
Compare
ec2d213
to
4507a0c
Compare
@itsjamie I've added the estimated bandwidth as yellow line to the bitrate graph. I had to to increase the height of the graph (not sure if this has an impact on something) |
6f19423
to
1672028
Compare
Was fixed with an early return on details being defined in master.
Just did a rebase and fixed the conflicts, looks like the core changes have all been merged already, so nice finding those @silltho. I'm running the demo changes locally while the CI runs. |
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 great! Thank you.
Why is this Pull Request needed?
The demo calculates the bandwidth for each fragment separately from the abr-controller. Therefore it needs to be updated to ensure that the same values are used for the bandwidth estimation and the bandwidth graph on the demo page.
Are there any points in the code the reviewer needs to double check?
Resolves issues:
Checklist