[RFC,wayland-protocols,v3,1/1] unstable: add color management protocol

Submitted by Sebastian Wick on March 18, 2019, 1:12 a.m.

Details

Message ID 20190318011259.8562-2-sebastian@sebastianwick.net
State New
Headers show
Series "color-management: Add HDR-metadata support" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Sebastian Wick March 18, 2019, 1:12 a.m.
This protocol allows clients to attach a color space, rendering intent
and HDR information to surfaces and to query outputs about their color
spaces and HDR capabilities.

Signed-off-by: Sebastian Wick <sebastian@sebastianwick.net>
---
 Makefile.am                                   |   1 +
 unstable/color-management/README              |   4 +
 .../color-management-unstable-v1.xml          | 228 ++++++++++++++++++
 3 files changed, 233 insertions(+)
 create mode 100644 unstable/color-management/README
 create mode 100644 unstable/color-management/color-management-unstable-v1.xml

Patch hide | download patch | download mbox

diff --git a/Makefile.am b/Makefile.am
index 345ae6a..80abc1d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -23,6 +23,7 @@  unstable_protocols =								\
 	unstable/xdg-decoration/xdg-decoration-unstable-v1.xml	\
 	unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \
 	unstable/primary-selection/primary-selection-unstable-v1.xml		\
+	unstable/color-management/color-management-unstable-v1.xml		\
 	$(NULL)
 
 stable_protocols =								\
diff --git a/unstable/color-management/README b/unstable/color-management/README
new file mode 100644
index 0000000..140f1e9
--- /dev/null
+++ b/unstable/color-management/README
@@ -0,0 +1,4 @@ 
+Color management protocol
+
+Maintainers:
+Sebastian Wick <sebastian@sebastianwick.net>
diff --git a/unstable/color-management/color-management-unstable-v1.xml b/unstable/color-management/color-management-unstable-v1.xml
new file mode 100644
index 0000000..7b4d08e
--- /dev/null
+++ b/unstable/color-management/color-management-unstable-v1.xml
@@ -0,0 +1,228 @@ 
+<?xml version="1.0" encoding="UTF-8"?>
+<protocol name="color_management_unstable_v1">
+
+  <copyright>
+	Copyright © 2019 Sebastian Wick
+	Copyright © 2019 Erwin Burema
+
+	Permission is hereby granted, free of charge, to any person obtaining a
+	copy of this software and associated documentation files (the "Software"),
+	to deal in the Software without restriction, including without limitation
+	the rights to use, copy, modify, merge, publish, distribute, sublicense,
+	and/or sell copies of the Software, and to permit persons to whom the
+	Software is furnished to do so, subject to the following conditions:
+
+	The above copyright notice and this permission notice (including the next
+	paragraph) shall be included in all copies or substantial portions of the
+	Software.
+
+	THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+	IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+	FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+	THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+	LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+	FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+	DEALINGS IN THE SOFTWARE.
+  </copyright>
+
+  <description summary="color management protocol">
+	This protocol specifies a way for a client to set the color space and
+	HDR metadata of a surface and to get information about the color spaces
+	and HDR capabilities of outputs.
+  </description>
+
+  <interface name="zwp_color_manager_v1" version="1">
+    <description summary="color manager">
+	A global interface used for getting color management surface and color
+	management output objects as well as creating color spaces from ICC
+	profiles.
+    </description>
+
+    <enum name="error">
+      <description summary="fatal color manager errors">
+	These fatal protocol errors may be emitted in response to illegal color
+	management requests.
+      </description>
+      <entry name="invalid_icc_profile" value="0" summary="invalid ICC profile"/>
+    </enum>
+
+    <request name="create_color_space">
+      <description summary="create a color space object">
+	Create a color space object from an ICC profile.
+
+	Only three channel display profiles are allowed. The file descriptor
+	must be mmap-able. If the conditions are not met a protocol error
+	"invalid_icc_profile" is raised by the compositor.
+
+	See the zwp_color_space interface for more details about the created
+	object.
+
+	See the ICC specification for more details about ICC profiles.
+      </description>
+      <arg name="id" type="new_id" interface="zwp_color_space_v1"/>
+      <arg name="icc" type="fd"/>
+    </request>
+
+    <request name="get_color_management_output">
+      <description summary="create a color management output from a wl_output">
+	This creates a new color zwp_color_management_output object for the
+	given wl_output.
+
+	See the zwp_color_management_output interface for more details.
+      </description>
+      <arg name="id" type="new_id" interface="zwp_color_management_output_v1"/>
+      <arg name="output" type="object" interface="wl_output"/>
+    </request>
+
+    <request name="get_color_management_surface">
+      <description summary="create a color management surface from a wl_surface">
+	This creates a new color zwp_color_management_surface object for the
+	given wl_surface.
+
+	See the zwp_color_management_surface interface for more details.
+      </description>
+      <arg name="id" type="new_id" interface="zwp_color_management_surface_v1"/>
+      <arg name="surface" type="object" interface="wl_surface"/>
+    </request>
+
+    <request name="destroy" type="destructor">
+      <description summary="destroy the color manager">
+	Destroy the zwp_color_manager object.
+      </description>
+    </request>
+  </interface>
+
+  <interface name="zwp_color_management_output_v1" version="1">
+    <description summary="output color properties">
+	A zwp_color_management_output describes the color properties of an
+	output.
+    </description>
+
+    <event name="color_space_changed">
+      <description summary="color space changed">
+	The color_space_changed event is sent after creating an zwp_color_management_output
+	(see zwp_color_manager.get_color_management_output) and whenever the color
+	space of the output changed.
+      </description>
+    </event>
+
+    <request name="get_color_space">
+      <description summary="get the color space of the output">
+	This creates a new zwp_color_space object for the current color space of
+	the output. There always is exactly one color space active so the client
+	should destroy the color space created by earlier invocations of this
+	request. This is usually called as a reaction to the color_space_changed
+	event.
+
+	See the zwp_color_space interface for more details.
+      </description>
+      <arg name="id" type="new_id" interface="zwp_color_space_v1"/>
+    </request>
+
+    <!-- TODO: HDR capabilities event -->
+
+    <request name="destroy" type="destructor">
+      <description summary="destroy the color management output">
+	Destroy the color zwp_color_management_output object.
+      </description>
+    </request>
+  </interface>
+
+  <interface name="zwp_color_management_surface_v1" version="1">
+    <description summary="set color management information of a surface">
+	A zwp_color_management_surface allows the client to set the color space and
+	HDR properties of a surface.
+    </description>
+
+    <enum name="render_intent">
+      <description summary="render intent">
+	Rendering intent allow the client to hint at how to perform color space
+	transformations.
+
+	See the ICC specification for more details about rendering intent.
+      </description>
+      <entry name="absolute" value="1" summary="ICC-absolute colorimetric"/>
+      <entry name="relative" value="2" summary="media-relative colorimetric"/>
+      <entry name="perceptual" value="3" summary="perceptual"/>
+      <entry name="saturation" value="4" summary="saturation"/>
+    </enum>
+
+    <request name="set_color_space">
+      <description summary="set the color space">
+	Set the color space of the underlying surface. The color space is double
+	buffered, and will be applied at the time wl_surface.commit of the
+	corresponding wl_surface is called.
+
+	The render intent gives the compositor a hint what to optimize for in
+	color space transformations.
+
+	The corresponding buffer is expected to contain un-premultiplied pixels when
+	a color space is set with this request.
+
+	If a surface has no color space set, sRGB and an arbitrary render intent
+	will be assumed.
+
+	If the color space of the surface matches the color space of an output
+	it is shown on the performance and color accuracy might improve. To find
+	those color spaces the client can listen to the preferred_color_space or
+	the wl_surface.enter/leave events.
+      </description>
+      <arg name="color_space" type="object" interface="zwp_color_space_v1"/>
+      <arg name="render_intent" type="uint" enum="render_intent"/>
+    </request>
+
+    <!-- TODO: HDR metadata request -->
+
+    <event name="preferred_color_space">
+      <description summary="preferred color space">
+	The preferred_color_space event is sent after creating an
+	zwp_color_management_surface (see zwp_color_manager.get_color_management_surface)
+	and whenever the preferred color space changed.
+
+	The event does not carry a zwp_color_space but a wl_output object. The
+	concret zwp_color_space can be created by calling
+	zwp_color_management_output.get_color_space on the output.
+
+	This is only a hint and clients can set any valid color space with
+	set_color_space but there might be performance and color accuracy
+	improvements by providing the surface in the preferred color space.
+      </description>
+      <arg name="output" type="object" interface="wl_output"/>
+    </event>
+
+    <request name="destroy" type="destructor">
+      <description summary="destroy the color management surface">
+	Destroy the zwp_color_management_surface object.
+      </description>
+    </request>
+  </interface>
+
+  <interface name="zwp_color_space_v1" version="1">
+    <description summary="color space">
+	Describes a color space which can be attached to a surface
+	(zwp_color_management_surface.set_color_space) and provides information
+	like the ICC profile to alow clients to do color space transformations.
+
+	The client can create a zwp_color_space object from an ICC profile by
+	calling zwp_color_manager.create_color_space or from an output by
+	calling zwp_color_management_output.get_color_space.
+    </description>
+
+    <event name="information">
+      <description summary="color space information">
+	Information describing the color space is send once after binding.
+
+	The icc argument contains a mmap-able fd to the corresponding ICC
+	profile.
+      </description>
+      <arg name="icc" type="fd"/>
+    </event>
+
+    <request name="destroy" type="destructor">
+      <description summary="destroy the color space">
+	Destroy the zwp_color_space object.
+      </description>
+    </request>
+  </interface>
+
+</protocol>

Comments

Hello,

Am 18.03.19 um 02:12 schrieb Sebastian Wick:

> This protocol allows clients to attach a color space, rendering intent
> and HDR information to surfaces and to query outputs about their color
> spaces and HDR capabilities.
...
> +    <enum name="render_intent">
> +      <description summary="render intent">
> +	Rendering intent allow the client to hint at how to perform color space
> +	transformations.
> +
> +	See the ICC specification for more details about rendering intent.
> +      </description>
> +      <entry name="absolute" value="1" summary="ICC-absolute colorimetric"/>
> +      <entry name="relative" value="2" summary="media-relative colorimetric"/>
> +      <entry name="perceptual" value="3" summary="perceptual"/>
> +      <entry name="saturation" value="4" summary="saturation"/>
> +    </enum>
Rendering intent order in ICC1v43_2010-12 7.2.15 Rendering intent field
Table 23 is:
0 - Perceptual
1 - Media-relative colorimetric
2 - Saturation
3 - ICC-absolute colorimetric

The same order is used in other popular libraries (little CMS) and
applications (Inkscape...).

In order to be feature complete, please add:

name="relative+bpc" summary="media-relative colorimetric + black point
compensation"

Info about BlackPointCompensation ISO 18619:2015:
http://www.color.org/whitepapers.xalter


regards
Kai-Uwe Behrmann

Hi,

Here are a few more comments in addition to Pekka's ones.

On Monday, March 18, 2019 3:12 AM, Sebastian Wick <sebastian@sebastianwick.net> wrote:
> This protocol allows clients to attach a color space, rendering intent
> and HDR information to surfaces and to query outputs about their color
> spaces and HDR capabilities.
>
> Signed-off-by: Sebastian Wick <sebastian@sebastianwick.net>
> ---
>  Makefile.am                                   |   1 +
>  unstable/color-management/README              |   4 +
>  .../color-management-unstable-v1.xml          | 228 ++++++++++++++++++
>  3 files changed, 233 insertions(+)
>  create mode 100644 unstable/color-management/README
>  create mode 100644 unstable/color-management/color-management-unstable-v1.xml
>
> diff --git a/Makefile.am b/Makefile.am
> index 345ae6a..80abc1d 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -23,6 +23,7 @@ unstable_protocols =								\
>  	unstable/xdg-decoration/xdg-decoration-unstable-v1.xml	\
>  	unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \
>  	unstable/primary-selection/primary-selection-unstable-v1.xml		\
> +	unstable/color-management/color-management-unstable-v1.xml		\
>  	$(NULL)
>
>  stable_protocols =								\
> diff --git a/unstable/color-management/README b/unstable/color-management/README
> new file mode 100644
> index 0000000..140f1e9
> --- /dev/null
> +++ b/unstable/color-management/README
> @@ -0,0 +1,4 @@
> +Color management protocol
> +
> +Maintainers:
> +Sebastian Wick <sebastian@sebastianwick.net>
> diff --git a/unstable/color-management/color-management-unstable-v1.xml b/unstable/color-management/color-management-unstable-v1.xml
> new file mode 100644
> index 0000000..7b4d08e
> --- /dev/null
> +++ b/unstable/color-management/color-management-unstable-v1.xml
> @@ -0,0 +1,228 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="color_management_unstable_v1">
> +
> +  <copyright>
> +	Copyright © 2019 Sebastian Wick
> +	Copyright © 2019 Erwin Burema
> +
> +	Permission is hereby granted, free of charge, to any person obtaining a
> +	copy of this software and associated documentation files (the "Software"),
> +	to deal in the Software without restriction, including without limitation
> +	the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +	and/or sell copies of the Software, and to permit persons to whom the
> +	Software is furnished to do so, subject to the following conditions:
> +
> +	The above copyright notice and this permission notice (including the next
> +	paragraph) shall be included in all copies or substantial portions of the
> +	Software.
> +
> +	THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +	IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +	FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +	THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +	LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +	FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +	DEALINGS IN THE SOFTWARE.
> +  </copyright>
> +
> +  <description summary="color management protocol">
> +	This protocol specifies a way for a client to set the color space and
> +	HDR metadata of a surface and to get information about the color spaces
> +	and HDR capabilities of outputs.
> +  </description>
> +
> +  <interface name="zwp_color_manager_v1" version="1">
> +    <description summary="color manager">
> +	A global interface used for getting color management surface and color
> +	management output objects as well as creating color spaces from ICC
> +	profiles.
> +    </description>
> +
> +    <enum name="error">
> +      <description summary="fatal color manager errors">
> +	These fatal protocol errors may be emitted in response to illegal color
> +	management requests.
> +      </description>
> +      <entry name="invalid_icc_profile" value="0" summary="invalid ICC profile"/>
> +    </enum>
> +
> +    <request name="create_color_space">
> +      <description summary="create a color space object">
> +	Create a color space object from an ICC profile.
> +
> +	Only three channel display profiles are allowed. The file descriptor
> +	must be mmap-able. If the conditions are not met a protocol error
> +	"invalid_icc_profile" is raised by the compositor.

As Pekka said, it would be better to add more access restrictions to the mmap'ed
region. In addition to saying the compositor shouldn't write to it, I believe we
should also require the compositor to map it with MAP_PRIVATE. See the
wl_keyboard.keymap event docs:

> From version 7 onwards, the fd must be mapped with MAP_PRIVATE by
> the recipient, as MAP_SHARED may fail.

> +	See the zwp_color_space interface for more details about the created

Interfaces names should either be in their stable form ("wp_color_space") or
their unstable one ("zwp_color_space_v1").

> +	object.
> +
> +	See the ICC specification for more details about ICC profiles.
> +      </description>
> +      <arg name="id" type="new_id" interface="zwp_color_space_v1"/>
> +      <arg name="icc" type="fd"/>
> +    </request>
> +
> +    <request name="get_color_management_output">
> +      <description summary="create a color management output from a wl_output">
> +	This creates a new color zwp_color_management_output object for the
> +	given wl_output.
> +
> +	See the zwp_color_management_output interface for more details.
> +      </description>
> +      <arg name="id" type="new_id" interface="zwp_color_management_output_v1"/>
> +      <arg name="output" type="object" interface="wl_output"/>
> +    </request>
> +
> +    <request name="get_color_management_surface">
> +      <description summary="create a color management surface from a wl_surface">
> +	This creates a new color zwp_color_management_surface object for the
> +	given wl_surface.

What happens if two zwp_color_management_surface_v1 objects are created for the
same wl_surface?

Many protocols just disallow this and send a protocol error.

> +	See the zwp_color_management_surface interface for more details.
> +      </description>
> +      <arg name="id" type="new_id" interface="zwp_color_management_surface_v1"/>
> +      <arg name="surface" type="object" interface="wl_surface"/>
> +    </request>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroy the color manager">
> +	Destroy the zwp_color_manager object.
> +      </description>
> +    </request>
> +  </interface>
> +
> +  <interface name="zwp_color_management_output_v1" version="1">
> +    <description summary="output color properties">
> +	A zwp_color_management_output describes the color properties of an
> +	output.

Pekka suggested to make the object inert when the proxy is destroyed. I'm not
sure this is required as long as the compositor state doesn't _need_ to
reference the resource. But it should at least become inert when the output is
destroyed.

> +    </description>
> +
> +    <event name="color_space_changed">
> +      <description summary="color space changed">
> +	The color_space_changed event is sent after creating an zwp_color_management_output
> +	(see zwp_color_manager.get_color_management_output) and whenever the color
> +	space of the output changed.
> +      </description>
> +    </event>
> +
> +    <request name="get_color_space">
> +      <description summary="get the color space of the output">
> +	This creates a new zwp_color_space object for the current color space of
> +	the output. There always is exactly one color space active so the client
> +	should destroy the color space created by earlier invocations of this
> +	request. This is usually called as a reaction to the color_space_changed
> +	event.
> +
> +	See the zwp_color_space interface for more details.
> +      </description>
> +      <arg name="id" type="new_id" interface="zwp_color_space_v1"/>
> +    </request>
> +
> +    <!-- TODO: HDR capabilities event -->
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroy the color management output">
> +	Destroy the color zwp_color_management_output object.
> +      </description>
> +    </request>
> +  </interface>
> +
> +  <interface name="zwp_color_management_surface_v1" version="1">
> +    <description summary="set color management information of a surface">
> +	A zwp_color_management_surface allows the client to set the color space and
> +	HDR properties of a surface.
> +    </description>
> +
> +    <enum name="render_intent">
> +      <description summary="render intent">
> +	Rendering intent allow the client to hint at how to perform color space
> +	transformations.
> +
> +	See the ICC specification for more details about rendering intent.
> +      </description>
> +      <entry name="absolute" value="1" summary="ICC-absolute colorimetric"/>
> +      <entry name="relative" value="2" summary="media-relative colorimetric"/>
> +      <entry name="perceptual" value="3" summary="perceptual"/>
> +      <entry name="saturation" value="4" summary="saturation"/>
> +    </enum>
> +
> +    <request name="set_color_space">
> +      <description summary="set the color space">
> +	Set the color space of the underlying surface. The color space is double
> +	buffered, and will be applied at the time wl_surface.commit of the
> +	corresponding wl_surface is called.
> +
> +	The render intent gives the compositor a hint what to optimize for in
> +	color space transformations.
> +
> +	The corresponding buffer is expected to contain un-premultiplied pixels when
> +	a color space is set with this request.
> +
> +	If a surface has no color space set, sRGB and an arbitrary render intent
> +	will be assumed.
> +
> +	If the color space of the surface matches the color space of an output
> +	it is shown on the performance and color accuracy might improve. To find
> +	those color spaces the client can listen to the preferred_color_space or
> +	the wl_surface.enter/leave events.
> +      </description>
> +      <arg name="color_space" type="object" interface="zwp_color_space_v1"/>
> +      <arg name="render_intent" type="uint" enum="render_intent"/>
> +    </request>
> +
> +    <!-- TODO: HDR metadata request -->
> +
> +    <event name="preferred_color_space">
> +      <description summary="preferred color space">
> +	The preferred_color_space event is sent after creating an
> +	zwp_color_management_surface (see zwp_color_manager.get_color_management_surface)
> +	and whenever the preferred color space changed.
> +
> +	The event does not carry a zwp_color_space but a wl_output object. The
> +	concret zwp_color_space can be created by calling
> +	zwp_color_management_output.get_color_space on the output.
> +
> +	This is only a hint and clients can set any valid color space with
> +	set_color_space but there might be performance and color accuracy
> +	improvements by providing the surface in the preferred color space.
> +      </description>
> +      <arg name="output" type="object" interface="wl_output"/>
> +    </event>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroy the color management surface">
> +	Destroy the zwp_color_management_surface object.
> +      </description>
> +    </request>
> +  </interface>
> +
> +  <interface name="zwp_color_space_v1" version="1">
> +    <description summary="color space">
> +	Describes a color space which can be attached to a surface
> +	(zwp_color_management_surface.set_color_space) and provides information
> +	like the ICC profile to alow clients to do color space transformations.
> +
> +	The client can create a zwp_color_space object from an ICC profile by
> +	calling zwp_color_manager.create_color_space or from an output by
> +	calling zwp_color_management_output.get_color_space.
> +    </description>
> +
> +    <event name="information">
> +      <description summary="color space information">
> +	Information describing the color space is send once after binding.
> +
> +	The icc argument contains a mmap-able fd to the corresponding ICC
> +	profile.
> +      </description>
> +      <arg name="icc" type="fd"/>
> +    </event>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroy the color space">
> +	Destroy the zwp_color_space object.
> +      </description>
> +    </request>
> +  </interface>
> +
> +</protocol>
> --
> 2.20.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Tuesday, March 19, 2019 1:23 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> Removing the initial uncertainty would require quite some more protocol
> and would apply to much more than just color space.

Yeah. There is some overlap between this protocol,
wl_surface.enter/leave and wp_presentation_time. Not sure a new
protocol would bring enough value to be worth it though.

[…]

> "after binding"?
>
> If a client creates a color space object from an ICC profile, there is
> no need to send the ICC file immediately back to the client.
>
> Should sending this event be tied to the request that created this
> object, or should this object have an explicit request to emit this
> event?

Does the client know in advance what the size of the profile will be?

(Maybe this should be an event, and the request to fill a FD a request)

> I think an explicit request would be better because... it is explicit.
> It does not add any roundtrips that are not already there. In the cases
> where a client likely wants to get the ICC file, it has already had to
> create the color space protocol object itself and sending another
> request on the object goes in the same message burst as creating it.

On Thursday, March 21, 2019 12:05 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > "after binding"?
> > > If a client creates a color space object from an ICC profile, there is
> > > no need to send the ICC file immediately back to the client.
> > > Should sending this event be tied to the request that created this
> > > object, or should this object have an explicit request to emit this
> > > event?
> >
> > Does the client know in advance what the size of the profile will be?
>
> Why would it need to?
>
> > (Maybe this should be an event, and the request to fill a FD a request)
>
> I meant that the proposed event with fd remains as is, but is never
> sent automatically; instead, a new request would be added that simply
> triggers sending the event.
>
> A request "please fill in this FD with the data" has a couple of
> issues: is the backing storage large enough, and how will the client know
> when it is done. These are certainly solvable, but complicate things.


Hmm, I see. This makes sense.
Sebastian Wick wrote:

> +<protocol name="color_management_unstable_v1">

> +  <interface name="zwp_color_manager_v1" version="1">

> +    <request name="get_color_management_surface">
> +      <description summary="create a color management surface from a wl_surface">
> +	This creates a new color zwp_color_management_surface object for the
> +	given wl_surface.
> +
> +	See the zwp_color_management_surface interface for more details.
> +      </description>
> +      <arg name="id" type="new_id" interface="zwp_color_management_surface_v1"/>
> +      <arg name="surface" type="object" interface="wl_surface"/>
> +    </request>

> +  <interface name="zwp_color_management_surface_v1" version="1">
> +    <description summary="set color management information of a surface">
> +	A zwp_color_management_surface allows the client to set the color space and
> +	HDR properties of a surface.
> +    </description>

> +    <request name="set_color_space">
> +      <description summary="set the color space">
> +	Set the color space of the underlying surface. The color space is double
> +	buffered, and will be applied at the time wl_surface.commit of the
> +	corresponding wl_surface is called.

Hi,
	source color space is not an attribute of a wl_surface though, it is an attribute
of a wl_buffer, the same as (implied) dpi and format. The compositor has to deal with a
mismatch in these attributes between a wl_buffer and an output frame buffert in the same
way: If the dpi's mismatch, the buffer has to be scaled. If the format's mismatch, the
pixel encoding has to be changed. If the colorspaces mismatch, the colorspace has to be
transformed.

Some formats (i.e. YCbCr) have different gamuts, and therefore might have different color
profiles that reflect differences in color handling due to the different gamuts sizes and
shapes. (A good compositor implementation would be forgiving in this regard though.)

Cheers,
	Graeme Gill.

Pekka Paalanen wrote:

Hi Pekka,

> yes, it is not an attribute of wl_surface, but it also is not an
> attribute of a wl_buffer. It is an attribute of the current contents in
> a wl_buffer, contents that will get transferred into the wl_surface on
> commit (by reference or by copy).

Right, the wl_buffer contains the meta information about the
buffer contents, such as its format, dimensions, layout, etc.

> In essence, the color space is a
> property of the commit (wl_surface.commit).

I don't think so. It's a property of the pixel contents.

> The proposed extension
> language models that mechanism correctly by referring to
> "double-buffered state". I can't think of better wording for this, the
> term double-buffered state has grown that special meaning when talking
> about Wayland.

That's not how I see it. "Double buffered state" just seems to be
a term covering the piecemeal assembly of the various bits
of information needed for an atomic update to the output state
(i.e. what happens between attach and commit).

Yes, you could do this by treating the source colorspace as just
another bit of information to be assembled, but it's an unnatural way
of doing things, something that becomes clear if you consider
re-using a buffer and its contents for some other update.

> More precisely, a wl_buffer is a chunk of memory with a size and pixel
> format, and they get re-used all the time. If a client changes the
> color properties of the content but not the pixel format, it likely
> will not allocate a new wl_buffer to hold it. All client frameworks aim
> to re-use existing buffers as much as they can, because destroying and
> allocating new ones takes effort.

Sure, so the wl_buffer would have its color profile re-set if the
color encoding of new pixel values is different. This is a perfectly
natural time to do this (i.e. say loading an image from
a .jpg file that is tagged with a different colorspace) Conversely,
if the contents were to be re-used, there is no need for the client
to have to keep track of the colorspace when submitting the buffer
as an update, since as a property of the wl_buffer, it tracks right
along with the pixel values.

Cheers,
	Graeme Gill.

Pekka Paalanen wrote:

Hello Pekka,

> yes, but the pixel contents are the property of the commit.

I don't see that - a wl_buffer appears to be quite independent of
a commit. For instance, the screencopy protocol creates a wl_buffer
without any reference to a wl_surface, and a color managed
screencopy buffer should be tagged with the outputs color
profile, so that the client can (say) retrieve the profile
and tag the image file it saves it to.

> They are not the property of wl_surface or wl_buffer, both of which
> change content during their lifetime. The only concept we have that
> matches the content lifetime at all better is a "commit". This is why
> buffer scale (HiDPI) and transform are also set up on commit and not
> with wl_buffer.

They aren't analogs though. The buffer scale and spatial transform
are both transforms. Conceptually they could be defined as transforms or they
could be derived by combining source and destination state.
For instance, HiDPI scale could be defined as a scale factor,
or as a product of the destination DPI divided by the source DPI. The
transformation is a property of the commit, yes. The source state (for
rendering to output) is a property of the wl_buffer, and the destination
state is a property of the wl_output.

For color management the state in question is the color device profile. The
transform is the result of combining the source and destination device profiles
(using a CMM) into device link transform, something that would happen at commit.

> As a Wayland protocol designer, it is the natural way to me. That said,
> this may be purely because I've accustomed to the existing practises.

From my perspective, representing the device color profile as an independent
object and having code keep track of it and the corresponding wl_buffer is
kind of the equivalent of "spaghetti programming". It can certainly be made
to work, but it is fragile and hard to maintain, and presents an overly
complex API.

> I don't think that works in practise for one huge awkward reason: EGL.
> 
> When an application uses EGL to deliver its contents to the screen, the
> application code will never even see a wl_buffer at all. The closest
> thing the application has access to is wl_surface. There simply is no
> opportunity to attach anything to a wl_buffer, because those are
> completely hidden inside the EGL implementation. Making an EGL
> extension to be able to drive specific Wayland protocol through EGL is
> not worth it.

From poking about the code, it doesn't seem to me to be any easier or harder
to deal with EGL than any of the other back end wl_buffer types :- they
are all difficult because of the nature of the current implementation.

It appears that from the Wayland compositor perspective (e.g. Weston),
that there is typically no concrete wl_buffer type - it is just a struct * declaration
and a protocol definition with one request and one event. Individual back end
implementations create their own protocol compatible objects and
cast it to wl_buffer, and therein lies the difficulty.

If wl_buffer was a virtual base class from which the back
ends inherited, then it would be simple to add the color profile
reference to the base class, as well as methods to access the profile.
But as this isn't the case, it currently appears to me that there are a
few possible approaches:

1) Define a concrete wl_buffer type in the compositor, and store a reference to
the back end types in it, along with the color profile reference.
The wl_buffer implementation would have to somehow intercept
all wl_buffer factories return value to wrap them, as well as
proxying the request and event back to them. Any other
protocol messages that pass wl_buffers to back ends would also
have to have the wl_buffer de-referenced somehow too.

2) Alter all the back end wl_buffer types to include the color profile
reference. There doesn't seem to be a mechanism for registering virtual
methods for the compositor implementation to provide access to the color
profile in a way that's independent of the particular back end structure
layout though, so you're left with ensuring binary compatibility of some
kind, something that doesn't seem idiomatic to Wayland (although I have
used it quite extensively and successfully in my C code by way of struct macros),
so some mechanism equivalent to a vtable would have to be added.

3) Extend the wl_buffer protocol to include methods for setting
and getting a color profile object. This then puts it on the
back ends to implement those requests and store and retrieve
the object reference, but implementation then follows the usual
Wayland pattern. Once again, having a way of defining
a base class would ease back end implementors lives, by
allowing them to call common core implementations of these methods, but
if it's a single object to store and retrieve, perhaps the burden isn't
great.

Hard for me to judge amongst these possibilities or to
know of any others, without delving deeper, but intuitively
the last options seems the cleanest. I presume it has core
protocol versioning implications though.

[ Of course every back end will need to be modified to actually
  support the color transform at commit time anyway, at which point
  either it will retrieve the color profile from the wl_buffer. ]

Cheers,
	Graeme Gill.

Pekka Paalanen wrote:

Hi,

> they are analogues. They are not transforms, instead they describe
> properties of the image. Buffer scale is not a command for the
> compositor scale the image, instead it describes the scale of the image
> so that a compositor can present it in a suitable way anywhere. One
> could say buffer scale is only half of a transform, just like a color
> profile; source state.

OK.

> The combining cannot be fixed at commit. It happens on composition.

So I guess I'm wrong in understanding that commit triggers composition ?

<https://wayland.freedesktop.org/docs/html/ch04.html#sect-Protocol-Surface>:

"Once the client has finished writing pixels, it 'commits' the buffer; this permits the
compositor to access the buffer and read the pixels. When the compositor is finished, it
releases the buffer back to the client."

So how does this actually work ? (Is there documentation somewhere ?)

> Let's consider a case where a client has committed an image (wl_buffer +
> color properties etc.). That image is now in the wl_surface. The
> wl_surface gets shown on output 1, for that the compositor creates the
> necessary device link transformation and uses it. Then, the wl_surface
> gets moved by the compositor and shown on output 2. The compositor
> needs to create a new different device link transformation to do the
> color mapping. Therefore the device link transformation cannot be fixed
> at commit time - not even if you generated one for each output, because
> a new output could be hotplugged later.

And I wasn't assuming so, but I was assuming commit = compose, and that
the color transform would be determined at composition time. What
concerns me is that a surface doesn't represent the pixel value state
without explicit coordination from the client. This seems like
a poor model of how things actually are, and I'd really like
to get this right.

> Right now there is no Wayland protocol object that would directly
> represent "a frame of content" with all the properties it may have. It
> could have been useful, but maybe not avoid the need for
> wl_surface.commit because the set of needed state varies and new
> extensions will need a point to hook their state updates to.

Sure. But a color profile is not a property that is divorced
from the pixel values - it is intimately tied to them in a similar
way to pixel encoding, width & height or stride. Yet these are not
sent to the surface as separate values, which would be the case if
a wl_buffer was just a pointer to a chunk of shared memory.

[ If color profile is a property of the wl_surface, how does the
  screencopy protocol communicate the colorspace of the wl_buffer
  it returns ? It couldn't really return a wl_surface could it ? ]

> A
> wl_buffer is only a part of content - more of an artifact of memory
> management, and wl_surface is more than just the content... or is it?

I think I understand, but it seems that a wl_buffer is not quite
a chunk of memory, since it has graphic attributes as part of it,
and not quite a complete description of what the pixels mean.

> Not having a Wayland object for something does not prevent a compositor
> from using an internal object for it. E.g. weston has struct
> weston_surface_state.

Right. But it's a problem if one needs to reference that object
in the protocol so that the color profile can be set or retrieved.

> The EGL problem is in the client side: a client cannot attach anything
> to a wl_buffer if it uses EGL Wayland platform, because it cannot get
> to the wl_buffer at all.

Right, but the server would attach the color profile to the wl_buffer
on the clients behalf.

So:

1) Is it reasonable to tag a wl_buffer with the color profile to make it
   a more complete description of the pixel values, and implement a more
   natural model of how things are ?

2) How would this be implemented, since (AFAIK) the server wl_buffer structure
   is assumed to be private to the back end implementation ?
   (That's something I have a couple more more ideas about though.)

Thanks,
	Graeme Gill.