Skip to content
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

Fix average bandwidth value from decaying when the system bandwidth being constant #1137

Conversation

KVPasupuleti
Copy link
Contributor

Description

In the movingAverageBandwidthSelector method, we are using this line to calculate the average.

average = decay * this.systemBandwidth + (1 - decay) * average;

I see a problem here.

To explain with an example,

I have an average value of 1000 currently and a new chunk got downloaded with a bandwidth of 500. And the next chunk is downloaded after 1 second.

The systemBandwidth is updated after each chunk gets downloaded.

So, my expectation is this 500 should be contributed to the average only once until the next chunk gets downloaded.

But, as the checkABR method is getting called for every 250ms, this 500 is getting contributed 4 times until the next chunk gets downloaded, by which the average value is getting decreased gradually because the systemBandwidth is being constant until then.

You can check the logs attached below.

Video-js-ABR.log

Specific Changes proposed

  • Change the average value only if the systemBandwidth changes and is greater than zero.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Jun 3, 2021

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #1137 (fd3bd54) into main (0be51eb) will increase coverage by 0.26%.
The diff coverage is 94.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1137      +/-   ##
==========================================
+ Coverage   86.20%   86.47%   +0.26%     
==========================================
  Files          39       39              
  Lines        9289     9501     +212     
  Branches     2127     2191      +64     
==========================================
+ Hits         8008     8216     +208     
- Misses       1281     1285       +4     
Impacted Files Coverage Δ
src/manifest.js 93.06% <37.50%> (-4.79%) ⬇️
src/videojs-http-streaming.js 90.30% <66.66%> (-0.29%) ⬇️
src/playlist.js 94.11% <89.74%> (-0.39%) ⬇️
src/playlist-loader.js 93.73% <91.11%> (+2.09%) ⬆️
src/media-segment-request.js 95.72% <98.52%> (+0.99%) ⬆️
src/master-playlist-controller.js 94.51% <100.00%> (+0.20%) ⬆️
src/media-groups.js 98.66% <100.00%> (-0.32%) ⬇️
src/playlist-selectors.js 86.07% <100.00%> (+0.84%) ⬆️
src/segment-loader.js 96.18% <100.00%> (+0.36%) ⬆️
src/sync-controller.js 96.89% <100.00%> (+0.16%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55726af...fd3bd54. Read the comment docs.

@gkatsev
Copy link
Member

gkatsev commented Jun 18, 2021

Wrote a test for this #1141
Once the other PR passes review (I'm doing something I'm unsure about) I'll get all the stuff merged in.

gkatsev added a commit that referenced this pull request Jun 22, 2021
@gkatsev gkatsev merged commit c22749b into videojs:main Jun 22, 2021
@welcome
Copy link

welcome bot commented Jun 22, 2021

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants