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

Refactor GetResolutionText #6256

Merged
merged 2 commits into from Jul 20, 2021
Merged

Conversation

heyhippari
Copy link
Contributor

Changes

This improves GetResolutionText a little by making it easier to read and better parsing resolutions (Also adding a few new ones like PAL resolutions and 8K).

Issues

This improves GetResolutionText a little by making it easier to read and better parsing resolutions (Also adding a few new ones like PAL resolutions and 8K)

Co-authored-by: Maxr1998 <max.rumpf1998@gmail.com>
Copy link
Member

@Bond-009 Bond-009 left a comment

Choose a reason for hiding this comment

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

Even tho you won't find any movies in 2560x1080, I still think this should handle it correctly

@heyhippari
Copy link
Contributor Author

Even tho you won't find any movies in 2560x1080, I still think this should handle it correctly

But what would you put it as? It's not 1080p/1080i, which is widely understood to be 1920x1080 at most.

@Bond-009
Copy link
Member

Not sure what "widely understood" means here, but wikipedia doesn't think so https://en.wikipedia.org/wiki/1080p#Resolutions

@heyhippari
Copy link
Contributor Author

For the average consumer, 1080p means "Full HD", which is 1920x1080. 2560x1080 is a monitor resolution, not really a video one.

Imo, this should be limited to actual common video resolutions, otherwise we may as well simply return a string of width x height instead of doing this.

@Maxr1998
Copy link
Member

Maxr1998 commented Jul 12, 2021

Wouldn't it theoretically be the best to match by height first and then width? Should make the code more robust to uncommon resolutions, especially since the text normally describes the vertical/smaller resolution. .

EDIT: well, 1920x800/808 would be matched incorrectly then, so maybe that also doesn't fit perfectly.

@heyhippari
Copy link
Contributor Author

heyhippari commented Jul 12, 2021

Wouldn't it theoretically be the best to match by height first and then width? Should make the code more robust to uncommon resolutions, especially since the text normally describes the vertical/smaller resolution. .

EDIT: well, 1920x800/808 would be matched incorrectly then, so maybe that also doesn't fit perfectly.

You usually match by width, because that's the most consistent.

With black bars being cutoff quite often, matching by height is inconsistent and prone to errors.

At worst, the width gets 4-6 pixels cut off.

This way also allows you to properly match 1080p at 4:3 to the proper resolution.

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.

None yet

5 participants