Video Tab - Incorrect Resolution Listed

We in fact only support these resolutions:

width >= 7680 -> "8K"
width >= 3840 -> "4K"
width >= 1920 -> "1080p"
width >= 1280 -> "720p"
else -> "SD"

We may want to support more in the future though.

1440p should be added to the list…

As well as others "in-between" resolutions I guess.

Yeah, adding things to the list would resolve this specific situation, but I believe that is honestly the wrong move to solve the general issue.

In my opinion, there should not be a list at all.
Plain and simple.

Your algorithm does not take into account vertical videos.
Your algorithm does not take into account videos with a 4:3 aspect ratio.
Your algorithm does not accurately report nonstandard resolutions.

To be perfectly frank, I find your current implementation to be reductionist, dishonest, and unhelpful. I'm honestly disappointed you're not just returning the exact precise correct answer every time.

If I had my way with the code, this is what I would do.
(And I apologize that I don't know Kotlin. This is written in Java. I trust you should be able to translate this as needed.)

String generateResolutionClass(int width, int height){
    if(width <= 0 || height <= 0) return null;
    int resolution = Integer.min(width, height);
    return Integer.toString(resolution) + "p";
}

If you need the precise information, you have the information dialog for this, which gives you the full resolution.

This is just a simple tool to know basic quality, without entering the dialog.

However, it works with vertical video, and 4:3 or other aspect ratios, because, by definition it counts the number of lines.

Finally, mind your tone.

I'm sorry you didn't believe me. Please try to understand, what you're saying is not true. Your current method does not work with vertical video or 4:3 or non-standard resolutions. I have collected proof, and I will provide it to you here.

Vertical Videos

First, I have a file called FailCase001.mkv. This is a vertical video, 1080x1920. This video should be classified as 1080p. Your code incorrectly labels it as SD.

To be sure, I uploaded this video to YouTube. YouTube correctly identified it as a 1080p video.

This proves that your algorithm does not take into account vertical videos.

4:3 Aspect Ratio

Second, I have a file called FailCase002.mkv. This is a 4:3 video from Meow Wolf on YouTube, 1440x1080. This video should be classified as 1080p. Your code incorrectly labels it as 720p.

YouTube once again gets this one correct. YouTube identifies this video as 1080p.

This proves that your algorithm does not take into account videos with a 4:3 aspect ratio.

Non-standard Resolutions

Third, I have a file called FailCase003.mp4. This video has a non-standard video resolution, because it is a screen recording from an iPad. This video was a recorded lecture from my Statistics professor. This video's dimensions are 1656x1246. This video should be classified as 1246p. It could also be classified as 1080p, which is a slight round-down. 1246 is larger than 1080, so this video should at minimum be classified as 1080p. But your code incorrectly goes too low, and labels it as 720p.

This proves that your algorithm does not take into account videos with non-standard resolutions.

Conclusion

Your current code consistently produces incorrect and inaccurate results. Your current code should be removed, and it should be replaced with a new approach that is not based on a list of hardcoded values.

There are in fact two different questions here:

What is this code meant to show to the user?
Is the code correct?

What is this code meant to show to the user?

The goal is not to show the exact video resolution. The information screen is meant to do that. This small piece of UI is here to just show a quick easily understandable hint for the video quality.

To be quickly processed, the value set has to be small. To be easily understandable, the values have to be from common knowledge.

You took YouTube as a reference, so let's do the same:

It fulfills both of our requirements: small set, easily understandable.

Now, the context between YouTube and VLC is a bit different:

we do not propose to change the resolution dynamically, so only one value will be shown
the area we have to show the information in is really small. In the grid for example, it should not be larger than 5 characters

That's what let us use the standard video resolutions as an entry set. See https://en.wikipedia.org/wiki/8K_resolution

Is the code correct?

I agree it's not and this for two reasons:

It doesn't take into account the video orientation
It generates the value on the width when it should do it on the height.

I just reworked it a bit to fix these two issues and here is what I got:

Code

fun generateResolutionClass(width: Int, height: Int): String? = if (width <= 0 || height <= 0) {
    null
} else {
    val realHeight = min(height, width)
    when {
        realHeight >= 4320 -> "8K"
        realHeight >= 2160 -> "4K"
        realHeight >= 1080 -> "1080p"
        realHeight >= 720 -> "720p"
        else -> "SD"
    }
}

Result

Resolution for 1080x1920 is 1080p
Resolution for 1920x1080 is 1080p
Resolution for 1440x1080 is 1080p
Resolution for 1080x1440 is 1080p
Resolution for 1656x1246 is 1080p
Resolution for 1246x1656 is 1080p
Resolution for 640x480 is SD
Resolution for 480x640 is SD
Resolution for 720x1200 is 720p
Resolution for 1200x720 is 720p
Resolution for 3840x2160 is 4K
Resolution for 2160x3840 is 4K
Resolution for 7680x4320 is 8K
Resolution for 4320x7680 is 8K

Conclusion

I think the new version is better.
I am not against adding new values as soon as it's easily understandable, belongs to a well known standard, is short enough to fit in the cards and the entry set is not too large

Thank you. I think your new version is much better.

I now have only one final request. This gets back to my original reason for opening this issue.
I would like to request that you add 1440p to the list.

I'm glad you and I agree that YouTube's list of resolutions (and YouTube's method of resolution classification) are acceptable as a sort of standard. As you can see in your own screenshot, YouTube recognizes 1440p as a distinct resolution class, sitting in-between 1080p and 4k.

I happen to use 1440p quite often. I produce 1440p videos regularly. 1440p happens to be the minimum resolution that, when uploaded to YouTube, will guarantee higher-quality codecs (VP9 and AV1) upon processing. Heck, my even my phone's display resolution is 1440x3168.

So I think it would be appropriate to include 1440p as a distinct classification in generateResolutionClass().

It was decided internally that we should use the commercial definitions for this feature. So the final code will be:

fun generateResolutionClass(width: Int, height: Int): String? = if (width <= 0 || height <= 0) {
    null
} else {
    val realHeight = min(height, width)
    when {
        realHeight >= 4320 -> "8K"
        realHeight >= 2160 -> "4K"
        realHeight >= 1440 -> "qHD"
        realHeight >= 1080 -> "FHD"
        realHeight >= 720 -> "HD"
        else -> "SD"
    }
}

I am happy with this decision, and I am happy with this result.

Thank you.

added feature label

mentioned in merge request !1562 (merged)

closed with commit 7d2bbb42

mentioned in commit a77af19a

Video Tab - Incorrect Resolution Listed

Description

Expected behavior

Actual behavior

Steps to reproduce

Screenshots

Context

App version

Android version

Device model

App mode

Designs

Child items 0

Activity

Vertical Videos

4:3 Aspect Ratio

Non-standard Resolutions

Conclusion

What is this code meant to show to the user?

Is the code correct?

Code

Result

Conclusion