-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: support more key system error detail #6807
feat: support more key system error detail #6807
Conversation
5c7f1a8
to
3cb4d8b
Compare
@robwalch Hi, do I need to add some unit tests for this? Or do you have your own ideas about the implementation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robwalch Hi, do I need to add some unit tests for this? Or do you have your own ideas about the implementation?
Take a look at tests/unit/controller/eme-controller.ts. I don't know if we have mocks for producing and cover similar errors so it's OK if you don't add new tests for these non-fatal errors.
You need to fix the build (https://github.com/video-dev/hls.js/actions/runs/11626288545/job/32377670037?pr=6807) before we can merge. Running npm run sanity-check
will run sub-task npm run docs
which will add the missing API changes to "api-extractor/report/hls.js.api.md" you're changing with this PR (new ErrorDetails
enums).
Thanks a lot. I will update it. I don't see any reply with the PR. I am not sure if I need to continue with it. |
This PR will...
We will trigger more DRM-related errors to help the app collect that information.
Why is this Pull Request needed?
Even though we covered most devices and browsers. But there are still some devices that can not run it in an expected way. And they are not easy to debug with real ones. So we want to support more detailed error information to trigger for us to report.
Are there any points in the code the reviewer needs to double check?
Resolves issues:
Checklist