Better handle multiple RepresentationStream errors in a row #1559
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed in recent tests that an error log was frequently triggered, about a "SharedReference" (our concept of a shared observable variable passed by reference) being updated despite it being "finished" (a state indicating that we should not be updating it anymore).
Generally, when we see those kind of issues, it means that our logic goes wrong somewhere, so I tried to track it.
After spending some (a lot of in reality :'() time on it, I found out that it was only reproduced in very specific scenarios:
We had to provoke a
QuotaExceededError
from the MSESourceBuffer.prototype.appendBuffer
APIMeaning that we had to "overflow" the buffer in terms of memory
We had to load segment faster than it takes to get a response from the
SourceBuffer.prototype.appendBuffer
API before thatQuotaExceededError
: meaning either be on a very slow device, either having a very fast network (which explains why I was mostly seeing the issue when playing contents served locally).In that case, all those queued segments waiting to be pushed were merged together as a unique big segment just before calling the
SourceBuffer.prototype.appendBuffer
API and thus the thrownQuotaExceededError
applied to all of those operations.Though the
RepresentationStream
isn't aware of the operations being merged together and thus see all operations it's waiting for leading to aQuotaExceededError
.In itself, it's hard to reason about, but having duplicated errors here isn't dramatic as it's technically still true: all those scheduled append operations led to an error.
However the way we were handling it was wrong: The
AdaptationStream
was receiving each of those errors, and performing its buffer-size-reduction algorithm for each of those receivedQuotaExceededError
(which have since been transformed into the RxPlayer'sBUFFER_FULL_ERROR
).The fix I found was to just consider the first fatal error from a
RepresentationStream
.This means we may be ignoring important errors in very rare scenarios (e.g. if we first receive a
QuotaExceededError
but then the sameSourceBuffer
completely errors), but listening to errors of aRepresentationStream
already having an error seems very hard to correctly handle to me, and such scenarios are very hypothetical anyway (moreover even if they happen, I would hope the next attempt would also lead to a related error).