Skip to content

Commit

Permalink
[MULTI_THREAD] Fix potential leak when cleaning now inexistant Period
Browse files Browse the repository at this point in the history
This is a fix for a small-scale leak happening in cases where we would
be unlucky with timings on a live content in a "multithreading" mode.

It was seen while investigating all possibilities for the issue reported
by #1643, though it has an extremely low chance to actually be linked to
that issue.

This fix is about the handling of `Init`'s `"periodStreamCleaning"`
event handling.

That event allows to clean-up resources that have previously been
allocated for a specific Period in a content: track handling, boundaries
checking etc.

Previously, only in "multithreading" mode, we checked that the Period of
a `"PeriodStreamCleaning"` event was found in the Manifest before
bubbling the event up to the `API` (the RxPlayer module).

That step was unnecessary (only the `id` property of a `Period` is
needed when cleaning up) and more importantly the check could fail in
a situation where the `Period` has since been removed from a dynamic
Manifest (e.g. after playing an old `Period` which has been loaded locally
but which now does not exist anymore in the Manifest).

The only issue I can think of with that for now are relatively-low memory
leaks (which will be collected once stopping/switching content). Still.
  • Loading branch information
peaBerberian committed Feb 7, 2025
1 parent d1dfe4c commit a8e0f35
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 45 deletions.
18 changes: 9 additions & 9 deletions src/main_thread/api/public_api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2849,12 +2849,12 @@ class Player extends EventEmitter<IPublicAPIEvent> {
*/
private _priv_onPeriodStreamCleared(
contentInfos: IPublicApiContentInfos,
value: { type: IBufferType; period: IPeriodMetadata },
value: { type: IBufferType; periodId: string },
): void {
if (contentInfos.contentId !== this._priv_contentInfos?.contentId) {
return; // Event for another content
}
const { type, period } = value;
const { type, periodId } = value;
const tracksStore = contentInfos.tracksStore;

// Clean-up track choices from TracksStore
Expand All @@ -2863,7 +2863,7 @@ class Player extends EventEmitter<IPublicAPIEvent> {
case "text":
case "video":
if (!isNullOrUndefined(tracksStore)) {
tracksStore.removeTrackReference(type, period);
tracksStore.removeTrackReference(type, periodId);
}
break;
}
Expand All @@ -2872,23 +2872,23 @@ class Player extends EventEmitter<IPublicAPIEvent> {
const { activeAdaptations, activeRepresentations } = contentInfos;
if (
!isNullOrUndefined(activeAdaptations) &&
!isNullOrUndefined(activeAdaptations[period.id])
!isNullOrUndefined(activeAdaptations[periodId])
) {
const activePeriodAdaptations = activeAdaptations[period.id];
const activePeriodAdaptations = activeAdaptations[periodId];
delete activePeriodAdaptations[type];
if (Object.keys(activePeriodAdaptations).length === 0) {
delete activeAdaptations[period.id];
delete activeAdaptations[periodId];
}
}

if (
!isNullOrUndefined(activeRepresentations) &&
!isNullOrUndefined(activeRepresentations[period.id])
!isNullOrUndefined(activeRepresentations[periodId])
) {
const activePeriodRepresentations = activeRepresentations[period.id];
const activePeriodRepresentations = activeRepresentations[periodId];
delete activePeriodRepresentations[type];
if (Object.keys(activePeriodRepresentations).length === 0) {
delete activeRepresentations[period.id];
delete activeRepresentations[periodId];
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/main_thread/init/media_source_content_initializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -917,7 +917,10 @@ export default class MediaSourceContentInitializer extends ContentInitializer {
if (cancelSignal.isCancelled()) {
return; // Previous call has stopped streams due to a side-effect
}
self.trigger("periodStreamCleared", value);
self.trigger("periodStreamCleared", {
type: value.type,
periodId: value.period.id,
});
},

bitrateEstimateChange: (value) => {
Expand Down
9 changes: 1 addition & 8 deletions src/main_thread/init/multi_thread_content_initializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -969,15 +969,8 @@ export default class MultiThreadContentInitializer extends ContentInitializer {
) {
return;
}
const period = arrayFind(
this._currentContentInfo.manifest.periods,
(p) => p.id === msgData.value.periodId,
);
if (period === undefined) {
return;
}
this.trigger("periodStreamCleared", {
period,
periodId: msgData.value.periodId,
type: msgData.value.bufferType,
});
break;
Expand Down
5 changes: 3 additions & 2 deletions src/main_thread/init/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,12 +192,13 @@ export interface IContentInitializerEvents {
*/
type: IBufferType;
/**
* The `Period` linked to the `PeriodStream` we just removed.
* The `id` property of the `Period` linked to the `PeriodStream` we just
* removed.
*
* The combination of this and `Period` should give you enough information
* about which `PeriodStream` has been removed.
*/
period: IPeriodMetadata;
periodId: string;
};
/** Emitted when a new `Adaptation` is being considered. */
adaptationChange: IAdaptationChangeEventPayload;
Expand Down
37 changes: 12 additions & 25 deletions src/main_thread/tracks_store/tracks_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -496,15 +496,22 @@ export default class TracksStore extends EventEmitter<ITracksStoreEvents> {
* Remove shared reference to choose an "audio", "video" or "text" Adaptation
* for a Period.
* @param {string} bufferType - The concerned buffer type
* @param {Period} period - The concerned Period.
* @param {string} periodId - The concerned Period's `id`.
*/
public removeTrackReference(
bufferType: "audio" | "text" | "video",
period: IPeriodMetadata,
periodId: string,
): void {
const periodIndex = findPeriodIndex(this._storedPeriodInfo, period);
let periodIndex;
for (let i = 0; i < this._storedPeriodInfo.length; i++) {
const periodI = this._storedPeriodInfo[i];
if (periodI.period.id === periodId) {
periodIndex = i;
break;
}
}
if (periodIndex === undefined) {
log.warn(`TS: ${bufferType} not found for period`, period.start);
log.warn(`TS: ${bufferType} not found for period`, periodId);
return;
}

Expand All @@ -513,7 +520,7 @@ export default class TracksStore extends EventEmitter<ITracksStoreEvents> {
if (choiceItem?.dispatcher === null) {
log.warn(
`TS: TrackDispatcher already removed for ${bufferType} ` +
`and Period ${period.start}`,
`and Period ${periodId}`,
);
return;
}
Expand Down Expand Up @@ -1378,26 +1385,6 @@ export default class TracksStore extends EventEmitter<ITracksStoreEvents> {
}
}

/**
* Returns the index of the given `period` in the given `periods`
* Array.
* Returns `undefined` if that `period` is not found.
* @param {Object} periods
* @param {Object} period
* @returns {number|undefined}
*/
function findPeriodIndex(
periods: ITSPeriodObject[],
period: IPeriodMetadata,
): number | undefined {
for (let i = 0; i < periods.length; i++) {
const periodI = periods[i];
if (periodI.period.id === period.id) {
return i;
}
}
}

/**
* Returns element in the given `periods` Array that corresponds to the
* `period` given.
Expand Down

0 comments on commit a8e0f35

Please sign in to comment.