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

Mode whitelist #13274

Merged
merged 2 commits into from May 4, 2018
Merged

Mode whitelist #13274

merged 2 commits into from May 4, 2018

Conversation

lrusak
Copy link
Contributor

@lrusak lrusak commented Dec 30, 2017

@FernetMenta wanted me to PR this to get some more visibility. I'll add what I had already posted to the internal boards.


Just wanted to start a discussion that won't get lost in slack.

For those that don't know the display and refresh rate switching logic in kodi is rather complex yet it doesn't quite fit everyone's needs. @FernetMenta proposed that we create a set of whitelisted resolutions that one can select and create a logic for kodi to switch to only these whitelisted modes.

Some reasons to do this is as follows:

  1. Some people want to switch to a lower resolution when watching 720P content however some people do not want this because when watching live TV the refresh rate switching would happen too often.
  2. We can disable modes that have too high of refresh rate ie. 120HZ.
  3. We can use the exact refresh rate (for low bandwidth systems playing 30hz content at 60hz on a 4k display is difficult)

The problem with this come down to adding yet another setting. More settings generally decrease the user experience. So how can we make things simpler not more complex?

  1. Pre define whitelisted modes. (add all refresh rates from the desktop resolution automatically)
  2. Revert to the old behaviour if no whitelisted modes are defined.
  3. Hide option at the same settings level as the refresh rate switching option

Currently the logic is this:

  1. for all the methods below remember that it will only select from the whitelist!
  2. the order goes from fuzziest match towards exact match
  3. allow resolutions that are larger than required but have double the refresh rate
  4. allow resolutions that are larger than required but have the correct refresh rate
  5. allow resolutions that are exact and have double the refresh rate
  6. allow resolutions that are exact and have the correct refresh rate

Sample GUI
Imgur

Sample outputs below.

10:35:21.752 T:140499105462656   DEBUG: display width: 1920 vs video width: 1920
10:35:21.752 T:140499105462656   DEBUG: display fps: 60.000000 vs video fps: 29.970030
10:35:21.752 T:140499105462656   DEBUG: float equals: false
10:35:21.752 T:140499105462656   DEBUG: display width: 1920 vs video width: 1920
10:35:21.752 T:140499105462656   DEBUG: display fps: 50.000000 vs video fps: 29.970030
10:35:21.752 T:140499105462656   DEBUG: float equals: false
10:35:21.752 T:140499105462656   DEBUG: display width: 1920 vs video width: 1920
10:35:21.752 T:140499105462656   DEBUG: display fps: 59.940201 vs video fps: 29.970030
10:35:21.752 T:140499105462656   DEBUG: float equals: false
10:35:21.752 T:140499105462656   DEBUG: Matched fuzzy whitelisted Resolution DVI-1: 1920x1080 @ 59.94Hz (19)
10:35:21.752 T:140499105462656   DEBUG: display width: 1920 vs video width: 1920
10:35:21.752 T:140499105462656   DEBUG: display fps: 24.000000 vs video fps: 29.970030
10:35:21.752 T:140499105462656   DEBUG: float equals: false
10:35:21.752 T:140499105462656   DEBUG: display width: 1920 vs video width: 1920
10:35:21.752 T:140499105462656   DEBUG: display fps: 23.976080 vs video fps: 29.970030
10:35:21.752 T:140499105462656   DEBUG: float equals: false
10:35:21.752 T:140499105462656   DEBUG: display width: 1280 vs video width: 1920
10:35:21.752 T:140499105462656   DEBUG: display fps: 60.000000 vs video fps: 29.970030
10:35:21.752 T:140499105462656   DEBUG: float equals: false
10:35:21.752 T:140499105462656  NOTICE: Display resolution ADJUST : DVI-1: 1920x1080 @ 59.94Hz (19) (weight: 0.000)

11:00:03.307 T:140082496567680   DEBUG: display width: 1920 vs video width: 1280
11:00:03.307 T:140082496567680   DEBUG: display fps: 60.000000 vs video fps: 60.000000
11:00:03.307 T:140082496567680   DEBUG: float equals: true
11:00:03.307 T:140082496567680   DEBUG: Matched fuzzy whitelisted Resolution DVI-1: 1920x1080 @ 60.00Hz (17)
11:00:03.307 T:140082496567680   DEBUG: display width: 1920 vs video width: 1280
11:00:03.307 T:140082496567680   DEBUG: display fps: 50.000000 vs video fps: 60.000000
11:00:03.307 T:140082496567680   DEBUG: float equals: false
11:00:03.307 T:140082496567680   DEBUG: display width: 1920 vs video width: 1280
11:00:03.307 T:140082496567680   DEBUG: display fps: 59.940201 vs video fps: 60.000000
11:00:03.307 T:140082496567680   DEBUG: float equals: false
11:00:03.307 T:140082496567680   DEBUG: display width: 1920 vs video width: 1280
11:00:03.307 T:140082496567680   DEBUG: display fps: 24.000000 vs video fps: 60.000000
11:00:03.307 T:140082496567680   DEBUG: float equals: false
11:00:03.307 T:140082496567680   DEBUG: display width: 1920 vs video width: 1280
11:00:03.307 T:140082496567680   DEBUG: display fps: 23.976080 vs video fps: 60.000000
11:00:03.307 T:140082496567680   DEBUG: float equals: false
11:00:03.307 T:140082496567680   DEBUG: display width: 1280 vs video width: 1280
11:00:03.307 T:140082496567680   DEBUG: display fps: 60.000000 vs video fps: 60.000000
11:00:03.307 T:140082496567680   DEBUG: float equals: true
11:00:03.307 T:140082496567680   DEBUG: Matched exact whitelisted Resolution DVI-1: 1280x720 @ 60.00Hz (26)
11:00:03.307 T:140082496567680  NOTICE: Display resolution ADJUST : DVI-1: 1280x720 @ 60.00Hz (26) (weight: 0.000)

Please comment to explain why you ❤️ or 💀 this idea and any further logic or comments about usability below

@lrusak lrusak added RFC PR submitted for gathering feedback v18 Leia Component: Video WIP PR that is still being worked on labels Dec 30, 2017
@Kwiboo
Copy link
Member

Kwiboo commented Dec 30, 2017

❤️ the idea about using a whitelist but have two comments / suggestions:

  1. Modes are currently stored as the index of all resolutions detected, this can cause issues when the user changes hdmi connection, deep color configuration on tv or edid parsing is failing for some reason, when the list of detected resolutions changes the configured whilelist is no longer valid or would list other resolutions then originally selected

  2. I would like to have some sort of option to default whitelist all 720p/1080p/2160p modes using appliance.xml or similar instead of reverting to the old behaviour, on Rockchip I currently have changed the default option for refresh-rate switching to at start/stop using appliance.xml and the old behaviour of selecting 4k@60hz for 4k@30hz content is flawed

@lrusak
Copy link
Contributor Author

lrusak commented Dec 30, 2017

@Kwiboo thanks for the input!

  1. we could possibly hash the the m_resolutions somehow and store that in an unreachable setting or something and validate that and if the hash changes then set the whitelist to default.
  2. I agree, it would be nice to somehow parse for specific modes and allow them to be set by default. This would be huge for applications like your DRMPRIME renderer and on imx6 where there is no HW scaler available to us.

I'll try and think about these two problems

@Kwiboo
Copy link
Member

Kwiboo commented Dec 30, 2017

@lrusak I think that if the setting was saved as a list of strings both my concerns could be addressed.

If the RESOLUTION_INFO could be represented by a string, lets say 1920x1080@60hz to simplify. And the whitelist settings was saved as 1920x1080@23.976hz,1920x1080@60hz, then it should also be possible to add all 720/1080/2160p modes in appliance.xml as the default value.

This will probably require a little bit more complex handling for selecting and saving the whitelisted modes.

@lrusak
Copy link
Contributor Author

lrusak commented Dec 31, 2017

updated to now save as strings rather than an index.

<setting id="videoscreen.whitelist">00192001080060.00000pstd,00192001080050.00000pstd,00192001080059.94020pstd,00192001080024.00000pstd,00192001080023.97608pstd</setting>```

@lrusak
Copy link
Contributor Author

lrusak commented Dec 31, 2017

pushed another quick fixup that will allow the default behaviour when the whitelist is empty

@lrusak
Copy link
Contributor Author

lrusak commented Jan 6, 2018

Any other opinions?

@FernetMenta
Copy link
Contributor

We could add a button to the dialog "Select all/none". Then blacklist fans can select all and then deactivate non desired ones

@FernetMenta
Copy link
Contributor

We need to persis the whitelisted modes with a platform indenpended name somewhere.

@lrusak
Copy link
Contributor Author

lrusak commented Jan 6, 2018

We need to persis the whitelisted modes with a platform indenpended name somewhere.

@FernetMenta can you please explain this statement?

@FernetMenta
Copy link
Contributor

oops, lots of typos. f*** tablet :)
1)
In this pr the modes don't get persisted yet, right? I see they are read from a setting but I can't find the location where they are stored.
2)
When reading back the setting you use GetResolutionFromString. This naming schema encodes the screen, which is not appropriate for our use case here. If you switch from one display to another, you'll lose the whitelist.

@iz8mbw
Copy link

iz8mbw commented Feb 23, 2018

I am very very interested to that feature of upscaling/downscaling.
Please, is it possible to commit in Kodi 18 this PR so I can test and give a look to it and give my "improvement opionion"? I sue Kodi 18 mainly on Nvidia Shield (Android).
Or give me an Android arm64-v8a APK that I can download and install into my Nvidia Shield.
Thanks.

@a1rwulf
Copy link
Member

a1rwulf commented Mar 23, 2018

@FernetMenta I did some tests. In my guisettings.xml modes are persisted like <setting id="videoscreen.whitelist">00384002160060.00000pstd,00384002160050.00000pstd,00384002160059.94010pstd,00384002160030.00000pstd,00384002160025.00000pstd,00384002160024.00000pstd,00384002160029.97000pstd,00384002160023.97600pstd,00192001080060.00000pstd,00192001080050.00000pstd,00192001080059.94020pstd,00192001080030.00000pstd,00192001080025.00000pstd,00192001080024.00000pstd,00192001080029.97010pstd,00192001080023.97608pstd</setting>

I guess b752a85#diff-8d943449895d0ff6b503e49c8ecf6868R697 is responsible for persisting the modes.

@a1rwulf
Copy link
Member

a1rwulf commented Mar 23, 2018

I also did the following test:

  • Hook kodi to my 4k TV
  • Enable 4k and 1080p with various resolutions
  • Hook kodi to my 1080p monitor
  • Only the available 1080p modes are whitelisted
  • Hook back to my 4k TV
  • All 4k modes and 1080p modes are whitelisted

So from my point of view, everything works as expected.

@FernetMenta
Copy link
Contributor

Works by chance because in both cases screen number is 0. When finding modes, screen has to be ignored.

@a1rwulf
Copy link
Member

a1rwulf commented Mar 23, 2018

@lrusak can you pick a1rwulf@a9496bd please.
This removes the screen from the serialized modes.

Edit: Update the commit to reflect feedback from @FernetMenta

@lrusak
Copy link
Contributor Author

lrusak commented Mar 24, 2018

thanks @a1rwulf !!

@lrusak lrusak removed RFC PR submitted for gathering feedback WIP PR that is still being worked on labels Mar 24, 2018
@lrusak lrusak added this to the Leia 18.0-alpha2 milestone Mar 24, 2018
@a1rwulf
Copy link
Member

a1rwulf commented Mar 25, 2018

Anything else needed for this to go in?

@lrusak
Copy link
Contributor Author

lrusak commented Mar 25, 2018

@a1rwulf I think everyone should be happy now. I should reset the commit dates though.

@a1rwulf
Copy link
Member

a1rwulf commented Mar 27, 2018

If nobody objects I'd like to merge this by end of this week.

@iz8mbw
Copy link

iz8mbw commented Mar 27, 2018

Good! When merged I can test on my Nvidia Shield.

@lrusak
Copy link
Contributor Author

lrusak commented Apr 19, 2018

pushed a fix that sorts the modes in the whitelist. Also I am including RES_DESKTOP now so I'll have to tweak gbm windowing so that it doesn't show the same mode twice.

@@ -655,12 +655,17 @@ RESOLUTION CDisplaySettings::GetResolutionForScreen()
return RES_DESKTOP;
}

static inline bool ModeSort(std::pair<std::string, std::string> i,std::pair<std::string, std::string> j)
{
return (i.second > j.second);

This comment was marked as spam.

This comment was marked as spam.

@lrusak
Copy link
Contributor Author

lrusak commented May 2, 2018

I've removed the verbose logging. I think this is ready to go 👍

@lrusak
Copy link
Contributor Author

lrusak commented May 4, 2018

merge?

@FernetMenta
Copy link
Contributor

+1

1 similar comment
@iz8mbw
Copy link

iz8mbw commented May 4, 2018

👍

@lrusak lrusak merged commit 23a4f53 into xbmc:master May 4, 2018
@iz8mbw
Copy link

iz8mbw commented May 5, 2018

Why on the latest nightly for Android the mode Whitelist is not available?

@sualfred
Copy link
Member

sualfred commented May 5, 2018

@lrusak

This PR breaks multimonitor setups. The switch "Fullscreen on Monitor #i" always jumps back to monitor No.1.

The only way to get the fullscreen on monitor 2 is to switch to window mode, manually move the window to the second monitor, hit alt+enter to get into the fullscreen mode and adjust the resolution to the correct one. On a restart of Kodi I have to do it again.

My monitor setup (if required to know)
Monitor 1 = 2560×1440
Monitor 2 = 1920x1200

Debug log: https://paste.ubuntu.com/p/bqS8q3ZbMK/

@FernetMenta
Copy link
Contributor

Windows is a bit behind in this regard. Instead of numbering monitors by 1,2,3,.., monitors are supposed to have a name. Screen numbers in resolutions will go away.

@natethomas
Copy link
Member

Hey guys, the Windows devs appear to have only found out about this causing windows breakage a few minutes ago. If a break is found and known, might be worthwhile to ping @afedchin or whoever the platform dev is and let him know at the time, as well as pointing out how and where to fix it. Communication is always good.

@FernetMenta
Copy link
Contributor

I am wondering if there is any dev left who has turned on github notifications.

@afedchin
Copy link
Member

afedchin commented May 11, 2018

I am wondering if there is any dev left who has turned on github notifications.

I'm wondering if you have a time to inspect all PRs. It's easy to skip something really important (like personal ping) in this spam from GH. Even If I had notifications enabled this PR has the title which has no words what it breaks multimon configs but in fact it does.

@FernetMenta
Copy link
Contributor

Since this PR does affect all platforms I assumed that all platform devs did observe it. Seems I was wrong.
No bad sttitude. Sorry.

RESOLUTION res = CDisplaySettings::GetInstance().GetDisplayResolution();
RESOLUTION_INFO info = CDisplaySettings::GetInstance().GetResolutionInfo(res);

for (auto index = (unsigned int)RES_DESKTOP; index < CDisplaySettings::GetInstance().ResolutionInfoSize(); ++index)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@the-dreamer
Copy link

the-dreamer commented Jun 9, 2018

Whitelist broke my 1080i resolution. 1080p ist not possible on my tv. In Resolution i can choose 1080i but it switch to a 1080p 50/59 or 60 resolution.
The Whitelist has no 1080i entry

@FernetMenta
Copy link
Contributor

1080p ist not possible on my tv

time to replace your tv with a newer model

@the-dreamer
Copy link

an Oled >65inch is on my whishlist... until then is there a way to disable it logic... until end of april it was no issue.

@bylderup
Copy link

Whitelist broke my 1080i resolution, too.
I tested this now with 2 different fresh untouched Installations on SD Cards with Mecool M8Spro+
and three different TVs - and I won´t buy new TVs..

In Settings-System-Display-Resolution 1920x1080i is listed.
If I try to activate this resolution then nothing happens!
As a follow up no 1920x1080i options are listed in whitelist.

Only way to get Display Resolution to 1920x1080i is manually editing the guisettings.xml

Didn´t have this before whitelist was introduced in KODI
log-2018-06-10-11.37.24.zip

@fritsch
Copy link
Member

fritsch commented Jun 10, 2018

Guys. Kodi outputs progressive. Whatever you force on your 1080 interlaced resolutions makes a bsolutely no sense.

Edit: Or is this some "3D hack"?

@bylderup
Copy link

just tested with LibreELEC-S905.arm-9.0-nightly-20180607-36c2619
same problem
I am not a developer - for whatever reason 2 of my TVs do have the best picture quality with 1920x1080i 50 Hz. This resolution is listed in Kodi settings and doesn´t work anymore since whitelist feature was introduced. And thats a bug imhop. No 1080 interlaced neither on a Hdready nor on a full HD TV.

@fritsch
Copy link
Member

fritsch commented Jun 10, 2018

@lrusak could you workout with your LE guys ;-) about pending / in place AMLogic hacks? Not sure how in their universe progressive content looks fine on interlaced outpud modes - therefore no idea what those builds include.

@bylderup
Copy link

bylderup commented Jun 10, 2018

Don´t know if we are talking about the same thing. All I said is :
since whitelist was introduced in Kodi, it is not possible so set the Display Resolution in Kodi to 1920x1080i.

# cat /sys/class/amhdmitx/amhdmitx0/disp_cap
480p60hz
576p50hz
720p60hz
1080i60hz*
720p50hz
1080i50hz

have to edit guisettings.xml with winscp :

<setting id="videoscreen.resolution">21</setting>
<setting id="videoscreen.screen" default="true">0</setting>
<setting id="videoscreen.screenmode">00192001080050.00000istd</setting>

otherwise it won´t work

@the-dreamer
Copy link

I need to relativate my initial problem. After update to a more recent Version Kodi from 2nd June my system switched to SD resolution. i tried to set to via resolution 1080i which was showed me twice. both was resulting in "mode not supported" on my Plasma HdReady screen. I downgraded and sleep a night. Today i tried again. Same bahaviour today. No suprise. But now i was looking into xrandr and it switched always to 60Hz 1080p when i selected 1080i. After connecting to system via vncviewer i tried different option and after swithing to 1080p i found 25p for refresh rate. I know 25p is the equivalent to 50i. Now i have at least picture again with 1080i. I am not sure if it was desired like it is now. For frame rate switching i have used advancedsetting xml to avoid switching to 50p for example.

@xbmc xbmc locked and limited conversation to collaborators Jun 10, 2018
@lrusak
Copy link
Contributor Author

lrusak commented Jun 10, 2018

Pleas use the forum's and/or trac for issue reports. This is a dev space.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet