fix: media buffer is not empty but video element buffer is empty #6571

JackPu · 2024-07-24T09:07:35Z

This PR will

It will solve one edge case: the mediaBuffer is not empty, but the video element buffered is empty, so the seekToStartPosition can be executed.

Why is this Pull Request needed?

The video will freeze if we can not seek the correct position in the special case:

the audio buffer [12, 17]
the video buffer [8, 11.9]
the video element buffer []

Then it will not increase the gap delta position because the bufferStart is 0.

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

robwalch · 2024-07-24T14:15:55Z

If the audio and video buffer are not aligned then there is a problem with the HLS source.

Can you file an issue that with steps to reproduce? I'd like to understand the root cause before accepting a fix.

robwalch

Please define two consts, one for media and one for mediaBuffer, then replace media where necessary.

JackPu · 2024-07-25T05:37:45Z

If the audio and video buffer are not aligned then there is a problem with the HLS source.

Can you file an issue that with steps to reproduce? I'd like to understand the root cause before accepting a fix.

Yeah the resource is correct. We cannot reproduce the issue in a stable way because the video position is not always accurate. Here is a reproducing step:

+ Launch your app with localhost:3000
+ Load the manifest  https://manifest.production-public.tubi.io/ac98fc83-9f0a-4261-9ac3-2503f4946617/xo5kvupj02.m3u8?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJjZG5fcHJlZml4IjoiaHR0cHM6Ly9mYXN0bHkyLnR1YmkudmlkZW8iLCJjb3VudHJ5IjoiVVMiLCJkZXZpY2VfaWQiOiJiZGIyNWU2NS05NjkwLTRjODAtYmEwNy1mZGZkZGQ0ZWM4OTYiLCJleHAiOjE3MjIxNDM3MDAsIm1lZGlhX3NpZyI6NTI4OTM4MjMsInBsYXRmb3JtIjoiQU1BWk9OIiwidXNlcl9pZCI6MTUyODYyNDUxfQ.lDQ1g97rKMUd0Hw7pATaA5gIxSzz2T0hl-pnhkJTdCI
+ Update the config `startPostion`: 5732.1 // important
+ Watch the issue

My config in your demo page is here:

https://hlsjs.video-dev.org/demo/?src=https%3A%2F%2Fmanifest.production-public.tubi.io%2Fac98fc83-9f0a-4261-9ac3-2503f4946617%2Fxo5kvupj02.m3u8%3Ftoken%3DeyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJjZG5fcHJlZml4IjoiaHR0cHM6Ly9mYXN0bHkyLnR1YmkudmlkZW8iLCJjb3VudHJ5IjoiVVMiLCJkZXZpY2VfaWQiOiJiZGIyNWU2NS05NjkwLTRjODAtYmEwNy1mZGZkZGQ0ZWM4OTYiLCJleHAiOjE3MjIxNDM3MDAsIm1lZGlhX3NpZyI6NTI4OTM4MjMsInBsYXRmb3JtIjoiQU1BWk9OIiwidXNlcl9pZCI6MTUyODYyNDUxfQ.lDQ1g97rKMUd0Hw7pATaA5gIxSzz2T0hl-pnhkJTdCI&demoConfig=eyJlbmFibGVTdHJlYW1pbmciOnRydWUsImF1dG9SZWNvdmVyRXJyb3IiOmZhbHNlLCJzdG9wT25TdGFsbCI6dHJ1ZSwiZHVtcGZNUDQiOmZhbHNlLCJsZXZlbENhcHBpbmciOi0xLCJsaW1pdE1ldHJpY3MiOi0xfQ==

A video snapshot is here:

https://drive.google.com/file/d/1grpQx0ROHjxV0K6DfpX-90tdA_Kp_vD4/view?usp=drive_link

Why this happen

I found the issue is imported by this PR: #5084

It will make an audio fragment and a video fragment check differently sometimes. My example shows it requests audio with the maxFragLookUpTolerance value like:

audio fragments: [0-2].... ,[5730.04, 5732.309], [5732.309, 5734.2]...
The requested position is 5732.1
But the maxFragLookUpTolerance(0.25) will make the request to the next fragment [5732.309, 5734.2] not [5730.04, 5732.309]

