Skip to content

Commit

Permalink
TracksStore keeps the last reference in case of several being communi…
Browse files Browse the repository at this point in the history
…cated

As #1623 reported, there seem to be an issue seen on lower-end
devices where the `TracksStore` gets multiple track references for the
same buffer type (`audio`, `video`...) and Period combination.

This should __NEVER__ happen as per the `TracksStore` API, there should
only be one reference linked to the `TracksStore` for any such
combination - if the external logic wanted to update the track
reference, it had to call `removeTrackReference` with the previous one
(or `resetPeriodObjects` to just reset everything) before adding the new
one.

Turns out it does happen in some unknown and difficult to reproduce
scenario. I guess a race condition allows for some cleaning-up phase to
be lost somewhere (either a reset is being skipped - which mostly happens
when `RELOADING`, or either some `PeriodStream` cleaning is being missed,
which happens when Period switching).

So what I propose here isn't a real fix for that problem, but to add
some resilience into the TracksStore by considering the last track
reference for a buffer type + Period combination - which IMO
makes more sense API-wise than keeping the first one and should "fix"
cases of infinite rebuffering (as I suppose some code awaited
indefinitely that a track choice be performed through that new track
reference).

The absence of clearing of the previous reference still indicates a
leak (though the leak should not matter anymore for the `TracksStore`
once the new one is added) and still indicate that something went
wrong somewhere, so I kept the `log.error` call which will help us
debug other issues that may have their source in that same weird
race condition.

I'm still looking how such race condition is even possible in the
meantime.
  • Loading branch information
peaBerberian committed Feb 13, 2025
1 parent 6a3ed0c commit 066fa59
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion src/main_thread/tracks_store/tracks_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ export default class TracksStore extends EventEmitter<ITracksStoreEvents> {
period: IPeriodMetadata,
adaptationRef: SharedReference<IAdaptationChoice | null | undefined>,
): void {
log.debug("TS: Adding Track Reference", bufferType, period.id);
let periodObj = getPeriodItem(this._storedPeriodInfo, period.id);
if (periodObj === undefined) {
// The Period has not yet been added.
Expand All @@ -383,7 +384,7 @@ export default class TracksStore extends EventEmitter<ITracksStoreEvents> {
log.error(
`TS: Subject already added for ${bufferType} ` + `and Period ${period.start}`,
);
return;
periodObj[bufferType].dispatcher.dispose();
}

const dispatcher = new TrackDispatcher(adaptationRef);
Expand Down Expand Up @@ -502,6 +503,7 @@ export default class TracksStore extends EventEmitter<ITracksStoreEvents> {
bufferType: "audio" | "text" | "video",
periodId: string,
): void {
log.debug("TS: Removing Track Reference", bufferType, periodId);
let periodIndex;
for (let i = 0; i < this._storedPeriodInfo.length; i++) {
const periodI = this._storedPeriodInfo[i];
Expand Down Expand Up @@ -600,6 +602,7 @@ export default class TracksStore extends EventEmitter<ITracksStoreEvents> {
* You might want to call this API when restarting playback.
*/
public resetPeriodObjects(): void {
log.debug("TS: Resetting Period Objects");
for (let i = this._storedPeriodInfo.length - 1; i >= 0; i--) {
const storedObj = this._storedPeriodInfo[i];
storedObj.audio.dispatcher?.dispose();
Expand Down

0 comments on commit 066fa59

Please sign in to comment.