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

small fragment incorrectly skipped #4919

Closed
5 tasks done
kedanielwu opened this issue Sep 21, 2022 · 8 comments · Fixed by #5084
Closed
5 tasks done

small fragment incorrectly skipped #4919

kedanielwu opened this issue Sep 21, 2022 · 8 comments · Fixed by #5084

Comments

@kedanielwu
Copy link

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

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 kedanielwu added Bug Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. labels Sep 21, 2022
@kedanielwu
Copy link
Author

kedanielwu commented Sep 21, 2022

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)

@robwalch
Copy link
Collaborator

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

@kedanielwu
Copy link
Author

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.

@robwalch
Copy link
Collaborator

robwalch commented Sep 22, 2022

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):

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.

@kedanielwu
Copy link
Author

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

@robwalch robwalch added Confirmed and removed Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. labels Sep 23, 2022
@robwalch
Copy link
Collaborator

robwalch commented Sep 23, 2022

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
Copy link
Collaborator

robwalch commented Dec 7, 2022

Hi @kedanielwu,

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

@kedanielwu
Copy link
Author

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!

robwalch added a commit that referenced this issue Dec 7, 2022
robwalch added a commit that referenced this issue Dec 8, 2022
* Eagerly match segments without tolerance offset
Fixes #4919

* Maintain expected `fragPrevious` behavior
infocity-hirata pushed a commit to INFOCITY/hls.js that referenced this issue Jun 30, 2023
* Eagerly match segments without tolerance offset
Fixes video-dev#4919

* Maintain expected `fragPrevious` behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

2 participants