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

Submitted by Niels Ole Salscheider on Oct. 13, 2014, 5:40 p.m.

Details

Message ID 1413222051-4637-3-git-send-email-niels_ole@salscheider-online.de
State Changes Requested
Headers show

Not browsing as part of any series.

Commit Message

Niels Ole Salscheider Oct. 13, 2014, 5:40 p.m.
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@salscheider-online.de>
---
 Makefile.am      |   3 +
 configure.ac     |   2 +-
 src/compositor.c | 411 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/compositor.h |  41 ++++++
 4 files changed, 453 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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"
 
 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
+static void
+weston_free_clut(struct weston_clut *clut)
+{
+	clut->points = 0;
+	free(clut->data);
+	clut->data = NULL;
+}
+
+static void
+weston_build_clut(struct weston_colorspace *colorspace)
+{
+	cmsHTRANSFORM xform;
+	unsigned r, g, b;
+	cmsHPROFILE input_profile, output_profile;
+	cmsUInt8Number input_id[16];
+	cmsUInt8Number output_id[16];
+	struct weston_clut *clut = &colorspace->clut;
+	struct weston_compositor *ec = colorspace->compositor;
+
+	weston_free_clut(clut);
+
+	if (colorspace->input) {
+		input_profile = colorspace->lcms_handle;
+		output_profile = ec->blending_colorspace.lcms_handle;
+	} else {
+		input_profile = ec->blending_colorspace.lcms_handle;
+		output_profile = colorspace->lcms_handle;
+	}
+
+	cmsGetHeaderProfileID(input_profile, input_id);
+	cmsGetHeaderProfileID(output_profile, output_id);
+	if (memcmp(input_id, output_id, 16) == 0)
+		return;
+
+	clut->points = 64;
+
+	xform = cmsCreateTransform(input_profile, TYPE_RGB_8, output_profile,
+				   TYPE_RGB_8, INTENT_PERCEPTUAL, 0);
+
+	clut->data = malloc(clut->points * clut->points * clut->points *
+			    3 * sizeof(char));
+
+	for (b = 0; b < clut->points; b++)
+		for (g = 0; g < clut->points; g++)
+			for (r = 0; r < clut->points; r++) {
+				char in[3];
+				unsigned entry = 3 * (r + clut->points *
+						 (g + clut->points * b));
+				const float step = 255.0 / (clut->points - 1);
+				in[0] = (char) (r * step + 0.5);
+				in[1] = (char) (g * step + 0.5);
+				in[2] = (char) (b * step + 0.5);
+				cmsDoTransform(xform, in,
+					       &clut->data[entry], 1);
+			}
+
+	cmsDeleteTransform(xform);
+}
+
+WL_EXPORT struct weston_colorspace *
+weston_colorspace_from_fd(int fd, int input, struct weston_compositor *ec)
+{
+	FILE *profile;
+	cmsHPROFILE lcms_handle;
+	cmsUInt8Number profile_id[17];
+	struct weston_colorspace *colorspace;
+
+	profile = fdopen(fd, "r");
+	if (!profile) {
+		return 0;
+	}
+
+	lcms_handle = cmsOpenProfileFromStream(profile, "r");
+	if (!lcms_handle) {
+		return 0;
+	}
+
+	cmsMD5computeID(lcms_handle);
+	cmsGetHeaderProfileID(lcms_handle, profile_id);
+	profile_id[16] = '\0';
+
+	if (input)
+		colorspace = g_hash_table_lookup(ec->input_colorspaces,
+						 profile_id);
+	else
+		colorspace = g_hash_table_lookup(ec->output_colorspaces,
+						 profile_id);
+	if (colorspace) {
+		colorspace->refcount++;
+
+		cmsCloseProfile(lcms_handle);
+		return colorspace;
+	}
+
+	colorspace = calloc(1, sizeof(struct weston_colorspace));
+	colorspace->refcount = 1;
+	colorspace->destroyable = 1;
+	colorspace->input = input;
+	colorspace->lcms_handle = lcms_handle;
+	colorspace->compositor = ec;
+
+	weston_build_clut(colorspace);
+
+	g_hash_table_insert(ec->input_colorspaces,
+			    g_strdup((const gchar *) profile_id),
+			    colorspace);
+
+	return colorspace;
+}
+
+/* This function is called when a color space is removed from the hash table */
+static void
+weston_colorspace_free(struct weston_colorspace *colorspace)
+{
+	if (!colorspace->destroyable)
+		return;
+
+	cmsCloseProfile(colorspace->lcms_handle);
+
+	weston_free_clut(&colorspace->clut);
+	free(colorspace);
+}
+
+WL_EXPORT void
+weston_colorspace_destroy(struct weston_colorspace *colorspace)
+{
+	cmsUInt8Number profile_id[17];
+
+	if (!colorspace->destroyable)
+		return;
+
+	colorspace->refcount--;
+	if (colorspace->refcount > 0)
+		return;
+
+	cmsGetHeaderProfileID(colorspace->lcms_handle, profile_id);
+	profile_id[16] = '\0';
+
+	if (colorspace->input)
+		g_hash_table_remove(colorspace->compositor->input_colorspaces,
+				    profile_id);
+	else
+		g_hash_table_remove(colorspace->compositor->output_colorspaces,
+				    profile_id);
+
+}
+
+static void
+weston_colorspace_resource_destroy(struct wl_resource *cms_colorspace)
+{
+	struct weston_colorspace *colorspace =
+		wl_resource_get_user_data(cms_colorspace);
+	weston_colorspace_destroy(colorspace);
+}
+#endif
+
 WL_EXPORT struct weston_surface *
 weston_surface_create(struct weston_compositor *compositor)
 {
@@ -597,7 +758,9 @@  weston_surface_create(struct weston_compositor *compositor)
 	surface->buffer_viewport.buffer.src_width = wl_fixed_from_int(-1);
 	surface->buffer_viewport.surface.width = -1;
 
-	weston_surface_state_init(&surface->pending);
+	surface->colorspace = &compositor->srgb_colorspace;
+
+	weston_surface_state_init(&surface->pending, compositor);
 
 	surface->output = NULL;
 
@@ -615,6 +778,7 @@  weston_surface_create(struct weston_compositor *compositor)
 	wl_list_init(&surface->subsurface_list);
 	wl_list_init(&surface->subsurface_list_pending);
 
+
 	return surface;
 }
 
@@ -2369,6 +2533,8 @@  weston_surface_commit_state(struct weston_surface *surface,
 	wl_list_insert_list(&surface->feedback_list,
 			    &state->feedback_list);
 	wl_list_init(&state->feedback_list);
+
+	surface->colorspace = state->colorspace;
 }
 
 static void
@@ -2609,6 +2775,8 @@  weston_subsurface_commit_to_cache(struct weston_subsurface *sub)
 	sub->cached.buffer_viewport.surface =
 		surface->pending.buffer_viewport.surface;
 
+	sub->cached.colorspace = surface->colorspace;
+
 	weston_surface_reset_pending_buffer(surface);
 
 	pixman_region32_copy(&sub->cached.opaque, &surface->pending.opaque);
@@ -3041,7 +3209,7 @@  weston_subsurface_create(uint32_t id, struct weston_surface *surface,
 				       sub, subsurface_resource_destroy);
 	weston_subsurface_link_surface(sub, surface);
 	weston_subsurface_link_parent(sub, parent);
-	weston_surface_state_init(&sub->cached);
+	weston_surface_state_init(&sub->cached, surface->compositor);
 	sub->cached_buffer_ref.buffer = NULL;
 	sub->synchronized = 1;
 
@@ -3363,6 +3531,10 @@  weston_output_destroy(struct weston_output *output)
 	wl_signal_emit(&output->compositor->output_destroyed_signal, output);
 	wl_signal_emit(&output->destroy_signal, output);
 
+#ifdef HAVE_LCMS
+	weston_colorspace_destroy(output->colorspace);
+#endif
+
 	free(output->name);
 	pixman_region32_fini(&output->region);
 	pixman_region32_fini(&output->previous_damage);
@@ -3554,6 +3726,7 @@  weston_output_init(struct weston_output *output, struct weston_compositor *c,
 	output->mm_height = mm_height;
 	output->dirty = 1;
 	output->original_scale = scale;
+	output->colorspace = &c->srgb_colorspace;
 
 	weston_output_transform_scale_init(output, transform, scale);
 	weston_output_init_zoom(output);
@@ -3917,6 +4090,181 @@  bind_presentation(struct wl_client *client,
 	presentation_send_clock_id(resource, compositor->presentation_clock);
 }
 
+#ifdef HAVE_LCMS
+static void
+cms_colorspace_destroy(struct wl_client *client,
+		       struct wl_resource *cms_colorspace)
+{
+	wl_resource_destroy(cms_colorspace);
+}
+
+static void
+cms_colorspace_get_profile_fd(struct wl_client *client,
+			      struct wl_resource *cms_colorspace)
+{
+	struct weston_colorspace *colorspace =
+		wl_resource_get_user_data(cms_colorspace);
+	char template[24];
+	FILE *profilefile;
+	int fd = -1;
+
+	strncpy(template, "/tmp/weston-cms-XXXXXX", 24);
+	fd = mkstemp(template);
+	profilefile = fdopen(fd, "w");
+	cmsSaveProfileToStream(colorspace->lcms_handle, profilefile);
+	rewind(profilefile);
+
+	wl_cms_colorspace_send_profile_data(cms_colorspace, fd);
+
+	close(fd);
+}
+
+static const struct wl_cms_colorspace_interface cms_colorspace_interface = {
+	cms_colorspace_destroy,
+	cms_colorspace_get_profile_fd
+};
+
+static void
+cms_set_colorspace(struct wl_client *client, struct wl_resource *cms,
+		   struct wl_resource *surface_resource,
+		   struct wl_resource *colorspace_resource)
+{
+	struct weston_surface *surface =
+		wl_resource_get_user_data(surface_resource);
+
+	struct weston_colorspace *colorspace =
+		wl_resource_get_user_data(colorspace_resource);
+	surface->pending.colorspace = colorspace;
+}
+
+static void
+cms_colorspace_from_fd(struct wl_client *client, struct wl_resource *cms,
+		       int fd, uint32_t id)
+{
+	struct weston_compositor *compositor =
+		wl_resource_get_user_data(cms);
+
+	struct weston_colorspace *colorspace;
+	struct wl_resource *colorspace_resource;
+
+	colorspace = weston_colorspace_from_fd(fd, 1, compositor);
+	if (colorspace == NULL) {
+		wl_resource_post_error(cms,
+				       WL_CMS_ERROR_INVALID_PROFILE,
+				       "the passed ICC profile is not valid");
+		return;
+	}
+
+	colorspace_resource = wl_resource_create(client,
+						 &wl_cms_colorspace_interface,
+						 1, id);
+	if (colorspace_resource == NULL) {
+		weston_colorspace_destroy(colorspace);
+		wl_client_post_no_memory(client);
+		return;
+	}
+	wl_resource_set_implementation(colorspace_resource,
+				       &cms_colorspace_interface, colorspace,
+				       weston_colorspace_resource_destroy);
+}
+
+static void
+cms_output_colorspace(struct wl_client *client, struct wl_resource *cms,
+		      struct wl_resource *output_resource, uint32_t id)
+{
+	struct weston_output *output =
+		wl_resource_get_user_data(output_resource);
+
+	struct weston_colorspace *colorspace;
+	struct wl_resource *colorspace_resource;
+
+	colorspace = output->colorspace;
+	colorspace->refcount++;
+
+	colorspace_resource = wl_resource_create(client,
+						 &wl_cms_colorspace_interface,
+						 1, id);
+	if (colorspace_resource == NULL) {
+		weston_colorspace_destroy(colorspace);
+		wl_client_post_no_memory(client);
+		return;
+	}
+	wl_resource_set_implementation(colorspace_resource,
+				       &cms_colorspace_interface, colorspace,
+				       weston_colorspace_resource_destroy);
+}
+
+static void
+cms_srgb_colorspace(struct wl_client *client, struct wl_resource *cms,
+		    uint32_t id)
+{
+	struct weston_compositor *compositor =
+		wl_resource_get_user_data(cms);
+	struct weston_colorspace *colorspace;
+	struct wl_resource *colorspace_resource;
+
+	colorspace = &compositor->srgb_colorspace;
+
+	colorspace_resource = wl_resource_create(client,
+						 &wl_cms_colorspace_interface,
+						 1, id);
+	if (colorspace_resource == NULL) {
+		weston_colorspace_destroy(colorspace);
+		wl_client_post_no_memory(client);
+		return;
+	}
+	wl_resource_set_implementation(colorspace_resource,
+				       &cms_colorspace_interface, colorspace,
+				       weston_colorspace_resource_destroy);
+}
+
+static void
+cms_blending_colorspace(struct wl_client *client, struct wl_resource *cms,
+			uint32_t id)
+{
+	struct weston_compositor *compositor =
+		wl_resource_get_user_data(cms);
+	struct weston_colorspace *colorspace;
+	struct wl_resource *colorspace_resource;
+
+	colorspace = &compositor->blending_colorspace;
+
+	colorspace_resource = wl_resource_create(client,
+						 &wl_cms_colorspace_interface,
+						 1, id);
+	if (colorspace_resource == NULL) {
+		weston_colorspace_destroy(colorspace);
+		wl_client_post_no_memory(client);
+		return;
+	}
+	wl_resource_set_implementation(colorspace_resource,
+				       &cms_colorspace_interface, colorspace,
+				       weston_colorspace_resource_destroy);
+}
+
+static const struct wl_cms_interface cms_interface = {
+	cms_set_colorspace,
+	cms_colorspace_from_fd,
+	cms_output_colorspace,
+	cms_srgb_colorspace,
+	cms_blending_colorspace
+};
+
+static void
+bind_cms(struct wl_client *client, void *data, uint32_t version, uint32_t id)
+{
+	struct wl_resource *resource;
+
+	resource = wl_resource_create(client, &wl_cms_interface, 1, id);
+	if (resource == NULL) {
+		wl_client_post_no_memory(client);
+		return;
+	}
+
+	wl_resource_set_implementation(resource, &cms_interface, data, NULL);
+}
+#endif
+
 static void
 compositor_bind(struct wl_client *client,
 		void *data, uint32_t version, uint32_t id)
@@ -4016,6 +4364,12 @@  weston_compositor_init(struct weston_compositor *ec,
 			      ec, bind_presentation))
 		return -1;
 
+#ifdef HAVE_LCMS
+	if (!wl_global_create(display, &wl_cms_interface, 1,
+			      ec, bind_cms))
+		return -1;
+#endif
+
 	wl_list_init(&ec->view_list);
 	wl_list_init(&ec->plane_list);
 	wl_list_init(&ec->layer_list);
@@ -4600,6 +4954,10 @@  int main(int argc, char *argv[])
 	struct wl_client *primary_client;
 	struct wl_listener primary_client_destroyed;
 	struct weston_seat *seat;
+#ifdef HAVE_LCMS
+	cmsUInt8Number srgb_id[17];
+	cmsUInt8Number blending_id[17];
+#endif
 
 	const struct weston_option core_options[] = {
 		{ WESTON_OPTION_STRING, "backend", 'B', &backend },
@@ -4690,6 +5048,45 @@  int main(int argc, char *argv[])
 	ec->idle_time = idle_time;
 	ec->default_pointer_grab = NULL;
 
+#ifdef HAVE_LCMS
+	ec->input_colorspaces =
+		g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
+				      (GDestroyNotify) weston_colorspace_free);
+	ec->output_colorspaces =
+		g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
+				      (GDestroyNotify) weston_colorspace_free);
+
+	ec->srgb_colorspace.destroyable = 0;
+	ec->srgb_colorspace.input = 1;
+	ec->srgb_colorspace.compositor = ec;
+	ec->blending_colorspace.destroyable = 0;
+	ec->blending_colorspace.input = 1;
+	ec->blending_colorspace.compositor = ec;
+
+	ec->srgb_colorspace.lcms_handle = cmsCreate_sRGBProfile();
+	/* TODO: We should really use a linear blending space. But this would
+	 * cause additional banding since we only use an 8 bit LUT for now. */
+	ec->blending_colorspace.lcms_handle = cmsCreate_sRGBProfile();
+
+	weston_build_clut(&ec->srgb_colorspace);
+	/* This is unnecessary but will initialize everything with 0 */
+	weston_build_clut(&ec->blending_colorspace);
+
+	cmsMD5computeID(ec->srgb_colorspace.lcms_handle);
+	cmsMD5computeID(ec->blending_colorspace.lcms_handle);
+	cmsGetHeaderProfileID(ec->srgb_colorspace.lcms_handle, srgb_id);
+	cmsGetHeaderProfileID(ec->blending_colorspace.lcms_handle, blending_id);
+	srgb_id[16] = '\0';
+	blending_id[16] = '\0';
+
+	g_hash_table_insert(ec->input_colorspaces,
+			    g_strdup((const gchar *) srgb_id),
+			    &ec->srgb_colorspace);
+	g_hash_table_insert(ec->input_colorspaces,
+			    g_strdup((const gchar *) blending_id),
+			    &ec->srgb_colorspace);
+#endif
+
 	for (i = 1; i < argc; i++)
 		weston_log("fatal: unhandled option: %s\n", argv[i]);
 	if (argc > 1) {
@@ -4760,6 +5157,14 @@  out:
 
 	wl_signal_emit(&ec->destroy_signal, ec);
 
+#ifdef HAVE_LCMS
+	g_hash_table_unref(ec->input_colorspaces);
+	g_hash_table_unref(ec->output_colorspaces);
+
+	weston_free_clut(&ec->srgb_colorspace.clut);
+	/* The blending colorspace cannot have a clut */
+#endif
+
 	weston_compositor_xkb_destroy(ec);
 
 	ec->destroy(ec);
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>
 
 #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;
+};
+
 struct weston_output {
 	uint32_t id;
 	char *name;
@@ -220,6 +243,8 @@  struct weston_output {
 	struct weston_mode *original_mode;
 	struct wl_list mode_list;
 
+	struct weston_colorspace *colorspace;
+
 	void (*start_repaint_loop)(struct weston_output *output);
 	int (*repaint)(struct weston_output *output,
 			pixman_region32_t *damage);
@@ -668,6 +693,11 @@  struct weston_compositor {
 	int32_t kb_repeat_delay;
 
 	clockid_t presentation_clock;
+
+	struct weston_colorspace srgb_colorspace;
+	struct weston_colorspace blending_colorspace;
+	GHashTable *input_colorspaces;
+	GHashTable *output_colorspaces;
 };
 
 struct weston_buffer {
@@ -844,6 +874,9 @@  struct weston_surface_state {
 	/* wl_surface.set_scaling_factor */
 	/* wl_viewport.set */
 	struct weston_buffer_viewport buffer_viewport;
+
+	/* wl_cms.set_colorspace*/
+	struct weston_colorspace *colorspace;
 };
 
 struct weston_surface {
@@ -887,6 +920,8 @@  struct weston_surface {
 	int32_t height_from_buffer;
 	int keep_buffer; /* bool for backends to prevent early release */
 
+	struct weston_colorspace *colorspace;
+
 	/* wl_viewport resource for this surface */
 	struct wl_resource *viewport_resource;
 
@@ -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);
+
 void
 weston_surface_set_color(struct weston_surface *surface,
 			 float red, float green, float blue, float alpha);

Comments

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@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?
  
>  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
> +static void
> +weston_free_clut(struct weston_clut *clut)
> +{
> +	clut->points = 0;
> +	free(clut->data);
> +	clut->data = NULL;
> +}
> +
> +static void
> +weston_build_clut(struct weston_colorspace *colorspace)
> +{
> +	cmsHTRANSFORM xform;
> +	unsigned r, g, b;
> +	cmsHPROFILE input_profile, output_profile;
> +	cmsUInt8Number input_id[16];
> +	cmsUInt8Number output_id[16];
> +	struct weston_clut *clut = &colorspace->clut;
> +	struct weston_compositor *ec = colorspace->compositor;
> +
> +	weston_free_clut(clut);
> +
> +	if (colorspace->input) {
> +		input_profile = colorspace->lcms_handle;
> +		output_profile = ec->blending_colorspace.lcms_handle;
> +	} else {
> +		input_profile = ec->blending_colorspace.lcms_handle;
> +		output_profile = colorspace->lcms_handle;
> +	}
> +
> +	cmsGetHeaderProfileID(input_profile, input_id);
> +	cmsGetHeaderProfileID(output_profile, output_id);
> +	if (memcmp(input_id, output_id, 16) == 0)
> +		return;

Don't hide the error here, propagate it by returning false.
And obviously all callers of this routine should check it.  :-)

> +	clut->points = 64;

Does this ever change to anything besides 64?  If not, wouldn't this be
better as a constant?

> +	xform = cmsCreateTransform(input_profile, TYPE_RGB_8, output_profile,
> +				   TYPE_RGB_8, INTENT_PERCEPTUAL, 0);

Looks like cmsCreateTransform has a return value that can be checked for
NULL to detect errors.  Looks like it signals some errors that could be
propagated too.

> +	clut->data = malloc(clut->points * clut->points * clut->points *
> +			    3 * sizeof(char));

Needs error check here too.

> +	for (b = 0; b < clut->points; b++)
> +		for (g = 0; g < clut->points; g++)
> +			for (r = 0; r < clut->points; r++) {
> +				char in[3];
> +				unsigned entry = 3 * (r + clut->points *
> +						 (g + clut->points * b));
> +				const float step = 255.0 / (clut->points - 1);
> +				in[0] = (char) (r * step + 0.5);
> +				in[1] = (char) (g * step + 0.5);
> +				in[2] = (char) (b * step + 0.5);
> +				cmsDoTransform(xform, in,
> +					       &clut->data[entry], 1);
> +			}
> +
> +	cmsDeleteTransform(xform);
> +}
> +
> +WL_EXPORT struct weston_colorspace *
> +weston_colorspace_from_fd(int fd, int input, struct weston_compositor *ec)
> +{
> +	FILE *profile;
> +	cmsHPROFILE lcms_handle;
> +	cmsUInt8Number profile_id[17];
> +	struct weston_colorspace *colorspace;
> +
> +	profile = fdopen(fd, "r");
> +	if (!profile) {
> +		return 0;

Should return NULL explicitly rather than 0 here.

> +	}
> +
> +	lcms_handle = cmsOpenProfileFromStream(profile, "r");
> +	if (!lcms_handle) {
> +		return 0;
> +	}

Same here.  In fact you may want to do a goto error handler, so you can
close the fd and other cleanup.

> +	cmsMD5computeID(lcms_handle);

Check return status for FALSE on this one.

> +	cmsGetHeaderProfileID(lcms_handle, profile_id);
> +	profile_id[16] = '\0';

Google isn't giving up an API definition on this one so don't know if it
returns an error code.

> +	if (input)
> +		colorspace = g_hash_table_lookup(ec->input_colorspaces,
> +						 profile_id);
> +	else
> +		colorspace = g_hash_table_lookup(ec->output_colorspaces,
> +						 profile_id);
> +	if (colorspace) {
> +		colorspace->refcount++;
> +
> +		cmsCloseProfile(lcms_handle);
> +		return colorspace;
> +	}
> +
> +	colorspace = calloc(1, sizeof(struct weston_colorspace));

Check for NULL on the calloc

> +	colorspace->refcount = 1;
> +	colorspace->destroyable = 1;
> +	colorspace->input = input;
> +	colorspace->lcms_handle = lcms_handle;
> +	colorspace->compositor = ec;
> +
> +	weston_build_clut(colorspace);

Check error return for weston_build_clut.

> +	g_hash_table_insert(ec->input_colorspaces,
> +			    g_strdup((const gchar *) profile_id),
> +			    colorspace);
> +
> +	return colorspace;
> +}
> +
> +/* This function is called when a color space is removed from the hash table */
> +static void
> +weston_colorspace_free(struct weston_colorspace *colorspace)
> +{
> +	if (!colorspace->destroyable)
> +		return;

No chance this can't result in memory leakage?

> +	cmsCloseProfile(colorspace->lcms_handle);
> +
> +	weston_free_clut(&colorspace->clut);
> +	free(colorspace);
> +}
> +
> +WL_EXPORT void
> +weston_colorspace_destroy(struct weston_colorspace *colorspace)
> +{
> +	cmsUInt8Number profile_id[17];
> +
> +	if (!colorspace->destroyable)
> +		return;
> +
> +	colorspace->refcount--;
> +	if (colorspace->refcount > 0)
> +		return;
> +
> +	cmsGetHeaderProfileID(colorspace->lcms_handle, profile_id);
> +	profile_id[16] = '\0';
> +
> +	if (colorspace->input)
> +		g_hash_table_remove(colorspace->compositor->input_colorspaces,
> +				    profile_id);
> +	else
> +		g_hash_table_remove(colorspace->compositor->output_colorspaces,
> +				    profile_id);
> +
> +}

Why is there both a *_free() and a *_destroy() for weston_colorspace?

> +static void
> +weston_colorspace_resource_destroy(struct wl_resource *cms_colorspace)
> +{
> +	struct weston_colorspace *colorspace =
> +		wl_resource_get_user_data(cms_colorspace);
> +	weston_colorspace_destroy(colorspace);
> +}
> +#endif
> +
>  WL_EXPORT struct weston_surface *
>  weston_surface_create(struct weston_compositor *compositor)
>  {
> @@ -597,7 +758,9 @@ weston_surface_create(struct weston_compositor *compositor)
>  	surface->buffer_viewport.buffer.src_width = wl_fixed_from_int(-1);
>  	surface->buffer_viewport.surface.width = -1;
>  
> -	weston_surface_state_init(&surface->pending);
> +	surface->colorspace = &compositor->srgb_colorspace;
> +
> +	weston_surface_state_init(&surface->pending, compositor);
>  
>  	surface->output = NULL;
>  
> @@ -615,6 +778,7 @@ weston_surface_create(struct weston_compositor *compositor)
>  	wl_list_init(&surface->subsurface_list);
>  	wl_list_init(&surface->subsurface_list_pending);
>  
> +
>  	return surface;

Extraneous newline?

>  }
>  
> @@ -2369,6 +2533,8 @@ weston_surface_commit_state(struct weston_surface *surface,
>  	wl_list_insert_list(&surface->feedback_list,
>  			    &state->feedback_list);
>  	wl_list_init(&state->feedback_list);
> +
> +	surface->colorspace = state->colorspace;
>  }
>  
>  static void
> @@ -2609,6 +2775,8 @@ weston_subsurface_commit_to_cache(struct weston_subsurface *sub)
>  	sub->cached.buffer_viewport.surface =
>  		surface->pending.buffer_viewport.surface;
>  
> +	sub->cached.colorspace = surface->colorspace;
> +
>  	weston_surface_reset_pending_buffer(surface);
>  
>  	pixman_region32_copy(&sub->cached.opaque, &surface->pending.opaque);
> @@ -3041,7 +3209,7 @@ weston_subsurface_create(uint32_t id, struct weston_surface *surface,
>  				       sub, subsurface_resource_destroy);
>  	weston_subsurface_link_surface(sub, surface);
>  	weston_subsurface_link_parent(sub, parent);
> -	weston_surface_state_init(&sub->cached);
> +	weston_surface_state_init(&sub->cached, surface->compositor);
>  	sub->cached_buffer_ref.buffer = NULL;
>  	sub->synchronized = 1;
>  
> @@ -3363,6 +3531,10 @@ weston_output_destroy(struct weston_output *output)
>  	wl_signal_emit(&output->compositor->output_destroyed_signal, output);
>  	wl_signal_emit(&output->destroy_signal, output);
>  
> +#ifdef HAVE_LCMS
> +	weston_colorspace_destroy(output->colorspace);
> +#endif
> +
>  	free(output->name);
>  	pixman_region32_fini(&output->region);
>  	pixman_region32_fini(&output->previous_damage);
> @@ -3554,6 +3726,7 @@ weston_output_init(struct weston_output *output, struct weston_compositor *c,
>  	output->mm_height = mm_height;
>  	output->dirty = 1;
>  	output->original_scale = scale;
> +	output->colorspace = &c->srgb_colorspace;
>  
>  	weston_output_transform_scale_init(output, transform, scale);
>  	weston_output_init_zoom(output);
> @@ -3917,6 +4090,181 @@ bind_presentation(struct wl_client *client,
>  	presentation_send_clock_id(resource, compositor->presentation_clock);
>  }
>  
> +#ifdef HAVE_LCMS
> +static void
> +cms_colorspace_destroy(struct wl_client *client,
> +		       struct wl_resource *cms_colorspace)
> +{
> +	wl_resource_destroy(cms_colorspace);
> +}
> +
> +static void
> +cms_colorspace_get_profile_fd(struct wl_client *client,
> +			      struct wl_resource *cms_colorspace)
> +{
> +	struct weston_colorspace *colorspace =
> +		wl_resource_get_user_data(cms_colorspace);
> +	char template[24];
> +	FILE *profilefile;
> +	int fd = -1;
> +
> +	strncpy(template, "/tmp/weston-cms-XXXXXX", 24);
> +	fd = mkstemp(template);
> +	profilefile = fdopen(fd, "w");
> +	cmsSaveProfileToStream(colorspace->lcms_handle, profilefile);
> +	rewind(profilefile);
> +
> +	wl_cms_colorspace_send_profile_data(cms_colorspace, fd);
> +
> +	close(fd);
> +}
> +
> +static const struct wl_cms_colorspace_interface cms_colorspace_interface = {
> +	cms_colorspace_destroy,
> +	cms_colorspace_get_profile_fd
> +};
> +
> +static void
> +cms_set_colorspace(struct wl_client *client, struct wl_resource *cms,
> +		   struct wl_resource *surface_resource,
> +		   struct wl_resource *colorspace_resource)
> +{
> +	struct weston_surface *surface =
> +		wl_resource_get_user_data(surface_resource);

wl_resource_get_user_data() returns a void*; does this need a cast?
Also, I can't tell whether it's guaranteed not to return NULL or not.
If it can return NULL, better check for it here.

> +
> +	struct weston_colorspace *colorspace =
> +		wl_resource_get_user_data(colorspace_resource);
> +	surface->pending.colorspace = colorspace;
> +}
> +
> +static void
> +cms_colorspace_from_fd(struct wl_client *client, struct wl_resource *cms,
> +		       int fd, uint32_t id)
> +{
> +	struct weston_compositor *compositor =
> +		wl_resource_get_user_data(cms);
> +
> +	struct weston_colorspace *colorspace;
> +	struct wl_resource *colorspace_resource;
> +
> +	colorspace = weston_colorspace_from_fd(fd, 1, compositor);
> +	if (colorspace == NULL) {
> +		wl_resource_post_error(cms,
> +				       WL_CMS_ERROR_INVALID_PROFILE,
> +				       "the passed ICC profile is not valid");
> +		return;

Should return false in case of an error.

> +	}
> +
> +	colorspace_resource = wl_resource_create(client,
> +						 &wl_cms_colorspace_interface,
> +						 1, id);
> +	if (colorspace_resource == NULL) {
> +		weston_colorspace_destroy(colorspace);
> +		wl_client_post_no_memory(client);
> +		return;

ditto

> +	}
> +	wl_resource_set_implementation(colorspace_resource,
> +				       &cms_colorspace_interface, colorspace,
> +				       weston_colorspace_resource_destroy);
> +}
> +
> +static void
> +cms_output_colorspace(struct wl_client *client, struct wl_resource *cms,
> +		      struct wl_resource *output_resource, uint32_t id)
> +{
> +	struct weston_output *output =
> +		wl_resource_get_user_data(output_resource);
> +
> +	struct weston_colorspace *colorspace;
> +	struct wl_resource *colorspace_resource;
> +
> +	colorspace = output->colorspace;
> +	colorspace->refcount++;
> +
> +	colorspace_resource = wl_resource_create(client,
> +						 &wl_cms_colorspace_interface,
> +						 1, id);
> +	if (colorspace_resource == NULL) {
> +		weston_colorspace_destroy(colorspace);
> +		wl_client_post_no_memory(client);
> +		return;
> +	}
> +	wl_resource_set_implementation(colorspace_resource,
> +				       &cms_colorspace_interface, colorspace,
> +				       weston_colorspace_resource_destroy);
> +}
> +
> +static void
> +cms_srgb_colorspace(struct wl_client *client, struct wl_resource *cms,
> +		    uint32_t id)
> +{
> +	struct weston_compositor *compositor =
> +		wl_resource_get_user_data(cms);
> +	struct weston_colorspace *colorspace;
> +	struct wl_resource *colorspace_resource;
> +
> +	colorspace = &compositor->srgb_colorspace;
> +
> +	colorspace_resource = wl_resource_create(client,
> +						 &wl_cms_colorspace_interface,
> +						 1, id);
> +	if (colorspace_resource == NULL) {
> +		weston_colorspace_destroy(colorspace);
> +		wl_client_post_no_memory(client);
> +		return;
> +	}
> +	wl_resource_set_implementation(colorspace_resource,
> +				       &cms_colorspace_interface, colorspace,
> +				       weston_colorspace_resource_destroy);
> +}
> +
> +static void
> +cms_blending_colorspace(struct wl_client *client, struct wl_resource *cms,
> +			uint32_t id)
> +{
> +	struct weston_compositor *compositor =
> +		wl_resource_get_user_data(cms);
> +	struct weston_colorspace *colorspace;
> +	struct wl_resource *colorspace_resource;
> +
> +	colorspace = &compositor->blending_colorspace;
> +
> +	colorspace_resource = wl_resource_create(client,
> +						 &wl_cms_colorspace_interface,
> +						 1, id);
> +	if (colorspace_resource == NULL) {
> +		weston_colorspace_destroy(colorspace);
> +		wl_client_post_no_memory(client);
> +		return;
> +	}
> +	wl_resource_set_implementation(colorspace_resource,
> +				       &cms_colorspace_interface, colorspace,
> +				       weston_colorspace_resource_destroy);
> +}
> +
> +static const struct wl_cms_interface cms_interface = {
> +	cms_set_colorspace,
> +	cms_colorspace_from_fd,
> +	cms_output_colorspace,
> +	cms_srgb_colorspace,
> +	cms_blending_colorspace
> +};
> +
> +static void
> +bind_cms(struct wl_client *client, void *data, uint32_t version, uint32_t id)
> +{
> +	struct wl_resource *resource;
> +
> +	resource = wl_resource_create(client, &wl_cms_interface, 1, id);
> +	if (resource == NULL) {
> +		wl_client_post_no_memory(client);
> +		return;
> +	}
> +
> +	wl_resource_set_implementation(resource, &cms_interface, data, NULL);
> +}
> +#endif
> +
>  static void
>  compositor_bind(struct wl_client *client,
>  		void *data, uint32_t version, uint32_t id)
> @@ -4016,6 +4364,12 @@ weston_compositor_init(struct weston_compositor *ec,
>  			      ec, bind_presentation))
>  		return -1;
>  
> +#ifdef HAVE_LCMS
> +	if (!wl_global_create(display, &wl_cms_interface, 1,
> +			      ec, bind_cms))
> +		return -1;
> +#endif
> +
>  	wl_list_init(&ec->view_list);
>  	wl_list_init(&ec->plane_list);
>  	wl_list_init(&ec->layer_list);
> @@ -4600,6 +4954,10 @@ int main(int argc, char *argv[])
>  	struct wl_client *primary_client;
>  	struct wl_listener primary_client_destroyed;
>  	struct weston_seat *seat;
> +#ifdef HAVE_LCMS
> +	cmsUInt8Number srgb_id[17];
> +	cmsUInt8Number blending_id[17];
> +#endif
>  
>  	const struct weston_option core_options[] = {
>  		{ WESTON_OPTION_STRING, "backend", 'B', &backend },
> @@ -4690,6 +5048,45 @@ int main(int argc, char *argv[])
>  	ec->idle_time = idle_time;
>  	ec->default_pointer_grab = NULL;
>  
> +#ifdef HAVE_LCMS
> +	ec->input_colorspaces =
> +		g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> +				      (GDestroyNotify) weston_colorspace_free);
> +	ec->output_colorspaces =
> +		g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> +				      (GDestroyNotify) weston_colorspace_free);
> +
> +	ec->srgb_colorspace.destroyable = 0;
> +	ec->srgb_colorspace.input = 1;
> +	ec->srgb_colorspace.compositor = ec;
> +	ec->blending_colorspace.destroyable = 0;
> +	ec->blending_colorspace.input = 1;
> +	ec->blending_colorspace.compositor = ec;
> +
> +	ec->srgb_colorspace.lcms_handle = cmsCreate_sRGBProfile();
> +	/* TODO: We should really use a linear blending space. But this would
> +	 * cause additional banding since we only use an 8 bit LUT for now. */
> +	ec->blending_colorspace.lcms_handle = cmsCreate_sRGBProfile();
> +
> +	weston_build_clut(&ec->srgb_colorspace);
> +	/* This is unnecessary but will initialize everything with 0 */
> +	weston_build_clut(&ec->blending_colorspace);
> +
> +	cmsMD5computeID(ec->srgb_colorspace.lcms_handle);
> +	cmsMD5computeID(ec->blending_colorspace.lcms_handle);
> +	cmsGetHeaderProfileID(ec->srgb_colorspace.lcms_handle, srgb_id);
> +	cmsGetHeaderProfileID(ec->blending_colorspace.lcms_handle, blending_id);
> +	srgb_id[16] = '\0';
> +	blending_id[16] = '\0';
> +
> +	g_hash_table_insert(ec->input_colorspaces,
> +			    g_strdup((const gchar *) srgb_id),
> +			    &ec->srgb_colorspace);
> +	g_hash_table_insert(ec->input_colorspaces,
> +			    g_strdup((const gchar *) blending_id),
> +			    &ec->srgb_colorspace);
> +#endif
> +
>  	for (i = 1; i < argc; i++)
>  		weston_log("fatal: unhandled option: %s\n", argv[i]);
>  	if (argc > 1) {
> @@ -4760,6 +5157,14 @@ out:
>  
>  	wl_signal_emit(&ec->destroy_signal, ec);
>  
> +#ifdef HAVE_LCMS
> +	g_hash_table_unref(ec->input_colorspaces);
> +	g_hash_table_unref(ec->output_colorspaces);
> +
> +	weston_free_clut(&ec->srgb_colorspace.clut);
> +	/* The blending colorspace cannot have a clut */
> +#endif
> +
>  	weston_compositor_xkb_destroy(ec);
>  
>  	ec->destroy(ec);
> 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>
>
>  #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;
> +};
> +
>  struct weston_output {
>  	uint32_t id;
>  	char *name;
> @@ -220,6 +243,8 @@ struct weston_output {
>  	struct weston_mode *original_mode;
>  	struct wl_list mode_list;
>  
> +	struct weston_colorspace *colorspace;
> +
>  	void (*start_repaint_loop)(struct weston_output *output);
>  	int (*repaint)(struct weston_output *output,
>  			pixman_region32_t *damage);
> @@ -668,6 +693,11 @@ struct weston_compositor {
>  	int32_t kb_repeat_delay;
>  
>  	clockid_t presentation_clock;
> +
> +	struct weston_colorspace srgb_colorspace;
> +	struct weston_colorspace blending_colorspace;
> +	GHashTable *input_colorspaces;
> +	GHashTable *output_colorspaces;
>  };
>  
>  struct weston_buffer {
> @@ -844,6 +874,9 @@ struct weston_surface_state {
>  	/* wl_surface.set_scaling_factor */
>  	/* wl_viewport.set */
>  	struct weston_buffer_viewport buffer_viewport;
> +
> +	/* wl_cms.set_colorspace*/
> +	struct weston_colorspace *colorspace;
>  };
>  
>  struct weston_surface {
> @@ -887,6 +920,8 @@ struct weston_surface {
>  	int32_t height_from_buffer;
>  	int keep_buffer; /* bool for backends to prevent early release */
>  
> +	struct weston_colorspace *colorspace;
> +
>  	/* wl_viewport resource for this surface */
>  	struct wl_resource *viewport_resource;
>  
> @@ -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);
> +
>  void
>  weston_surface_set_color(struct weston_surface *surface,
>  			 float red, float green, float blue, float alpha);
> -- 
> 2.1.2
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
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@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?

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.

> >  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
> > +static void
> > +weston_free_clut(struct weston_clut *clut)
> > +{
> > +	clut->points = 0;
> > +	free(clut->data);
> > +	clut->data = NULL;
> > +}
> > +
> > +static void
> > +weston_build_clut(struct weston_colorspace *colorspace)
> > +{
> > +	cmsHTRANSFORM xform;
> > +	unsigned r, g, b;
> > +	cmsHPROFILE input_profile, output_profile;
> > +	cmsUInt8Number input_id[16];
> > +	cmsUInt8Number output_id[16];
> > +	struct weston_clut *clut = &colorspace->clut;
> > +	struct weston_compositor *ec = colorspace->compositor;
> > +
> > +	weston_free_clut(clut);
> > +
> > +	if (colorspace->input) {
> > +		input_profile = colorspace->lcms_handle;
> > +		output_profile = ec->blending_colorspace.lcms_handle;
> > +	} else {
> > +		input_profile = ec->blending_colorspace.lcms_handle;
> > +		output_profile = colorspace->lcms_handle;
> > +	}
> > +
> > +	cmsGetHeaderProfileID(input_profile, input_id);
> > +	cmsGetHeaderProfileID(output_profile, output_id);
> > +	if (memcmp(input_id, output_id, 16) == 0)
> > +		return;
> 
> Don't hide the error here, propagate it by returning false.
> And obviously all callers of this routine should check it.  :-)

This is no error. If the IDs of the input and output profile are the same then 
no color conversion has to be done and therefore we do not need to build a 
clut. I have added a comment here.

> > +	clut->points = 64;
> 
> Does this ever change to anything besides 64?  If not, wouldn't this be
> better as a constant?

I have fixed this to 64 for now but we might want to change it at some point. 
I have added a define for the default number grid points...

> > +	xform = cmsCreateTransform(input_profile, TYPE_RGB_8, output_profile,
> > +				   TYPE_RGB_8, INTENT_PERCEPTUAL, 0);
> 
> Looks like cmsCreateTransform has a return value that can be checked for
> NULL to detect errors.  Looks like it signals some errors that could be
> propagated too.
> 
> > +	clut->data = malloc(clut->points * clut->points * clut->points *
> > +			    3 * sizeof(char));
> 
> Needs error check here too.
> 
> > +	for (b = 0; b < clut->points; b++)
> > +		for (g = 0; g < clut->points; g++)
> > +			for (r = 0; r < clut->points; r++) {
> > +				char in[3];
> > +				unsigned entry = 3 * (r + clut->points *
> > +						 (g + clut->points * b));
> > +				const float step = 255.0 / (clut->points - 1);
> > +				in[0] = (char) (r * step + 0.5);
> > +				in[1] = (char) (g * step + 0.5);
> > +				in[2] = (char) (b * step + 0.5);
> > +				cmsDoTransform(xform, in,
> > +					       &clut->data[entry], 1);
> > +			}
> > +
> > +	cmsDeleteTransform(xform);
> > +}
> > +
> > +WL_EXPORT struct weston_colorspace *
> > +weston_colorspace_from_fd(int fd, int input, struct weston_compositor
> > *ec)
> > +{
> > +	FILE *profile;
> > +	cmsHPROFILE lcms_handle;
> > +	cmsUInt8Number profile_id[17];
> > +	struct weston_colorspace *colorspace;
> > +
> > +	profile = fdopen(fd, "r");
> > +	if (!profile) {
> > +		return 0;
> 
> Should return NULL explicitly rather than 0 here.
> 
> > +	}
> > +
> > +	lcms_handle = cmsOpenProfileFromStream(profile, "r");
> > +	if (!lcms_handle) {
> > +		return 0;
> > +	}
> 
> Same here.  In fact you may want to do a goto error handler, so you can
> close the fd and other cleanup.
> 
> > +	cmsMD5computeID(lcms_handle);
> 
> Check return status for FALSE on this one.
> 
> > +	cmsGetHeaderProfileID(lcms_handle, profile_id);
> > +	profile_id[16] = '\0';
> 
> Google isn't giving up an API definition on this one so don't know if it
> returns an error code.

I think it cannot fail.

> > +	if (input)
> > +		colorspace = g_hash_table_lookup(ec->input_colorspaces,
> > +						 profile_id);
> > +	else
> > +		colorspace = g_hash_table_lookup(ec->output_colorspaces,
> > +						 profile_id);
> > +	if (colorspace) {
> > +		colorspace->refcount++;
> > +
> > +		cmsCloseProfile(lcms_handle);
> > +		return colorspace;
> > +	}
> > +
> > +	colorspace = calloc(1, sizeof(struct weston_colorspace));
> 
> Check for NULL on the calloc
> 
> > +	colorspace->refcount = 1;
> > +	colorspace->destroyable = 1;
> > +	colorspace->input = input;
> > +	colorspace->lcms_handle = lcms_handle;
> > +	colorspace->compositor = ec;
> > +
> > +	weston_build_clut(colorspace);
> 
> Check error return for weston_build_clut.
> 
> > +	g_hash_table_insert(ec->input_colorspaces,
> > +			    g_strdup((const gchar *) profile_id),
> > +			    colorspace);
> > +
> > +	return colorspace;
> > +}
> > +
> > +/* This function is called when a color space is removed from the hash
> > table */ +static void
> > +weston_colorspace_free(struct weston_colorspace *colorspace)
> > +{
> > +	if (!colorspace->destroyable)
> > +		return;
> 
> No chance this can't result in memory leakage?

There are only two colorspaces for which destroyable is 0 (the blending and 
srgb default color spaces) and these are handled separately in main.

> > +	cmsCloseProfile(colorspace->lcms_handle);
> > +
> > +	weston_free_clut(&colorspace->clut);
> > +	free(colorspace);
> > +}
> > +
> > +WL_EXPORT void
> > +weston_colorspace_destroy(struct weston_colorspace *colorspace)
> > +{
> > +	cmsUInt8Number profile_id[17];
> > +
> > +	if (!colorspace->destroyable)
> > +		return;
> > +
> > +	colorspace->refcount--;
> > +	if (colorspace->refcount > 0)
> > +		return;
> > +
> > +	cmsGetHeaderProfileID(colorspace->lcms_handle, profile_id);
> > +	profile_id[16] = '\0';
> > +
> > +	if (colorspace->input)
> > +		g_hash_table_remove(colorspace->compositor->input_colorspaces,
> > +				    profile_id);
> > +	else
> > +		g_hash_table_remove(colorspace->compositor->output_colorspaces,
> > +				    profile_id);
> > +
> > +}
> 
> Why is there both a *_free() and a *_destroy() for weston_colorspace?

weston_colorspace_free is called from GHashTable when an element is removed 
from the table. weston_colorspace_destroy handles the reference counting and 
removes the colorspace from the list if the refcount reaches 0 (which causes 
weston_colorspace_free to be called).

> > +static void
> > +weston_colorspace_resource_destroy(struct wl_resource *cms_colorspace)
> > +{
> > +	struct weston_colorspace *colorspace =
> > +		wl_resource_get_user_data(cms_colorspace);
> > +	weston_colorspace_destroy(colorspace);
> > +}
> > +#endif
> > +
> > 
> >  WL_EXPORT struct weston_surface *
> >  weston_surface_create(struct weston_compositor *compositor)
> >  {
> > 
> > @@ -597,7 +758,9 @@ weston_surface_create(struct weston_compositor
> > *compositor)> 
> >  	surface->buffer_viewport.buffer.src_width = wl_fixed_from_int(-1);
> >  	surface->buffer_viewport.surface.width = -1;
> > 
> > -	weston_surface_state_init(&surface->pending);
> > +	surface->colorspace = &compositor->srgb_colorspace;
> > +
> > +	weston_surface_state_init(&surface->pending, compositor);
> > 
> >  	surface->output = NULL;
> > 
> > @@ -615,6 +778,7 @@ weston_surface_create(struct weston_compositor
> > *compositor)> 
> >  	wl_list_init(&surface->subsurface_list);
> >  	wl_list_init(&surface->subsurface_list_pending);
> > 
> > +
> > 
> >  	return surface;
> 
> Extraneous newline?
> 
> >  }
> > 
> > @@ -2369,6 +2533,8 @@ weston_surface_commit_state(struct weston_surface
> > *surface,> 
> >  	wl_list_insert_list(&surface->feedback_list,
> >  	
> >  			    &state->feedback_list);
> >  	
> >  	wl_list_init(&state->feedback_list);
> > 
> > +
> > +	surface->colorspace = state->colorspace;
> > 
> >  }
> >  
> >  static void
> > 
> > @@ -2609,6 +2775,8 @@ weston_subsurface_commit_to_cache(struct
> > weston_subsurface *sub)> 
> >  	sub->cached.buffer_viewport.surface =
> >  	
> >  		surface->pending.buffer_viewport.surface;
> > 
> > +	sub->cached.colorspace = surface->colorspace;
> > +
> > 
> >  	weston_surface_reset_pending_buffer(surface);
> >  	
> >  	pixman_region32_copy(&sub->cached.opaque, &surface->pending.opaque);
> > 
> > @@ -3041,7 +3209,7 @@ weston_subsurface_create(uint32_t id, struct
> > weston_surface *surface,> 
> >  				       sub, subsurface_resource_destroy);
> >  	
> >  	weston_subsurface_link_surface(sub, surface);
> >  	weston_subsurface_link_parent(sub, parent);
> > 
> > -	weston_surface_state_init(&sub->cached);
> > +	weston_surface_state_init(&sub->cached, surface->compositor);
> > 
> >  	sub->cached_buffer_ref.buffer = NULL;
> >  	sub->synchronized = 1;
> > 
> > @@ -3363,6 +3531,10 @@ weston_output_destroy(struct weston_output *output)
> > 
> >  	wl_signal_emit(&output->compositor->output_destroyed_signal, output);
> >  	wl_signal_emit(&output->destroy_signal, output);
> > 
> > +#ifdef HAVE_LCMS
> > +	weston_colorspace_destroy(output->colorspace);
> > +#endif
> > +
> > 
> >  	free(output->name);
> >  	pixman_region32_fini(&output->region);
> >  	pixman_region32_fini(&output->previous_damage);
> > 
> > @@ -3554,6 +3726,7 @@ weston_output_init(struct weston_output *output,
> > struct weston_compositor *c,> 
> >  	output->mm_height = mm_height;
> >  	output->dirty = 1;
> >  	output->original_scale = scale;
> > 
> > +	output->colorspace = &c->srgb_colorspace;
> > 
> >  	weston_output_transform_scale_init(output, transform, scale);
> >  	weston_output_init_zoom(output);
> > 
> > @@ -3917,6 +4090,181 @@ bind_presentation(struct wl_client *client,
> > 
> >  	presentation_send_clock_id(resource, compositor->presentation_clock);
> >  
> >  }
> > 
> > +#ifdef HAVE_LCMS
> > +static void
> > +cms_colorspace_destroy(struct wl_client *client,
> > +		       struct wl_resource *cms_colorspace)
> > +{
> > +	wl_resource_destroy(cms_colorspace);
> > +}
> > +
> > +static void
> > +cms_colorspace_get_profile_fd(struct wl_client *client,
> > +			      struct wl_resource *cms_colorspace)
> > +{
> > +	struct weston_colorspace *colorspace =
> > +		wl_resource_get_user_data(cms_colorspace);
> > +	char template[24];
> > +	FILE *profilefile;
> > +	int fd = -1;
> > +
> > +	strncpy(template, "/tmp/weston-cms-XXXXXX", 24);
> > +	fd = mkstemp(template);
> > +	profilefile = fdopen(fd, "w");
> > +	cmsSaveProfileToStream(colorspace->lcms_handle, profilefile);
> > +	rewind(profilefile);
> > +
> > +	wl_cms_colorspace_send_profile_data(cms_colorspace, fd);
> > +
> > +	close(fd);
> > +}
> > +
> > +static const struct wl_cms_colorspace_interface cms_colorspace_interface
> > = { +	cms_colorspace_destroy,
> > +	cms_colorspace_get_profile_fd
> > +};
> > +
> > +static void
> > +cms_set_colorspace(struct wl_client *client, struct wl_resource *cms,
> > +		   struct wl_resource *surface_resource,
> > +		   struct wl_resource *colorspace_resource)
> > +{
> > +	struct weston_surface *surface =
> > +		wl_resource_get_user_data(surface_resource);
> 
> wl_resource_get_user_data() returns a void*; does this need a cast?
> Also, I can't tell whether it's guaranteed not to return NULL or not.
> If it can return NULL, better check for it here.

I don't think that a cast from void* to another pointer is needed. And 
wl_resource_get_user_data can only return NULL if the user data is set to 
NULL.

> > +
> > +	struct weston_colorspace *colorspace =
> > +		wl_resource_get_user_data(colorspace_resource);
> > +	surface->pending.colorspace = colorspace;
> > +}
> > +
> > +static void
> > +cms_colorspace_from_fd(struct wl_client *client, struct wl_resource *cms,
> > +		       int fd, uint32_t id)
> > +{
> > +	struct weston_compositor *compositor =
> > +		wl_resource_get_user_data(cms);
> > +
> > +	struct weston_colorspace *colorspace;
> > +	struct wl_resource *colorspace_resource;
> > +
> > +	colorspace = weston_colorspace_from_fd(fd, 1, compositor);
> > +	if (colorspace == NULL) {
> > +		wl_resource_post_error(cms,
> > +				       WL_CMS_ERROR_INVALID_PROFILE,
> > +				       "the passed ICC profile is not valid");
> > +		return;
> 
> Should return false in case of an error.

Are you sure? The prototype of this functions is generated from the protocol 
description and does not return anything.

> > +	}
> > +
> > +	colorspace_resource = wl_resource_create(client,
> > +						 &wl_cms_colorspace_interface,
> > +						 1, id);
> > +	if (colorspace_resource == NULL) {
> > +		weston_colorspace_destroy(colorspace);
> > +		wl_client_post_no_memory(client);
> > +		return;
> 
> ditto
> 
> > +	}
> > +	wl_resource_set_implementation(colorspace_resource,
> > +				       &cms_colorspace_interface, colorspace,
> > +				       weston_colorspace_resource_destroy);
> > +}
> > +
> > +static void
> > +cms_output_colorspace(struct wl_client *client, struct wl_resource *cms,
> > +		      struct wl_resource *output_resource, uint32_t id)
> > +{
> > +	struct weston_output *output =
> > +		wl_resource_get_user_data(output_resource);
> > +
> > +	struct weston_colorspace *colorspace;
> > +	struct wl_resource *colorspace_resource;
> > +
> > +	colorspace = output->colorspace;
> > +	colorspace->refcount++;
> > +
> > +	colorspace_resource = wl_resource_create(client,
> > +						 &wl_cms_colorspace_interface,
> > +						 1, id);
> > +	if (colorspace_resource == NULL) {
> > +		weston_colorspace_destroy(colorspace);
> > +		wl_client_post_no_memory(client);
> > +		return;
> > +	}
> > +	wl_resource_set_implementation(colorspace_resource,
> > +				       &cms_colorspace_interface, colorspace,
> > +				       weston_colorspace_resource_destroy);
> > +}
> > +
> > +static void
> > +cms_srgb_colorspace(struct wl_client *client, struct wl_resource *cms,
> > +		    uint32_t id)
> > +{
> > +	struct weston_compositor *compositor =
> > +		wl_resource_get_user_data(cms);
> > +	struct weston_colorspace *colorspace;
> > +	struct wl_resource *colorspace_resource;
> > +
> > +	colorspace = &compositor->srgb_colorspace;
> > +
> > +	colorspace_resource = wl_resource_create(client,
> > +						 &wl_cms_colorspace_interface,
> > +						 1, id);
> > +	if (colorspace_resource == NULL) {
> > +		weston_colorspace_destroy(colorspace);
> > +		wl_client_post_no_memory(client);
> > +		return;
> > +	}
> > +	wl_resource_set_implementation(colorspace_resource,
> > +				       &cms_colorspace_interface, colorspace,
> > +				       weston_colorspace_resource_destroy);
> > +}
> > +
> > +static void
> > +cms_blending_colorspace(struct wl_client *client, struct wl_resource
> > *cms,
> > +			uint32_t id)
> > +{
> > +	struct weston_compositor *compositor =
> > +		wl_resource_get_user_data(cms);
> > +	struct weston_colorspace *colorspace;
> > +	struct wl_resource *colorspace_resource;
> > +
> > +	colorspace = &compositor->blending_colorspace;
> > +
> > +	colorspace_resource = wl_resource_create(client,
> > +						 &wl_cms_colorspace_interface,
> > +						 1, id);
> > +	if (colorspace_resource == NULL) {
> > +		weston_colorspace_destroy(colorspace);
> > +		wl_client_post_no_memory(client);
> > +		return;
> > +	}
> > +	wl_resource_set_implementation(colorspace_resource,
> > +				       &cms_colorspace_interface, colorspace,
> > +				       weston_colorspace_resource_destroy);
> > +}
> > +
> > +static const struct wl_cms_interface cms_interface = {
> > +	cms_set_colorspace,
> > +	cms_colorspace_from_fd,
> > +	cms_output_colorspace,
> > +	cms_srgb_colorspace,
> > +	cms_blending_colorspace
> > +};
> > +
> > +static void
> > +bind_cms(struct wl_client *client, void *data, uint32_t version, uint32_t
> > id) +{
> > +	struct wl_resource *resource;
> > +
> > +	resource = wl_resource_create(client, &wl_cms_interface, 1, id);
> > +	if (resource == NULL) {
> > +		wl_client_post_no_memory(client);
> > +		return;
> > +	}
> > +
> > +	wl_resource_set_implementation(resource, &cms_interface, data, NULL);
> > +}
> > +#endif
> > +
> > 
> >  static void
> >  compositor_bind(struct wl_client *client,
> >  
> >  		void *data, uint32_t version, uint32_t id)
> > 
> > @@ -4016,6 +4364,12 @@ weston_compositor_init(struct weston_compositor
> > *ec,
> > 
> >  			      ec, bind_presentation))
> >  		
> >  		return -1;
> > 
> > +#ifdef HAVE_LCMS
> > +	if (!wl_global_create(display, &wl_cms_interface, 1,
> > +			      ec, bind_cms))
> > +		return -1;
> > +#endif
> > +
> > 
> >  	wl_list_init(&ec->view_list);
> >  	wl_list_init(&ec->plane_list);
> >  	wl_list_init(&ec->layer_list);
> > 
> > @@ -4600,6 +4954,10 @@ int main(int argc, char *argv[])
> > 
> >  	struct wl_client *primary_client;
> >  	struct wl_listener primary_client_destroyed;
> >  	struct weston_seat *seat;
> > 
> > +#ifdef HAVE_LCMS
> > +	cmsUInt8Number srgb_id[17];
> > +	cmsUInt8Number blending_id[17];
> > +#endif
> > 
> >  	const struct weston_option core_options[] = {
> >  	
> >  		{ WESTON_OPTION_STRING, "backend", 'B', &backend },
> > 
> > @@ -4690,6 +5048,45 @@ int main(int argc, char *argv[])
> > 
> >  	ec->idle_time = idle_time;
> >  	ec->default_pointer_grab = NULL;
> > 
> > +#ifdef HAVE_LCMS
> > +	ec->input_colorspaces =
> > +		g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> > +				      (GDestroyNotify) weston_colorspace_free);
> > +	ec->output_colorspaces =
> > +		g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> > +				      (GDestroyNotify) weston_colorspace_free);
> > +
> > +	ec->srgb_colorspace.destroyable = 0;
> > +	ec->srgb_colorspace.input = 1;
> > +	ec->srgb_colorspace.compositor = ec;
> > +	ec->blending_colorspace.destroyable = 0;
> > +	ec->blending_colorspace.input = 1;
> > +	ec->blending_colorspace.compositor = ec;
> > +
> > +	ec->srgb_colorspace.lcms_handle = cmsCreate_sRGBProfile();
> > +	/* TODO: We should really use a linear blending space. But this would
> > +	 * cause additional banding since we only use an 8 bit LUT for now. */
> > +	ec->blending_colorspace.lcms_handle = cmsCreate_sRGBProfile();
> > +
> > +	weston_build_clut(&ec->srgb_colorspace);
> > +	/* This is unnecessary but will initialize everything with 0 */
> > +	weston_build_clut(&ec->blending_colorspace);
> > +
> > +	cmsMD5computeID(ec->srgb_colorspace.lcms_handle);
> > +	cmsMD5computeID(ec->blending_colorspace.lcms_handle);
> > +	cmsGetHeaderProfileID(ec->srgb_colorspace.lcms_handle, srgb_id);
> > +	cmsGetHeaderProfileID(ec->blending_colorspace.lcms_handle, 
blending_id);
> > +	srgb_id[16] = '\0';
> > +	blending_id[16] = '\0';
> > +
> > +	g_hash_table_insert(ec->input_colorspaces,
> > +			    g_strdup((const gchar *) srgb_id),
> > +			    &ec->srgb_colorspace);
> > +	g_hash_table_insert(ec->input_colorspaces,
> > +			    g_strdup((const gchar *) blending_id),
> > +			    &ec->srgb_colorspace);
> > +#endif
> > +
> > 
> >  	for (i = 1; i < argc; i++)
> >  	
> >  		weston_log("fatal: unhandled option: %s\n", argv[i]);
> >  	
> >  	if (argc > 1) {
> > 
> > @@ -4760,6 +5157,14 @@ out:
> >  	wl_signal_emit(&ec->destroy_signal, ec);
> > 
> > +#ifdef HAVE_LCMS
> > +	g_hash_table_unref(ec->input_colorspaces);
> > +	g_hash_table_unref(ec->output_colorspaces);
> > +
> > +	weston_free_clut(&ec->srgb_colorspace.clut);
> > +	/* The blending colorspace cannot have a clut */
> > +#endif
> > +
> > 
> >  	weston_compositor_xkb_destroy(ec);
> >  	
> >  	ec->destroy(ec);
> > 
> > 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>
> > 
> >  #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;
> > +};
> > +
> > 
> >  struct weston_output {
> >  
> >  	uint32_t id;
> >  	char *name;
> > 
> > @@ -220,6 +243,8 @@ struct weston_output {
> > 
> >  	struct weston_mode *original_mode;
> >  	struct wl_list mode_list;
> > 
> > +	struct weston_colorspace *colorspace;
> > +
> > 
> >  	void (*start_repaint_loop)(struct weston_output *output);
> >  	int (*repaint)(struct weston_output *output,
> >  	
> >  			pixman_region32_t *damage);
> > 
> > @@ -668,6 +693,11 @@ struct weston_compositor {
> > 
> >  	int32_t kb_repeat_delay;
> >  	
> >  	clockid_t presentation_clock;
> > 
> > +
> > +	struct weston_colorspace srgb_colorspace;
> > +	struct weston_colorspace blending_colorspace;
> > +	GHashTable *input_colorspaces;
> > +	GHashTable *output_colorspaces;
> > 
> >  };
> >  
> >  struct weston_buffer {
> > 
> > @@ -844,6 +874,9 @@ struct weston_surface_state {
> > 
> >  	/* wl_surface.set_scaling_factor */
> >  	/* wl_viewport.set */
> >  	struct weston_buffer_viewport buffer_viewport;
> > 
> > +
> > +	/* wl_cms.set_colorspace*/
> > +	struct weston_colorspace *colorspace;
> > 
> >  };
> >  
> >  struct weston_surface {
> > 
> > @@ -887,6 +920,8 @@ struct weston_surface {
> > 
> >  	int32_t height_from_buffer;
> >  	int keep_buffer; /* bool for backends to prevent early release */
> > 
> > +	struct weston_colorspace *colorspace;
> > +
> > 
> >  	/* wl_viewport resource for this surface */
> >  	struct wl_resource *viewport_resource;
> > 
> > @@ -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);
> > +
> > 
> >  void
> >  weston_surface_set_color(struct weston_surface *surface,
> >  
> >  			 float red, float green, float blue, float alpha);
> > 
> > --
> > 2.1.2
> > 
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> Wayland has a wl_map data structure; could that be used instead of
> GHashTable, in order to avoid the glib dependency?

wl_map is not designed as a general hash map data structure. If we don't
want to use GHashTable, then the closest alternative is a linear scan over
a list. Which, really, is actually faster than a hash table for sizes less
than 100 or so. I'd just do that.
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@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@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
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@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@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
Am 27.11.2014 um 15:58 schrieb Niels Ole Salscheider:
>>>>>  #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.

Agreed, lcms internals are not nice to get exposed at this level.


Generally speaking, lcms provides a great API for a CMM. On the other
side, lcms is just one CMM and people in the open source world decided
many times to define and implement different API's. E.g. ArgyllCMS
(features, speed), SampleICC (spec playground) and Mozilla/Chrome with
qcms (secuirity, speed). So it is not wise to stick to especially one
CMM, be it lcms or whatever.

A CMM module can register its own functions for their handle type. You
could either wrap the CMM functions or use initial dummy functions
instead for function pointers. The type of the handle is not really
needed to get exposed. As long as the CMM registers its API at startup,
that procedure is simple.

A different approach is to store the data blob of the profile. The lcms
expanded profile handle needs certainly more memory, than the plain
memory block. And loading the profile on the fly is really fast. You
need the CMM profile handle mostly in the weston_build_clut() function
and for the header ID computation in other places. So profile memory
blob and profile header ID would suffice?

Then following might work out for the later approach:

+struct weston_colorspace {
+
+	void * profile_data;
+       int profile_data_size;
+       unsigned char profile_id[16];
+
+	struct weston_clut clut;
+
+	int destroyable;
+	int refcount;
+	int input;
+
+	struct weston_compositor *compositor;
+};


This opens the path to exchange the CMM without much fuzz on start time
depending on the system requirements.

Thanks
Kai-Uwe
On Thu, 27 Nov 2014 15:58:51 +0100
Niels Ole Salscheider <niels_ole@salscheider-online.de> wrote:

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

What happens if someone builds Weston with LCMS and full color
management support, but does not load neither cms-static nor cms-colord?

I'm not even sure how cms-static gets used since it's built as a static
lib.


> > On Mon, 27 Oct 2014 18:54:06 +0100
> > 
> > Niels Ole Salscheider <niels_ole@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@salscheider-online.de>
> > > > > ---
> > > > > 
> > > > >  Makefile.am      |   3 +
> > > > >  configure.ac     |   2 +-
> > > > >  src/compositor.c | 411
> > > > >  ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > >  src/compositor.h

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

I'm not sure. Maybe for hardware that has a hw LUT, but doing full color
management would be prohibitively slow? Like with the Pixman renderer?

Or maybe Raspberry Pi, although I don't think we program any LUTs atm.
but I'd imagine it has a hw LUT somewhere. rpi-renderer cannot do color
management with DispmanX. (Take the rpi-renderer as an example of a
full hardware compositor, as opposed to an arbitrary rendering pipeline
like OpenGL.)

How would you handle these cases?

This patch registers the wl_cms global if LCMS was available at build
time. However, I think you need to make that conditional on the backend
and renderer capabilities. If the renderer cannot do color management,
it is useless to advertise wl_cms at all, because it won't have any
effect at all, right? It would only fool apps to believe that they
could get color managed output.

Another thing I think this patch is missing, is making the DRM backend
check for the color profile compatibility. If a surface needs color
correction at all, the buffer cannot be scanned out directly and you
need to make backends not put such surfaces on hw planes.

The DRM backend currently does not use hw overlay planes, but it does
use a cursor plane and the primary plane. A fullscreen window may get
promoted to the primary plane which bypasses compositing and therefore
also color correction.

Oh btw. would apps want to know if an output's color profile is
"unknown"? That is, the compositor just assumes it's sRGB because it
has no information about it. I could imagine some apps wanting to show
a warning about unmanaged output in that case, maybe?


Thanks,
pq