[PATCH 3/3] Add a colord implementation of a CMS plugin for weston
Kristian Høgsberg
hoegsberg at gmail.com
Wed May 1 13:21:02 PDT 2013
On Wed, Apr 24, 2013 at 02:58:04PM +0100, Richard Hughes wrote:
> This allows users to change the assigned display profile in GNOME (using
> gnome-control-center) or KDE (using colord-kde) and also allows the profiling
> tools to correctly inhibit the calibration state whilst measuring the native
> screen response.
> ---
> configure.ac | 10 ++
> src/Makefile.am | 9 ++
> src/colord.c | 440 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 459 insertions(+)
> create mode 100644 src/colord.c
I think this is fine, but I'm not sure about the threading conventions
for the colord objects. Currently the CdDevices are added and removed
in the weston thread for hotplug and in the glib thread for cold plug.
colord_update_output_from_device() is called both when adding outputs
(weston thread) and when the "changed" signal fires (in the glib
thread). In both cases it goes through the pipe and
weston_cms_set_color_profile() is always called from the weston
thread, which is good because that ends up calling the output vfunc,
which has to run in the weston thread.
Maybe a simpler approach would be to only handle the "changed" signal
in the glib thread and do everything else in the weston thread? That
is, cold-plug everything before starting the thread and have the
"changed" handler only write to the pipe?
Kristian
> diff --git a/configure.ac b/configure.ac
> index 1552d73..20011d2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -277,6 +277,16 @@ AC_ARG_ENABLE(tablet-shell,
> AM_CONDITIONAL(ENABLE_TABLET_SHELL,
> test "x$enable_tablet_shell" = "xyes")
>
> +# CMS modules
> +AC_ARG_ENABLE(colord,
> + AS_HELP_STRING([--disable-colord],
> + [do not build colord CMS support]),,
> + enable_colord=yes)
> +AM_CONDITIONAL(ENABLE_COLORD,
> + test "x$enable_colord" = "xyes")
> +if test x$enable_colord = xyes; then
> + PKG_CHECK_MODULES(COLORD, colord >= 0.1.8)
> +fi
>
> AC_ARG_ENABLE(wcap-tools, [ --disable-wcap-tools],, enable_wcap_tools=yes)
> AM_CONDITIONAL(BUILD_WCAP_TOOLS, test x$enable_wcap_tools = xyes)
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 7489086..513b446 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -96,6 +96,7 @@ moduledir = $(libdir)/weston
> module_LTLIBRARIES = \
> $(desktop_shell) \
> $(tablet_shell) \
> + $(cms_colord) \
> $(x11_backend) \
> $(drm_backend) \
> $(wayland_backend) \
> @@ -253,6 +254,14 @@ tablet_shell_la_SOURCES = \
> tablet-shell-server-protocol.h
> endif
>
> +if ENABLE_COLORD
> +cms_colord = colord.la
> +colord_la_LDFLAGS = -module -avoid-version
> +colord_la_LIBADD = $(COMPOSITOR_LIBS) $(COLORD_LIBS)
> +colord_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(COLORD_CFLAGS)
> +colord_la_SOURCES = colord.c
> +endif
> +
> BUILT_SOURCES = \
> screenshooter-server-protocol.h \
> screenshooter-protocol.c \
> diff --git a/src/colord.c b/src/colord.c
> new file mode 100644
> index 0000000..1a7fce0
> --- /dev/null
> +++ b/src/colord.c
> @@ -0,0 +1,440 @@
> +/*
> + * Copyright © 2013 Richard Hughes
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission. The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose. It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define _GNU_SOURCE
> +#include <fcntl.h>
> +#include <unistd.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <colord.h>
> +
> +#include "compositor.h"
> +#include "cms.h"
> +
> +struct colord_color_manager {
> + struct weston_color_manager base;
> + CdClient *client;
> + GHashTable *devices;
> + GMainLoop *loop;
> + GThread *thread;
> + GList *pending;
> + GMutex pending_mutex;
> + struct wl_event_source *source;
> + int readfd;
> + int writefd;
> +};
> +
> +static inline struct colord_color_manager *
> +to_colord_cm(struct weston_color_manager *base)
> +{
> + return container_of(base, struct colord_color_manager, base);
> +}
> +
> +typedef struct {
> + struct weston_output *o;
> + struct weston_color_profile *p;
> +} ColordIdleHelper;
> +
> +static void
> +colord_idle_destroy_helper(ColordIdleHelper *helper)
> +{
> + g_slice_free(ColordIdleHelper, helper);
> +}
> +
> +static gint
> +colord_idle_find_output_cb(gconstpointer a, gconstpointer b)
> +{
> + ColordIdleHelper *helper = (ColordIdleHelper *) a;
> + struct weston_output *o = (struct weston_output *) b;
> + return helper->o == o ? 0 : -1;
> +}
> +
> +static void
> +colord_idle_cancel_for_output(struct weston_output *o)
> +{
> + struct colord_color_manager *colord_cm = to_colord_cm(o->compositor->cms);
> + GList *l;
> +
> + /* cancel and remove any helpers that match the output */
> + g_mutex_lock(&colord_cm->pending_mutex);
> + l = g_list_find_custom (colord_cm->pending, o, colord_idle_find_output_cb);
> + if (l) {
> + ColordIdleHelper *helper = l->data;
> + colord_cm->pending = g_list_remove (colord_cm->pending, helper);
> + colord_idle_destroy_helper(helper);
> + }
> + g_mutex_unlock(&colord_cm->pending_mutex);
> +}
> +
> +static int
> +edid_value_valid(const char *str)
> +{
> + if (str == NULL)
> + return 0;
> + if (str[0] == '\0')
> + return 0;
> + if (strcmp(str, "unknown") == 0)
> + return 0;
> + return 1;
> +}
> +
> +static gchar *
> +get_output_id(struct weston_output *o)
> +{
> + GString *device_id;
> +
> + /* see https://github.com/hughsie/colord/blob/master/doc/device-and-profile-naming-spec.txt
> + * for format and allowed values */
> + device_id = g_string_new("xrandr");
> + if (edid_value_valid(o->make))
> + g_string_append_printf(device_id, "-%s", o->make);
> + if (edid_value_valid(o->model))
> + g_string_append_printf(device_id, "-%s", o->model);
> + if (edid_value_valid(o->serial_number))
> + g_string_append_printf(device_id, "-%s", o->serial_number);
> +
> + /* no EDID data, so use fallback */
> + if (strcmp(device_id->str, "xrandr") == 0)
> + g_string_append_printf(device_id, "-drm-%i", o->id);
> +
> + return g_string_free(device_id, FALSE);
> +}
> +
> +static void
> +update_device_with_profile_in_idle(struct weston_output *o,
> + struct weston_color_profile *p)
> +{
> + ColordIdleHelper *helper;
> + gboolean signal_write = FALSE;
> + struct colord_color_manager *colord_cm = to_colord_cm(o->compositor->cms);
> +
> + colord_idle_cancel_for_output(o);
> + helper = g_slice_new(ColordIdleHelper);
> + helper->o = o;
> + helper->p = p;
> + g_mutex_lock(&colord_cm->pending_mutex);
> + if (colord_cm->pending == NULL)
> + signal_write = TRUE;
> + colord_cm->pending = g_list_prepend(colord_cm->pending, helper);
> + g_mutex_unlock(&colord_cm->pending_mutex);
> +
> + /* signal we've got updates to do */
> + if (signal_write) {
> + gchar tmp = '\0';
> + write(colord_cm->writefd, &tmp, 1);
> + }
> +}
> +
> +static void
> +colord_update_output_from_device (struct weston_output *o, CdDevice *device)
> +{
> + CdProfile *profile;
> + gboolean ret;
> + GError *error = NULL;
> + struct weston_color_profile *p = NULL;
> +
> + ret = cd_device_connect_sync(device, NULL, &error);
> + if (!ret) {
> + weston_log("colord: failed to connect to device %s: %s\n",
> + cd_device_get_object_path (device),
> + error->message);
> + g_error_free(error);
> + goto out;
> + }
> + profile = cd_device_get_default_profile(device);
> + if (!profile) {
> + weston_log("colord: no assigned color profile for %s\n",
> + cd_device_get_id (device));
> + goto out;
> + }
> + ret = cd_profile_connect_sync(profile, NULL, &error);
> + if (!ret) {
> + weston_log("colord: failed to connect to profile %s: %s\n",
> + cd_profile_get_object_path (profile),
> + error->message);
> + g_error_free(error);
> + goto out;
> + }
> + p = weston_cms_load_profile(cd_profile_get_filename(profile));
> + if (p == NULL) {
> + weston_log("colord: warning failed to load profile %s: %s\n",
> + cd_profile_get_object_path (profile),
> + error->message);
> + g_error_free(error);
> + goto out;
> + }
> +out:
> + update_device_with_profile_in_idle(o, p);
> +}
> +
> +static void
> +colord_device_changed_cb(CdDevice *device, struct weston_output *o)
> +{
> + weston_log("colord: device %s changed, update output\n",
> + cd_device_get_object_path (device));
> + colord_update_output_from_device(o, device);
> +}
> +
> +static int
> +colord_output_added(struct weston_output *o,
> + enum weston_color_manager_flags flags)
> +{
> + CdDevice *device;
> + gchar *device_id;
> + GError *error = NULL;
> + GHashTable *device_props;
> + gint rc = 0;
> + struct colord_color_manager *colord_cm = to_colord_cm(o->compositor->cms);
> +
> + /* create device */
> + device_id = get_output_id(o);
> + weston_log("colord: output added %s\n", device_id);
> + device_props = g_hash_table_new_full(g_str_hash, g_str_equal,
> + g_free, g_free);
> + g_hash_table_insert (device_props,
> + g_strdup(CD_DEVICE_PROPERTY_KIND),
> + g_strdup(cd_device_kind_to_string (CD_DEVICE_KIND_DISPLAY)));
> + g_hash_table_insert (device_props,
> + g_strdup(CD_DEVICE_PROPERTY_FORMAT),
> + g_strdup("ColorModel.OutputMode.OutputResolution"));
> + g_hash_table_insert (device_props,
> + g_strdup(CD_DEVICE_PROPERTY_COLORSPACE),
> + g_strdup(cd_colorspace_to_string(CD_COLORSPACE_RGB)));
> + if (edid_value_valid(o->make)) {
> + g_hash_table_insert (device_props,
> + g_strdup(CD_DEVICE_PROPERTY_VENDOR),
> + g_strdup(o->make));
> + }
> + if (edid_value_valid(o->model)) {
> + g_hash_table_insert (device_props,
> + g_strdup(CD_DEVICE_PROPERTY_MODEL),
> + g_strdup(o->model));
> + }
> + if (edid_value_valid(o->serial_number)) {
> + g_hash_table_insert (device_props,
> + g_strdup(CD_DEVICE_PROPERTY_SERIAL),
> + g_strdup(o->serial_number));
> + }
> + if ((flags & WESTON_COLOR_MANAGER_FLAG_OUTPUT_INTERNAL) > 0) {
> + g_hash_table_insert (device_props,
> + g_strdup (CD_DEVICE_PROPERTY_EMBEDDED),
> + NULL);
> + }
> + device = cd_client_create_device_sync(colord_cm->client,
> + device_id,
> + CD_OBJECT_SCOPE_TEMP,
> + device_props,
> + NULL,
> + &error);
> + if (!device) {
> + rc = -1;
> + weston_log("colord: failed to create device: %s", error->message);
> + g_error_free(error);
> + goto out;
> + }
> +
> + /* add to local cache */
> + g_hash_table_insert (colord_cm->devices, g_strdup(device_id), g_object_ref(device));
> + g_signal_connect (device, "changed", G_CALLBACK (colord_device_changed_cb), o);
> +
> + /* get profiles */
> + colord_update_output_from_device (o, device);
> +out:
> + g_hash_table_unref (device_props);
> + if (device)
> + g_object_unref (device);
> + g_free (device_id);
> + return rc;
> +}
> +
> +static int
> +colord_output_removed(struct weston_output *o)
> +{
> + CdDevice *device;
> + gboolean ret;
> + gchar *device_id;
> + GError *error = NULL;
> + gint rc = 0;
> + struct colord_color_manager *colord_cm = to_colord_cm(o->compositor->cms);
> +
> + colord_idle_cancel_for_output(o);
> + device_id = get_output_id(o);
> + weston_log("colord: output removed %s\n", device_id);
> + device = g_hash_table_lookup(colord_cm->devices, device_id);
> + if (!device) {
> + rc = -1;
> + weston_log("colord: failed to find device\n");
> + goto out;
> + }
> + g_signal_handlers_disconnect_by_data(device, o);
> + ret = cd_client_delete_device_sync (colord_cm->client,
> + device,
> + NULL,
> + &error);
> +
> + if (!ret) {
> + rc = -1;
> + weston_log("colord: failed to delete device: %s", error->message);
> + g_error_free(error);
> + goto out;
> + }
> +out:
> + g_hash_table_remove (colord_cm->devices, device_id);
> + g_free (device_id);
> + return rc;
> +}
> +
> +static void
> +colord_module_destroy(struct weston_compositor *compositor)
> +{
> + struct colord_color_manager *colord_cm;
> + if (!compositor->cms)
> + return;
> + colord_cm = to_colord_cm(compositor->cms);
> +
> + if (colord_cm->loop) {
> + g_main_loop_quit(colord_cm->loop);
> + g_main_loop_unref(colord_cm->loop);
> + }
> + if (colord_cm->thread)
> + g_thread_join(colord_cm->thread);
> + if (colord_cm->devices)
> + g_hash_table_unref(colord_cm->devices);
> + if (colord_cm->client)
> + g_object_unref(colord_cm->client);
> + if (colord_cm->readfd)
> + close(colord_cm->readfd);
> + if (colord_cm->writefd)
> + close(colord_cm->writefd);
> + free(colord_cm);
> +
> + /* mark as unused */
> + compositor->cms = NULL;
> +}
> +
> +static gpointer
> +colord_run_loop_thread(gpointer data)
> +{
> + struct weston_compositor *c = (struct weston_compositor *) data;
> + struct weston_output *output;
> + struct colord_color_manager *colord_cm = to_colord_cm(c->cms);
> +
> + /* coldplug outputs */
> + wl_list_for_each(output, &c->output_list, link)
> + colord_output_added(output, 0);
> +
> + g_main_loop_run(colord_cm->loop);
> + return NULL;
> +}
> +
> +static int
> +colord_dispatch_all_pending(int fd, uint32_t mask, void *data)
> +{
> + ColordIdleHelper *helper;
> + gchar tmp;
> + GList *l;
> + struct colord_color_manager *colord_cm = data;
> +
> + weston_log("colord: dispatching events\n");
> + g_mutex_lock(&colord_cm->pending_mutex);
> + for (l = colord_cm->pending; l != NULL; l = l->next) {
> + helper = l->data;
> + weston_cms_set_color_profile(helper->o, helper->p);
> + }
> + g_list_free_full (colord_cm->pending, (GDestroyNotify) colord_idle_destroy_helper);
> + colord_cm->pending = NULL;
> + g_mutex_unlock(&colord_cm->pending_mutex);
I would do
g_mutex_lock(&colord_cm->pending_mutex);
list = colord->pending;
colord->pending = NULL;
g_mutex_unlock(&colord_cm->pending_mutex);
/* process and free list */
to avoid callbacks with the lock held and minimize the time we hold
the lock... not that it matters here.
> + /* done */
> + read(colord_cm->readfd, &tmp, 1);
> + return 1;
> +}
> +
> +WL_EXPORT int
> +module_init(struct weston_compositor *compositor,
> + int *argc, char *argv[], const char *config_file)
> +{
> + gboolean ret;
> + GError *error = NULL;
> + int fd[2];
> + struct colord_color_manager *colord_cm;
> + struct wl_event_loop *loop;
> +
> + if (compositor->cms)
> + return -1;
> +
> + weston_log("colord: initialized\n");
> +
> + /* create local state object */
> + colord_cm = malloc(sizeof *colord_cm);
> + if (colord_cm == NULL)
> + return -1;
> + memset(colord_cm, 0, sizeof *colord_cm);
> +#if !GLIB_CHECK_VERSION(2,36,0)
> + g_type_init();
> +#endif
> + colord_cm->client = cd_client_new();
> + ret = cd_client_connect_sync(colord_cm->client, NULL, &error);
> + if (!ret) {
> + weston_log("colord: failed to contact daemon: %s", error->message);
> + g_error_free(error);
> + colord_module_destroy(compositor);
> + return -1;
> + }
> + g_mutex_init(&colord_cm->pending_mutex);
> + colord_cm->devices = g_hash_table_new_full(g_str_hash, g_str_equal,
> + g_free, (GDestroyNotify) g_object_unref);
> + colord_cm->base.destroy = colord_module_destroy;
> + colord_cm->base.output_added = colord_output_added;
> + colord_cm->base.output_removed = colord_output_removed;
> + compositor->cms = &colord_cm->base;
> +
> + /* setup a thread for the GLib callbacks */
> + colord_cm->loop = g_main_loop_new(NULL, TRUE);
> + colord_cm->thread = g_thread_new("colord CMS main loop",
> + colord_run_loop_thread,
> + compositor);
> +
> + /* batch device<->profile updates */
> + if (pipe2(fd, O_CLOEXEC) == -1) {
> + colord_module_destroy(compositor);
> + return -1;
> + }
> + colord_cm->readfd = fd[0];
> + colord_cm->writefd = fd[1];
> + loop = wl_display_get_event_loop(compositor->wl_display);
> + colord_cm->source = wl_event_loop_add_fd (loop,
> + colord_cm->readfd,
> + WL_EVENT_READABLE,
> + colord_dispatch_all_pending,
> + colord_cm);
> + if (!colord_cm->source) {
> + colord_module_destroy(compositor);
> + return -1;
> + }
> + return 0;
> +}
> --
> 1.8.2
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list