[PATCH 2/6] Attach input profiles and build corresponding LUTs

[Pekka Paalanen]

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?


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?

Or well, just do what Jasper said.

> > >  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.


> > > 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