[RFC PATCH 1/6] Add colorcorrection protocol

[Pekka Paalanen]

On Sun, 30 Mar 2014 13:36:32 +0200
Niels Ole Salscheider <niels_ole at salscheider-online.de> wrote:

> The color correction 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/colorcorrection.xml | 87 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 98 insertions(+), 4 deletions(-)
>  create mode 100644 protocol/colorcorrection.xml
> 
> diff --git a/Makefile.am b/Makefile.am
> index 5ff4f83..ec0a30b 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -76,7 +76,9 @@ nodist_weston_SOURCES =					\
>  	protocol/workspaces-protocol.c			\
>  	protocol/workspaces-server-protocol.h		\
>  	protocol/scaler-protocol.c			\
> -	protocol/scaler-server-protocol.h
> +	protocol/scaler-server-protocol.h		\
> +	protocol/colorcorrection-protocol.c		\
> +	protocol/colorcorrection-server-protocol.h
>  
>  BUILT_SOURCES += $(nodist_weston_SOURCES)
>  
> @@ -426,7 +428,9 @@ nodist_libtoytoolkit_la_SOURCES =			\
>  	protocol/workspaces-protocol.c			\
>  	protocol/workspaces-client-protocol.h		\
>  	protocol/xdg-shell-protocol.c			\
> -	protocol/xdg-shell-client-protocol.h
> +	protocol/xdg-shell-client-protocol.h		\
> +	protocol/colorcorrection-protocol.c		\
> +	protocol/colorcorrection-client-protocol.h
>  
>  BUILT_SOURCES += $(nodist_libtoytoolkit_la_SOURCES)
>  
> @@ -606,7 +610,9 @@ BUILT_SOURCES +=					\
>  	protocol/workspaces-client-protocol.h		\
>  	protocol/workspaces-protocol.c			\
>  	protocol/xdg-shell-protocol.c			\
> -	protocol/xdg-shell-client-protocol.h
> +	protocol/xdg-shell-client-protocol.h		\
> +	protocol/colorcorrection-protocol.c		\
> +	protocol/colorcorrection-client-protocol.h
>  
>  
>  westondatadir = $(datadir)/weston
> @@ -920,7 +926,8 @@ EXTRA_DIST +=					\
>  	protocol/text-cursor-position.xml	\
>  	protocol/wayland-test.xml		\
>  	protocol/xdg-shell.xml			\
> -	protocol/scaler.xml
> +	protocol/scaler.xml			\
> +	protocol/colorcorrection.xml
>  
>  man_MANS = weston.1 weston.ini.5
>  
> diff --git a/protocol/colorcorrection.xml b/protocol/colorcorrection.xml
> new file mode 100644
> index 0000000..7986e93
> --- /dev/null
> +++ b/protocol/colorcorrection.xml
> @@ -0,0 +1,87 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="colorcorrection">
> +
> +  <copyright>
> +    Copyright © 2014 Niels Ole Salscheider
> +
> +    Permission to use, copy, modify, distribute, and sell this
> +    software and its documentation for any purpose is hereby granted
> +    without fee, provided that the above copyright notice appear in
> +    all copies and that both that copyright notice and this permission
> +    notice appear in supporting documentation, and that the name of
> +    the copyright holders not be used in advertising or publicity
> +    pertaining to distribution of the software without specific,
> +    written prior permission.  The copyright holders make no
> +    representations about the suitability of this software for any
> +    purpose.  It is provided "as is" without express or implied
> +    warranty.
> +
> +    THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> +    SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> +    FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> +    SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +    WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN
> +    AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION,
> +    ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
> +    THIS SOFTWARE.
> +  </copyright>
> +
> +  <interface name="wl_colorcorrection" version="1">
> +    <description summary="allows to attach a color profile to a wl_surface">
> +      This interface allows to attach a color profile to a wl_surface.
> +      This is used by the compositor to display the colors correctly.
> +      The client is informed by two events about the blending space used
> +      by the compositor and the color spaces of the outputs.
> +    </description>
> +
> +    <request name="set_profile">
> +      <description summary="set the color profile of a wl_surface">
> +	With this request, the color profile of a wl_surface can be set.
> +	When mode is set to "profile", an ICC profile can be passed in the
> +	"profile_data" argument. In all other cases, "profile_data" is
> +	ignored.
> +	"mode" should only be set to "uncorrected" for fullscreen applications
> +	or applications that really require uncorrected output (e. g. color
> +	profiling tools).
> +      </description>
> +      <arg name="surface" type="object" interface="wl_surface"/>
> +      <arg name="mode" type="uint" />
> +      <arg name="profile_data" type="array"/>

Hi,

how much data can an ICC profile be?

I'm wondering whether it makes sense to send it inline in the protocol
stream like this. E.g. we transmit XKB keymaps by sending only an fd,
and then reading the actual data from the fd. If the receiver can mmap
the fd, it could read the data zero-copy, if the profile data in a file
is the very same, i.e. the sender does not need to transform it.

Also, what if a client has several surfaces all with the same ICC
profile, would it not be useful to have some notion of re-using an
already sent and parsed color profile? Otherwise I would imagine lots
of overhead if every surface has a private copy of the profile
sent over the wire, parsed, and stored in the compositor's renderer.

If 'mode' is 'blending_space', what does it mean? Explaining 'srgb'
would be good, too, for completeness.

Did we have specified somewhere already, what is the surface's default
'mode', if this request is never used?

> +    </request>
> +
> +    <event name="blending_space">
> +      <description summary="tell the client what blending space is used">
> +	This event will be sent when the blending space of the compositor
> +	is changed. A client can use this information to output its surface
> +	in the blending space of the compositor so that only one color
> +	transform (from blending to output space) has to be performed by the
> +	compositor. This can result in better performance.
> +      </description>
> +      <arg name="profile_data" type="array"/>

The same comments here about data size and using an fd.

> +    </event>
> +
> +    <event name="output_space">
> +      <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.
> +	A client can use this information e. g. when it sets the mode to
> +	"uncorrected".
> +      </description>
> +      <arg name="output_id" type="uint"/>

What is this output_id? Isn't there a way we could use the existing
wl_output objects, like send this event for every wl_output instance
the client has created by wl_registry.bind, and also every time when a
client creates a new wl_output object?

That would allow us to use wl_output object as the argument here, while
also guaranteeing that when outputs come and go, and clients bind to
them at random times, they would not miss any information.

> +      <arg name="profile_data" type="array"/>

The same comments here about data size and using an fd.

> +    </event>
> +
> +    <enum name="error">
> +      <entry name="invalid_mode" value="0"
> +	     summary="mode is set to an invalid value"/>
> +      <entry name="invalid_profile" value="1"
> +	     summary="the passed icc data is invalid"/>
> +    </enum>
> +
> +    <enum name="mode">
> +      <entry name="srgb" value="0"/>
> +      <entry name="blending_space" value="1"/>
> +      <entry name="uncorrected" value="2"/>
> +      <entry name="profile" value="3"/>
> +    </enum>
> +  </interface>
> +</protocol>

You asked about double-buffering the color profile/mode.
Double-buffering is needed, if you expect that clients should be able
to change the profile/mode while the surface is mapped. If you do not
double-buffer, then clients cannot flickerlessly change the
profile/mode without unmapping the surface.

In the very old plans, like the sketch I did in
http://people.collabora.com/~pq/wayland-color-management-proposal.png
we had emphasis on clients doing the hard work. This was trying to
reduce the burden on the compositor, while still allowing to implement
high-quality compositors as needed.

Compared to that, I see your protocol adds the option for clients to
provide a custom ICC profile to the server, and expects the server to
process it properly.

Is that a reasonable requirement for all compositors that support
per-output ICC profiles?

Or, should we perhaps have a way for a compositor to advertise the
ability to use custom, per-surface ICC profiles separately? So that
clients would not use them, if the compositor does not want to do that
work.

Your proposal also adds the explicit notion of the blending space into
the protocol. Should that be somehow optional? What if the compositor
blends in whatever like sRGB, should it still send a complete profile
describing it? Should we have special shorthands for sRGB and "current
output's profile"? Would "linear" be one, too, or is "linear" ambiguous?


Thanks,
pq