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

Bugfix: fix missing load stats for LL-HLS #4283

Conversation

Oleksandr0xB
Copy link

This PR will...

Fix problem with missing load stats for LL-HLS.

Why is this Pull Request needed?

With missing stats ABR for LL-HLS usually stuck on lowers channels. This PR will fix it.

Problem with existing code is following.

  1. When new manifest parsed oldPart stats copied to newPart
    // Merge parts
    mapPartIntersection(
    oldDetails.partList,
    newDetails.partList,
    (oldPart: Part, newPart: Part) => {
    newPart.elementaryStreams = oldPart.elementaryStreams;
    newPart.stats = oldPart.stats;
    }
    );
  2. If part loading will start after that loader stats stats copied to oldPart
    // Assign part stats to the loader's stats reference
    part.stats = loader.stats;

    As result ABR will use empty newPart stats.

Changing direction of assign fix this problem

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

Maybe the same logic can be applied to fragment loading stats

Resolves issues:

#3578

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

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the Stale label Apr 16, 2022
@stale
Copy link

stale bot commented Apr 27, 2022

This issue has been automatically closed because it has not had recent activity. If this issue is still valid, please ping a maintainer and ask them to label it accordingly.

@stale stale bot closed this Apr 27, 2022
@alienpavlov
Copy link

Why this PR didn't go through? It looks good to me

@robwalch
Copy link
Collaborator

This creates a new problem. It is not clear how to reproduce the issue described or how this fix is valid. stats are accumulated from loaders, which is why HLS.js copies stats from the (part) loader to the part.

Please file an issue with steps to reproduce. I could not reproduce the issue following the details provided in this PR.

@robwalch robwalch reopened this Aug 4, 2022
@robwalch robwalch removed the Confirmed label Aug 4, 2022
@robwalch robwalch closed this Aug 4, 2022
@robwalch
Copy link
Collaborator

robwalch commented Aug 4, 2022

Hi @Oleksandr0xB,

Thanks for the fix. I confirmed the issue and have another way to fix it that involved always selecting parts to load from level.details.partList rather than reusing the same partList present when selecting the first part in a fragment.

robwalch added a commit that referenced this pull request Aug 4, 2022
…ents and higher TTFB

Fixes #3578 (special thanks to @Oleksandr0xB for submitting #4283)
Fixes #3563 and Closes #3595 (special thanks to @kanongil)
robwalch added a commit that referenced this pull request Aug 4, 2022
…ents and higher TTFB

Fixes #3578 (special thanks to @Oleksandr0xB for submitting #4283)
Fixes #3563 and Closes #3595 (special thanks to @kanongil)
robwalch added a commit that referenced this pull request Aug 5, 2022
…ents and higher TTFB

Fixes #3578 (special thanks to @Oleksandr0xB for submitting #4283)
Fixes #3563 and Closes #3595 (special thanks to @kanongil)
robwalch added a commit that referenced this pull request Nov 28, 2022
…ents and higher TTFB

Fixes #3578 (special thanks to @Oleksandr0xB for submitting #4283)
Fixes #3563 and Closes #3595 (special thanks to @kanongil)
silltho pushed a commit to silltho/hls.js that referenced this pull request Dec 2, 2022
…ents and higher TTFB

Fixes video-dev#3578 (special thanks to @Oleksandr0xB for submitting video-dev#4283)
Fixes video-dev#3563 and Closes video-dev#3595 (special thanks to @kanongil)
robwalch added a commit that referenced this pull request Jan 9, 2023
…ents and higher TTFB

Fixes #3578 (special thanks to @Oleksandr0xB for submitting #4283)
Fixes #3563 and Closes #3595 (special thanks to @kanongil)
robwalch added a commit that referenced this pull request Jan 12, 2023
…ents and higher TTFB

Fixes #3578 (special thanks to @Oleksandr0xB for submitting #4283)
Fixes #3563 and Closes #3595 (special thanks to @kanongil)
robwalch added a commit that referenced this pull request Jan 18, 2023
* Improve bandwidth estimation and adaptive switching with smaller segments and higher TTFB
Fixes #3578 (special thanks to @Oleksandr0xB for submitting #4283)
Fixes #3563 and Closes #3595 (special thanks to @kanongil)

* Load rate and loaded delay calculation fixes

* Convert ttfbEstimate to seconds

* Include main variant init segments in TTFB sampling

* Use ttfb estimate in abandon rules down-switch timing
robwalch added a commit that referenced this pull request Jan 26, 2023
* Improve bandwidth estimation and adaptive switching with smaller segments and higher TTFB
Fixes #3578 (special thanks to @Oleksandr0xB for submitting #4283)
Fixes #3563 and Closes #3595 (special thanks to @kanongil)

* Load rate and loaded delay calculation fixes

* Convert ttfbEstimate to seconds

* Include main variant init segments in TTFB sampling

* Use ttfb estimate in abandon rules down-switch timing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants