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

Fix VideoQuality.fromLabel to match higher resolutions first #254

Closed
wants to merge 1 commit into from

Conversation

jamiepiccolo
Copy link

@jamiepiccolo jamiepiccolo commented Sep 8, 2023

VideoQuality.fromLabel would give incorrect qualities in some cases.

An example being the 1440p label where the method would assign VideoQuality.low144 because of a premature match of the string's prefix "144":

if (label.startsWith('144')) {
    return VideoQuality.low144;
}

...

if (label.startsWith('1440')) {
    return VideoQuality.high1440;
}

I simply reversed the resolution map and looped over it so that we start matching from the highest resolution first.

@khaled-0
Copy link
Contributor

khaled-0 commented Dec 3, 2024

Now how would one target 144p
because it'll always match 1440p
previous solution was fine.

@jamiepiccolo
Copy link
Author

If you could provide more insight into how this would prematurely match 1440p for 144p let me know.

The previous solution didn’t work, which is why I made this pull request. Although I haven’t used the package for quite some time, so things may have changed.


if (label.startsWith('4320')) {
return VideoQuality.high4320;
if (label.startsWith(videoResolution.height.toString())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For 144p
if label startsWith "144" then by reverse order "1440p" will be the first match,

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes, you’re right. I just reversed the issue. Unfortunately it is still a bug either way, but this isn’t the solution.

Feel free to close, I have no time to propose a proper solution.

@jamiepiccolo jamiepiccolo closed this by deleting the head repository Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants