[PATCH 03/10] cms-helper/static: Add "identity" builtin cms profile (v2)

[Mario Kleiner]

On 06/29/2015 06:09 PM, Derek Foreman wrote:
> On 28/06/15 10:17 PM, Mario Kleiner wrote:
>> On 06/26/2015 08:29 PM, Derek Foreman wrote:
>>> On 21/06/15 02:25 PM, Mario Kleiner wrote:
>>>> Allows to force loading an identity gamma table if
>>>> option icc_profile=identity is given in weston.ini for
>>>> an output.
>>>>
>>>> Some special display output devices, e.g., for
>>>> neuro-science applications, and special display
>>>> testing hardware need a guaranteed perfect pixel
>>>> passthrough from framebuffer to output. This is
>>>> an easy way to set this up for cms-static.
>>>>
>>>> v2: Remove confusing/redundant weston_log debug output.
>>>>
>>>> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
>>>> ---
>>>>    src/cms-static.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/cms-static.c b/src/cms-static.c
>>>> index 7166f57..e6073df 100644
>>>> --- a/src/cms-static.c
>>>> +++ b/src/cms-static.c
>>>> @@ -56,7 +56,7 @@ cms_output_created(struct cms_static *cms, struct
>>>> weston_output *o)
>>>>        if (weston_config_section_get_string(s, "icc_profile",
>>>> &profile, NULL) < 0)
>>>>            return;
>>>>        p = weston_cms_load_profile(profile);
>>>> -    if (p == NULL) {
>>>> +    if (p == NULL && strcmp(profile, "identity")) {
>>>>            weston_log("cms-static: failed to load %s\n", profile);
>>>>        } else {
>>>>            weston_log("cms-static: loading %s for %s\n",
>>>>
>>>
>>> Can we use the empty string instead of "identity"?  Or is it intentional
>>> that someone could override "identity" with a file of that name?
>>>
>>
>> I think there isn't much danger of a mixup with the "identity" keyword,
>> as 'profile' otherwise needs to be a full "path/filename" to the color
>> profile file. Also these files usually end with a suffix like .icc
>
> Ah, ok, so a user would have to have a profile called "identity"
> (without the usual .icc extension) in the current directory for
> something weird to happen.
>
> I'll concede that this is rather unlikely.
>
> Personally, I still don't like using a potentially valid filename like
> this.  However, my say isn't final, so you're probably better off
> waiting for someone else's review before making any changes. :)
>
> Why isn't there just an identity.icc that can be loaded normally?
>

That could be done as well, just given that the code already implements 
the identity setup, although normally only triggered in case of an 
error, this seems like a convenient way to put that code to good use 
without the need to have an extra .icc file for this case.

The other thought i had is that currently the builtin identity code only 
sets an identity lut, but modern gpu's have more and more hw to do color 
transformations, digital display dithering, - or from the perspective of 
special neuroscience equipment or other special hardware testing 
equipment - more ways to mess up an identity pixel passthrough from 
framebuffer to video output. If we already have dedicated code builtin 
to get an identity mapping, one can extend that code as needed, whereas 
i don't know if everything can be covered by a custom loadable color 
profile.

>> I'm not opposed to changing it, but i'd find it a bit confusing to have
>> a config option with an empty assignment in the config file a la
>>
>> icc_profile=
>>
>> It would look like a configuration error in the config file to me?
>
> We currently test for "no input method" in exactly this way (as of very
> recently)
>
> path=   means no input method
> path being absent entirely means use the default (weston-keyboard)
>

If this is a common way to do it in westons config file, i can change 
the patch to accept the empty assignment as "set identity mapping", no 
problem.

>>> Should we just load an identity gamma curve if there's no profile
>>> specified anyway?  Or is there a situation where someone wants to mess
>>> up the gamma curve before launching weston?
>>>
>>
>> You could have a setup where the user wants to set a profile on some
>> output, e.g., "identity", for such outputs with special display
>> equipment connected, but leave it to system defaults on others. E.g., i
>> usually have weston with drm/kms backend running on one VT for testing,
>> but classic XOrg on another VT for development. It's kind of convenient
>> that XOrg will color-manage the outputs which don't get overriden by a
>> specific profile.
>
> Interesting, I'd have considered X not reverting to the previous color
> management state on a VT switch a bug. :)
>

Sometimes a bug can be a feature ;-). At least for me at the moment.

colord support in weston is not in such a robust shape atm. Without the 
last patch in my updated series you can't even use the cms-colord plugin 
without crashing weston at compositor shutdown most of the time - 
including a hard lockup of VT switching in most cases, so a reboot of 
the machine is needed. There are other ways left to crash it, e.g., if 
one tries to request updates of color profiles too fast. For some of my 
use cases i'd ideally need a way to set gamma tables quickly, so there's 
more work to do...

And if another DE runs under X in parallel on another VT, you have 
multiple instances of colord and some DBUS funkyness which makes it 
quite a tedious experience to try to test or use color management.

> Is "system default" a meaningful concept here?  Or does it just mean
> "whatever the last thing set it to"?

"whatever the last thing set it to" is what i meant a bit sloppy with 
"system defaults".

-mario