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.

Sign up for GitHub

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

Jump to bottom

Update how MINIMUM_SEGMENT_SIZE is used in the SegmentInventory #1424

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Update how MINIMUM_SEGMENT_SIZE is used in the SegmentInventory #1424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97316df
Update how MINIMUM_SEGMENT_SIZE is used in the SegmentInventory
peaBerberian Apr 3, 2024
05e7f51
Fix logic issue in segmentinventory ambiguous range determination
peaBerberian Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 49 additions & 20 deletions src/core/segment_sinks/inventory/segment_inventory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,14 @@ export default class SegmentInventory {
/** Type of buffer considered, used for logs */
const bufferType: string | undefined = thisSegment?.infos.adaptation.type;

if (log.hasLevel("DEBUG")) {
const prettyPrintedRanges = ranges.map((r) => `${r.start}-${r.end}`).join(",");
log.debug(
`SI: synchronizing ${bufferType ?? "unknown"} buffered ranges:`,
prettyPrintedRanges,
);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a log so it becomes easier to determine if a SegmentInventory's synchronization result (logged at the end) do seem to correspond to its SourceBuffer's buffered property at that time (logged here)


const rangesLength = ranges.length;
for (let i = 0; i < rangesLength; i++) {
if (thisSegment === undefined) {
Expand Down Expand Up @@ -312,12 +320,28 @@ export default class SegmentInventory {
let thisSegmentStart = thisSegment.bufferedStart ?? thisSegment.start;
let thisSegmentEnd = thisSegment.bufferedEnd ?? thisSegment.end;
const nextRangeStart = i < rangesLength - 1 ? ranges[i + 1].start : undefined;
while (
thisSegment !== undefined &&
rangeEnd - thisSegmentStart >= MINIMUM_SEGMENT_SIZE &&
(nextRangeStart === undefined ||
rangeEnd - thisSegmentStart >= thisSegmentEnd - nextRangeStart)
) {
while (thisSegment !== undefined) {
if (rangeEnd < thisSegmentStart) {
// `thisSegment` is part of the next range
break;
}
if (
rangeEnd - thisSegmentStart < MINIMUM_SEGMENT_SIZE &&
thisSegmentEnd - rangeEnd >= MINIMUM_SEGMENT_SIZE
) {
// Ambiguous, but `thisSegment` seems more to come after the current
// range than during it.
break;
}
if (
nextRangeStart !== undefined &&
rangeEnd - thisSegmentStart < thisSegmentEnd - nextRangeStart
) {
// Ambiguous, but `thisSegment` has more chance to be part of the
// next range than the current one
break;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I broke the while condition into separate if -> break for readability purposes (yes your heard right, I relied on the much-hated break statements for readability purposes :p)


const prevSegment = inventory[inventoryIndex - 1];

// those segments are contiguous, we have no way to infer their real
Expand Down Expand Up @@ -353,20 +377,25 @@ export default class SegmentInventory {
// if we still have segments left, they are not affiliated to any range.
// They might have been garbage collected, delete them from here.
if (!isNullOrUndefined(thisSegment)) {
log.debug(
"SI: last segments have been GCed",
bufferType,
inventoryIndex,
inventory.length,
);
const removed = inventory.splice(inventoryIndex, inventory.length - inventoryIndex);
for (const seg of removed) {
if (
seg.bufferedStart === undefined &&
seg.bufferedEnd === undefined &&
seg.status !== ChunkStatus.Failed
) {
this._bufferedHistory.addBufferedSegment(seg.infos, null);
const { SEGMENT_SYNCHRONIZATION_DELAY } = config.getCurrent();
const now = getMonotonicTimeStamp();
for (let i = inventoryIndex; i < inventory.length; i++) {
const segmentInfo = inventory[i];
if (now - segmentInfo.insertionTs >= SEGMENT_SYNCHRONIZATION_DELAY) {
log.debug(
"SI: A segment at the end has been completely GCed",
bufferType,
`${segmentInfo.start}-${segmentInfo.end}`,
);
if (
segmentInfo.bufferedStart === undefined &&
segmentInfo.bufferedEnd === undefined &&
segmentInfo.status !== ChunkStatus.Failed
) {
this._bufferedHistory.addBufferedSegment(segmentInfo.infos, null);
}
inventory.splice(i, 1);
i--;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/default_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ const DEFAULT_CONFIG = {
* this logic could lead to bugs with the current code.
* @type {Number}
*/
MINIMUM_SEGMENT_SIZE: 0.005,
MINIMUM_SEGMENT_SIZE: 0.001,

/**
* Append windows allow to filter media data from segments if they are outside
Expand Down
Loading