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

3dlut and simple ICC linking support for color correction #9731

Merged
merged 24 commits into from Jul 1, 2016

Conversation

Projects
None yet
10 participants
@laurimyllari
Copy link
Contributor

commented May 2, 2016

This PR adds a ColorManager class that supports loading madVR 3DLUT files, and loading ICC display profiles, creating source profiles and linking them.

The end user can use this to correct their display response with a 3DLUT file, or emulate other displays (with whitepoint, primaries and gamma selectable at runtime) with an ICC profile of their own display.

Code changes are in two commits. The first one implements a (hopefully) platform independent ColorManager class. If lcms2 is not available, only 3DLUT file format is supported. The second commit adds the necessary bits to the Linux OpenGL renderer, building on the previously added output stage.

The ICC profile linking is still work in progress, but I included it as it is functional and shows what can be done with it. It will also be needed if/when color management is implemented for the photo viewer. The 3DLUT files provide the best accuracy (at the cost of runtime flexibility) - ArgyllCMS is very smart and the linker here is quite simple.

Slight adjustments may be needed for #9720 - I'll update if/when that is merged to master.

One significant improvement I would like help with: it would be nice to keep the 3D lookup texture around as it takes a while to upload. It is most noticeable when running many short video clips back to back. @FernetMenta, any thoughts?

I'm still pretty new to the Kodi codebase. Any feedback is welcome.

Pinging @Memphiz and @popcornmix since my previous work broke gles - would you mind checking this out? Are there any gles platforms that could be supported, or better to just disable this and make sure not to break anything?


void main()
{
vec4 rgb = process();

#if defined(XBMC_3DLUT)
// FIXME: can this be optimized?

This comment has been minimized.

Copy link
@Memphiz

Memphiz May 2, 2016

Member

KODI_3DLUT please

@@ -0,0 +1,531 @@
#include <boost/algorithm/clamp.hpp>

This comment has been minimized.

Copy link
@Memphiz

Memphiz May 2, 2016

Member

missing gpl header

This comment has been minimized.

Copy link
@Memphiz

Memphiz May 2, 2016

Member

and we don't have boost anymore iirc

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 2, 2016

Member

Looks like VideoRederer is the only client of this class. Then it should be moved to this component.

@@ -0,0 +1,243 @@
/*
* Copyright (C) 2005-2014 Team XBMC

This comment has been minimized.

Copy link
@Memphiz

Memphiz May 2, 2016

Member

should by kodified

This comment has been minimized.

Copy link
@laurimyllari

laurimyllari Jun 2, 2016

Author Contributor

@Memphiz what's the right way to do this? I'm hesitant to touch the copyright notices for others :)

This comment has been minimized.

Copy link
@Memphiz

Memphiz Jun 2, 2016

Member

Copyright (C) 2016 - your name - then exchange everything that points to XBMC in the rest of the header to Kodi and kodi.tv - in case you don't want to put your name in there you can also set copyright to Team Kodi

This comment has been minimized.

Copy link
@laurimyllari

laurimyllari Jun 2, 2016

Author Contributor

Thank you! I just set it to Team Kodi since most of the file is copied directly from another dialog.

@Memphiz

This comment has been minimized.

Copy link
Member

commented May 2, 2016

Well if it can't be done for gles with the human ressources this pr has available ( you ;) ) - then it should be as defensive as possible for gles. As of the looks it doesn't touch gles - so theoretically this should be ok.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented May 2, 2016

nice!

@laurimyllari

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2016

I updated the code to address the feedback above, and to apply to current master.

@laurimyllari laurimyllari force-pushed the laurimyllari:v17-3dlut branch from 13128ba to 1ee2a6d May 3, 2016


CMS_PRIMARIES videoFlagsToPrimaries(int flags)
{
if (flags & CONF_FLAGS_COLPRI_BT709) return CMS_PRIMARIES_BT709;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 3, 2016

Member

return statement on new line please

cmsCreateTransform(sourceProfile, TYPE_RGB_FLT,
m_hProfile, TYPE_RGB_FLT,
INTENT_ABSOLUTE_COLORIMETRIC, 0);
#ifdef _DEBUG

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 3, 2016

Member

please drop those blocks. this won't ever get used. devs who are interested can add the measurements


if (!lutFile.Open(filename))
{
CLog::Log(LOGERROR, "%s: Could not open 3DLUT file: %s", __FUNCTION__, filename.c_str());

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 3, 2016

Member

wrong indent

\brief Access the global color management system
\return the global instance
*/
static CColorManager& Get();

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 3, 2016

Member

no singletons, this is an anti pattern. embed the object where you want to use it.


m_dither = useDithering;
m_ditherDepth = ditherDepth;
m_fullRange = fullrange;
// make sure CMS is enabled - this allows us to keep the texture
// around to quickly switch between CMS on and off
m_3DLUT = CColorManager::Get().IsEnabled() && (clutTex > 0);

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 3, 2016

Member

pass a reference to CM instead of using a global

@@ -69,6 +81,11 @@ void GLSLOutput::OnCompiledAndLinked(GLuint programHandle)
m_hDitherQuant = glGetUniformLocation(programHandle, "m_ditherquant");
m_hDitherSize = glGetUniformLocation(programHandle, "m_dithersize");
}
// 3DLUT
if (m_3DLUT) {

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 3, 2016

Member

braces at new line

"Input offset",
"Output offset",
"Absolute",
}; // FIXME: should be moved to ColorManager.h?

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 3, 2016

