Remove expired playlists

[dzianis-dashkevich]

This PR will...

• Removing expired playlists

Why is this Pull Request needed?

• Release memory

Are there any points in the code the reviewer needs to double check?

• base-playlist-controller.ts

Resolves issues:

• Delete expired playlist details #7429

Checklist

• changes have been done against master branch, and PR does not conflict
• new unit / functional tests have been added (whenever applicable)
• API or design changes are documented in API.md

[itsjamie]

I wonder if a single event emitted like SWEEP_PLAYLISTS could keep this API nice and easy and not have to add getters for each playlist controller to expose internals that the base-playlist unsets directly taking on a referential set.

[itsjamie]

This will limit it to only cleaning up the current group ever, so inactive groups won't have the details released. Is that the intended behaviour?

[robwalch]

Good point. In terms of behavior, either:

1. All tracks (as opposed to tracksInGroup ) should be considered for cleanup
2. details from other groups should be removed when the active group changes (then the rest of these changes take care of the rest)

[itsjamie]

Thought about this a little bit more...

I think we should move this out of BasePlaylistController and put it directly into LevelController, AudioTrackController, and SubtitleTrackController. Running the sweep from BasePlaylistController.playlistLoaded requires each controller to expose its private state through new protected API so that the base controller can reach in and unset details. It also means audio and subtitles only sweep tracksInGroup, leaving expired details in other groups untouched.

After a successful playlist refresh, each controller should sweep its own private _levels or tracks collection and exclude the current playlist. We should also clear an expired playlist before assigning it to currentLevel or currentTrack, and before calling switchParams. Otherwise an expired playlist selected between sweeps becomes active, is excluded from cleanup, and its stale details are used for the next request and merge. This keeps the ordering explicit and avoids adding either protected API or another event.

Thoughts?

[robwalch]

I think we should move this out of BasePlaylistController and put it directly into LevelController, AudioTrackController, and SubtitleTrackController. Running the sweep from BasePlaylistController.playlistLoaded requires each controller to expose its private state through new protected API so that the base controller can reach in and unset details. It also means audio and subtitles only sweep tracksInGroup, leaving expired details in other groups untouched.

I think it's better to have a single function that handles the cleanup. It doesn't have to be a method, though - Level and MediaPlaylist share a common details property (and we're fine with apps deleting it externally as a workaround for now). Good catch that playlists is not scoped correctly. So yes, maybe instead of adding protected methods we could just pass levels or tracks. In fact, if you want "all levels" (variants), they now live in Content-Steering, so maybe that is where this should live. I'd even go so far as to suggest that the level update drive the clearing of all playlist types (rather than have disparate main, audio, and subs cleanup (food for thought, @dzianis-dashkevich).

After a successful playlist refresh, each controller should sweep its own private _levels or tracks collection and exclude the current playlist.

After advancing (but also after everything else that currently follows advancing, which is of higher priority) would be ideal.

We should also clear an expired playlist before assigning it to currentLevel or currentTrack, and before calling switchParams.

Not sure I agree here. Expired playlist details will not be used to load segments - that requires a refresh. The old details may still be useful in aligning the refreshed ones.

Otherwise an expired playlist selected between sweeps becomes active, is excluded from cleanup, and its stale details are used for the next request and merge. This keeps the ordering explicit and avoids adding either protected API or another event.

Intentional. Stale details are used in merge, but not to pick segments or (and this requires more scrutiny) regress estimates (like latency).

[itsjamie]

Defer to @robwalch.

I like it just being a simple function that knows the bits that can be dropped for GC to pickup!

Totally understand on the expired playlist side. Forget the switchParams bit 💯.