Make goalBufferLength and bufferLowWaterLine use time since last seek for calculation

[squarebracket]

Description

Attempt to fix the problem where the ABR will select the lowest quality on seek. It should instead pick the last quality (or something similar).

Specific Changes proposed

Instead of using the currentTime to determine the buffer goal details, use the time elapsed since the last seek (or start, if there was no seek).

Requirements Checklist

• Feature implemented / Bug fixed
• If necessary, more likely in a feature request than a bug fix
  • Unit Tests updated or fixed
  • Docs/guides updated
  • Example created (starter template on JSBin)
• Reviewed by Two Core Contributors

---

There's still some problems. The quality drops significantly for a couple seconds sometimes before it's kicked back to an appropriate rate. Sometimes it'll start playing and stall for a second split second then resume.

[squarebracket]

Added cool off for a segment duration post-seek. Let me know if you want me to add any tests.

[mjneil]

A test for the cooloff behavior and a test for elapsedSinceStart behavior would be great

[squarebracket]

I added tests for the dynamic buffer stuff and elapsedSinceStart. It'll probably take me a while to figure out how to write a test for the cool-off behaviour. I saw this test but couldn't really figure out what's going on. I guess I'll see if I can find a test that uses the actual download workflow since I think that's what needed here.

On another note, is there a test for aborting when the buffer is low? i.e. like this but for buffer size? I saw a test for the logic inside minRebuffer... but not for the abort logic.

[mjneil]

I believe XHR within the tests do not trigger progress events on their own, so you will have to do something similar to the test you linked by manually waiting clock ticks and triggering progress events when you want them. Basically what is happening in that test is mimicking the behavior of playback start, where the request is the first segment, and you basically take the loaded property divided by the number of clock ticks that have passed to get the calculated bandwidth.

So

tick(999)
loaded: 2000 // 2000bytes/999ms
tick(2)
loaded(2001) // 2001bytes/1001ms

At which point the code decides to abort early.

I would say you could use this test as the ground work for the cool-off test. Basically before the progress events for the first test, you'd want to mock seeking to true, check that its not aborted from progress events even though it would've if seeking was false, then youd want to complete the request and make seeking false again so another gets started, then show that it does not abort that one because of the cool-off. Finally I would then complete that request and use enough clock ticks so that you get past the cool-off period and show that it will abort again post cool-off if bandwidth is still low

It looks like there isn't a specific test for when there is forward buffer but it is low. The test you linked doesn't seem to mock anything with the buffer so its basically testing the already buffering case

[squarebracket]

Why doesn't it trigger at the first (well, second) progress event? 2B/s is lower than all the playlist bandwidths.

[squarebracket]

Oh... wait... it's 2000B/s..... nevermind. I can't math good :)

[mjneil]

You were right the first time with 2B/s :) The reason it does not trigger right away is that it waits until it has been a full second since the first progress event before considering an abort because we've seen that progress events have an initial ramp-up period where the calculated bandwidth is very innacurate and tend to stabilize after a second

[squarebracket]

Oh, my bad, I thought it was "wait until first byte", not "wait until 1 second after first byte".

[squarebracket]

@mjneil you meant something like this, right? Note, I had to tick 1000ms first so that I could reach the seeking code path.

[mjneil]

nice test, ya thats what I meant

[gesinger]

Thanks for the PR!

I didn't review the noAbortsUntil_ component of the PR yet, as I was wondering if it would make sense as a separate PR, or is needed for the elapsed time change.

[gesinger]

Minor, but since this is time elapsed since either start or last seek, maybe we should leave it somewhat generic and call it elapsedTime()?

[squarebracket]

Sure, if you think that's descriptive enough, I'm fine with changing it.

[gesinger]

It probably is still a bit ambiguous, I may just be hooked onto it reading Time somewhere

[squarebracket]

Yeah, it's hard to think of a good name that's not super long. I'd say maybe like a timeSinceEmptyBuffer but I'm not sure that accurately encapsulates the purpose of the function.

[squarebracket]

Renamed to elapsedTime.

[gesinger]

Preference, but the function could just be return this.tech_.currentTime() - this.startTime_

[squarebracket]

Good point, not sure why I did it like that.

[gesinger]

Would this change make sense as part of a separate PR?

[squarebracket]

Consider seeking to a time which is right before a segment ends. The segment will download and be appended to the buffer, and you'll seek within that buffer to right before its end. Without this, I imagine the rate selection logic will abort the download for the higher rate since the buffer is so low.

[gesinger]

Isn't that an issue that's present without the other change?

[squarebracket]

Ok, I went back and verified the behaviour to make sure I clearly remember what each change does. In short, the GBL change makes the time spent at a lower rate smaller, and the noAbortsUntil removes it completely. So, even though they're disjoint, without the noAbortsUntil addition, there's still a drop to the lowest quality when seeking.

I'm gonna use the example above again but go a bit more into detail about what's actually happening so you can understand what's actually changing.

Current behaviour
(playing at high rate)
-> seek to right before segment ends
-> ABR selects the lowest rate because buffer is so low
-> player buffers a few segments (6 in my case) at lowest rate
(playback resumes after 1st segment complete)
-> player starts buffering higher rate segments
(resume normal behaviour)

With GBL change
(playing at high rate)
-> seek to right before segment ends
-> ABR selects the lowest rate because buffer is so low
-> player buffers 1 segment at lowest rate
(playback resumes)
-> player starts buffering higher rate segments
(resume normal behaviour)

With noAbortsUntil
(playing at high rate)
-> seek to right before segment ends
-> ABR selects current rate because of noAbortsUntil
-> player buffers 1 segment at current rate
(playback resumes)
-> player starts buffering higher rate segments
(player stalls because it can't download the next segment fast enough)
(playback resumes when the next segment finishes downloading)
(resume normal behaviour)

Notice the tradeoff with the noAbortsUntil -- that the player stalls.

I considered the changes "omnibus" because in my mind I was trying to fix the downswitch in quality when seeking, which required the noAbortsUntil. But I have no problem separating them -- I can see why the noAbortsUntil change would be contentious, and the behaviour is still improved with just the GBL changes.

LMK what you think is best.

[gesinger]

Thanks for all the examples. I see what you mean. It's an interesting question, and we may want more input on it. One thing we may also want to consider is we do have the minimumTimeSaving variable which is adjusted in the case where we're already rebuffering, which should count for the case of seeking.

[squarebracket]

So... is that a "yes, split them up"?

Setting minimumTimeSaving to be a segmentDuration for 2 segments after a seek would be a clever way of keeping the same rate but still aborting on too-long downloads. Of course, if you did get an abort, you'd still be kicked down to the lowest rate; the playlist selection logic will always choose the lowest rate since it'll have the least rebufferingImpact.

[gesinger]

Yeah, I think the stalling issue after 1 second of playback is what we should avoid.

In my mind, the perfect case would be: you seek, we load the same quality. If it's going to take a while we can still wait, up to a point (though that "point" is not well defined).

I think the "best" answer would be to change our seeking behavior such that we only resume playback after we have enough segment data to continue playback without a stall for our current throughput and rendition. If that takes (or will take) in excess of X time, then we consider downswitching.

However, that case may be tricky to implement. In the meantime, I don't think the abort early with downswitching is a bad situation, so long as we avoid playback stalls.

[squarebracket]

Removed the noAbortsUntil_ stuff

[gesinger]

This is technically not after a seek, right?

[squarebracket]

Care to elaborate? This test exists to make sure the dynamic GBL works as expected after a seek. I'm making the assumption in this PR that a seek will clear the buffer, so we should treat that as if we've started fresh. So here I'm seeking via setCurrentTime and testing to see that GBL responds in the same way as it does when starting from a fresh playback.

[gesinger]

My mistake. I saw the seek a couple tests higher and assumed the test after had to do with the seek, but then following tests had to do with just normal playback from that point (since only currentTime changed, but not via setCurrentTime again), but you're right, it's still after a seek.

[gesinger]

This is technically not after a seek, right?

[squarebracket]

Same comment as above.

[squarebracket]

Made the changes and rebased. Changes since the last review are in commit 4509642 and forwards.

[forbesjo]

Thank you for your PR but we will not be accepting new changes for this repo and will be archived very soon. I would advise you to open your PR against the next iteration of this project at https://github.com/videojs/http-streaming.