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

Documentation - Add interface for AbrController #4842

Merged

Conversation

lpommers
Copy link
Contributor

@lpommers lpommers commented Aug 11, 2022

This PR will...

Create a type interface for the AbrController

Why is this Pull Request needed?

Users can implement their own Custom AbrController. Having a typed interface for Abr Controllers will help developers understand what properties and methods their Custom AbrControllers must implement to operate with hls.js

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

re: #4824 (comment)

If you could define an interface for abr controller classes to implement in src/types and link to it in the docs, that would be most helpful

I didn't see any prior art for linking directly to src/types in the API documentation, so I just updated the API.md file directly, but am willing to link directly to src/types if that's preferred.


It wasn't specified in the above comment, but because the docs also mention:

For hls.bandwidthEstimate() to return an estimate from your custom controller, it will also need to satisfy abrController.bwEstimator.getEstimate().

I added the bwEstimator to the interface. This could probably use some 👀, since I'm not sure if that was correct or not.

Resolves issues:

Closes #4824

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

@lpommers lpommers force-pushed the feature/interface-for-abr-controller branch 3 times, most recently from b69361c to c01d891 Compare August 16, 2022 13:20
docs/API.md Outdated

- get/set `nextAutoLevel`: return next auto-quality level/force next auto-quality level that should be returned (currently used for emergency switch down)
- get/set `autoLevelCapping`: capping/max level value that could be used by ABR Controller
- `clearTimer()` - Stops the abandon rules check polling until another fragment is loaded
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than defining what clearTimer() does in the current ABR controller, we should describe when it is called from hls.ts, and why. Well, this is why, but as mentioned there are probably better ways to check and stop checking if a fragment is taking too long to load.

I am tempted to leave this undocumented. It was only added as fix in #3556, and I would like to remove in the next minor. Let me know if that conflicts with your own custom ABR controller implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't conflict with our with custom ABR Controller at all. I can push up a commit dropping clearTimer from the documentation

Copy link
Contributor Author

@lpommers lpommers Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed clearTimer in d7bc232

@robwalch robwalch added this to the 1.4.0 milestone Aug 17, 2022
@robwalch robwalch added the ABR label Aug 17, 2022
@lpommers lpommers force-pushed the feature/interface-for-abr-controller branch from 5e8dd2a to d7bc232 Compare August 22, 2022 12:57
@lpommers lpommers force-pushed the feature/interface-for-abr-controller branch from d7bc232 to fff8c75 Compare August 31, 2022 12:16
@lpommers lpommers force-pushed the feature/interface-for-abr-controller branch from fff8c75 to 2faf9b7 Compare December 16, 2022 10:23
@robwalch robwalch merged commit 617d78b into video-dev:master Jan 16, 2023
robwalch pushed a commit that referenced this pull request Jan 16, 2023
robwalch pushed a commit that referenced this pull request Jan 16, 2023
* create an interface for abr controllers

* use AbrComponentApi interface in abr-controller.ts

* update docs for custom abr controllers

* remove clearTimer from abr interface and documentation
robwalch pushed a commit that referenced this pull request Jan 26, 2023
* create an interface for abr controllers

* use AbrComponentApi interface in abr-controller.ts

* update docs for custom abr controllers

* remove clearTimer from abr interface and documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Do Custom ABR Controllers really need get/set autoLevelCapping?
2 participants