Skip to content

Commit

Permalink
Do not add failed push operations in the BufferedHistory
Browse files Browse the repository at this point in the history
This commit fixes a kind of rare issue that is tricky to reproduce that
may happen after a `QuotaExceededError` is encountered when pushing a
chunk to the audio and video `SourceBuffer` due to memory limitations.

In that situation, we could be left in the following scenario:
  1. We're loading a chunk and pushing it to the `SourceBuffer`
  2. The `SourceBuffer` throws a `QuotaExceededError`, generally this
     means that the device needs more memory to add that chunk
  3. The SegmentBuffer code still adds the chunk to its `SegmentInventory`
     (as it may still have been partially pushed to the SourceBuffer,
     we're not 100% sure of what happened on the lower-level code) and
     to that `SegmentInventory`'s `BufferedHistory`.
  4. The stream code catches the error, try to remove some buffered data
     and retry to push the same chunk.
  5. Same QuotaExceededError
  6. The SegmentBuffer code agains, re do the whole `SegmentInventory` +
     `BufferedHistory` insertion thing for the same reasons
  7. The stream code re-catches the issue, here it takes more drastic
     measures, mainly reducing the amount of buffer that is built in
     advance.
  8. The stream code then check if it re-needs the chunk. However, it
     sees through the `BufferedHistory` that it already tried to load it
     two times, and that in those two times, the browser did not buffer
     it (there ignoring that there was a `QuotaExceededError` involved).
  9. The stream code thus assumes a browser/device issue with that
     segment, and loads the next one instead.
 10. When it's time to decode the skipped chunk, the RxPlayer
     automatically seek over it, which is good, but we would have
     prefered to actually log it.

This commit fixes the issue by clearly identifying in the
`SegmentInventory` chunks whose push operation had an issue. Those are:
  1. never added to the `BufferedHistory`
  2. always re-loaded if they become needed again

We could also have not added them to the `SegmentInventory` altogether
but I still think it's better to do it, as in the very rare chance that
chunk still had been at least partially pushed, our logic would be more
accurate.
  • Loading branch information
peaBerberian committed Dec 11, 2023
1 parent ff213e1 commit 79ebbfc
Show file tree
Hide file tree
Showing 8 changed files with 68 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,15 @@ export default class AudioVideoSegmentBuffer extends SegmentBuffer {
err :
new Error("An unknown error occured when doing operations " +
"on the SourceBuffer");
this._pendingTask.reject(error);
const task = this._pendingTask;
if (task.type === SegmentBufferOperation.Push &&
task.data.length === 0 &&
task.inventoryData !== null)
{
this._segmentInventory.insertChunk(task.inventoryData, false);
}
this._pendingTask = null;
task.reject(error);
}
}

Expand Down Expand Up @@ -429,7 +437,7 @@ export default class AudioVideoSegmentBuffer extends SegmentBuffer {
switch (task.type) {
case SegmentBufferOperation.Push:
if (task.inventoryData !== null) {
this._segmentInventory.insertChunk(task.inventoryData);
this._segmentInventory.insertChunk(task.inventoryData, true);
}
break;
case SegmentBufferOperation.EndOfSegment:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export default class ImageSegmentBuffer extends SegmentBuffer {
try {
this._buffered.insert(startTime, endTime);
if (infos.inventoryInfos !== null) {
this._segmentInventory.insertChunk(infos.inventoryInfos);
this._segmentInventory.insertChunk(infos.inventoryInfos, true);
}
} catch (err) {
return Promise.reject(err);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ export default class HTMLTextSegmentBuffer extends SegmentBuffer {
}

if (infos.inventoryInfos !== null) {
this._segmentInventory.insertChunk(infos.inventoryInfos);
this._segmentInventory.insertChunk(infos.inventoryInfos, true);
}
this._buffer.insert(cues, start, end);
this._buffered.insert(start, end);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ export default class NativeTextSegmentBuffer extends SegmentBuffer {
}
this._buffered.insert(start, end);
if (infos.inventoryInfos !== null) {
this._segmentInventory.insertChunk(infos.inventoryInfos);
this._segmentInventory.insertChunk(infos.inventoryInfos, true);
}
} catch (err) {
return Promise.reject(err);
Expand Down
2 changes: 2 additions & 0 deletions src/core/segment_buffers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
SegmentBufferOperation,
} from "./implementations";
import {
ChunkStatus,
IBufferedChunk,
IChunkContext,
IInsertedChunkInfos,
Expand All @@ -40,6 +41,7 @@ import SegmentBuffersStore, {
export default SegmentBuffersStore;
export {
BufferGarbageCollector,
ChunkStatus,

ISegmentBufferOptions,
ITextTrackSegmentBufferOptions,
Expand Down
2 changes: 2 additions & 0 deletions src/core/segment_buffers/inventory/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@
*/

import SegmentInventory, {
ChunkStatus,
IBufferedChunk,
IInsertedChunkInfos,
} from "./segment_inventory";

export default SegmentInventory;
export {
ChunkStatus,
IBufferedChunk,
IInsertedChunkInfos,
};
Expand Down
66 changes: 49 additions & 17 deletions src/core/segment_buffers/inventory/segment_inventory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,27 @@ import BufferedHistory, {
} from "./buffered_history";
import { IChunkContext } from "./types";

/** Categorization of a given chunk in the `SegmentInventory`. */
export const enum ChunkStatus {
/**
* This chunk is only a part of a partially-pushed segment for now, meaning
* that it is only a sub-part of a requested segment that was not yet
* fully-loaded and pushed.
*
* Once and if the corresponding segment is fully-pushed, its `ChunkStatus`
* switches to `Complete`.
*/
PartiallyPushed = 0,
/** This chunk corresponds to a fully-loaded segment. */
Complete = 1,
/**
* This chunk's push operation failed, in this scenario there is no certitude
* about the presence of that chunk in the buffer: it may not be present,
* partially-present, or fully-present depending on why that push operation
* failed, which is generally only known by the lower-level code.
*/
Failed = 2,
}

/** Information stored on a single chunk by the SegmentInventory. */
export interface IBufferedChunk {
Expand Down Expand Up @@ -82,14 +103,10 @@ export interface IBufferedChunk {
/** Information on what that chunk actually contains. */
infos : IChunkContext;
/**
* If `true`, this chunk is only a partial chunk of a whole segment.
*
* Inversely, if `false`, this chunk is a whole segment whose inner chunks
* have all been fully pushed.
* In that condition, the `start` and `end` properties refer to that fully
* pushed segment.
* Status of this chunk.
* @see ChunkStatus
*/
partiallyPushed : boolean;
status : ChunkStatus;
/**
* If `true`, the segment as a whole is divided into multiple parts in the
* buffer, with other segment(s) between them.
Expand Down Expand Up @@ -244,7 +261,11 @@ export default class SegmentInventory {
log.debug(`SI: ${numberOfSegmentToDelete} segments GCed.`, bufferType);
const removed = inventory.splice(indexBefore, numberOfSegmentToDelete);
for (const seg of removed) {
if (seg.bufferedStart === undefined && seg.bufferedEnd === undefined) {
if (
seg.bufferedStart === undefined &&
seg.bufferedEnd === undefined &&
seg.status !== ChunkStatus.Failed
) {
this._bufferedHistory.addBufferedSegment(seg.infos, null);
}
}
Expand Down Expand Up @@ -318,7 +339,11 @@ export default class SegmentInventory {
bufferType, inventoryIndex, inventory.length);
const removed = inventory.splice(inventoryIndex, inventory.length - inventoryIndex);
for (const seg of removed) {
if (seg.bufferedStart === undefined && seg.bufferedEnd === undefined) {
if (
seg.bufferedStart === undefined &&
seg.bufferedEnd === undefined &&
seg.status !== ChunkStatus.Failed
) {
this._bufferedHistory.addBufferedSegment(seg.infos, null);
}
}
Expand All @@ -343,7 +368,8 @@ export default class SegmentInventory {
segment,
chunkSize,
start,
end } : IInsertedChunkInfos
end } : IInsertedChunkInfos,
succeed: boolean
) : void {
if (segment.isInit) {
return;
Expand All @@ -357,7 +383,8 @@ export default class SegmentInventory {
}

const inventory = this._inventory;
const newSegment = { partiallyPushed: true,
const newSegment = { status: succeed ? ChunkStatus.PartiallyPushed :
ChunkStatus.Failed,
chunkSize,
splitted: false,
start,
Expand Down Expand Up @@ -565,7 +592,7 @@ export default class SegmentInventory {
// ===> : |--|====|-|
log.warn("SI: Segment pushed is contained in a previous one",
bufferType, start, end, segmentI.start, segmentI.end);
const nextSegment = { partiallyPushed: segmentI.partiallyPushed,
const nextSegment = { status: segmentI.status,
/**
* Note: this sadly means we're doing as if
* that chunk is present two times.
Expand Down Expand Up @@ -743,7 +770,9 @@ export default class SegmentInventory {
this._inventory.splice(firstI + 1, length);
i -= length;
}
this._inventory[firstI].partiallyPushed = false;
if (this._inventory[firstI].status === ChunkStatus.PartiallyPushed) {
this._inventory[firstI].status = ChunkStatus.Complete;
}
this._inventory[firstI].chunkSize = segmentSize;
this._inventory[firstI].end = lastEnd;
this._inventory[firstI].bufferedEnd = lastBufferedEnd;
Expand All @@ -760,8 +789,11 @@ export default class SegmentInventory {
this.synchronizeBuffered(newBuffered);
for (const seg of resSegments) {
if (seg.bufferedStart !== undefined && seg.bufferedEnd !== undefined) {
this._bufferedHistory.addBufferedSegment(seg.infos, { start: seg.bufferedStart,
end: seg.bufferedEnd });
if (seg.status !== ChunkStatus.Failed) {
this._bufferedHistory.addBufferedSegment(seg.infos,
{ start: seg.bufferedStart,
end: seg.bufferedEnd });
}
} else {
log.debug("SI: buffered range not known after sync. Skipping history.",
seg.start,
Expand Down Expand Up @@ -810,7 +842,7 @@ function bufferedStartLooksCoherent(
thisSegment : IBufferedChunk
) : boolean {
if (thisSegment.bufferedStart === undefined ||
thisSegment.partiallyPushed)
thisSegment.status !== ChunkStatus.Complete)
{
return false;
}
Expand All @@ -837,7 +869,7 @@ function bufferedEndLooksCoherent(
thisSegment : IBufferedChunk
) : boolean {
if (thisSegment.bufferedEnd === undefined ||
thisSegment.partiallyPushed)
thisSegment.status !== ChunkStatus.Complete)
{
return false;
}
Expand Down
3 changes: 2 additions & 1 deletion src/core/stream/representation/utils/get_buffer_status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import Manifest, {
import isNullOrUndefined from "../../../../utils/is_null_or_undefined";
import { IReadOnlyPlaybackObserver } from "../../../api";
import SegmentBuffersStore, {
ChunkStatus,
IBufferedChunk,
IEndOfSegmentOperation,
SegmentBuffer,
Expand Down Expand Up @@ -299,7 +300,7 @@ function getPlayableBufferedSegments(
const eltInventory = segmentInventory[i];

const { representation } = eltInventory.infos;
if (!eltInventory.partiallyPushed &&
if (eltInventory.status === ChunkStatus.Complete &&
representation.decipherable !== false &&
representation.isSupported)
{
Expand Down

0 comments on commit 79ebbfc

Please sign in to comment.