Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better handle multiple RepresentationStream errors in a row #1559

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

peaBerberian
Copy link
Collaborator

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).

@peaBerberian peaBerberian added this to the 4.2.0 milestone Oct 1, 2024
@peaBerberian peaBerberian added Priority: 0 (Very high) This issue or PR has a very high priority. Efforts should be concentrated on it first. bug This is an RxPlayer issue (unexpected result when comparing to the API) labels Oct 1, 2024
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).
@peaBerberian peaBerberian force-pushed the fix/multiple-representation-stream-errors branch from 2b0d235 to 45a26a1 Compare October 1, 2024 17:57
@peaBerberian peaBerberian merged commit 1319cdd into dev Oct 2, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is an RxPlayer issue (unexpected result when comparing to the API) Priority: 0 (Very high) This issue or PR has a very high priority. Efforts should be concentrated on it first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants