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

Sort whitelist lowres to highres and match exact framerate at higher #16149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

graham8
Copy link
Contributor

@graham8 graham8 commented May 16, 2019

resolutions

Description

For displays which don't have an exact or 2x framerate at default resolution, prefer higher resolutions
at exact framerate to scaling the source framerate. Effect is similar to fritsch@613d5f9 but implemented by sorting the whitelist from lowest to highest resolution. This sorting also stops 4096x2160 being chosen above 3840x2160 when both are whitelisted.
Suggest also to sort whitelist low to high in settings since it's the lowres modes people are more likely to whitelist and the higher modes (eg 75Hz+) more likely to blacklist, and it's consistent with sorting of the GUI resolution choices

Motivation and Context

Users without 25Hz or 50Hz at 1080p but having 25/50Hz at 2160p get smoother playback of PAL content at 2160p than at 1080p60Hz
Fixes bug choosing 4096 when 3840 is available.

How Has This Been Tested?

Tested on AML (Vero 4k).

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@lrusak
Copy link
Contributor

lrusak commented May 16, 2019

Suggest also to sort whitelist low to high in settings since it's the lowres modes people are more likely to whitelist and the higher modes (eg 75Hz+) more likely to blacklist, and it's consistent with sorting of the GUI resolution choices

This doesn't really make sense. If I want to whitelist 4K and 1080P modes then I want them at the top of the list. Why would you only want to whitelist low res modes?

Also, I don't really get the problem you are trying to solve with this PR. Why don't you just whitelist the higher modes? If your tv doesn't support 1080p50 then whitelist 4K50 and when 1080p50 content is played it will play at 4K50. Is that not right?

@graham8
Copy link
Contributor Author

graham8 commented May 16, 2019

With v17, users would typically have a 1080p GUI and refresh rate switching would give them 2160p when required. The value of whitelisting was seen to be the ability to output lower resolutions than the GUI (ie 480 and 576) hence my assumption those resolutions are of more interest.

In fact, I can't think why you would want to whitelist 4k resolutions (alone). They are already available without whitelisting.

If you whitelist the higher modes, you won't get a match to, say, 1080p25 because the search is for exact resolution match (+/- 8).

@lrusak
Copy link
Contributor

lrusak commented May 16, 2019

With v17, users would typically have a 1080p GUI and refresh rate switching would give them 2160p when required. The value of whitelisting was seen to be the ability to output lower resolutions than the GUI (ie 480 and 576) hence my assumption those resolutions are of more interest.

If you do not have any modes whitelisted you will get the V17 behavior and kodi will do the logic for your.

The whole purpose of the whitelist was to add control to what kodi will actually switch to. So yes, if you want to switch to 720P modes then you need to whitelist them but you will also need to whitelist all the other modes you want to use (1080P and/or 4K). I hope that makes sense.

If you whitelist the higher modes, you won't get a match to, say, 1080p25 because the search is for exact resolution match (+/- 8).

Current logic is this

  1. find exact resolution and exact refresh rate
  2. find exact resolution and double refresh rate
  3. find exact resolution and 3:2 pull down refresh rate
  4. use current resolution at exact or double refresh rate
  5. use desktop resolution with exact refresh rate
  6. use desktop resolution with double refresh rate
  7. use desktop resolution with 3:2 pull down refresh rate

desktop resolution is the resolution you have the kodi gui set to.

@kib
Copy link
Member

kib commented May 16, 2019

I always believed the value of the whitelist was to control when not to switch to some modes in cases where the information returned to you by the connection (let's say an AVR) and detected to be available do not match the actual capabilities of the display currently in use (let's say a projector hooked up to it)

@graham8
Copy link
Contributor Author

graham8 commented May 16, 2019

The whole purpose of the whitelist was to add control to what kodi will actually switch to. So yes, if you want to switch to 720P modes then you need to whitelist them but you will also need to whitelist all the other modes you want to use (1080P and/or 4K). I hope that makes sense.

Exactly. I fully understand that whether it makes sense or not!

Current logic is this

Yes. It does not include use higher resolution (than source or desktop) and exact refresh rate. That's what ppl are asking for. Apparently, the weighting system used in v17 did result in 2160p25 when playing 1080p25.

@lrusak
Copy link
Contributor

lrusak commented May 16, 2019

Yes. It does not include use higher resolution (than source or desktop) and exact refresh rate. That's what ppl are asking for.

It did in it's original inception, the change is here 922a409

We cannot keep adding exceptions to the whitelist like has already been done. We need to figure out a proper solution so that people can fine tune their specific needs. Even if that means adding more settings.

@graham8
Copy link
Contributor Author

graham8 commented May 16, 2019

using a larger resolution would result in unwanted mode changes

We haven't seen this in testing when choosing On start/stop. Besides, what I'm suggesting would only match the next higher resolution than the target with the right refresh rate which wasn't the case when that commit was made.

Are you suggesting the right answer is a matrix of source modes vs display modes? Overkill, surely?

@robofunk
Copy link

I like that this PR also picks the closest resolution for SD content (720p in my case). Previously it would pick 1080p.

@graham8
Copy link
Contributor Author

graham8 commented May 18, 2019

I like that this PR also picks the closest resolution for SD content (720p in my case).

Thanks. That's the idea :)

@fritsch
Copy link
Member

fritsch commented May 18, 2019

It was decided that "double" scaling should be avoided by any extent, e.g. "bilinear from 640 to 1280 and afterwards TV scaler from 1280 to 2160.

I think the real resolution to this is implementing of a configurable policy behaviour. Example:

  • Choose nearest res with matching refreshrate
  • Only switch resolution if it exactly matches, just switch refreshrate else
  • And so on

This in combination with the whitelist would be preferred. Currently there is "too much logic" in the switching code, totally intransparent for the user while not fitting 100% of users anyways.

Therefore:
Single Point of settings (whitelist + policy), with the code following the settings and not hiding logic in some Resolution.cpp.

@graham8
Copy link
Contributor Author

graham8 commented May 18, 2019

+1. I guess some combinations of device/display work better giving preference to resolution scaling and some work better with framerate scaling.

I'd have thought some modification to the v17 method by adjusting the relative weightings of resolution and refresh rate might suit. I wasn't involved when whitelists came in but was there a reason to go away from the v17 weighting system? Was it not considered to just apply that system to all whitelisted modes rather than just modes > desktop?

I ask because users seemed happy with the v17 system for the higher modes.

It was decided that "double" scaling should be avoided by any extent, e.g. "bilinear from 640 to 1280 and afterwards TV scaler from 1280 to 2160.

Yes, I remember reading that. ATM it has the unfortunate effect of choosing 4096 width over 3840. As a minimum, that should be corrected, say, with the diff/temp_diff system you had in your commit. Also, choosing 1080 on a 'HD-ready' (1366x768) screen when 720 might produce a better result as reported above.

I'm happy to work on this if some consensus can be reached.

@FernetMenta
Copy link
Contributor

Don't forget that you always have the chance to manually select desired mode after playback has started. This is not more clicks on a remote than selecting desired i.e. desired language for audio or subs.

@phunkyfish
Copy link
Contributor

@graham8 do you want to progress this or close it?

@phunkyfish phunkyfish added the PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. label Mar 9, 2020
@graham8
Copy link
Contributor Author

graham8 commented Mar 10, 2020

Nothing that's been said above convinces me this is a bad idea. We have implemented it in OSMC many months ago. The user that requested it is happy and no-one else has complained. There are other issues with whitelisting which this does not attempt to address but I don't think this is incompatible with #16216 or whatever may emerge from that.

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label May 5, 2020
@jenkins4kodi
Copy link
Contributor

@graham8 this needs a rebase

@phunkyfish phunkyfish added PR Cleanup: Recent Checked as part of PR cleanup. PR has been followed up on recently. and removed PR Cleanup: Ask Author PR appears to still be relevant and mergeable. Comment to author to check on status. labels May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR Cleanup: Recent Checked as part of PR cleanup. PR has been followed up on recently. Rebase needed PR that does not apply/merge cleanly to current base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants