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

[Pekka Paalanen]

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.

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

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?


Thanks,
pq