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

Add maxBufferAhead exception for text garbage collection #1325

Merged
merged 2 commits into from Dec 19, 2023

Conversation

peaBerberian
Copy link
Collaborator

The maxBufferAhead RxPlayer option allows to manually garbage collect media data that is too far ahead from the playing position.

It can be set to a number of seconds, and was previously applied to audio, video and text media segments.

For example when setting maxBufferAhead to 30, and playing at position 20, we would remove if present media data already buffered a position 50 or more (which could for example mostly happen when seeking back in the content).

We found out however that applying that same value to the text buffer may not always be sensible:

  • maxBufferAhead is most likely defined by an application to restrict memory usage. The space taken by the text cues have much less weight than audio and video
  • We encounter several contents where loaded text segments span for 1 minute, most likely because of how small those segments are. Here we risk progressively reloading the same segment as its end will be GCed multiple times.

Due to both of these situations, I decided for now to have a lower bound on the maxBufferAhead applied on the text buffer to 2 minutes. We also could have made the maxBufferAhead API more configurable by letting an application set a different setting per media type, but I thought that an application may not easily realize the issue.

The `maxBufferAhead` RxPlayer option allows to manually garbage collect
media data that is too far ahead from the playing position.

It can be set to a number of seconds, and was previously applied to
audio, video and text media segments.

For example when setting `maxBufferAhead` to `30`, and playing at
position `20`, we would remove if present media data already buffered a
position `50` or more (which could for example mostly happen when
seeking back in the content).

We found out however that applying that same value to the text buffer
may not always be sensible:
 - `maxBufferAhead` is most likely defined by an application to restrict
   memory usage. The space taken by the text cues have much less weight
   than audio and video
 - We encounter several contents where loaded text segments span for 1
   minute because of how small those segments are. Here we risk
   progressively loading the same segment as its end will be GCed
   multiple times.

Due to both of these situations, I decided for now to have a lower bound
on the `maxBufferAhead` applied on the text buffer to 2 minutes. We also
could have made the `maxBufferAhead` API more configurable by letting an
application set a different setting per media type, but I thought that
an application may not easily realize the issue.
@peaBerberian peaBerberian added this to the 3.33.0 milestone Nov 30, 2023
@Florent-Bouisset
Copy link
Collaborator

When clearing data due to a maxBufferAhead, we may clear partial data from a segment, as we remove directly from the buffer: segmentBuffer.removeBuffer()
Maybe we can check the segmentInventory before removing data from the buffer, and don't remove data if the segment corresponding to it is within the range of the maxAheadPosition.

Before clearing:

         A                       B                     C
|=========|=====X=====|=========|
^CurrentPosition     ^MaxAheadPosition

After clearing (current behavior):

         A                     B                  
|=========|======|
^CurrentPosition       ^MaxAheadPosition

  • B was partially cleared
  • C was entirely cleared

After clearing (suggestion):

         A                     B                  
|=========|======x====|
^CurrentPosition       ^MaxAheadPosition

  • B was not cleared
  • C was entirely cleared

This may add to much complexities but it would handle the case if text segments are > 120s

@peaBerberian
Copy link
Collaborator Author

peaBerberian commented Dec 4, 2023

By maxAheadPosition, I suppose you mean the wantedBufferAhead? (or its derived value, the buffer goal)

So basically when GCing forward we shouldn't remove a segment which has a part of the data below it?
It may fix many unwanted behaviors but I'm still not totally convinced, as wantedBufferAhead may be set to limit memory usage. Here it would be a little un-predictible to an application.

Though in the current code we do risk re-downloading the same segment every maxBufferAhead - wantedBufferAhead seconds for segments that are larger than this difference (which should still be relatively rare).

We maybe could POC it.

// of duration. Let's make an exception here by authorizing a
// larger text buffer ahead, to avoid unnecesarily reloading the
// same text track.
Math.max(val, 2 * 60) :
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can set this in the config file something like: MINIMUM_MAX_BUFFER_AHEAD

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@peaBerberian peaBerberian merged commit ffc1a5d into master Dec 19, 2023
3 checks passed
@peaBerberian peaBerberian mentioned this pull request Jan 24, 2024
@peaBerberian peaBerberian deleted the fix/maxBufferAhead-text-exception branch February 7, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants