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

[cmake] Add support for LCMS2 #11072

Merged
merged 2 commits into from Mar 13, 2017

Conversation

@hudokkow
Copy link
Member

commented Dec 6, 2016

Description

Add missing icc support to color management.

Motivation and Context

Fix http://trac.kodi.tv/ticket/17106

How Has This Been Tested?

Built and briefly runtime tested with and without liblcms2 present in system.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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

ping @fetzerch and @wsnipex

find_library(LCMS2_LIBRARY NAMES lcms2 liblcms2
PATHS ${PC_LCMS2_LIBDIR})

set(LCMS2_VERSION ${PC_LCMS2_VERSION})

This comment has been minimized.

Copy link
@wsnipex

wsnipex Dec 6, 2016

Member

will be emtpy if pkg_config is not used

This comment has been minimized.

Copy link
@hudokkow

hudokkow Dec 6, 2016

Author Member

humm, than we have problems with other modules. I used https://github.com/xbmc/xbmc/blob/master/project/cmake/modules/FindFribidi.cmake as base.

This comment has been minimized.

Copy link
@wsnipex

wsnipex Dec 6, 2016

Member

right, I've skimmed over it too fast. LCMS2_VERSION isn't in REQUIRED_VARS, so it will just be empty.
But we generally have a few find modules that still need work.

This comment has been minimized.

Copy link
@hudokkow

hudokkow Dec 6, 2016

Author Member

Yeap.

This comment has been minimized.

Copy link
@fetzerch

fetzerch Dec 6, 2016

Member

As long as we don't request specific versions, it doesn't cause any harm. The advantage is that fphsa below will show the version number if it's set. (and just omits it if it wasn't found through pkgconfig)

@wsnipex

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

do we need lcms in depends as well?

@hudokkow

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2016

Don't know. Do we want to enforce it?

@wsnipex

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

depends if all platforms can use color correction or just linux.

@hudokkow

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2016

That's above my pay-grade. Who's the main dev? fernet?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

Currently this is implemented for OpenGL. OSX can use it too once it's profile has been upgraded.

@wsnipex

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

Do we want it on OSX? If yes, we need to provide the lib in depends

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

Do we want it on OSX?

yes we do. I hope migration to shader based pipeline on OSX will be completed soon. After that we can bump GL version on OSX.

@lrusak lrusak referenced this pull request Dec 7, 2016
@hudokkow

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2016

Apparently we have some core problems with ICC profiles: LibreELEC/LibreELEC.tv#1038

@arucard21

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2016

I use the ICC Profile with Colour Management on my Linux installation without any problems. I noticed that the problem in LibreELEC occurs with the lcms2 library, and I'm currently using the 2.7 version whereas the crash log shows that they're using the 2.8 version.

Just looking at the code of these 2 versions, there do seem to be some substantial changes in the code, especially in the underlying methods used (the xform method). It seems to use something called "strides" now (no idea what that is though). Here is a full comparison of the changes in the source file between 2.7 and 2.8:
mm2/Little-CMS@lcms2-2.7...lcms2.8diff-c9c81367c364178743f2533cc6dfd3a3

I believe this is the relevant commit where most of these changes seem to be made:
mm2/Little-CMS@a7fc6e3

I don't know what these changes in the library actually do, but I think it may have changed the behaviour of the cmsDoTransform method in way that could cause this problem or maybe even have introduced a bug there. I hope this information will help solve the problem.

@hudokkow

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2016

Thx for taking the time to investigate.
@lrusak ^^

@arucard21

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2016

I just tested whether the new version really does break the color correction, but it doesn't seem like it does. On Debian Unstable, I upgraded liblcms2-dev from 2.7-1 to 2.8-2 and Kodi still plays everything correctly with color correction. I tried turning color management on and off while playing a video (as described in the LibreELEC PR), but that works correctly too. I looked at the patches applied to liblcms2 on Debian (https://sources.debian.net/patches/lcms2/2.8-2/) and they don't seem relevant to this problem.

I think a more consistent reproduction recipe would be needed here.

@arucard21

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2016

I think I managed to reproduce the problem. It seems to occur when you use the ICC mode but don't configure an actual ICC profile file.

Reproduction recipe:

  • Enable Colour Management
  • Choose to use ICC instead of 3D LUT
  • Don't select an ICC Profile file
  • Play a video
  • Kodi crashes (Segmentation fault) on thread 1 in cmsDoTransform()

The obvious solution is to actually select an ICC profile when you configure that mode in Colour Management. Once I selected my ICC profile, it all worked correctly. Perhaps this can be enforced so you always have to provide an ICC profile when you use that mode in Colour Management.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 10, 2016

@laurimyllari ping ^^

@laurimyllari

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2016

I'll take a look. Any suggestions on how the UI should work are welcome.

Maybe the mode and filename should be required before CMS can be turned on?

@arucard21

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2016

I looked at some other places in the Settings where similar behaviour is implemented and I think there are 2 ways for this to work:

  1. You could check the settings and disable Colour Management if it's not configured correctly
    • This should require few changes to the implementation but may cause people to think Colour Management is working (because it's enabled) when it's actually not (because it's mis-configured). This could be covered by providing a warning in the OSD menu for Colour Management or just showing it as disabled in the OSD when it's mis-configured (allowing the user to fix the configuration right there).
  2. You could add the filename setting to the popup dialog for choosing a Colour Management mode.
    • I think that this would technically make it an entirely different kind of popup (not a list choice, but an actual settings dialog). But it would force the user to choose a filename before the "OK" button is enabled.
    • A benefit of this is that the dialog used here can actually be the same as the dialog used in the OSD (if technically possible). You can just put all settings in that dialog so it gets configured in one go. This also ensures that the OSD settings page is always the same as the System settings page (which currently it isn't, as the filename setting is missing in the OSD and the table size seems to use a different list of values)

I would personally prefer the second option as it feels the most user-friendly (but I'm not an expert in these things). Either way though, I think the discrepancies between the OSD and System settings I mentioned in the second option, should also be resolved.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jan 21, 2017

any update?

@hudokkow

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2017

Nope?

@arucard21

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2017

Shouldn't the change in how the UI looks and behaves for Colour Management be completely separate from whether or not lcms2 should be included in cmake?

I think this could be merged and the UI and behaviour fixes for Colour Management can be done independently. Currently, Colour Management works just fine, the problems in the LibreELEC PR were due to improper configuration (caused by the current UI and behaviour).

Of course, the question of which platforms this should apply to is still open.

@hudokkow hudokkow force-pushed the hudokkow:cmake_find_lcms2 branch from 7012d3f to 594b2b3 Mar 9, 2017
@hudokkow hudokkow force-pushed the hudokkow:cmake_find_lcms2 branch from 594b2b3 to cef0b51 Mar 9, 2017
@hudokkow

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2017

jenkins build this please

@wsnipex

This comment has been minimized.

Copy link
Member

commented Mar 13, 2017

lets get this in. Adding the lib to depends can be done once needed

@hudokkow hudokkow merged commit e65f8c0 into xbmc:master Mar 13, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins4kodi You did a great job. Have a cookie.
Details
@hudokkow hudokkow deleted the hudokkow:cmake_find_lcms2 branch Mar 13, 2017
@hudokkow hudokkow added this to the L 18.0-alpha1 milestone Mar 13, 2017
@MilhouseVH MilhouseVH referenced this pull request Mar 13, 2017
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.