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

Flush Front Buffer #5479

Closed
whatever1211 opened this issue May 10, 2023 · 8 comments · Fixed by #5761
Closed

Flush Front Buffer #5479

whatever1211 opened this issue May 10, 2023 · 8 comments · Fixed by #5761
Assignees
Labels
Feature proposal Verify Fixed An unreleased bug fix has been merged and should be verified before closing.
Milestone

Comments

@whatever1211
Copy link

Is your feature request related to a problem? Please describe.

On Tizen and WebOS platforms, memory management is extremely important.

We saw that on Movie (not Live) media, when users repeatly seek back to start play media, the front buffer is not flushed, which leads to memory growth and eventually freezing experiences.

Version hls.js: 1.4.1

Log:
Untitled

Describe the solution you'd like

Desired a flushFrontBuffer feature based on currentTime and maxBufferLength, that run along side with flushBackBuffer.

    _proto.onFragChanged = function onFragChanged(event, data) {
      this.flushBackBuffer();
      this.flushFrontBuffer();
    }
    _proto.flushFrontBuffer = function flushFrontBuffer () {
      var hls = this.hls,
        details = this.details,
        media = this.media,
        sourceBuffer = this.sourceBuffer;
      if (!media || details === null) {
        return;
      }
      var sourceBufferTypes = this.getSourceBufferTypes();
      if (!sourceBufferTypes.length) {
        return;
      }

      var currentTime = media.currentTime;
      var targetDuration = details.levelTargetDuration;
      var maxBufferLength = this.hls.config.maxBufferLength;
      var targetForwardBufferPosition = Math.ceil(currentTime / targetDuration) * targetDuration + maxBufferLength + targetDuration * 2; // x2 to tolerate one extra segment without remove it
      sourceBufferTypes.forEach(function (type) {
        var sb = sourceBuffer[type];
        if (sb) {
          var buffered = BufferHelper.getBuffered(sb);
          if (buffered.length > 0 && targetForwardBufferPosition > buffered.start(0) && targetForwardBufferPosition < buffered.end(buffered.length - 1)) {
            hls.trigger(Events.BUFFER_FLUSHING, {
              startOffset: targetForwardBufferPosition,
              endOffset: Number.POSITIVE_INFINITY,
              type: type
            });
          }
        }
      });
    }

Additional context

No response

@whatever1211 whatever1211 added Feature proposal Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. labels May 10, 2023
@robwalch
Copy link
Collaborator

Desired a flushFrontBuffer feature based on currentTime and maxBufferLength, that run along side with flushBackBuffer

flushBackBuffer uses backBufferLength which has a default value of Infinity. This means that the default behavior of HLS.js is to leave buffer ejection to the user agent. For this reason we should not implement forward buffer ejection based on maxBufferLength or maxMaxBufferLength which have default values of 30 and 600 respectively.

@whatever1211
Copy link
Author

Then, can you add a config forwardBufferLength which has a default value of -1. And user can modify the value based on neededs. I ask this because the handler of buffer ejection on Tizen and WebOS are very poor. Thank you.

@robwalch
Copy link
Collaborator

forwardBufferLength with a default value of Infinity could work. null, NaN (anything non-finite) and negative values should all be ignored, meaning do not enforce buffer length.

We should also then update the docs to distinguish "buffer management" settings that are for buffer ejection (backBufferLength, forwardBufferLength) and limiting streaming activity (maxBufferLength, maxMaxBufferLength and maxBufferSize).

@robwalch robwalch removed the Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. label May 11, 2023
@robwalch robwalch added this to the 1.5.0 milestone May 11, 2023
@robwalch robwalch assigned robwalch and iamboorrito and unassigned robwalch Aug 4, 2023
@robwalch
Copy link
Collaborator

Hi @whatever1211,

Could you take a look at #5761? Would setting a frontBufferFlushThreshold resolve this for you?

@robwalch robwalch added the Verify Fixed An unreleased bug fix has been merged and should be verified before closing. label Aug 28, 2023
robwalch added a commit that referenced this issue Aug 31, 2023
…5761)

Fixes #5479

Co-authored-by: Evan Burton <eburton2@apple.com>
@whatever1211
Copy link
Author

Hi @robwalch, sorry for the late comment.

I have tried your new frontBufferFlushThreshold value and it did resolve most of the cases for me.

Except 1 contiguous case, where I set these configures

{
    maxBufferLength: 30,
    frontBufferFlushThreshold: 30
}

And I seek back 5 seconds repeatedly. The whole buffer is still contiguous and hls.js desire not to clear front buffer eventhough the buffer length has exceeded frontBufferFlushThreshold value, 30 seconds. Example log belows:

[log] > [stream-controller]: Buffered main sn: 501 of level 4 (frag:[2004.937-2009.000] > buffer:[1997.000-2292.818])

Please also check frontBufferFlushThreshold value for contiguous case too. Thanks.

@whatever1211
Copy link
Author

Once more I notice is the different ended check between flushBackBuffer and flushFrontBuffer.

File: src/controller/buffer-controller.ts

flushBackBuffer ended check:

if (sb.ended && buffered.end(buffered.length - 1) - currentTime < targetDuration * 2) {

flushFrontBuffer ended check:

if (sb.ended && currentTime - bufferEnd < 2 * targetDuration) {

Shouldn't the flushFrontBuffer ended check is bufferEnd - currentTime instead of currentTime - bufferEnd. Just like frontBackBuffer.

if (sb.ended && bufferEnd - currentTime < targetDuration * 2) {

@robwalch
Copy link
Collaborator

robwalch commented Sep 8, 2023

Hi @whatever1211,

The difference in back and front buffer ejection is by design and described in the API and migrating docs:

https://github.com/video-dev/hls.js/blob/v1.5.0-alpha.0/docs/API.md#frontbufferflushthreshold

https://github.com/video-dev/hls.js/blob/v1.5.0-alpha.0/MIGRATING.md#front-buffer-eviction

We do not want to remove from a contiguous forward buffer unless there is a quota exceeded error. That eviction is the responsibility of the user agent on append. This change removes media from disconnected ranges not being removed by certain UAs, keeping a contiguous forward buffer intact to avoid reloading.

@whatever1211
Copy link
Author

Ok @robwalch, I get it now. Thank you for the clarification.

All others cases are resolved for me now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature proposal Verify Fixed An unreleased bug fix has been merged and should be verified before closing.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants