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

[Mario Kleiner]

On 07/02/2015 01:57 PM, Pekka Paalanen wrote:
> On Thu, 02 Jul 2015 09:36:47 +0200
> Mario Kleiner <mario.kleiner.de at gmail.com> wrote:
>
>> 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.
>
> Yeah, I'd prefer this too. Or, if we went with "identity", it would
> need to be checked before any file loading is attempted, so that a
> random file called "identity" would not even be attempted to be loaded,
> even if it's not a valid icc profile.
>

Ok, just sent out v3 of the patch. Now loads the identity mapping if an 
empty assignment "icc_profile=" is given.

>>>>> 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.
>
> A bug indeed. When a display server takes over on VT switch, I'd expect
> it either needs to reinitialize all display hardware state it intends
> to manage or load the current setup from the driver, so it's up-to-date on
> what is in the hardware.
>
> Anything that relies on sloppiness there is bound to break when it gets
> fixed.

Ok, just retested this again. X protects itself properly and apparently 
reloads its own gamma tables when VT switching to it.

Weston otoh. doesn't restore its setting when switching to its VT. That 
was the bug/"feature" that came in handy for me during initial testing 
with my equipment, as i could use the parallel X-Session to set proper 
gamma tables and then VT switch to Weston.

>
> So, adding "identity" feature is nice, but then it should really
> enforce identity at all times it is controlling the output.
>
> I thought we already did reload the gamma tables on VT-switch in. Or at
> least I have a vague recollection of it being discussed. Maybe backends
> do it on their own, but don't call to CMS? Which means you'd get your
> profile on first start, but VT-switch would reset it to identity or
> something. Anyone seen such problems? Or was it just fbdev backend?
>

Currently only the drm_backend implements gamma table control by hooking 
up the output->base.set_gamma method to drm_output_set_gamma. And that 
method is only called from the cms code and not on VT switch.

-mario