GNOME Bugzilla – Bug 651626
color: Move the device registration functionality from gcm-session to the color plugin
Last modified: 2011-06-11 11:39:24 UTC
Created attachment 188983 [details] [review] [patch] patch for review We have to monitor X devices in the session as colord as a system daemon and is unable to access the per-session X screens and outputs without evil hacks. When a display output is added or changed we: * Register the display device with colord and add the required metadata entries * Create an auto-EDID profile based from the chromacity data in the EDID (if it does not already exist) * Ensure the generated profile is assigned to the correct device, async. * Check to see if it's the default device, and if so, set the _ICC_PROFILE atom * Make sure the default profile VCGT data for the device is loaded into X. When a display is removed we: * Unregister the device from colord. ---- This is an initial patch adding quite a bit of code cut-and-pasted from gnome-color-manager. Rather than a line-by-line review, could the first pass of the patch review please concentrate on the high level stuff (like, "Don't use GcmEdid, we have a better version somewhere already") -- I'm also aware there is a bit of #ifdef 0 stuff, and also a few sync calls. They can all be fixed when you guys agree the basics of the patch is valid, so please gloss over them for now. From an architectural point of view, this is pretty much how ColorSync works on OSX -- subsystems register devices and applications watch for changes (if they care). I'm open to changing pretty much all of the patch (and add / change API in colord), so don't worry at this stage if it smells a bit fishy. Thanks, Richard.
Review of attachment 188983 [details] [review]: Here goes. ::: plugins/color/gcm-dmi.c @@ +113,3 @@ + +const gchar * +gcm_dmi_get_vendor (GcmDmi *dmi) Really don't like those. Instead, create a few arrays, and loop those arrays. You could also load those at create time, rather than checking for NULL 3 times for each of the calls. ::: plugins/color/gcm-edid.h @@ +20,3 @@ + */ + +#ifndef __GCM_EDID_H Really don't like the idea of EDID parsing here. Can't the X server do this for us and export the results? (FWIW, I'm pretty certain that Xorg hackers would hate us adding a new EDID parser, so they'll probably be ok with moving this functionality to the server and/or drivers). ::: plugins/color/gcm-tables.c @@ +24,3 @@ +#include <glib-object.h> + +#include "gcm-tables.h" Where's the test program for this functionality? @@ +133,3 @@ + gboolean ret; + + /* shipped in hwdata, e.g. Red Hat */ I'd rather have a single line checking a value generated from configure.ac. See also: http://git.kernel.org/?p=bluetooth/bluez.git;a=commitdiff;h=3a9811d70863b4922f26f70d8aa1a8eb77e41998 ::: plugins/color/gcm-x11-screen.c @@ +28,3 @@ +#include <X11/Xatom.h> + +#include "gcm-x11-screen.h" You don't seem to check for the XRandR extension being available at any point. This might break on VNC and similar. @@ +688,3 @@ + if (rc != Success) { + g_set_error (error, 1, 0, "failed to get atom with rc %i", rc); + goto out; return FALSE; data_tmp won't contain data if it failed. @@ +734,3 @@ + if (rc != Success) { + ret = FALSE; + g_set_error (error, 1, 0, "failed to delete root window atom with rc %i", rc); Real errors please.
Created attachment 189118 [details] [review] [patch] guts! (In reply to comment #1) > ::: plugins/color/gcm-dmi.c > @@ +113,3 @@ > + > +const gchar * > +gcm_dmi_get_vendor (GcmDmi *dmi) > > Really don't like those. Instead, create a few arrays, and loop those arrays. > You could also load those at create time, rather than checking for NULL 3 times > for each of the calls. Fixed. > ::: plugins/color/gcm-edid.h > @@ +20,3 @@ > + */ > + > +#ifndef __GCM_EDID_H > > Really don't like the idea of EDID parsing here. Can't the X server do this for > us and export the results? > (FWIW, I'm pretty certain that Xorg hackers would hate us adding a new EDID > parser, so they'll probably be ok with moving this functionality to the server > and/or drivers). It looks like I'm going to hijack ajax's libminitru and add the color management functionality there, but this might take quite some time. I'll have a look at this more tomorrow, but it'll likely be many months before the new libedid will be a suitable dep for gnome. > ::: plugins/color/gcm-tables.c > @@ +24,3 @@ > +#include <glib-object.h> > + > +#include "gcm-tables.h" > > Where's the test program for this functionality? Fixed. > @@ +133,3 @@ > + gboolean ret; > + > + /* shipped in hwdata, e.g. Red Hat */ > > I'd rather have a single line checking a value generated from configure.ac. See > also: > http://git.kernel.org/?p=bluetooth/bluez.git;a=commitdiff;h=3a9811d70863b4922f26f70d8aa1a8eb77e41998 Fixed. > ::: plugins/color/gcm-x11-screen.c > @@ +28,3 @@ > +#include <X11/Xatom.h> > + > +#include "gcm-x11-screen.h" > > You don't seem to check for the XRandR extension being available at any point. > This might break on VNC and similar. I am, see line 2097 on the new patch. > @@ +688,3 @@ > + if (rc != Success) { > + g_set_error (error, 1, 0, "failed to get atom with rc %i", rc); > + goto out; > > return FALSE; > data_tmp won't contain data if it failed. Fixed, thanks. > @@ +734,3 @@ > + if (rc != Success) { > + ret = FALSE; > + g_set_error (error, 1, 0, "failed to delete root window atom with rc > %i", rc); > > Real errors please. Fixed, thanks. New patch attached. I've un-synced some things, and also added the auto-edid functionality that I wasn't sure how to add. Review #2 please. Thanks dude. Richard.
Review of attachment 189118 [details] [review]: Biggest problem I have is all the custom XRandR code, when we have gnome-desktop to handle that for us for other parts of the system. ::: configure.ac @@ +22,3 @@ AM_PROG_CC_C_O AC_PROG_LIBTOOL +AC_PATH_PNPIDS Error message somewhere if no path was given, and the file doesn't actually exist? Otherwise a lot of people will have no pnp.ids file, and they won't know about it. ::: plugins/color/gcm-edid.c @@ +297,3 @@ + g_return_val_if_fail (GCM_IS_EDID (edid), FALSE); + g_return_val_if_fail (data != NULL, FALSE); + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); Why? @@ +300,3 @@ + + /* check header */ + if (data[0] != 0x00 || data[1] != 0xff) { Check the length as well. Maybe from a g_return_val_if_fail(). At the very least, I wouldn't expect it to crash if the data was too small. @@ +301,3 @@ + /* check header */ + if (data[0] != 0x00 || data[1] != 0xff) { + g_set_error_literal (error, 1, 0, "failed to parse header"); I believe I mentioned that in the past, use proper error definitions. @@ +373,3 @@ + + /* parse EDID data */ + for (i=GCM_EDID_OFFSET_DATA_BLOCKS; i <= GCM_EDID_OFFSET_LAST_BLOCK; i+=18) { Spaces please. ::: plugins/color/gcm-self-test.c @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- Could you add EDID testing as well please? That code should be able to be fed garbage. @@ +52,3 @@ + + vendor = gcm_tables_get_pnp_id (tables, "XXX", &error); + g_assert_error (error, 1, 0); error quarks here, rather than magic numbers. @@ +67,3 @@ + g_assert (dmi != NULL); + g_assert (gcm_dmi_get_name (dmi) != NULL); +// g_assert (gcm_dmi_get_version (dmi) != NULL); Remove if it shouldn't be there. ::: plugins/color/gcm-x11-output.c @@ +24,3 @@ + +#include <glib-object.h> +#include <X11/extensions/Xrandr.h> Is there any reason why you're not using gnome-desktop's GnomeRR stuff? gnome_rr_output_get_edid_data() for example looks ripe for your usage, and things that aren't there can easily be added. @@ +206,3 @@ + &actual_type, &actual_format, + &nitems, &bytes_after, &prop); + gdk_flush (); Why is that needed? ::: plugins/color/gsd-color-manager.c @@ +232,3 @@ + /* get a file from the URI / path */ + file = g_file_new_for_path (filename); + g_debug ("edid hash %s ignored: %s", Why would it fail? g_file_new_for_path() never fails. Either those are URIs, or they're paths. If you're taking user-input, use g_file_new_for_commandline_arg(). Otherwise choose path or URI and stick to it.
Created attachment 189655 [details] [review] new patch New patch, with all issue resolved so far. You need gnome-desktop and colord from git master. I think we're nearly there :-) Please review, thanks. Richard.
Review of attachment 189655 [details] [review]: Few niggles. Don't forget to file a separate bug about using libedid instead of the custom parsing here. ::: plugins/color/Makefile.am @@ +70,3 @@ + $(SETTINGS_PLUGIN_LIBS) + +gcm_self_test_SOURCES = \ Please make it be run by default. ::: plugins/color/gsd-color-manager.c @@ +454,3 @@ + /* get parent directory */ + file = g_file_new_for_path (filename); + g_object_unref (helper->device); add g_object_unref (file); here, and get rid of the gotos. @@ +456,3 @@ + parent_dir = g_file_get_parent (file); + if (parent_dir == NULL) { + g_free (helper); Gah! No. Magic. Numbers. In. GError. @@ +461,3 @@ + + /* ensure desination exists */ + That's still all sync, you might as well just run g_file_make_directory_with_parents() directly. It won't fail if the dirs already exist. @@ +615,3 @@ + if (data != NULL) + _cmsDictAddEntryAscii (dict, "EDID_manufacturer", data); + g_warning ("not found device %s which should have been added: %s", add /* HAVE_NEW_LCMS */ here, it's long enough that people wouldn't know what you were #ifdef'ing for.
(In reply to comment #5) > Review of attachment 189655 [details] [review]: > > Few niggles. Don't forget to file a separate bug about using libedid instead of > the custom parsing here. Filed as #652337. > ::: plugins/color/Makefile.am > @@ +70,3 @@ > + $(SETTINGS_PLUGIN_LIBS) > + > +gcm_self_test_SOURCES = \ > > Please make it be run by default. It is, it's run automatically on "make check" > @@ +456,3 @@ > + parent_dir = g_file_get_parent (file); > + if (parent_dir == NULL) { > + g_free (helper); > > Gah! No. Magic. Numbers. In. GError. Ohh, even in modules where we don't care? :-) Fair enough, I've added this now. > @@ +461,3 @@ > + > + /* ensure desination exists */ > + > > That's still all sync, you might as well just run > g_file_make_directory_with_parents() directly. It won't fail if the dirs > already exist. Point, fixed. > @@ +615,3 @@ > + if (data != NULL) > + _cmsDictAddEntryAscii (dict, "EDID_manufacturer", data); > + g_warning ("not found device %s which should have been added: > %s", > > add /* HAVE_NEW_LCMS */ > here, it's long enough that people wouldn't know what you were #ifdef'ing for. Fixed. Thanks for all the reviews. I've pushed this to master. I think I owe you a couple of beers now. :-)
(In reply to comment #6) <snip> > > ::: plugins/color/Makefile.am > > @@ +70,3 @@ > > + $(SETTINGS_PLUGIN_LIBS) > > + > > +gcm_self_test_SOURCES = \ > > > > Please make it be run by default. > > It is, it's run automatically on "make check" I meant running it all the time, not just from make check. > > @@ +456,3 @@ > > + parent_dir = g_file_get_parent (file); > > + if (parent_dir == NULL) { > > + g_free (helper); > > > > Gah! No. Magic. Numbers. In. GError. > > Ohh, even in modules where we don't care? :-) Fair enough, I've added this now. Why do you report errors if you don't care about them?