hls.js icon indicating copy to clipboard operation
hls.js copied to clipboard

small fragment incorrectly skipped

Open kedanielwu opened this issue 2 years ago • 2 comments

What version of Hls.js are you using?

latest

What browser (including version) are you using?

irrelevent

What OS (including version) are you using?

irrelevent

Test stream

No response

Configuration

default

Additional player setup steps

No response

Checklist

  • [X] The issue observed is not already reported by searching on Github under https://github.com/video-dev/hls.js/issues
  • [X] The issue occurs in the stable client (latest release) on https://hls-js.netlify.com/demo and not just on my page
  • [X] The issue occurs in the latest client (main branch) on https://hls-js-dev.netlify.com/demo and not just on my page
  • [X] The stream has correct Access-Control-Allow-Origin headers (CORS)
  • [X] There are no network errors such as 404s in the browser console when trying to play the stream

Steps to reproduce

grab any stream with fragment duration less than default maxFragLookUpTolerance

in our case, the small fragment is at the end of range between two discontinue tag

the last fragment is always skipped by fragment finder.

Expected behaviour

fragment not being skipped

What actually happened?

fragment is skipped

Console output

irrelevent

Chrome media internals output

No response

kedanielwu avatar Sep 21 '22 14:09 kedanielwu

I think the issue is coming from https://github.com/video-dev/hls.js/blob/master/src/controller/fragment-finders.ts#L123

candidateLookupTolerance will be the minimum of config.maxFragLookUpTolerance and candidate.duration, then the first if clause checks whether candidate's end time with that tolerance will beyond current buffer end.

but consider the following case, my current buffer end is 5 sec, and my candidate start time is also 5sec, with 0.2 sec duration. The calculation simply became candidate.start <= bufferEnd, since maxFragLookUpTolerance by default is 0.25s, and this candidate will be considered invalid.

my thought is, the check shouldn't be <= bufferEnd, the case === bufferEnd should be considered as valid candidate.

I could also change the maxFragLookUpTolerance to a lower value to cope this issue, but the problem is for an streaming service provider I can't really determine/predict the smallest possible fragment duration.(for extreme case, yes, it won't be lower than 0.01 sec for sure)

kedanielwu avatar Sep 21 '22 14:09 kedanielwu

Please include as part of each bug report:

  • Actual hls.js version (not "latest")
  • Test stream/page that can be used to reproduce the issue

robwalch avatar Sep 21 '22 17:09 robwalch

sorry for that, I am working on a custom fork (from v1.0.0 and merged v1.1.0 changes as well). Since I haven't modified related code, I do believe the issue exists in this official version as well (v1.0.0 to v1.2.x version).

For test streams I will try to provide one. The one I use require authentication so it can't be shared.

kedanielwu avatar Sep 22 '22 06:09 kedanielwu

Hi @kedanielwu,

Thanks for this info. Some work has gone into v1.2.x to avoid situations like the one described here, but I don't have a specific example on the demo page that I can point to do reproduce or verify a fix. These recent changes may be relevant (and there are even more in v1.2.0):

  • #4794
  • #4864 (requires #4887)

In the above cases, the playlist segment durations were not shorter than maxFragLookUpTolerance, but the media parsed was shorter than advertised. Let me know when you have a stream to share that repros the issue.

robwalch avatar Sep 22 '22 21:09 robwalch

Hi @robwalch,

I am not familiar with stream production, so I'm still looking for a way to produce a sample stream, that might cause some time.

Meanwhile I have added a unit test to demonstrate this issue, I am wondering why in the original unit test for very small fragment, a delta PTS is present in the test case, is this always the case for small fragment? You can find the unit test here

https://github.com/kedanielwu/hls.js/blob/master/tests/unit/controller/fragment-finders.js#L204

and this unit test is failed in master branch

kedanielwu avatar Sep 23 '22 08:09 kedanielwu

It's not an issue of delta PTS. The previous test only passes because the start time so low that the fragment is determined to be at the start of the playlist. This test that includes deltaPTS but also starts at 5 also fail with 1 because the derived tolerance of 0.2 is greater than the fragment duration of 0.1:

    it('does not skip very small fragments', function () {
      const frag = {
        start: 5,
        duration: 0.1,
        deltaPTS: 0.1,
      };
      const actual = fragmentWithinToleranceTest(5, tolerance, frag);
      expect(actual).to.equal(0);
    });

fragmentWithinToleranceTest will not match a candidate whose duration is <= the derived tolerance.

This may seem wrong, but it is actually important for selection to have some bias toward what comes next when we care for performance when seeking. This comes at the cost of accuracy, but if you want to wait for such content, then set maxFragLookUpTolerance to 0.

What we should be looking at is how a stream controller skips segments in the playlist. Even though generally the stream controller looks for segments based on buffer end, it keeps track of the last appended fragment, and should always check if the next segment is viable before searching for other candidates. This is why we need a test stream to verify the issue.

robwalch avatar Sep 23 '22 19:09 robwalch

Hi @kedanielwu,

#5084 addresses this issue and includes your tests for small segments. Thank you for the contribution.

robwalch avatar Dec 07 '22 02:12 robwalch

Thanks! Sorry that I am not able to provide a test stream, all the relevent contents I have are encryped and can not be shared publically.

I have tried #5084 and the issue is resolved!

kedanielwu avatar Dec 07 '22 06:12 kedanielwu