From 45a26a156d3df40ef11445a388550e076acd7bad Mon Sep 17 00:00:00 2001 From: Paul Berberian Date: Tue, 1 Oct 2024 19:25:13 +0200 Subject: [PATCH] Better handle multiple RepresentationStream errors in a row 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: 1. We had to provoke a `QuotaExceededError` from the MSE `SourceBuffer.prototype.appendBuffer` API Meaning that we had to "overflow" the buffer in terms of memory 2. We had to load segment faster than it takes to get a response from the `SourceBuffer.prototype.appendBuffer` API before that `QuotaExceededError`: 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 thrown `QuotaExceededError` 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 a `QuotaExceededError`. 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 received `QuotaExceededError` (which have since been transformed into the RxPlayer's `BUFFER_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 same `SourceBuffer` completely errors), but listening to errors of a `RepresentationStream` 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). --- .../stream/adaptation/adaptation_stream.ts | 24 ++++++++++++++++++- .../representation/representation_stream.ts | 11 +++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/core/stream/adaptation/adaptation_stream.ts b/src/core/stream/adaptation/adaptation_stream.ts index 00af25753a..f680885e1b 100644 --- a/src/core/stream/adaptation/adaptation_stream.ts +++ b/src/core/stream/adaptation/adaptation_stream.ts @@ -376,8 +376,13 @@ export default function AdaptationStream( representationStreamCallbacks: IRepresentationStreamCallbacks, fnCancelSignal: CancellationSignal, ): void { + /** Set to `true` if we've encountered an error with this `RepresentationStream` */ + let hasEncounteredError = false; + const bufferGoalCanceller = new TaskCanceller(); bufferGoalCanceller.linkToSignal(fnCancelSignal); + + /** Actually built buffer size, in seconds. */ const bufferGoal = createMappedReference( wantedBufferAhead, (prev) => { @@ -385,6 +390,7 @@ export default function AdaptationStream( }, bufferGoalCanceller.signal, ); + const maxBufferSize = adaptation.type === "video" ? maxVideoBufferSize : new SharedReference(Infinity); log.info( @@ -394,7 +400,18 @@ export default function AdaptationStream( representation.bitrate, ); const updatedCallbacks = objectAssign({}, representationStreamCallbacks, { - error(err: unknown) { + error(err: Error) { + if (hasEncounteredError) { + // A RepresentationStream might trigger multiple Errors (for example + // multiple segments it tried to push at once led to errors). + // In that case, we'll only consider the first Error. + // + // That could mean that we're hiding legitimate issues but handling + // multiple of those errors at once is too hard a task for now. + log.warn("Stream: Ignoring RepresentationStream error", err); + return; + } + hasEncounteredError = true; const formattedError = formatError(err, { defaultCode: "NONE", defaultReason: "Unknown `RepresentationStream` error", @@ -402,6 +419,11 @@ export default function AdaptationStream( if (formattedError.code !== "BUFFER_FULL_ERROR") { representationStreamCallbacks.error(err); } else { + log.warn( + "Stream: received BUFFER_FULL_ERROR", + adaptation.type, + representation.bitrate, + ); const wba = wantedBufferAhead.getValue(); const lastBufferGoalRatio = bufferGoalRatioMap.get(representation.id) ?? 1; // 70%, 49%, 34.3%, 24%, 16.81%, 11.76%, 8.24% and 5.76% diff --git a/src/core/stream/representation/representation_stream.ts b/src/core/stream/representation/representation_stream.ts index c8fee252f2..4fd7aab17a 100644 --- a/src/core/stream/representation/representation_stream.ts +++ b/src/core/stream/representation/representation_stream.ts @@ -90,6 +90,11 @@ export default function RepresentationStream( callbacks: IRepresentationStreamCallbacks, parentCancelSignal: CancellationSignal, ): void { + log.debug( + "Stream: Creating RepresentationStream", + content.adaptation.type, + content.representation.bitrate, + ); const { period, adaptation, representation } = content; const { bufferGoal, maxBufferSize, drmSystemId, fastSwitchThreshold } = options; const bufferType = adaptation.type; @@ -488,6 +493,12 @@ export default function RepresentationStream( // We can thus ignore it, it is very unlikely to lead to true buffer issues. return; } + log.warn( + "Stream: Received fatal buffer error", + adaptation.type, + representation.bitrate, + err instanceof Error ? err : null, + ); globalCanceller.cancel(); callbacks.error(err); }