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

The measureShortEdge option to use a short edge to display the resolution for vertical video #17

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

annahassel
Copy link

Using the minimum side to display the vertical video resolution as popular video players do. In fact, 1280x720 and 720x1280 videos have the same quality, but are now displayed as 720p and 1280p, which is incorrect.

@mister-ben
Copy link
Contributor

p doesn't mean "short edge", it's the vertical resolution of a progressive scan video. It doesn't strictly imply a particular aspect ratio, although 16:9 would usually be assumed in the absence of other information.

@annahassel
Copy link
Author

p doesn't mean "short edge", it's the vertical resolution of a progressive scan video. It doesn't strictly imply a particular aspect ratio, although 16:9 would usually be assumed in the absence of other information.

You are definitely right! But in real cases, there is a product owner who needs features like the most popular services do. For example, YouTube, which shows a short edge to display the current quality (in the screenshot). Maybe you can make an option for this feature?

Screenshot Screenshot 2024-10-28 at 16 04 55 ```

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Thanks so much for the PR! This makes sense to me.

While Ben's right that p strictly refers to the vertical size of progressive scan video, it's become something of a shorthand for quality/resolution.

However, I think putting it behind an option would be ideal, so that we don't change the behavior out from under users of videojs-contrib-quality-menu.

Perhaps a boolean option called measureShortEdge?

It could be false by default and then in the next major version of the plugin we can flip it to true if there's consensus.

@annahassel annahassel force-pushed the fix/resolution-vertial-video branch from f88aaed to ec176ee Compare October 28, 2024 20:25
@annahassel annahassel changed the title Correct display of resolutions for vertical video measureShortEdge option to use a short edge to display the resolution for horizontal video Oct 28, 2024
@annahassel annahassel changed the title measureShortEdge option to use a short edge to display the resolution for horizontal video The measureShortEdge option to use a short edge to display the resolution for horizontal video Oct 28, 2024
@annahassel annahassel force-pushed the fix/resolution-vertial-video branch from ec176ee to fef32e9 Compare October 28, 2024 20:28
@annahassel annahassel changed the title The measureShortEdge option to use a short edge to display the resolution for horizontal video The measureShortEdge option to use a short edge to display the resolution for vertical video Oct 28, 2024
@annahassel annahassel force-pushed the fix/resolution-vertial-video branch from fef32e9 to 2a57041 Compare October 28, 2024 20:31
Copy link
Contributor

@mister-ben mister-ben left a comment

Choose a reason for hiding this comment

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

Looks good, just a suggestion to add to the option description.

README.md Outdated Show resolved Hide resolved
Co-authored-by: mister-ben <[email protected]>
@annahassel
Copy link
Author

Hi @misteroneill!

I've made the requested changes to the PR as per your feedback. If you have a moment, could you please take another look? Let me know if there’s anything else I can adjust. Thank you!

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.

3 participants