But the video buffer request didn't use maxFragLookUpTolerance value because of the change.

So it request the fragment [5729.650, 5732.277]

So the buffer in two tracks will be

the audio buffer [5732.309, 5734.2]
the video buffer [5729.650, 5732.277]
the video element buffer []

So I think we would better use the media element buffer to check.

robwalch · 2024-07-26T17:38:42Z

My config in your demo page is here:

The asset has segments which do not start with a key frame. That explains why initial audio and video segment selection and buffered ranges do not align. hls.js backtracks or requests earlier video segments so that it can buffer video starting from a time earlier than the target start/seek position.

robwalch · 2024-07-26T18:23:19Z

Hi @JackPu,

Have you been able to reproduce this issue in dev (https://hlsjs-dev.video-dev.org/demo/)?

I cannot. The issue is really that the audio segment needed is not loaded and that has been fixed.

If you are looking for a fix in v1.5.x then we need a change made against the "patch/v1.5.x" branch.

robwalch

A change is needed to ensure seekToStartPos happens only once there is combined buffer with adjustment if needed. The thing that is missing from this PR is coordination of audio-stream-controller and subtitle-stream-controller:

hls.js/src/controller/audio-stream-controller.ts

Lines 676 to 684 in cedf96d

    
           onFragBuffered(event: Events.FRAG_BUFFERED, data: FragBufferedData) { 
        
             const { frag, part } = data; 
        
             if (frag.type !== PlaylistLevelType.AUDIO) { 
        
               if (!this.loadedmetadata && frag.type === PlaylistLevelType.MAIN) { 
        
                 const bufferable = this.videoBuffer || this.media; 
        
                 if (bufferable) { 
        
                   const bufferedTimeRanges = BufferHelper.getBuffered(bufferable); 
        
                   if (bufferedTimeRanges.length) { 
        
                     this.loadedmetadata = true;

hls.js/src/controller/subtitle-stream-controller.ts

Lines 194 to 197 in cedf96d

    
           onFragBuffered(event: Events.FRAG_BUFFERED, data: FragBufferedData) { 
        
             if (!this.loadedmetadata && data.frag.type === PlaylistLevelType.MAIN) { 
        
               if (this.media?.buffered.length) { 
        
                 this.loadedmetadata = true;

If we only gate where setting of loadedmetadata in stream-controller to delay seek to start, audio and subtitles may set theirloadedmetadata to true first and on next tick load fragments at currentTime which is still set to 0:

hls.js/src/controller/base-stream-controller.ts

Lines 1482 to 1492 in cedf96d

    
           protected getLoadPosition(): number { 
        
             const { media } = this; 
        
             // if we have not yet loaded any fragment, start loading from start position 
        
             let pos = 0; 
        
             if (this.loadedmetadata && media) { 
        
               pos = media.currentTime; 
        
             } else if (this.nextLoadPosition) { 
        
               pos = this.nextLoadPosition; 
        
             } 
        
             return pos;

There's a few other places where loadedmetadata is read so this is a risky change to make. It's adding a dependency that the combined buffer be populated after appending a 'main' fragment when that could also happen when appending audio (if main is already buffered). This could delay start times or lock up seek to start entirely - which is what we want to fix, but I cannot reproduce in dev.

robwalch

The fix for this in patch/v1.5.x would be to add || !this.startFragRequested to to this conditional so that on start the audio and main stream controllers use strict tolerance (0) for initial fragment selection independent of loadedmetadata:

hls.js/src/controller/base-stream-controller.ts

Lines 1304 to 1305 in cedf96d

    
           const lookupTolerance = 
        
             bufferEnd > end - maxFragLookUpTolerance ? 0 : maxFragLookUpTolerance;

Same here for dev where there is already an additional check for paused added in #6524. I cannot reproduce this issue in dev because media is a paused at start prior to seeking to the start position (but you could break that by calling play() successfully before reaching this point):

hls.js/src/controller/base-stream-controller.ts

Lines 1439 to 1444 in b472831

    
           const lookupTolerance = 
        
             backwardSeek || 
        
             bufferEnd > end - maxFragLookUpTolerance || 
        
             this.media?.paused 
        
               ? 0 
        
               : maxFragLookUpTolerance;

robwalch · 2024-07-30T22:01:32Z

Hi @JackPu,

I've incorporated v1.6.0 fixes for this issue in #6591. It addresses the fragment selection that misses the audio needed on startup, and also delays seek to start until both audio and video source buffers are provisioned (loadedmetadata in all stream controllers is removed in favor of stream-controller/top-level hasEnoughToStart).

Please try the branch build here https://feature-interstitials.hls-js-4zn.pages.dev/

JackPu · 2024-07-31T03:31:26Z

A change is needed to ensure seekToStartPos happens only once there is combined buffer with adjustment if needed. The thing that is missing from this PR is coordination of audio-stream-controller and subtitle-stream-controller:

hls.js/src/controller/audio-stream-controller.ts

Lines 676 to 684 in cedf96d

onFragBuffered(event: Events.FRAG_BUFFERED, data: FragBufferedData) {

const { frag, part } = data;

if (frag.type !== PlaylistLevelType.AUDIO) {

if (!this.loadedmetadata && frag.type === PlaylistLevelType.MAIN) {

const bufferable = this.videoBuffer || this.media;

if (bufferable) {

const bufferedTimeRanges = BufferHelper.getBuffered(bufferable);

if (bufferedTimeRanges.length) {

this.loadedmetadata = true;

hls.js/src/controller/subtitle-stream-controller.ts

Lines 194 to 197 in cedf96d

onFragBuffered(event: Events.FRAG_BUFFERED, data: FragBufferedData) {

if (!this.loadedmetadata && data.frag.type === PlaylistLevelType.MAIN) {

if (this.media?.buffered.length) {

this.loadedmetadata = true;

If we only gate where setting of loadedmetadata in stream-controller to delay seek to start, audio and subtitles may set theirloadedmetadata to true first and on next tick load fragments at currentTime which is still set to 0:

hls.js/src/controller/base-stream-controller.ts

Lines 1482 to 1492 in cedf96d

protected getLoadPosition(): number {

const { media } = this;

// if we have not yet loaded any fragment, start loading from start position

let pos = 0;

if (this.loadedmetadata && media) {

pos = media.currentTime;

} else if (this.nextLoadPosition) {

pos = this.nextLoadPosition;

}

return pos;

There's a few other places where loadedmetadata is read so this is a risky change to make. It's adding a dependency that the combined buffer be populated after appending a 'main' fragment when that could also happen when appending audio (if main is already buffered). This could delay start times or lock up seek to start entirely - which is what we want to fix, but I cannot reproduce in dev.

Thank you so much. It works for me. And it did solve my problems. And my PR is not good enough. It will increase the join time(loadStart -> loadeddata). And I have patched your PR now #6524

JackPu · 2024-07-31T03:37:57Z

I have another issue. But I cannot find the reason. Maybe it could be about the level switch. I have upgraded the branch in our own repo from 1.2.7 -> 1.5.7. We use the startPsotion config. And we noticed the join time(loadStart -> loadeddatat time) increased a little. And I found when we begin, we will switch level 0 -> 1 and flush the level 0 chunks buffer. And then, it will also switch to level 1-> level 3. It also flushes the level 1 buffer. So now it will play the level 3 video in the start position. Should I create an issue about this? And I can provide the logs and video snapshot later.

robwalch · 2024-07-31T18:36:30Z

Should I create an issue about this? And I can provide the logs and video snapshot later.

Yes. Please file an issue.

robwalch · 2024-08-08T16:53:31Z

Closing as this is being addressed in #6591.

fix: media buffer is not empty but video element buffer is empty

577db30

robwalch requested changes Jul 24, 2024

View reviewed changes

fix: resolve the comments

ae4b28c

JackPu requested a review from robwalch July 25, 2024 05:50

robwalch added non-independent-segments missing-keyframe Stream has non-independent segments (no key-frames) or missing key-frame at start labels Jul 26, 2024

robwalch reviewed Jul 26, 2024

View reviewed changes

robwalch requested changes Jul 26, 2024

View reviewed changes

robwalch added Bug Confirmed and removed non-independent-segments missing-keyframe Stream has non-independent segments (no key-frames) or missing key-frame at start labels Jul 26, 2024

This was referenced Jul 29, 2024

Freezing on seeking and black screen on stream Load. #6576

Open

HLS Interstitials Support #6591

Open

robwalch closed this Aug 8, 2024

robwalch added this to the 1.6.0 milestone Aug 8, 2024

robwalch added the Verify Fixed An unreleased bug fix has been merged and should be verified before closing. label Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: media buffer is not empty but video element buffer is empty #6571

fix: media buffer is not empty but video element buffer is empty #6571

JackPu commented Jul 24, 2024 •

edited

Loading

robwalch commented Jul 24, 2024 •

edited

Loading

robwalch left a comment

JackPu commented Jul 25, 2024 •

edited

Loading

robwalch commented Jul 26, 2024

robwalch commented Jul 26, 2024 •

edited

Loading

robwalch left a comment •

edited

Loading

robwalch left a comment •

edited

Loading

robwalch commented Jul 30, 2024

JackPu commented Jul 31, 2024 •

edited

Loading

JackPu commented Jul 31, 2024

robwalch commented Jul 31, 2024

robwalch commented Aug 8, 2024

	onFragBuffered(event: Events.FRAG_BUFFERED, data: FragBufferedData) {
	const { frag, part } = data;
	if (frag.type !== PlaylistLevelType.AUDIO) {
	if (!this.loadedmetadata && frag.type === PlaylistLevelType.MAIN) {
	const bufferable = this.videoBuffer \|\| this.media;
	if (bufferable) {
	const bufferedTimeRanges = BufferHelper.getBuffered(bufferable);
	if (bufferedTimeRanges.length) {
	this.loadedmetadata = true;

	onFragBuffered(event: Events.FRAG_BUFFERED, data: FragBufferedData) {
	if (!this.loadedmetadata && data.frag.type === PlaylistLevelType.MAIN) {
	if (this.media?.buffered.length) {
	this.loadedmetadata = true;

	protected getLoadPosition(): number {
	const { media } = this;
	// if we have not yet loaded any fragment, start loading from start position
	let pos = 0;
	if (this.loadedmetadata && media) {
	pos = media.currentTime;
	} else if (this.nextLoadPosition) {
	pos = this.nextLoadPosition;
	}

	return pos;

	const lookupTolerance =
	bufferEnd > end - maxFragLookUpTolerance ? 0 : maxFragLookUpTolerance;

	const lookupTolerance =
	backwardSeek \|\|
	bufferEnd > end - maxFragLookUpTolerance \|\|
	this.media?.paused
	? 0
	: maxFragLookUpTolerance;

fix: media buffer is not empty but video element buffer is empty #6571

fix: media buffer is not empty but video element buffer is empty #6571

Conversation

JackPu commented Jul 24, 2024 • edited Loading

This PR will

Why is this Pull Request needed?

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

Resolves issues:

Checklist

robwalch commented Jul 24, 2024 • edited Loading

robwalch left a comment

Choose a reason for hiding this comment

JackPu commented Jul 25, 2024 • edited Loading

Why this happen

robwalch commented Jul 26, 2024

robwalch commented Jul 26, 2024 • edited Loading

robwalch left a comment • edited Loading

Choose a reason for hiding this comment

robwalch left a comment • edited Loading

Choose a reason for hiding this comment

robwalch commented Jul 30, 2024

JackPu commented Jul 31, 2024 • edited Loading

JackPu commented Jul 31, 2024

robwalch commented Jul 31, 2024

robwalch commented Aug 8, 2024

JackPu commented Jul 24, 2024 •

edited

Loading

robwalch commented Jul 24, 2024 •

edited

Loading

JackPu commented Jul 25, 2024 •

edited

Loading

robwalch commented Jul 26, 2024 •

edited

Loading

robwalch left a comment •

edited

Loading

robwalch left a comment •

edited

Loading

JackPu commented Jul 31, 2024 •

edited

Loading