Member

who is supposed to answer this?


protected:
// implementations of ISettingCallback
virtual void OnSettingChanged(const CSetting *setting);

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta May 3, 2016

Member

use override key word for overrides

@laurimyllari laurimyllari force-pushed the laurimyllari:v17-3dlut branch from 1ee2a6d to 8ad28f2 May 8, 2016

@laurimyllari laurimyllari force-pushed the laurimyllari:v17-3dlut branch from 8ad28f2 to 9017878 Jun 2, 2016

@laurimyllari

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

Updated to remove the singleton. @FernetMenta - would you mind giving this another look?

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jun 4, 2016

@laurimyllari sorry, I almost missed this. I will have a look this weekend.

@FernetMenta FernetMenta self-assigned this Jun 4, 2016

#endif // HAVE_LCMS2

// current configuration:
CMS_PRIMARIES curVideoPrimaries;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jun 5, 2016

Member

we prefix members with m_

This comment has been minimized.

Copy link
@laurimyllari

laurimyllari Jun 9, 2016

Author Contributor

Fixed it. Thanks for your patience with these style issues - hopefully these become natural for me soon.

using namespace XFILE;

CColorManager::CColorManager() :
m_hProfile(NULL)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jun 5, 2016

Member

this member is only available with LCSM2, right? did you try to compile with LCSM2 disabled?

This comment has been minimized.

Copy link
@laurimyllari

laurimyllari Jun 9, 2016

Author Contributor

I missed these while working with the lcms2 features and previous fixes. I found a few similar issues - I'll make sure to test with lcms2 disabled before requesting another review.

@@ -186,6 +193,11 @@ CLinuxRendererGL::~CLinuxRendererGL()
delete m_pYUVShader;
m_pYUVShader = NULL;
}

if (m_ColorManager)

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jun 5, 2016

Member

why do you check this being not zero?

This comment has been minimized.

Copy link
@laurimyllari

laurimyllari Jun 9, 2016

Author Contributor

Out of habit. I'll take it out as unnecessary.

@@ -713,6 +741,8 @@ void CLinuxRendererGL::UpdateVideoFilter()
bool pixelRatioChanged = (CDisplaySettings::GetInstance().GetPixelRatio() > 1.001f || CDisplaySettings::GetInstance().GetPixelRatio() < 0.999f) !=
(m_pixelRatio > 1.001f || m_pixelRatio < 0.999f);
bool nonLinStretchChanged = false;
bool cmsChanged = (m_cmsOn != m_ColorManager->IsEnabled())

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jun 5, 2016

Member

that results in a call so settings every cycle
if ColorManager is not enabled, I see not need to check its configuration

This comment has been minimized.

Copy link
@laurimyllari

laurimyllari Jun 9, 2016

Author Contributor

My thinking here is that we need to check if cms has been toggled on/off or its configuration has changed. If cms is not on, CheckConfiguration is not called.

The second check misses the case where cms was just switched on and skips the call to CheckConfiguration, but that's ok since it will be checked later to see if a new CLUT texture needs to be loaded.

I did it this way to avoid reloading the CLUT texture when cms is toggled on/off repeatedly.

@@ -278,6 +279,17 @@ class CLinuxRendererGL : public CBaseRenderer
bool m_nonLinStretch;
bool m_nonLinStretchGui;
float m_pixelRatio;

// color management
CColorManager *m_ColorManager;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jun 5, 2016

Member

please use unique_ptr, are stopped making use unmanaged pointers.

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 23, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 23, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 24, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 24, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 24, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 24, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 24, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 24, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 24, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 24, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 24, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 25, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 25, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 25, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 26, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 26, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 27, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 27, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 27, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 28, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 28, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 29, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 29, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 30, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 30, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731

bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jul 30, 2016

package/kodi: add optional support for lcms2
Colormanager support for was added with
xbmc/xbmc#9731
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jul 30, 2016

@bkuhls STOP linking this PR please!!!

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Jul 30, 2016

@laurimyllari could you please update the TODO strings as we are going to beta it's best that we have proper strings.

@laurimyllari

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2016

@MartijnKaijser I've created pull request #10296 with updated strings.

{
CLog::Log(LOGDEBUG, "CMS configuration changed, reload LUT");
if (!LoadCLUT())
return false;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 17, 2016

Member

@laurimyllari ColorManager needs to be disabled in case or error or the system won't recover.
I enabled ColorManager without a 3dlut file. After this I had to manually edit guisettings to disable colorManager again

This comment has been minimized.

Copy link
@laurimyllari

laurimyllari Aug 17, 2016

Author Contributor

Do you think it's ok to disable it here?

A user can go in the settings later, re-enable ColorManager and set the 3dlut file.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Aug 17, 2016

Member

here it is error handling last resort. if LoadCLUT fails, there is no chance to recover.
that does not mean that other error handling is not needed

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.