AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.79k stars 455 forks source link

ICC parser fails when reading some common ICC profiles #1623

Closed est77 closed 2 years ago

est77 commented 2 years ago

When trying to use virtual views in OpenColorIO 2.1 the ICC file format parser fails for some common profiles. One example is the Display P3 profile included with OSX (I am not sure I can redistribute this file). The error is:

'International Color Consortium profile' failed with: Error parsing .icc file (/System/Library/ColorSync/Profiles/Display P3.icc). Illegal curve tag in ICC profile.

remia commented 2 years ago

Hi @est77,

I did try on my default "Color LCD" profile shipping with the MacBookPro and it worked fine. Could you check that your ICC profile contain the rTRC, gTRC and bTRC tags? OCIO only support Matrix / TRC profiles for virtual displays currently.

Edit:

I managed to reproduce the error with the "Display P3" profile, the TRC is using Parametric Type 3 which is not supported at this time but I think it should be reasonably easy to add support for it as it should match ExponentWithLinearTransform behaviour.

Screenshot 2022-04-02 at 20 07 49

doug-walker commented 2 years ago

@remia , you are right, the ICC spec defines a bunch of different Parametric Curve Types and OCIO does not support all of them currently. I don't think they all would work as an ExponentWithLinear, but if not, one could either build a Lut1D or define a new FixedFunction for the other parametric types.

remia commented 2 years ago

Agree @doug-walker that they won't all fit the ExponentWithLinearTransform, maybe it would be cleaner to have a FixedFunctionTransform supporting each form of parametric curve through parameters to have it all defined in one place.

est77 commented 2 years ago

Thanks for looking into it. I think that it would be great if this could be fixed. Using ICC profiles in OCIO is an important feature IMO and we have some user requests to support it in our software.

remia commented 2 years ago

Just checking before diving into this, has anyone planed to work on this yet?

About the above proposal of adding a new FixedFunctionTransform for the ICC Parametric Curve's 5 different types, is there any opinions compared to simply adding a 1D LUT baking directly in FileFormatICC as proposed by @doug-walker ? Having this new fixed function will duplicates some of the already existing transforms (like the pure exponent and moncurve style) and using it instead may introduce some performance regression in case we don't support the same optimisations (SSE on CPU).

It seems that if the only foreseen user for this will be FileFormatICC, the effort of adding this new FixedFunction might be hard to justify?

doug-walker commented 2 years ago

@remia , it would be great to have you work on this!

Of the five parametric curve types, type 0 is already supported as an ExponentTransform. You could add type 3 as an ExponentWithLinearTransform. I think type 1 and 2 could also be an ExponentWithLinear plus a Range to clamp at a specific value (and shift in the case of type 2). Type 4 could maybe be done with a Matrix, ExponentWithLinear, and Range? If not, any of these could be done as a Lut1DTransform, to keep it more straight-forward. I don't think it's necessary to add a FixedFunction, in any case.

One of the problems with the ICC spec is that there are combinations of parameter values that don't make sense, e.g. that create a discontinuity between the segments. The OCIO ExponentWithLinear requires a continuous curve. So I would just throw an exception if the parameters don't make sense. One scenario we may want to support though is to allow two segments that are continuous in value but not slope (i.e. a slope discontinuity). ExponentWithLinear doesn't support that. So that is making me think maybe it's better just to do types 1-4 with Lut1D.

Looking at the spec, it says the domain and range of the function shall be [0,1], so actually we may need to add a RangeTransform with any Exponent* transforms if we want to follow that. (The current type 0 support is missing that.)

remia commented 2 years ago

Thanks for the pointers @doug-walker !