Fix any possible fillBuffer_ race conditions by debouncing all fillBuffers_ #959
Conversation
…ffers_ - Convert all calls to fillBuffer_ to calls to monitorBuffer_ - Rename monitorBuffer_ to monitorBufferTick_ which becomes the 500ms buffer check timer loop - Make monitorBuffer_ schedule an immediate timer for monitorBufferTick_
| * | ||
| * @private | ||
| */ | ||
| monitorBuffer_() { |
There was a problem hiding this comment.
Why do we need this in addition to monitorBufferTick_? It seems that by changing calls from fillBuffer_ to monitorBuffer_, we already get the guard that checks whether the state is READY.
fillBuffer_, if it starts a segment request, will synchronously get to the WAITING state. This means that any subsequent calls to monitorBuffer would be safe with the READY guard.
There was a problem hiding this comment.
The real difference is in how long in the future things are scheduled. Calling monitorBuffer_ forces the tick to happen next (it either schedules or reschedules the timer tick to be just one millisecond). Whereas monitorBufferTick_ always schedules the next time to be 500ms in the future.
Originally, I had a piece of code with an optional parameter to monitorBuffer_ to set/reset different timer durations in one function but I decided that was "too clever".
There was a problem hiding this comment.
As a followup, spoke with @imbcmdth , and he pointed out that monitorBuffer_ contextually is async, and is safer as async when there are events that are being listened to that may happen around a segment's end of processing (as an example, if monitorBuffer_ was synchronous, by the time listeners received the events, a new segment may have already been requested and a lot of state changed).
|
|
||
| // so the loader should try the next segment | ||
| assert.equal(this.requests[0].url, '1.ts', 'moved ahead a segment'); | ||
|
|
||
| // verify stats | ||
| assert.equal(loader.mediaBytesTransferred, 20, '20 bytes'); | ||
| assert.equal(loader.mediaTransferDuration, 2, '2 ms (clocks above)'); | ||
| assert.equal(loader.mediaTransferDuration, 1, '1 ms (clocks above)'); |
There was a problem hiding this comment.
Prior to this change, the request happened immediately. As a result, there were two clock ticks between the request and the response. Now, the request happens later and only one tick happens now (the first tick becomes the tick that triggers the request).
|
LGTM |
Description
On a rendition switch, SegmentLoader seems to be able to enter a state where it fetches the same segment twice at the same time causing bandwidth starvation. The requests then timeout and an emergency rendition switch which can trigger the same behavior again.
What's gone wrong?
The root problem is that
handleUpdateEnd_can trigger a rendition switch (by emitting the 'progress' event). Eventually, a rendition switch results inloadbeing called on the SegmentLoader which has the side effect of callingfillBuffer_and starting a request for a segment. At the same time, the originalhandleUpdateEnd_function callsfillBuffer_itself triggering a second request.Why doesn't this happen every time we change renditions?
The issue is "masked" by two different mechanisms each of which can stop the second request from happening:
fillBuffer_is never called bySegmentLoader#load.fillBuffer_will, vialoadSegment_, result in aSourceUpdater.removethat causes theSourceBuffer#updatingto betrue. When the nextfillBuffer_runs, it will exit early because the sourceBuffer is updating.Specific Changes proposed
fillBuffer_into calls tomonitorBuffer_monitorBuffer_tomonitorBufferTick_which becomes the 500ms buffer check timer-loopmonitorBuffer_that schedule an immediate (1 ms) timer formonitorBufferTick_Requirements Checklist