[RFC wayland-protocols 1/1] unstable: add color management protocol
Pekka Paalanen
ppaalanen at gmail.com
Thu Feb 28 12:51:35 UTC 2019
On Thu, 14 Feb 2019 03:46:21 +0100
Sebastian Wick <sebastian at sebastianwick.net> wrote:
> This protocol allows clients to attach a color space and rendering
> intent to a wl_surface. It further allows the client to be informed
> which color spaces a wl_surface was converted to on the last presented.
>
> Signed-off-by: Sebastian Wick <sebastian at sebastianwick.net>
Hi Sebastian,
thank you very much for working on this!
It would have been nice if you referred to Neils' proposal and maybe
compared this to that, especially if something here was modelled after
Neils' proposal.
Comments inline.
> ---
> Makefile.am | 1 +
> unstable/color-management/README | 4 +
> .../color-management-unstable-v1.xml | 183 ++++++++++++++++++
> 3 files changed, 188 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 at 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..1615fe6
> --- /dev/null
> +++ b/unstable/color-management/color-management-unstable-v1.xml
> @@ -0,0 +1,183 @@
> +<?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 provides the ability to specify the color space
> + of a wl_surface. If further enumerates the color spaces currently
> + in the system and allows to query feedback about which color spaces
> + a wl_surface was converted to on the last present.
> + The idea behind the feedback system is to allow the client to do color
> + conversion to a color space which will likely be used to show the surface
> + which allows the compositor to skip the color conversion step.
> + </description>
> +
> + <interface name="zwp_color_manager_v1" version="1">
> + <description summary="color manager">
> + The color manager is a singleton global object that provides access
> + to outputs' color spaces, let's you create new color spaces and attach
> + a color space to surfaces.
> + </description>
This interface is missing the destroy request.
> +
> + <enum name="render_intent">
> + <description summary="render intent">
> + render intents
> + </description>
> + <entry name="absolute" value="1"/>
> + <entry name="relative" value="2"/>
> + <entry name="perceptual" value="3"/>
> + <entry name="saturation" value="4" />
> + </enum>
> +
> + <enum name="well_known_color_space">
> + <description summary="well-known color spaces">
> + Well-known color spaces
Is a compositor required to send zwp_color_space_v1 objects for all of
these always?
Is the intention here that the compositor sends zwp_color_space_v1
objects for all those it happens to have the ICC profile at hand, and
if a client wants to use a yet another profile which is listed in this
enum, it still needs to provide it as an ICC profile file because the
compositor otherwise could not interpret it?
What if no output nor the compositor itself uses a profile listed here,
but it is still available to the compositor? E.g. sRGB when the
compositor only uses device specific profiles for all outputs and uses
something else as the blending/preferred color space?
> + </description>
> + <entry name="none" value="0" summary="no known color space" />
> + <entry name="sRGB" value="1" summary="sRGB color space" />
> + <entry name="adobeRGB" value="2" summary="Adobe RGB" />
> + <entry name="DCI-3P" value="3" summary="DCI-3P" />
> + <entry name="rec2020" value="4" summary="rec2020" />
> + <entry name="rec2020-pq" value="5" summary="rec2020 with pq curve (HDR)" />
> + <entry name="rec2020-hlg" value="6" summary="rec2020 with HLG curve (HDR)" />
Have you done a compile test with this? I would assume the dash with
produce illegal C. Names should be limited to the allowed C symbol
names.
It seems wayland-protocols doesn't actually try to compile the
resulting C code.
> + </enum>
> +
> + <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="set_color_space">
> + <description summary="set the color space of a surface">
> + Set the color space of a 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 conversions.
> +
> + If a surface has no color space set, sRGB and an arbitrary render
> + intent will be assumed.
> + </description>
> + <arg name="surface" type="object" interface="wl_surface"/>
> + <arg name="color_space" type="object" interface="zwp_color_space_v1"/>
> + <arg name="render_intent" type="uint" enum="render_intent"/>
The same comments as to Neils: this is one way to design this. Another
is to make the additional interface a new object which allows you to
hook up to the destruction of that object for resetting to original
state.
What happens when the zwp_color_space_v1 object has already emitted the
'removed' event, but the client has not seen it yet and issues this
request?
What happens with the surface, if the zwp_color_space_v1 object is
destroyed after this request?
What happens with the surface, if the zwp_color_manager_v1 object is
destroyed after this request?
> + </request>
> +
> + <request name="color_space_feedback">
> + <description summary="color space feedback">
> + Request color space feedback for the current content submission
> + on the given surface. This creates a new color_space_feedback
> + object, which will deliver the feedback information once. If
> + multiple color_space_feedback objects are created for the same
> + submission, they will all deliver the same information.
> +
> + For details on what information is returned, see the
> + color_space_feedback interface.
With this, there is a built-in assumption that the conversions happen
just once per content submission. I don't think that holds. The same
frame can be used repeatedly until the client provides a new one.
During that time, the window may move to another output or something
else changes in the compositor that changes what conversions were made
on latter repaints. The compositor's preferred color space is something
that can change at any time, not only when the client updates.
It might be more suitable to instead formulate this as an extension
object to wl_surface, that would deliver an event every time the
compositor's preferred color space changes. You could also put the
set_color_space request into that extension object and drop the
wl_surface argument.
> + </description>
> + <arg name="surface" type="object" interface="wl_surface"/>
> + <arg name="feedback" type="new_id" interface="zwp_color_space_feedback_v1"/>
> + </request>
> +
> + <request name="color_space_from_icc">
> + <description summary="create a color space from an ICC profile">
> + Create a color space from an ICC profile. Only three channel
> + profiles are allowed.
We already have pixel formats which have up to three color channels
plus alpha. Necessarily the ICC profile must agree with the pixel
format's number of color channels. Is that not a sufficient and
necessary restriction rather than just three channels?
> + </description>
> + <arg name="color_space" type="new_id" interface="zwp_color_space_v1"/>
> + <arg name="icc" type="fd"/>
I think with all fds, there should be at least length of the payload,
since this is something to be mmapped, not a pipe. Right?
Moreover, it would be good to define already here for every fd in the
protocols how they should be expected to be used by the receiver:
mmapped as read-only, with seals allowed but not required. The file
size must not be changed to smaller than the communicated size. The
sender of the fd must not change the contents after sending the fd.
> + </request>
> +
> + <event name="color_space_added">
> + <description summary="color space added">
> + Send after binding or when a new color space is added to the system.
> + </description>
> + <arg name="color_space" type="new_id" interface="zwp_color_space_v1"/>
A server-created object. This seems innocent enough, but if we look at
zwp_color_space_v1 interface, it defines an event to be sent
immediately the object is created and that event carries even a fd.
This could potentially result in a big flurry of events and fds every
time a client creates a zwp_color_manager_v1 object.
Was this event supposed to deliver all existing color space definitions
on zwp_color_manager_v1 creation automatically?
Is this sent for all color spaces added by clients as well
(color_space_from_icc request)? If yes, I think that has potential to
become denial-of-service attack vector. A malicious client adding a ton
of color spaces could easily cause all new clients to disconnect when
the compositor send buffer overflows on creating a zwp_color_manager_v1
object. It is even more prone because the initial events will carry a
fd per profile, and the number of fds is much more limited than
messages.
The sending also cannot be done in pieces, because there is currently
no way to freeze a request processing in libwayland. If you wanted to
send in pieces, that would have to be built in to the protocol
specification here in the form of a "done" event.
> + </event>
> + </interface>
> +
> + <interface name="zwp_color_space_v1" version="1">
Chris had some good comments on terminology. Is "color space" correct
here? "Color image encoding"?
I think the terminology needs to follow what color management
professionals use. Otherwise they will be confused and misinterpret
things, while the rest of us who learn these for the first time are
fine with any sufficiently unique terms and blissfully ignorant of the
details.
> + <description summary="a color space">
> + A color space can be attached to a wl_surface and sends
> + information like the ICC profile to let the client perform color
> + correction.
> + </description>
> +
> + <event name="information">
> + <description summary="color space information">
> + Information describing the color space.
> +
> + The icc argument contains a fd to the corresponding ICC profile.
> + well-known can be used to easily identify a color-space.
> +
> + A color space can be associated with a wl_output.
> + </description>
> + <arg name="icc" type="fd"/>
> + <arg name="well-known" type="uint" enum="well_known_color_space"/>
> + <arg name="output" type="object" interface="wl_output" allow-null="true"/>
Also argument names should be valid C symbol names I believe. Dash is
invalid.
I think this event needs to be split into three:
- fd and file size, to be sent only if the client is actually
interested in it. Wouldn't clients just ignore most of the ICC files
and use just the few that they actually need? The fd might not even
be necessary at all if well_known is set, right?
- well_known, but sent only if there actually is a matching value, no
need to send "none".
- zero/one or more outputs, can't the same color space apply to
multiple outputs? Most likely if the outputs are not actually
calibrated but use either the standard sRGB assumption or maybe an
ICC profile from then vendor rather than individually measured.
Btw. we're going to need multiple output events in any case, because
one client can bind the same wl_output global multiple times, producing
multiple wl_output objects all referring to the same output. I believe
the usual case when that happens is that the client is using libraries
that independently bind to wl_outputs. The compositor cannot know which
wl_output the client expects to see with the output event, so the
compositor must send output events for all of them.
Another problem with the wl_output argument here is that you implicitly
require clients to bind to wl_outputs before they bind to
zwp_color_manager_v1. Otherwise the clients do not have a wl_output
object the compositor could use in the event argument, and the client
will miss the association with an output. This could be solved by
delivering the color space event from an extension object explicitly
created for a wl_output.
> + </event>
> +
> + <event name="removed">
> + <description summary="color space removed">
> + This event is sent when the color space is removed from the system.
> + When this event is received, the client must destroy the object.
"Removed from the system" sounds ambiguous. What does it mean? Probably
something to do with the color spaces registered with the compositor
somehow?
Does this have any consequences to e.g. wl_surfaces where the color
space was attached to? Or outputs?
> + </description>
> + </event>
> +
> + <request name="destroy" type="destructor">
> + <description summary="destroy the color space object">
> + Destroy the color_space object.
The same question: does this have any consequences to e.g. wl_surfaces
where the color space was attached to? Or outputs?
> + </description>
> + </request>
> + </interface>
> +
> + <interface name="zwp_color_space_feedback_v1" version="1">
> + <description summary="color space feedback">
> + The surface content was converted to a number of color spaces
> + on the last content update. They get listed in decreasing order
> + of importance by the converted event.
What is the use case for delivering a list of color spaces, rather than
just one the compositor would have preferred? How would a client ever
pick or use anything but the first one?
> + Once a color_space_feedback object has delivered a done
> + event it is automatically destroyed.
> + </description>
> +
> + <event name="converted">
> + <description summary="color space conversion">
> + The surface was converted to a color space.
> + </description>
> + <arg name="color_space" type="object" interface="zwp_color_space_v1"/>
> + </event>
> +
> + <event name="done">
> + <description summary="done listing color space conversions">
> + All color space conversion have been listed.
> + </description>
> + </event>
> + </interface>
> +
> +</protocol>
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190228/1612e18f/attachment.sig>
More information about the wayland-devel
mailing list