[PATCH 2/6] Attach input profiles and build corresponding LUTs
Niels Ole Salscheider
niels_ole at salscheider-online.de
Thu Nov 27 06:58:51 PST 2014
Hi,
> first a general question, since I'm at loss here on the big picture:
> How does all this relate to the cms-static and cms-colord modules
> already in Weston?
>
> Are those modules only about configuring the output's color space?
> And then this work simply leverages that to have some output color
> spaces to work with?
>
> Hmm, cms-static/colord set the gamma ramps..? So the hardware LUT?
Yes, cms-static and cms-colord allow to get a color space for each output.
This is done by querying colord or by reading a static configuration from
weston.ini.
Without my patches, only the hardware LUT is programmed with the gamma ramp
from the corresponding profile. My patches reuse the existing modules to get
the profile data for each output but then to do full color correction.
> On Mon, 27 Oct 2014 18:54:06 +0100
>
> Niels Ole Salscheider <niels_ole at salscheider-online.de> wrote:
> > On Wednesday 15 October 2014, 21:54:37, Bryce Harrington wrote:
> > > On Mon, Oct 13, 2014 at 07:40:47PM +0200, Niels Ole Salscheider wrote:
> > > > This implements the functionality to attach a profile to a surface
> > > > in weston. An LUT is built from the profile that can be used to
> > > > transform colors from the input color space to the blending color
> > > > space.
> > > >
> > > > An sRGB color space is assumed for all newly created outputs
> > > >
> > > > This patch uses the sRGB color space as blending space because it
> > > > uses 8 bit LUTs for now and I want to avoid additional banding. In
> > > > the long term we should use a linear blending space though to get
> > > > correct results.
> > > >
> > > > Signed-off-by: Niels Ole Salscheider <niels_ole at salscheider-online.de>
> > > > ---
> > > >
> > > > Makefile.am | 3 +
> > > > configure.ac | 2 +-
> > > > src/compositor.c | 411
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > src/compositor.h
> > > >
> > > > | 41 ++++++
> > > >
> > > > 4 files changed, 453 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 3af3b46..1ecab56 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -45,6 +45,9 @@ weston_CPPFLAGS = $(AM_CPPFLAGS) -DIN_WESTON
> > > >
> > > > weston_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
> > > > $(LIBUNWIND_CFLAGS)
> > > > weston_LDADD = $(COMPOSITOR_LIBS) $(LIBUNWIND_LIBS) \
> > > >
> > > > $(DLOPEN_LIBS) -lm libshared.la
> > > >
> > > > +if HAVE_LCMS
> > > > +weston_LDADD += $(LCMS_LIBS)
> > > > +endif
> > > >
> > > > weston_SOURCES = \
> > > >
> > > > src/git-version.h \
> > > >
> > > > diff --git a/configure.ac b/configure.ac
> > > > index 1c133bd..00b7cca 100644
> > > > --- a/configure.ac
> > > > +++ b/configure.ac
> > > > @@ -59,7 +59,7 @@ AC_CHECK_HEADERS([execinfo.h])
> > > >
> > > > AC_CHECK_FUNCS([mkostemp strchrnul initgroups posix_fallocate])
> > > >
> > > > -COMPOSITOR_MODULES="wayland-server >= 1.5.91 pixman-1 >= 0.25.2"
> > > > +COMPOSITOR_MODULES="wayland-server >= 1.5.91 pixman-1 >= 0.25.2
> > > > glib-2.0"
> > >
> > > Depending on glib seems a pretty big step. I see it's needed for
> > > GHashTable, anything else?
> > >
> > > Wayland has a wl_map data structure; could that be used instead of
> > > GHashTable, in order to avoid the glib dependency?
>
> Yeah, we can't have an unconditional dependency to glib.
>
> > Yes, it is only needed for GHashTable. "cms-colord.c" already uses
> > GHashTable and therefore I used it in my code, too. But the use in
> > "cms-colord.c" only adds a dependency to glib if weston is build with
> > colord support, so this might be more acceptable.
> > I could use another map implementation, e. g. the one from wayland - but
> > then we would have to move it from "wayland-private.h" to some header
> > that gets installed.
>
> wl_map from wayland-private.h is not going to happen either. libwayland
> doesn't need a wl_map in its public API, so it has no place to export
> it either, even if it was generic.
>
> I think you need to copy in a hash table implementation, if it is
> needed regardless of... of... where is the switch to disable color
> management support?
It is enabled if weston is compiled with LCMS and disabled otherwise (or do
you see any reason why you would want to have LCMS but not full color
management?).
> Or well, just do what Jasper said.
Ok, I'll do so.
> > > > AC_ARG_ENABLE(egl, [ --disable-egl],,
> > > >
> > > > enable_egl=yes)
> > > >
> > > > diff --git a/src/compositor.c b/src/compositor.c
> > > > index 29731c7..4f959a4 100644
> > > > --- a/src/compositor.c
> > > > +++ b/src/compositor.c
> > > > @@ -56,6 +56,7 @@
> > > >
> > > > #include "compositor.h"
> > > > #include "scaler-server-protocol.h"
> > > > #include "presentation_timing-server-protocol.h"
> > > >
> > > > +#include "cms-server-protocol.h"
> > > >
> > > > #include "../shared/os-compatibility.h"
> > > > #include "git-version.h"
> > > > #include "version.h"
> > > >
> > > > @@ -518,7 +519,8 @@ surface_state_handle_buffer_destroy(struct
> > > > wl_listener
> > > > *listener, void *data)>
> > > >
> > > > }
> > > >
> > > > static void
> > > >
> > > > -weston_surface_state_init(struct weston_surface_state *state)
> > > > +weston_surface_state_init(struct weston_surface_state *state,
> > > > + struct weston_compositor *compositor)
> > > >
> > > > {
> > > >
> > > > state->newly_attached = 0;
> > > > state->buffer = NULL;
> > > >
> > > > @@ -539,6 +541,8 @@ weston_surface_state_init(struct
> > > > weston_surface_state
> > > > *state)>
> > > >
> > > > state->buffer_viewport.buffer.src_width = wl_fixed_from_int(-1);
> > > > state->buffer_viewport.surface.width = -1;
> > > > state->buffer_viewport.changed = 0;
> > > >
> > > > +
> > > > + state->colorspace = &compositor->srgb_colorspace;
> > > >
> > > > }
> > > >
> > > > static void
> > > >
> > > > @@ -576,6 +580,163 @@ weston_surface_state_set_buffer(struct
> > > > weston_surface_state *state,>
> > > >
> > > > &state->buffer_destroy_listener);
> > > >
> > > > }
> > > >
> > > > +#if HAVE_LCMS
>
> Instead of all the #ifdeffery, I'd really prefer to put most of this
> stuff into a separate .c file that gets built conditionally into weston.
>
> Then, if you also make a header that provides no-op stubs (where
> possible) when not HAVE_LCMS, you can reduce the #ifs even more. It's a
> bit of a trade-off and needs some judgement on what would be most
> readable.
Ok, I can do that.
> > > > diff --git a/src/compositor.h b/src/compositor.h
> > > > index 44379fe..5f198a9 100644
> > > > --- a/src/compositor.h
> > > > +++ b/src/compositor.h
> > > > @@ -31,6 +31,7 @@ extern "C" {
> > > >
> > > > #include <time.h>
> > > > #include <pixman.h>
> > > > #include <xkbcommon/xkbcommon.h>
> > > >
> > > > +#include <glib.h>
>
> Requiring glib to be able to build any external modules is not nice.
>
> > > > #define WL_HIDE_DEPRECATED
> > > > #include <wayland-server.h>
> > > >
> > > > @@ -40,6 +41,10 @@ extern "C" {
> > > >
> > > > #include "config-parser.h"
> > > > #include "zalloc.h"
> > > >
> > > > +#ifdef HAVE_LCMS
> > > > +#include <lcms2.h>
> > > > +#endif
> > > > +
> > > >
> > > > #ifndef MIN
> > > > #define MIN(x,y) (((x) < (y)) ? (x) : (y))
> > > > #endif
> > > >
> > > > @@ -179,6 +184,24 @@ enum weston_mode_switch_op {
> > > >
> > > > WESTON_MODE_SWITCH_RESTORE_NATIVE
> > > >
> > > > };
> > > >
> > > > +struct weston_clut {
> > > > + unsigned points;
> > > > + char *data;
> > > > +};
> > > > +
> > > > +struct weston_colorspace {
> > > > +#ifdef HAVE_LCMS
> > > > + cmsHPROFILE lcms_handle;
> > > > +#endif
> > > > + struct weston_clut clut;
> > > > +
> > > > + int destroyable;
> > > > + int refcount;
> > > > + int input;
> > > > +
> > > > + struct weston_compositor *compositor;
> > > > +};
>
> Note, that compositor.h is a public, installed header. You cannot use
> HAVE_LCMS, because any external project using this header would then
> receive the definition based on its own configuration, not how the
> Weston that is already installed was configured.
>
> > > > @@ -1410,6 +1445,12 @@ struct weston_view_animation *
> > > >
> > > > weston_slide_run(struct weston_view *view, float start, float stop,
> > > >
> > > > weston_view_animation_done_func_t done, void *data);
> > > >
> > > > +struct weston_colorspace *
> > > > +weston_colorspace_from_fd(int fd, int input, struct weston_compositor
> > > > *ec); +
> > > > +void
> > > > +weston_colorspace_destroy(struct weston_colorspace *colorspace);
>
> The definitions of these functions are inside #if HAVE_LCMS, but I
> supposed that does no harm. An external module using these would just
> fail to load, if Weston was built without LCMS.
>
>
> Thanks,
> pq
More information about the wayland-devel
mailing list