Adding a method to get the ratio of video length buffered compared to bufferingGoal #3392

surajkumar-sk · 2021-05-06T20:41:52Z

Description

closes #3389

Screenshots (optional)

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

I have signed the Google CLA https://cla.developers.google.com
My code follows the style guidelines of this project
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
I have verified my change on multiple browsers on different platforms
I have run ./build/all.py and the build passes
I have run ./build/test.py and all tests pass

updating my repo

Updating my repo

ui/controls.js

joeyparrish · 2021-05-07T18:12:06Z

ui/controls.js

+    if (bufferedEnd_ >= bufferingGoal_) {
+      return 100;
+    } else if (bufferedEnd_ < bufferingGoal_) {
+      return ((bufferedEnd_/bufferingGoal_)*100);


This calculation is wrong, because it doesn't account for the starting point. The start of the range should be currentTime, not 0.

@joeyparrish the formula of bufferingGoal_ does account for the starting point. I named the variable wrong, I'll try coming up with a better name. I am bad at it so I am open to suggestions.

I think the formula is wrong . The fraction should be between buffering goal and the length buffered after the current time . So the formula will be (bufferedEnd_-currentTime)/bufferingGoal

ui/controls.js

joeyparrish · 2021-05-11T17:30:03Z

lib/player.js

+  getBufferFullness() {
+    const bufferedLength_ = this.video_ ?
+        this.video_.buffered.length : 0;
+    const bufferedEnd_ =


Style-wise, we don't end local variables with underscores. That is the convention for private member variables.

joeyparrish · 2021-05-11T17:31:49Z

lib/player.js

+      } else if (bufferedEnd_ <= this.video_.currentTime) {
+        return 0;
+      } else if (bufferedEnd_ < lengthToBeBuffered) {
+        return ((bufferedEnd_ - this.video_.currentTime) / bufferingGoal_);


This calculation looks correct to me now. Thanks!

lib/player.js

joeyparrish · 2021-05-11T18:03:42Z

lib/player.js

+      } else if (bufferedEnd <= this.video_.currentTime) {
+        return 0;
+      } else if (bufferedEnd < lengthToBeBuffered) {
+        return ((bufferedEnd - this.video_.currentTime) / bufferingGoal);


On further consideration, I think this is not quite right. In the edge case where seekRange.end() limits buffering, this would not approach 1.0, because the buffered amount would never reach bufferingGoal.

I think it should probably be:

return (bufferedEnd - this.video_.currentTime) / (lengthToBeBuffered - this.video_.currentTime);

Does that make sense?

Yes, it makes complete sense. I used this formula in my previous commit thinking about that edge case but when I rechecked later I couldn't recall the exception case and changed the formula.
Thank you

surajkumar-sk · 2021-05-11T18:38:26Z

@joeyparrish Thank you for being this patient, From next time I'll make sure I avoid any extra conditions, lines, variables, and their names.
It's really embarrising writing the formula wrong after doing competitive programming for years.
This is also a very new experience, I never really cared much about few extra conditions, random laying variables, and their names. Only time and memory mattered. But I was just dealing with few 100 lines, now I realize the importance of every comment, every extra line, and how difficult things can get if the code is not properly structured.
Well as Sheldon once said "Change is the only constant" and I'll change my ways of writing code.
Thank you again.

joeyparrish · 2021-05-11T19:05:47Z

Thank you for being this patient

No, thank you for persevering! We appreciate our contributors, and it can take time to learn how we do things.

Well as Sheldon once said "Change is the only constant"

Oh, I beg to differ: https://en.wikipedia.org/wiki/List_of_mathematical_constants 😁

surajkumar-sk · 2021-05-11T19:12:01Z

@joeyparrish

Oh, I beg to differ: https://en.wikipedia.org/wiki/List_of_mathematical_constants

😂😂😂

The lines in PR #3373 are 412 but the changes are pretty small. Is there a reason for delaying the review?
It's not a hurry but just wanted to know the reason for the delay.
Thank you.

joeyparrish · 2021-05-11T20:11:16Z

The lines in PR #3373 are 412 but the changes are pretty small. Is there a reason for delaying the review?
It's not a hurry but just wanted to know the reason for the delay.

We just haven't gotten to it yet. It's hard to keep up with all PRs, and with everything we juggle as a team (beyond this one project), we're not very consistent. Please accept our apologies.

surajkumar-sk · 2021-05-11T20:18:21Z

Ohh. I understand. No problem.

joeyparrish · 2021-05-11T20:24:52Z

Oops! I just realized I forgot to run the build bot before I merged this. We really need to migrate to GitHub Actions and make that fully automated.

I've just kicked off a new test run. If the linter or tests fail, we may have to revert the change or quickly fix it in a follow-up change, depending on the severity.

surajkumar-sk · 2021-05-11T20:28:57Z

I was surprised too seeing the changes being merged without the bot run .
But i did run the linter before pushing changes.

PR #3392 Issue #3389 Change-Id: I425ccf7036416c185be141a181fabd26aa92e41e

joeyparrish · 2021-05-11T21:23:09Z

Just fixed the failing test in 0a62396. Thanks again!

PR #3392 Issue #3389 Change-Id: I425ccf7036416c185be141a181fabd26aa92e41e

Closes #3389

PR #3392 Issue #3389 Change-Id: I425ccf7036416c185be141a181fabd26aa92e41e

This reverts commit 9c0126b. This should not have been cherry-picked to a release branch.

surajkumar-sk and others added 3 commits May 5, 2021 11:25

Merge pull request #1 from google/master

26dc16c

updating my repo

Merge pull request #2 from google/master

68499bf

Updating my repo

added method for checking video efficiency

9a2fcf3

joeyparrish requested changes May 7, 2021

View reviewed changes

ui/controls.js Outdated Show resolved Hide resolved

surajkumar-sk requested a review from joeyparrish May 7, 2021 20:19

surajkumar-sk changed the title ~~Adding a method for checking efficiency~~ Adding a method to check if the downloading can keep pace with the video playback May 7, 2021

Update player.js

f075f84

surajkumar-sk changed the title ~~Adding a method to check if the downloading can keep pace with the video playback~~ Adding a method to check if the downloading is keeping pace with the video playback May 8, 2021

surajkumar-sk added 4 commits May 8, 2021 01:47

updated changes

3d69e48

updated player.js

073e0c9

updated player.js

83262a3

updated player.js

245a3b2

surajkumar-sk changed the title ~~Adding a method to check if the downloading is keeping pace with the video playback~~ Adding a method to get the ratio of video length buffered compared to bufferingGoal May 11, 2021

joeyparrish requested changes May 11, 2021

View reviewed changes

surajkumar-sk requested a review from joeyparrish May 11, 2021 17:56

joeyparrish requested changes May 11, 2021

View reviewed changes

surajkumar-sk requested a review from joeyparrish May 11, 2021 18:38

updated comment in player.js

ad964c8

joeyparrish approved these changes May 11, 2021

View reviewed changes

joeyparrish merged commit 0f00d1c into shaka-project:master May 11, 2021

surajkumar-sk deleted the bufferfill branch May 11, 2021 19:54

shaka-bot pushed a commit that referenced this pull request May 11, 2021

fix: Fix new method missing from cast proxy

0a62396

PR #3392 Issue #3389 Change-Id: I425ccf7036416c185be141a181fabd26aa92e41e

surajkumar-sk added 2 commits May 11, 2021 23:23

updated variables

ff4979f

updated changes

3172ce0

ismena pushed a commit that referenced this pull request May 17, 2021

fix: Fix new method missing from cast proxy

c9d394d

PR #3392 Issue #3389 Change-Id: I425ccf7036416c185be141a181fabd26aa92e41e

ismena pushed a commit that referenced this pull request May 17, 2021

fix: Fix new method missing from cast proxy

0102cba

PR #3392 Issue #3389 Change-Id: I425ccf7036416c185be141a181fabd26aa92e41e

ismena pushed a commit that referenced this pull request May 17, 2021

feat: Add a buffer fullness method (#3392)

9c0126b

Closes #3389

ismena pushed a commit that referenced this pull request May 17, 2021

fix: Fix new method missing from cast proxy

1c02c58

PR #3392 Issue #3389 Change-Id: I425ccf7036416c185be141a181fabd26aa92e41e

joeyparrish added a commit that referenced this pull request Jun 16, 2021

Revert "feat: Add a buffer fullness method (#3392)"

9aaf55b

This reverts commit 9c0126b. This should not have been cherry-picked to a release branch.

renovate bot mentioned this pull request Jul 14, 2021

Update dependency shaka-player to v3.2.0 EtherealEngine/etherealengine#2442

Merged

1 task

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a method to get the ratio of video length buffered compared to bufferingGoal #3392

Adding a method to get the ratio of video length buffered compared to bufferingGoal #3392

surajkumar-sk commented May 6, 2021 •

edited

Loading

joeyparrish May 7, 2021

surajkumar-sk May 7, 2021

surajkumar-sk May 7, 2021 •

edited

Loading

joeyparrish May 11, 2021

surajkumar-sk May 11, 2021

joeyparrish May 11, 2021

joeyparrish May 11, 2021

surajkumar-sk May 11, 2021

surajkumar-sk commented May 11, 2021

joeyparrish commented May 11, 2021

surajkumar-sk commented May 11, 2021

joeyparrish commented May 11, 2021

surajkumar-sk commented May 11, 2021

joeyparrish commented May 11, 2021

surajkumar-sk commented May 11, 2021 •

edited

Loading

joeyparrish commented May 11, 2021

Adding a method to get the ratio of video length buffered compared to bufferingGoal #3392

Adding a method to get the ratio of video length buffered compared to bufferingGoal #3392

Conversation

surajkumar-sk commented May 6, 2021 • edited Loading

Description

Screenshots (optional)

Type of change

Checklist:

joeyparrish May 7, 2021

Choose a reason for hiding this comment

surajkumar-sk May 7, 2021

Choose a reason for hiding this comment

surajkumar-sk May 7, 2021 • edited Loading

Choose a reason for hiding this comment

joeyparrish May 11, 2021

Choose a reason for hiding this comment

surajkumar-sk May 11, 2021

Choose a reason for hiding this comment

joeyparrish May 11, 2021

Choose a reason for hiding this comment

joeyparrish May 11, 2021

Choose a reason for hiding this comment

surajkumar-sk May 11, 2021

Choose a reason for hiding this comment

surajkumar-sk commented May 11, 2021

joeyparrish commented May 11, 2021

surajkumar-sk commented May 11, 2021

joeyparrish commented May 11, 2021

surajkumar-sk commented May 11, 2021

joeyparrish commented May 11, 2021

surajkumar-sk commented May 11, 2021 • edited Loading

joeyparrish commented May 11, 2021

surajkumar-sk commented May 6, 2021 •

edited

Loading

surajkumar-sk May 7, 2021 •

edited

Loading

surajkumar-sk commented May 11, 2021 •

edited

Loading