[PATCH 1/6] Add cms protocol

[Pekka Paalanen]

Hi,

I had a quick look of our previous discussion to refresh my mind. It is
very good that you moved to using a protocol object for a color
profile, and fd passing for the profile data.

Here's some comments.

On Mon, 13 Oct 2014 19:40:46 +0200
Niels Ole Salscheider <niels_ole at salscheider-online.de> wrote:

> The cms protocol allows to attach an ICC profile to a surface. It also tells
> the client about the blending color space and the color spaces of all outputs.
> 
> Signed-off-by: Niels Ole Salscheider <niels_ole at salscheider-online.de>
> ---
>  Makefile.am      |  15 +++++--
>  protocol/cms.xml | 132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 143 insertions(+), 4 deletions(-)
>  create mode 100644 protocol/cms.xml
> 

> diff --git a/protocol/cms.xml b/protocol/cms.xml
> new file mode 100644
> index 0000000..34c1b16
> --- /dev/null
> +++ b/protocol/cms.xml
> @@ -0,0 +1,132 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="cms">
> +
> +  <copyright>
> +    Copyright © 2014 Niels Ole Salscheider

> +  </copyright>
> +
> +  <interface name="wl_cms" version="1">
> +    <description summary="allows to attach a color space to a wl_surface">

So techincally speaking, wl_cms is a global singleton. That should be
mentioned here, so it's obvious that creating a wl_cms object just
allows you to access the interface, but has no state or other effects.

> +      This interface allows to attach a color space to a wl_surface. This is
> +      used by the compositor to display the colors correctly. For this, the
> +      compositor converts any attached surfaces to the blending color space
> +      before the blending operations. After blending, the output surface is
> +      converted to the color space of the output device.
> +      This interface also provides requests for the sRGB and the blending color
> +      space. It further allows to create a color space from an ICC profile.
> +      The client is informed by an event if the color space of one of the
> +      outputs changes.
> +    </description>
> +
> +    <request name="set_colorspace">
> +      <description summary="set the color space of a wl_surface">
> +        With this request, the color space of a wl_surface can be set.
> +        Initially, the sRGB colorspace as returned by the srgb_colorspace
> +        request is attached to a surface.

Need to tell about the double-buffering and interaction with
wl_surface.commit.

> +      </description>
> +      <arg name="surface" type="object" interface="wl_surface" />
> +      <arg name="colorspace" type="object" interface="wl_cms_colorspace" />
> +    </request>
> +
> +    <request name="colorspace_from_fd">
> +      <description summary="creates a color space from an ICC profile">
> +        This request allows to create a wl_cms_colorspace object from an ICC
> +        profile. The fd argument is the file descriptor to the ICC profile.
> +      </description>
> +      <arg name="fd" type="fd" />
> +      <arg name="id" type="new_id" interface="wl_cms_colorspace" />

No need for a size argument? You assume the whole file from zero to end
is the profile? How does one get the size?

You should also carefully specify, what the data behind the fd is. Do
you need an extra argument to specify a type? Maybe in the future one
would like to add another type as an alternative?

For reference, see wl_keyboard.keymap event.

> +    </request>
> +
> +    <request name="output_colorspace">
> +      <description summary="returns the color space for the requested output">
> +        This request returns a wl_cms_colorspace object for the requested

It does not "return" anything. This request creates a wl_cms_colorspace
object that refers to the color profile of the given output. (I want to
avoid the term "return" in protocol language, because "return" implies
a roundtrip. Requests are semantically not function calls either, a
common misconception.) This applies to most requests below also.

> +        output. A client can use this when it does not want its surfaces to be
> +        color-corrected. In this case it can attach the color space of its main
> +        output to its surfaces.
> +      </description>
> +      <arg name="output" type="object" interface="wl_output" />
> +      <arg name="id" type="new_id" interface="wl_cms_colorspace" />
> +    </request>
> +
> +    <request name="srgb_colorspace">
> +      <description summary="tell the client what blending space is used">

I think the summary is not right, is it?

> +        This request returns a wl_cms_colorspace object for the sRGB color
> +        space. The sRGB color space is initially attached to all surfaces.

I like this very much: a shorthand for getting an object for a standard
color space.

> +      </description>
> +      <arg name="id" type="new_id" interface="wl_cms_colorspace" />
> +    </request>
> +
> +    <request name="blending_colorspace">
> +      <description summary="tell the client what blending space is used">
> +        This request returns a wl_cms_colorspace object for the blending color
> +        space of the compositor. All surfaces are converted by the compositor
> +        to the blending color space before the blending operations. A client
> +        should render in this color space if it does any color conversion on
> +        its own.
> +      </description>
> +      <arg name="id" type="new_id" interface="wl_cms_colorspace" />
> +    </request>
> +
> +    <event name="output_colorspace_changed">
> +      <description summary="tell the client what color space an output has">
> +        This event will be sent when the color space of an output is changed.

Oh, so output color space can change?
Ok, I assume the correct reaction from a client is to get the output
color space again.

> +      </description>
> +      <arg name="output" type="object" interface="wl_output" />
> +    </event>
> +
> +    <enum name="error">
> +      <entry name="invalid_profile" value="0"
> +             summary="the passed icc data is invalid" />

You forgot to document this in the request that may raise this error.

Protocol errors are always fatal. Is invalid ICC data reason enough to
disconnect the whole client? It's certainly easy and clear.

However, I saw Bryce mentioning different ICC versions. If the
compositor understands only version 2, but a client provides a correct
version 4 profile, does it get killed for it?

I think you may want an explicit profile format argument, that
unambiguously describes what the format is. Actually, you are going to
need an event, that tell the client what profile formats the server
supports. Otherwise clients cannot know if their profile is ok or they
get killed.

> +    </enum>

This interface is missing a destructor. For more information, see:
http://ppaalanen.blogspot.fi/2014/07/wayland-protocol-design-object-lifespan.html

> +  </interface>
> +
> +  <interface name="wl_cms_colorspace" version="1">
> +    <description summary="represents a color space">
> +      This interface represents a color space that can be attached to surfaces.
> +      It is used by the wl_cms interface.
> +    </description>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroys the wl_cms_colorspace object">
> +        Informs the server that the client will not be using this protocol
> +        object anymore.
> +      </description>
> +    </request>
> +
> +    <request name="get_profile_fd">
> +      <description summary="get a file descriptor to the profile data">
> +        This request will cause a profile_fd event that returns a file
> +        descriptor to the ICC profile data of this colorspace.
> +      </description>
> +    </request>
> +
> +    <event name="profile_data">
> +      <description summary="file descriptor to the profile data">
> +        This event occurs after a get_profile_fd request and returns the file
> +        descriptor to the ICC profile data of this colorspace.

This is a naive way of specifying it, but it works. wl_cms_colorspace
objects are immutable (once created, the color profile it refers to
will never(?) change - hey, you should document that), so this simple
approch is good enough. For possibly changing data where you care about
which moment you asked for it, we use a separate reply object created by
the request for delivering the event instead.

So, one can use this to retrieve the sRGB profile, which means every
compositor should always have one at hand, right?

> +      </description>
> +      <arg name="fd" type="fd" />

Missing size argument? Format argument?

What happens, if a wl_cms_colorspace object refers to something that
does not exist anymore? For instance, it referred to an output color
space, but that output is now gone. Not just for get_profile_fd
request, but if a client continues using the object (attach or already
attached) for a wl_surface? I suppose it simply retains the color
profile data and continues working as usual, no?

> +    </event>

Ok, the same format problems here too. How does the server know to use
a format the client can understand? What if a client doesn't understand
the format it is offered.

Would you say the compositor will never convert a profile from one
format to another? So that it makes no sense for a client to ask for a
specific format, it will just get whatever it gets and needs to deal
with it.

> +  </interface>
> +</protocol>

The protocol looks simple and clean, like it should be. I can't really
comment on the deeper semantics on color management, though.


Thanks,
pq