`manifest.manifestUpdate` event listeners been added many times for live events

[KunXi-Fox]

Hi there,

I noticed that manifest.addEventListener('manifestUpdate', () => {}) been added for many times without clear previous listeners. Which will eventually lead to unexpected resource (CPU/Memory) usage.

Code block as below:

rx-player/src/core/stream/adaptation/adaptation_stream.ts

Lines 519 to 548 in 00f9b0c

	manifest.addEventListener(
	"manifestUpdate",
	(updates) => {
	for (const element of updates.updatedPeriods) {
	if (element.period.id === period.id) {
	for (const updated of element.result.updatedAdaptations) {
	if (updated.adaptation === adaptation.id) {
	for (const rep of updated.removedRepresentations) {
	if (rep === representation.id) {
	if (fnCancelSignal.isCancelled()) {
	return;
	}
	return callbacks.waitingMediaSourceReload({
	bufferType: adaptation.type,
	period,
	timeOffset: 0,
	stayInPeriod: true,
	});
	}
	}
	}
	}
	} else if (element.period.start > period.start) {
	break;
	}
	}
	},
	fnCancelSignal,
	);
	}

Reproduce steps

1. Make sure you're using single mode
2. Play any live events
3. Check rxPlayerInstance._priv_contentInfos.manifest._listeners.manifestUpdate been increased timely

[peaBerberian]

Hi @KunXi-Fox,

There could be many legitimate manifestUpdate listeners at once. The one in the AdaptationStream code linked for example just check that the currently-handled track still exist after a Manifest update, but you could also have logic elsewhere checking that new Periods are available, whether some attributes evolved etc.

Do you see an actual problematic memory leak or logic issue when playing a content for a long time?

[KunXi-Fox]

I've heard from another team that they're trying to integrate rx-player to their products, and they said that they met a performance issue and seems because of manifest updated been added for many times.

[KunXi-Fox]

I've been read all those code again, If I understand correctly, what we want to do here actually is: when manifestUpdate been triggered, we want to check if the adaptation that we linked to is been removed, if so, we want to call waitingMediaSourceReload method.

So ideally add event listener per period.id + adaptation.id + representation.id is enough to do that. And the timing to remove this event listener is after period changed or adaptation been removed?

Am I understand correctly ?

[KunXi-Fox]

I add a log:

console.log(
  "kx check representation still exists after manifest update:",
  period.id,
  adaptation.id,
  representation.id,
  Date.now()
);

I mark the event listener with same color when period id + adaptation id + representation id are same, ideally it should be only 1 listener

[nathanielwright]

@peaBerberian I've been investigating a memory leak that has been causing us issues with low memory devices
Each time the player switches representations, it is leaving a copy of the manifest in memory. When the manifests grow to 300k, this can quickly consume memory. Tracking it down, the retainers are in 2 places - _priv_contentInfos

and error based

If you want to reproduce, here's my steps:
Using Chrome, start playback of a live event. Create a memory snapshot. Sort by retained size, check under the (string) Constructors, look for <?xml... the dash manifest.
Using the network tools, change the speed of the connection (Fast 4G, Slow 4G) forcing the player to switch to a lower bitrate.
Take another snapshot - check the strings for multiple copies of the manifest (still referenced, ignore any that are due for GC root)

[peaBerberian]

OK thanks.

If there's a leak for that listener, it might be that fnCancelSignal is not the right signal for that event listener.
Or alternatively that we could skip some listeners, but I think the intent here is to only do that code for the currently-chosen Representation (quality), so it may be that fnCancelSignal is the wrong signal?

check the strings for multiple copies of the manifest (still referenced, ignore any that are due for GC root)

This I don't get. Why would that event listener keep alive several copies of the Manifest's string representation, and that just on quality change?

The aforementioned identified closure seems to keep a ref to:

• period (so I guess the ref to the period object - which hopefully should keep being the same after an update)
• adaptation (same)
• the callbacks object (which is also the same one for other qualities)
• fnCancelSignal (did not check that one yet to see if shared)

That with the closure itself, the retained size should be very small (not saying it shouldn't be fixed, but I want to understand the extent of the issue first).

How much leaking memory per quality switch are we talking about here?

Did not do the test yet, will try this afternoon.