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

[Derek Foreman]

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?

> 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)

>> 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. :)

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