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

TimeRangeUtils bufferedAheadOf breaks with >1 buffer #472

Closed
baconz opened this issue Aug 2, 2016 · 10 comments
Closed

TimeRangeUtils bufferedAheadOf breaks with >1 buffer #472

baconz opened this issue Aug 2, 2016 · 10 comments
Assignees
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@baconz
Copy link
Contributor

baconz commented Aug 2, 2016

Imagine two buffers:

b0: 0 -> 10
b1: 11 -> 20

Given a time of 8, the current code would return a bufferedAhead of 2, when it should return 11.

I think something like this would fix it:

  var buffered = 0;
  var in_range = false;
  for (var i = 0; i < b.length; ++i) {
    if (time + fudge >= b.start(0) && time < b.end(i)) {
      buffered += b.end(i) - time;
      in_range = true;
    } else if (in_range) {
      buffered += b.end(i) - b.start(i)
    }
  }
  return buffered;
@joeyparrish joeyparrish added the status: working as intended The behavior is intended; this is not a bug label Aug 2, 2016
@joeyparrish
Copy link
Member

I'm sorry, this is working as intended. It was never intended to jump gaps that browsers will not jump at play time.

Would changing this solve a problem for you? I'd be happy to discuss.

@baconz
Copy link
Contributor Author

baconz commented Aug 3, 2016

Thanks @joeyparrish. I was playing around with some video that had small gaps due to the PTS/DTS bug, and the behavior seemed unexpected. Given that some browsers can recover from small gaps, doesn't it seem like the player should acknowledge media that's buffered past a gap?

Is there an enhancement to be had here? I guess this is related to #180.

@joeyparrish
Copy link
Member

Since we don't have any direct way to know how large a gap any given browser will jump, we can't really treat that media as buffered.

StreamingEngine only queries the buffered range when initializing or switching streams, though, so small gaps should not be a problem while streaming. Once streaming begins, segments are fetched and appended in order without examining the buffered range. If a gap can be jumped, it will, and StreamingEngine won't notice.

Where this breaks down is when you enter a buffering state (underflow). We don't exit that buffering state until a certain amount is buffered. Then we're checking the buffered range on an interval, and that condition may never be met due to the gap.

@baconz
Copy link
Contributor Author

baconz commented Aug 4, 2016

Yeah, this popped up when I seeked to a point just before the start of a period that introduced a gap.

The player would enter a buffering state, and it would never recover because it thought that it had nothing buffered ahead. Worse yet, it would try to buffer the same segments over and over again, which ultimately led to a QUOTA_EXCEEDED_ERROR. @joeyparrish can you think of a way to protect against this? It seems reasonable that somebody might accidentally introduce a gap, and the player shouldn't react by attacking your CDN with segment requests.

Do you see a problem with Shaka behaving optimistically and assuming that the browser will play through the gap? It seems like it should honor GAP_OVERLAP_TOLERANCE and treat buffered ranges with gaps smaller than that value as continuous.

@joeyparrish joeyparrish added type: bug Something isn't working correctly and removed status: working as intended The behavior is intended; this is not a bug labels Aug 4, 2016
@joeyparrish joeyparrish added this to the v2.0.0 milestone Aug 4, 2016
@joeyparrish
Copy link
Member

That's a good point. We definitely shouldn't be re-requesting the same segment over and over.

I don't see any reason we couldn't create a separate GAP_TOLERANCE constant in TimeRangesUtils, but give us some time to investigate. We need to make sure there's not some other pathology causing the repeated requests, and we need to make sure that ignoring small gaps will not cause some other problem elsewhere in the system.

@ismena ismena self-assigned this Aug 10, 2016
shaka-bot pushed a commit that referenced this issue Aug 25, 2016
time and playhead time.

This prevents streaming engine from requesting more segments
if it encounters a gap.

Related to #472.

Change-Id: Ie9fac1b427b631ac467656c15fd8608e12c07efc
@joeyparrish
Copy link
Member

@baconz, we believe this is fixed now. Let us know if you have any further issues.

@baconz
Copy link
Contributor Author

baconz commented Aug 25, 2016

This looks great @joeyparrish. Thanks.

Wouldn't there still be a problem with getting into fetch-loops? Should a gap larger than GAP_TOLERANCE throw an error?

@joeyparrish
Copy link
Member

We decided against throwing. There should be no fetch loop now that we have changed the way we compare against buffering goals.

@baconz
Copy link
Contributor Author

baconz commented Aug 25, 2016

Sweet, sounds good.

@ismena
Copy link
Contributor

ismena commented Aug 25, 2016

Commit that changes buffering goal comparison for reference.

@shaka-project shaka-project locked and limited conversation to collaborators Mar 22, 2018
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

4 participants