[RFC PATCH 1/6] Add colorcorrection protocol
Pekka Paalanen
ppaalanen at gmail.com
Mon Mar 31 07:10:36 PDT 2014
On Mon, 31 Mar 2014 11:29:03 +0200
Kai-Uwe Behrmann <ku.b-list at gmx.de> wrote:
> Am 31.03.2014 09:46, schrieb 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.
>
> Not sure if that can work in every form of session. E.g. a client side
> in memory generated profile might not be visible to the compositor.
Sure it can, just like wl_shm buffers work. Yes, the sender may have to
copy from a some malloc'd piece of memory into a mmap'd file, but
that's only one copy, compared to the copies with array here: into
protocol buffer, socket buffer, receiving socket buffer(?), receiving
protocol buffer, and maybe the 5th time into malloc'd memory.
> IMO what would make sense here, is to provide additional information
> about identifying the profile (ICC profile ID - a 16byte binary hash or
> 32 byte textual) and a mechanism skip or to request the not yet server
> cached profile from the client.
>
> [That could help clients, which want to show a collection of images each
> one colour corrected but most of the images have the same source profile.]
That would help for cross-client coalescing, sure. OTOH, if
transmission is zero-copy, the hashing could be an internal detail of
the server and would not need to specify it at the protocol level.
> > 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.
>
> Agreed, caching is certainly needed on both ends. The question is,
> should caching somehow be defined inside the protocol to avoid resending
> the same data? I think it would be helpful.
We can do that with just protocol objects, I think. Zoxc in irc had
some ideas: https://github.com/Zoxc/weston/blob/cms/protocol/cms.xml
wl_color_space object represents a profile.
> > 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.
>
> With per subsurface colour correction things are much more inviting to
> implementors. That situation is not remotly compareable to the time your
> early CM effort started. Even though I think it was a very helpful step.
Sorry, what has changed? Sub-surfaces are just wl_surfaces like any.
I you refer to my diagram, that was just a sketch, not even an
effort. :-)
> [ I had quite some trouble to implement the client side per window
> concept in CompICC / KWin and stalled it immediatly after reading about
> the subsurface plans. Given that very few people code at all on colour
> management compositor stuff, IMO it makes sense to do put the work in
> one place and lessen the reuirements to compositor implementors.]
What do you mean? On the Wayland protocol level, I see no difference
between a single-surface window and a sub-surface wrt. color management.
Were you trying to add some notion of "this sub-region is this color
space, the outside of it is other color space" kind of stuff?
> > 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.
>
> ... a no brainer for compositors.
Somehow when we were talking about color management a long time ago, I
understood that there are so many (wrong) ways to implement it, that it
is better left for the color-aware clients or libraries to do it
properly, so they get exactly what they want. Was that a
misunderstanding, or maybe I just remember wrong?
Thanks,
pq
> > Is that a reasonable requirement for all compositors that support
> > per-output ICC profiles?
>
> CM with per subsurface shaders is much easier to implement for
> compositors. Compared to that the code paths without subsurfaces is
> considerable in CompICC and KWin/KolorManager.
>
> > 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.
>
> Such a protocol is helpful for extending over the most basic
> capabilites. Oyranos use it in several parts of the stack to react to
> divergine colour server feature sets.
>
> > 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?
>
> linear + sRGB primaries is non ambiguous for floating point buffers.
>
> > Thanks,
> > pq
>
> Niels, thanks for you work.
>
> kind regards
> Kai-Uwe
More information about the wayland-devel
mailing list