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

kodi/Intel: Various improvements from fritsch #573

Merged
merged 4 commits into from Aug 16, 2016

Conversation

Projects
None yet
7 participants
@MilhouseVH
Copy link
Contributor

commented Jul 27, 2016

@fritsch I've had these three commits in my Generic builds for quite a while now, originally from your OE repo. Are they all still needed? If not please let me know which (if any) should be dropped. Thanks.

@piotrasd

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2016

All are needed for Intel HD Gen8 like cherry trail

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2016

The kodi patch is bad for nvidia and others expecting full range.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2016

In that case I'll drop the Kodi patch.

@MilhouseVH MilhouseVH force-pushed the MilhouseVH:fritsch4k branch from 24f121f to ce6e6ed Jul 29, 2016

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2016

Nope. Make it intel Auto detect, please

Am 29.07.2016 6:33 vorm. schrieb "MilhouseVH" notifications@github.com:

In that case I'll drop the Kodi patch.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#573 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABCfHflzlPlp4qVt69iDV22c5AiBuyp-ks5qaYKIgaJpZM4JWoyY
.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2016

Make it intel Auto detect, please

Sorry, not really sure how to accomplish that. This is the patch I dropped:

fritsch/OpenELEC.tv@98c65e6

How do we make that "Intel auto detect"?

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2016

Query the running hardware and change kodi's settings accordingly. It's no
solution already there, but I thought the best "out of the box
distribution" can accomplish that :p

2016-07-30 16:35 GMT+02:00 MilhouseVH notifications@github.com:

Make it intel Auto detect, please

Sorry, not really sure how to accomplish that. This is the patch I dropped:

fritsch/OpenELEC.tv@98c65e6
fritsch/OpenELEC.tv@98c65e6

How do we make that "Intel auto detect"?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#573 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABCfHaYFzNtIhb8Oy9eQ1Tj74JTASj2aks5qa2ErgaJpZM4JWoyY
.

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2016

I'm not a coder (these days) so either we change the default for Intel (and possibly annoy nvidia users) or do nothing and keep the default as it is (which probably isn't great for Intel users, but hardly the end of the world). I'd vote for doing nothing - keep the current default - and merge this as-is, just the two commits.

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2016

The problem is: With the kernel patch you have, intel defaults to Video-Range 16:235. That means signal Limited Range, but don't touch color values.

A normal TV is Limited Range by default.

Before the intel driver scaled down all input to Limited Range no matter what (!) input. Users did not notice in this combination. Now - we send Video Range by ourself without touching the color values, but kodi without the setting we discuss here, outputs full range!

That means: Everything too dark and no whites.

In short: If you set Video Range 16:235 for intel gpus, you need to set Use Limited Range, too.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2016

Forget it, I'll drop these patches from my builds. Sorry for the noise, I thought the patches were ready to go.

@MilhouseVH MilhouseVH closed this Jul 31, 2016

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2016

Without them intel will just look bad and users need to manually fiddle with autostart.sh ... you will see tomorrow in your forum thread.

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2016

Without them the following will happen:

intel driver: scales full range to limited range
kodi: outputs full range

-> kodi will produce a full rgb image from limited content
-> kernel will just scale it down again

With above patches:
intel driver: does not care
kodi: outputs the original colors

-> kodi will just keep the colors
-> kernel will show them the way they are.

We will see if users see an effect.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2016

Like I say, not a coder so don't have the time/skill/knowledge to fix this. If the commits aren't suitable for merging upstream they have no place in the builds.

@piotrasd

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2016

if i good remember without this patches i had brighter picture ... like with bigger gamma or something like that, but i will check test build without this and let you know. Anyway what the problem left patches and people in kodi will manual enable or disable limited color range (if will have some picture issue like to bright)

@CvH

This comment has been minimized.

Copy link
Member

commented Jul 31, 2016

@fritsch could you tell for the record what has to be added to the autostart.sh ?

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2016

3c21755#diff-527e71ca33779ccd70d936fc587d82e7R1 <- or a more hard coded subset ...

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

I want to emphasize on what I have written here: #573 (comment)

Though this will happen, there is a simple solution: User just enables "Use Limited Range" and it will work as intended.

To summarize, what is missing:

It's a condition in the default settings, that if we are running on intel hardware && if we are having Video Range set && user has not altered this setting by himself set Limited Range to true.

^^ As the VideoRange patch is not in any upstream kernel this is nothing kodi in general could do. But LibreELEC that ships this patch anyways should offer such an out of the box experience for their intel user.

  1. So please add the systemd patch back -> VideoRange
  2. Let's think of a way to ship a custom minimal kodi patch that alters this setting on first start
@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2016

Thanks for the clarification. I thought that's what would happen - Intel users would just need to toggle the default setting - but all the discussion suggested otherwise.

  1. This PR - in it's current form, just the 2 commits - will be added back to nightly test builds (starting with #810 tonight) (Edit: Decided against putting this back into #810, will include this once we have the third commit working again)

  2. I'm having trouble thinking of an easy/elegant solution. Is it possible to detect the presence of Intel hardware in appliance.xml?

@MilhouseVH MilhouseVH reopened this Aug 10, 2016

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2016

Yes it is. See the codec settings for mpeg4 and such.

Am 10.08.2016 7:14 nachm. schrieb "MilhouseVH" notifications@github.com:

Thanks for the clarification. I thought that's what would happen - Intel
users would just need to toggle the default setting - but all the
discussion suggested otherwise.

  1. This PR - in it's current form, just the 2 commits - will be added back
    to nightly test builds (starting with #810 tonight)

  2. I'm having trouble thinking of an easy/elegant solution. Is it possible
    to detect the presence of Intel hardware in appliance.xml?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#573 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABCfHdBs0P1-BljJhMBE8r5aOjrDsHaKks5qegcMgaJpZM4JWoyY
.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2016

Added Intel GPU detection to Kodi. When an Intel GPU is detected, the default value for Use limited colour range (16-235) will be set to true - for all other GPUs it will be left as false.

The first kodi commit is based on two commits from @fritsch (commit, commit) - I've simply cleaned them up, squashed them and replaced the SettingCondition.cpp logic with a direct call to SetDefault() as it's the only way to toggle the default value (it can't be done in appliance.xml or settings.xml as there is no support for conditionally changing the value of the default property).

The kodi commit works in conjunction with the systemd passthrough commit on this PR, and also the passthrough kernel patch added in Linux 4.3.0.

This is how things would normally work without all of above mentioned changes:

@fritsch:
20:54 so I tell you what happens
20:54 your driver is on "Automatic" which means: Limited 16:235 <- the default of intel
20:54 so the value 0 is scaled by kodi to 16
20:55 and again by the intel driver to 24
20:55 255 is scaled by kodi to 235 and again by the driver to something like 216

With the kernel/systemd passthrough patches we avoid the clamping in the Intel driver, however we still want to use limited range in Kodi as this is the best setting for TV displays, so limited range will now be the default for Intel GPUs. Essentially, with all these changes we will avoid clamping twice - once by Kodi and then again by the Intel driver - with Kodi alone dictating the range.

The downside of all this is that if the user is using LE with an Intel GPU and a computer display the new default setting will result in washed out colours as computer monitors are full range, in which case the Use limited colour range setting will need to be manually disabled by the user. On balance, there's likely to be more LibreELEC TV viewers than LibreELEC computer display viewers.

The limited range setting is currently an Expert level setting, but I've moved it to Standard with the final commit in order to make it more accessible (I will remove the final commit if requested).

I'd like to test this for a few days in my nightly builds, and if there are no issues then consider merging from Monday.

@fritsch many thanks for the time spent pulling this together!

@MilhouseVH MilhouseVH force-pushed the MilhouseVH:fritsch4k branch 2 times, most recently from d0297bc to 18cb8c7 Aug 11, 2016

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2016

Updated to use appropriate default for VAAPI/VDPAU depending on available Intel hardware.

@lrusak

This comment has been minimized.

Copy link
Member

commented Aug 13, 2016

This looks good, thanks for all your effort here. If everything works out please just remember to update your commit dates 👍

@MilhouseVH MilhouseVH force-pushed the MilhouseVH:fritsch4k branch from 18cb8c7 to 8dc0cf3 Aug 13, 2016

@fritsch

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2016

@lrusak if you are not(!) busy at some time - we can also think of a appliance.xml solution that detects the hardware from a systemd service or something and symlinks the proper appliance.xml per application that way no kodi hacking needs to happen.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2016

No problems reported. Ready to merge.

@chewitt chewitt merged commit 949a835 into LibreELEC:master Aug 16, 2016

@MilhouseVH

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2016

@fritsch: What about having appliance_hardware_<intel|nvidia|radeon|whatever>.xml files, the specific hardware settings file being copied to /storage/.cache/appliance_hardware.xml by kodi-config according to hardware detection, and loaded as a third settings file after appliance.xml (ie. a two line addition to kodi source code)? :

   if (CFile::Exists(SETTINGS_XML_FOLDER "appliance.xml") && !Initialize(SETTINGS_XML_FOLDER "appliance.xml"))
     CLog::Log(LOGFATAL, "Unable to load appliance-specific settings definitions");
+
+  if (CFile::Exists("/storage/.cache/appliance_hardware.xml") && !Initialize("/storage/.cache/appliance_hardware.xml"))
+    CLog::Log(LOGFATAL, "Unable to load appliance-specific hardware settings definitions");

This would be cleaner than having potentially multiple appliance.xml files, which would mostly contain duplicated appliance settings and only minor hardware specific variations. Such variations would instead only be present in the hardware specific xml file.

Thus we could have an appliance_hardware_intel.xml that enables VAAPI, disabled VDPAU and enables limited range. An appliance_hardware_nvidia.xml could do the opposite, or alternatively we simply disable VAAPI/enable VDPAU/disable limited range in appliance.xml by default, and these settings would be overridden only when Intel hardware is detected.

This would be my best idea, perhaps @lrusak has other/better ideas. Although for now I'd only go this xml-based route if additional hardware-specific settings become a requirement in future, or Kodi source code changes cause us to revisit the current implementation.

Edit: MilhouseVH@aa4bd16 as an example (not sure of the best way to identify Nvidia GPUs - for example my ION2 returns VESA VGA - maybe grep Xorg.0.log?) No idea about WeTek, Odroid or imx6 etc.

@FernetMenta

This comment has been minimized.

Drop this. I don't know why but after upgrade from Ubuntu 18.04 to 18.10 this caused slight stuttering on my system. Setting TripleBuffer to true, which is default, cured it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.