Retain buffer when downgrading bitrate

[jonoward]

[We were planning on submitting this as a PR, as we implemented this in our fork and found it useful in reducing rebuffering; but seems that it may be worth waiting for the refactoring to land. Thought it might be useful just to raise it now]

When changing bitrates, the player currently starts buffering at the new bitrate as soon as it can (based on an estimate on how far to offset the segment index based on the current bandwidth etc). This makes sense for upgrading bitrates, as you want the user to get the better quality bitrate as soon as possible.

But for downgrading bitrate, we may as well let the user watch all of the higher-quality media currently in the buffer, i.e. set the offset at the end of the current buffer. This was probably more noticeable for us as we set a relatively large buffer - 2 minutes - so if the buffer is full, we want to let that media play out.

As a side effect, it reduces the pressure on the buffer, as any bandwidth fluctuations that occur just after a bitrate switch are then less likely to cause a rebuffer, which I think is why we saw an improvement in our stats after adding the behaviour.

[joeyparrish]

Thanks for bringing this up. We're about to publish something similar to this in the next stage of #51. We are no longer going to clear the buffer at all when switching bitrates.

More changes might be needed to implement exactly what you described. Did I understand you correctly that you want switches to higher quality to overwrite the buffer at the playhead, while switches to lower quality content are appended at the end of the buffer?

[jonoward]

Yeah that's pretty much it, when switching to a higher quality we want to switch as soon as possible (so retain just enough buffer in front of the playhead to avoid a rebuffer), but when switching to a lower quality, we may as well play out the buffer (which should be filled with better quality media), so we don't discard any of the buffer, and we append new fragments to the end of the buffer.

[joeyparrish]

The patch we are finishing now will allow something close to this, but not quite. We have a parameter on the video source's switch() method which allows the caller to optionally clear the buffer when switching representations.

One of the outcomes of clearing the buffer is that it naturally requires content to be inserted at video.currentTime, but this results in a noticeable stutter. We would use this flag, for example, when a user manually selects a fixed resolution (disabling ABR), at which point it is acceptable. But for an automated switch, we would obviously want to avoid the stutter.

So it sounds like you would need some way to ask that the content be appended at the current time without clearing the source buffer. SourceBufferManager has two different layers it can query: a map (soon to be list) of inserted segments, and the buffered time ranges of the underlying SourceBuffer. If you were able to say, on switch, that we should forget all (or most) of the content we've already inserted, but not remove it from the SourceBuffer, the result would be that we would start downloading segments that replace those already in the SourceBuffer, but playback wouldn't be interrupted.

[jonoward]

Yes that would work, if we could instruct SBM to forget about all the segments X seconds in front of currentTime, and only apply this rule when upshifting to a higher quality bitrate, that would work for us. Not sure what the value of X should - would probably be ok to be a constant value, e.g. 5-10 seconds, just needs to be a long enough period to download the new initialisation segment and at least one other segment.

[joeyparrish]

You could estimate the best value for X based on the new bitrate & current bandwidth estimate if you wanted to. X = sizeOfSegmentInSeconds * newBandwidthRequirement / currentBandwidthEstimate * fudgeFactorGreaterThanOne. This would be a little larger than the amount of time it would take to download the first segment from the new representation.

[joeyparrish]

v1.4.0 is out now, which no longer clears the buffer by default when switching bitrates. A custom IAbrManager implementation can decide whether or not to clear the buffer via the clearBuffer parameter to IVideoSource.selectVideoTrack().

We do not have an API to clear the buffer starting X seconds ahead, since we do not have an IAbrManager implementation that would make use of it at the moment. That being said, I'd be happy to either add this to our queue to work on for v1.5.0, or accept a pull request for it, whatever you prefer.

Design-wise, perhaps we should have a secondary API along-side selectVideoTrack() like clearBuffer(). Then you could compose the two without making selectVideoTrack more complicated:

videoSource.clearBuffer(video.currentTime + 10, Number.POSITIVE_INFINITY);
videoSource.selectVideoTrack(newTrackId);

[jonoward]

Hi Joey, sorry for the long delay in responding, we're just starting our migration to 1.4 now. I've been looking at your suggestion; do you think it would work for us to leave selectVideoTrack as-is (so no signature change) but also add additional clearVideoBuffer/clearAudioBuffer methods on StreamVideoSource, which we could then use in our custom AbrManager, after we have changed track? I'm happy to submit a PR for the additional methods.

So the pseudo-code in our custom AbrManager would look like:

videoSource_.selectVideoTrack(chosen.id, false); // same as in SimpleAbrManager, buffer not cleared

if (isNewVideoTrackHigherBandwidth) {
this.videoSource_.clearVideoBuffer(video.currentTime + 10, Number.POSITIVE_Infinity);
}

Thanks

[tdrews]

Hi,

My concern with generic clearAudioBuffer and clearVideoBuffer functions are that they would remove/reduce some simplifying assumptions made by Stream, e.g., "there are never any gaps in the buffer".

The safest way to support partial clears is to 1. only allow removing whole segments, and 2. only allow clearing the buffer from the video's current time (plus some offset if desired) to the end of the buffer.

Would something like this work?

/**
 * Clears the audio buffer forward from the segment boundary nearest the video's
 * current time plus |offset| seconds.
 */
VideoSource.clearAudioBuffer(offset) {
  // ...
};

I believe this function could be implemented in Stream without major complications, and would only require the addition of one small function to SourceBufferManager (to remove SegmentRefrences from |inserted_|).

If the "nearest segment boundary" part is too restrictive then I think we could remove this restriction, but it would require a slightly more clever change to SourceBufferManager.

Would something like VideoSource.clearAudioBuffer(offset) work for you? Or do you think you would still need something more generic?

[jonoward]

Yeah I think this will be fine for what we need to do, we don't need to create a gap, we do just want to clear the buffer after a certain time position. Also, aligning to the nearest segment boundary is fine for us. Thanks.

[tdrews]

Great. We're happy to accept a PR for this change :) But we can tackle this if you're short on time.

[jonoward]

Hi @tdrews, I've been working on this change, but have been running into some co-ordination issues in Stream + SBM; I'm trying to use a similar flow to Stream.resync in implementing the new Stream.clearBuffer method:

1. cancelUpdateTimer_()
2. sbm.abort()
3. • then smb.clearAfter(video.currentTime + offset) /* New method on SBM*/

4. - then setUpdateTimer_(0)
   

But the issue is that I'm ultimately calling this some time after StreamVideoSource.selectVideoTrack, which means the fetching process in onUpdate is underway and I think there is race-condition between SBM.fetch and SBM.clearAfter. Assuming I can fix this, do you think this general approach is right? Here is what we've done so far (on our fork): blinkbox@1327efe

An alternative is not to SBM.abort(), but simply to remove the entries from the inserted_ list, which causes fragments to be downloaded from that point (meaning that the actual data isn't removed from the native SourceBuffer, but merely replaced later on). It's simpler to do this, but feels a bit of a short-cut.

[tdrews]

Hey, thanks for doing the work on this.

I skimmed blinkbox@1327efe and I think your approach is correct, shaka.media.Stream.prototype.clearBuffer also looks fine (although perhaps you can just modify resync() to take a clear offset param?)

From your code, I don't see where the race is (between clearAhead and fetch, there may be one with clearAhead and resync though): if the update timer is canceled and the current SBM operation is aborted then no more fetches should occur until setUpdateTimer_(0) has been called. Is something specific going wrong?

An alternative is not to SBM.abort(), but simply to remove the entries from the inserted_ list...

I think doing the abort() is the right way to do it since if you're clearing the buffer ahead then whatever is being fetched should not be inserted since it will get cleared.

[jonoward]

Yup re-using resync with an additional "offset" param is cleaner. But I still get the same issue - it's a failed assertion at the top of resync():

shaka.asserts.assert((this.updateTimer_ != null) || this.fetching_);
/* this.updateTimer is null and this.fetching_ is false */

I think this is happening because I'm coming into resync after calling StreamVideoSource.selectVideoTrack which at some point cancels the update timer, so I think I'm calling resync during the switch, which I think is the race (if I delay the clearing by a short delay it doesn't happen). If selectVideoTrack returned a promise I could chain this to ensure the order. It looks like this could be possible, as the main method it uses - Stream.switch could probably return a promise (but the deferredSwitches could be tricky).

But this seems quite a large change, so another way for me to do it would be to do the buffer clear before calling StreamVideoSource.selectVideoTrack, which I think will work.

[tdrews]

I think this is happening because I'm coming into resync after calling StreamVideoSource.selectVideoTrack which at some point cancels the update timer ...

Just wondering, are you calling StreamVideoSource.selectVideoTrack(...) + resync(offset) in the middle of playback or during startup? I'm trying to fail the assertion myself, but I can't seem to do it.

Regardless, I think calling clear before switching could work, also wondering if we could add the clear ahead offset to switch itself? This may eliminate the race.

[jonoward]

These are being called in the middle of playback (when bitrate is being switched). Can also confirm that calling clearBuffer before selectVideoTrack does work without the assertion failing. But to your final point, yes it may be easier to wrap up the offset logic within the call to selectVideoTrack, which probably would avoid the race. I think the original idea was from Joey (see higher up on this thread) to split up the buffer clearing and switching, to avoid making selectVideoTrack too complicated, but given these issues maybe it's worth a shot? I'll try this out, will let you know how it goes.

[tdrews]

Design-wise I like

videoSource.clearBuffer(video.currentTime + 10, Number.POSITIVE_INFINITY);
videoSource.selectVideoTrack(newTrackId);

However, the issue is that if we separate the operations on Stream then SBM may begin downloading a segment that the ABRManager implementation doesn't want, i.e., SBM may begin downloading a segment between stream.resync(offset) and stream.switch(). This shouldn't cause any problems, but it feels a bit untidy.

We could avoid this issue by implementing resync_() such that if an offset is provided then it doesn't call setUpdateTimer_(); however, this will require some additional changes to switch() and perhaps some other changes elsewhere.

I'm open to either solution, but if separating the two operations introduces too many extra corner-cases in Stream then I think adding an extra parameter to selectVideoTrack and switch may be the safer approach.

[jonoward]

Just for comparison sake, I've got a branch that adds "offset" as a parameter onto IVideoSource.selectVideoTrack, which in turn passes that down to Stream.switch: blinkbox@486f049

What do you think? You might be wondering why I made those changes to SimpleAbrManager; this was just to allow us to easily customise the parameters for videoSource.selectVideoTrack, in a custom ABR manager; i.e. I'm inheriting from SimpleAbrManager and changing just the selectVideoTrack method.

And in terms of your other suggestion, i.e. to get resync() to avoid restarting the timer, I wasn't too sure how to go about this, because if the timer is not started in resync, I can't see where it would be restarted again?

[tdrews]

I think blinkbox@486f049 looks good overall.

And in terms of your other suggestion, i.e. to get resync() to avoid restarting the timer, I wasn't too sure how to go about this, because if the timer is not started in resync, I can't see where it would be restarted again?

I was thinking that after resync() with an offset (aka clearAhead()) the VideoSource would be responsible for restarting the update loop itself by calling switch(), i.e., if the VideoSource didn't call switch() after clearAhead() then the Stream would just stop.

Are you getting any race conditions with blinkbox@486f049? I like this approach as I'm not sure how we could separate clearAhead + switch without either stopping the update loop, having to abort the update loop when switching, or having some type of race.

[jonoward]

Nope I haven't come across any races in the approach as yet. I've got a PR I'm about to submit, so let me know what you think, and whether we should investigate the separate clearAhead/switch approach some more. Thanks

[tdrews]

Closed via PR #144.