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.

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

VTT cues are never cleaned up. Fix memory leak in live. #2309

Conversation

JordanPawlett
Copy link

@JordanPawlett JordanPawlett commented Jul 17, 2019

This PR will...

Implement flushing of the TextTrackCueList in accordance with the liveBackBufferLength config option, for live mode only.
This means that the id3 cues relevant to the current live buffer will still be maintained. However, we avoid the continuous growth that eventually consumes all the memory.

Why is this Pull Request needed?

VTT cues are not cleaned up until a stream is destroyed.
Long Live streams lead to a memory leak, as the TextTrackList continues growing with Cues until the browser runs out of memory and becomes un-responsive.

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

Resolves issues:

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

…o the buffer offset and remove all cues before then
Copy link
Collaborator

@OrenMe OrenMe left a comment

Choose a reason for hiding this comment

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

In general this kind of logic should reside in the ID3 controller as it is the one that manages the ID3 track, and you can see this by the fact that you had to reference the controller through the coreComponents in a way which is non safe, as no one guarantees it will retain its index (7) in anyway.
I also think it is a bit "confusing" that we have a buffer-controller but it handles only AV buffer and not all kinds of buffer, although I do believe the only other buffer we have is for text tracks(captions, subtitles and ID3), but the use cases for handling are so different that there's no point for mixing them probably.

@JordanPawlett
Copy link
Author

JordanPawlett commented Jul 17, 2019

I left the coreComponents index (7) lookup in purposefully to try exaggerate where i needed a point in the right direction.
Agreed that the logic is better residing in the id3-controller (as mentioned in my pr). However, there's no concept of a buffer in there - I feel it makes sense to tie the flushing of the VTT cues with the live buffer, as you would avoid seeking into the flushed section anyway. Any suggestions?

Jordan Pawlett added 2 commits July 18, 2019 12:36
@JordanPawlett
Copy link
Author

Had a chance to look at how hls is communicating with events.
I've moved the logic into the id3-controller.
An LIVE_BACK_BUFFER_REACHED Event is now being fired once the liveBackBufferLength option is exceeded. This is triggering the VTT cue flush, to be inline with the live buffer flush.

@JordanPawlett
Copy link
Author

It seems like the tests on master are currently breaking.

Copy link
Member

@michaelcunningham19 michaelcunningham19 left a comment

Choose a reason for hiding this comment

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

I tested this out on a live stream w/ many ID3 cues that I have access to, comparing against this branch and current master. Both were using liveBackBufferLength of 120

I verified that the ID3 cues are being removed properly 👍

Thoughts on getting this in soon, @johnBartos, @itsjamie, @OrenMe ?

@JordanPawlett
Copy link
Author

I tested this out on a live stream w/ many ID3 cues that I have access to, comparing against this branch and current master. Both were using liveBackBufferLength of 120

I verified that the ID3 cues are being removed properly 👍

Thoughts on getting this in soon, @johnBartos, @itsjamie, @OrenMe ?

Bump

desidiver added a commit to Pluto-tv/hls.js that referenced this pull request Oct 9, 2019
merging changes from video-dev#2309 and applying the CC fixes.
itsjamie
itsjamie previously approved these changes Oct 11, 2019
Copy link
Collaborator

@itsjamie itsjamie left a comment

Choose a reason for hiding this comment

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

LGTM.

@robwalch targeting this for 0.13.1 with the gap controller code.

@itsjamie itsjamie added this to the 0.13.1 milestone Oct 11, 2019
@desidiver desidiver mentioned this pull request Oct 17, 2019
3 tasks
@itsjamie itsjamie self-requested a review October 30, 2019 23:05
@itsjamie itsjamie dismissed their stale review October 30, 2019 23:05

Rob brought up a good point that I missed about the new event.

@robwalch robwalch self-requested a review November 11, 2019 17:07
@robwalch robwalch modified the milestones: 0.13.1, 0.13.0 Nov 11, 2019
@robwalch robwalch merged commit 8422e15 into video-dev:master Nov 12, 2019
@JordanPawlett JordanPawlett deleted the clear-vtt-cues-with-live-back-buffer-length branch November 27, 2019 21:50
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants