GNOME Bugzilla – Bug 652062
Add the ability to set the ICC profile atom on the root screen
Last modified: 2011-06-09 20:45:36 UTC
Created attachment 189413 [details] [review] [patch] patch for review We need this for the color plugin
Review of attachment 189413 [details] [review]: Huh... is the color profile per-screen, not per-monitor? Also, both the set() and remove() functions have each two almost-identical sections frob ("atom name"); handle_errors (); Please factor out that code so it is not repeated in each function.
Also, wait a second... why is this in GnomeRR at all? Those functions don't deal with any RANDR outputs or the like.
(In reply to comment #1) > Review of attachment 189413 [details] [review]: > > Huh... is the color profile per-screen, not per-monitor? Yes, it's a major failing of the ICC_PROFILE_IN_X specification. Long term I'm going to add the required code to mutter to do this per-window in the compositor, but there are a number of programs that need this atom set (gimp, eog, etc). The compositor trick is likely to be for 3.4, not 3.2. > Also, both the set() and remove() functions have each two almost-identical > sections > > frob ("atom name"); > handle_errors (); > > Please factor out that code so it is not repeated in each function. Sure, this patch was more of a RFC, I'll fix up things now. (In reply to comment #2) > Also, wait a second... why is this in GnomeRR at all? Those functions don't > deal with any RANDR outputs or the like. Maybe a valid point. hadess, do you think this belongs in the color plugin again? It's very CM specific, and we can easily get the xdisplay and xroot there. Richard.
Created attachment 189449 [details] [review] patch for review New patch with fixes.
I don't feel very strongly one way or the other for that part of the code.
If you feel that there's any chance of having individual RANDR outputs have their own color profile, then yes, this code should live in GnomeRR. (Maybe it gets a bit hairier with the one-to-many CRTCs -> outputs, but you can't get things right in that case, anyway). If not, I'd rather have it in the color plugin.
Created attachment 189532 [details] [review] patch for review (In reply to comment #6) > If you feel that there's any chance of having individual RANDR outputs have > their own color profile, then yes, this code should live in GnomeRR. (Maybe it > gets a bit hairier with the one-to-many CRTCs -> outputs, but you can't get > things right in that case, anyway). Well, gnome-color-manager used to put the ICC profile as a property on the output, *as well* as putting it on the screen as was required by the spec -- but no apps used it, and so I removed the per-output out-of-spec idea. It may well come back in future versions of the spec, I dunno. > If not, I'd rather have it in the color plugin. Right, I do think adding the ability to delete and change properties in libgnome-desktop is a good idea tho -- which makes me think I've been being too CM specific. How about the attached patch which just adds gnome_rr_screen_change_property() and gnome_rr_screen_remove_property() -- that seems like a good compromise to me. Comments please. Thanks, Richard.
(In reply to comment #7) > Well, gnome-color-manager used to put the ICC profile as a property on the > output, *as well* as putting it on the screen as was required by the spec -- > but no apps used it, and so I removed the per-output out-of-spec idea. It may > well come back in future versions of the spec, I dunno. Apps are already used to looking at the monitor they are in, so I'd really rather have them query the output-specific ICC profile, instead of the (conceptually limited) per-screen profile. How about this - we maintain a gboolean gnome_rr_output_set_icc_profile (GnomeRROutput *output, guchar *icc, gsize icc_size, GError **error); which internally just sets the per-screen profile, and live with that for a while. If another output's profile is set, then either last-one-wins for the whole screen, or you figure out a way to really maintain per-output profiles, even outside the spec. > Right, I do think adding the ability to delete and change properties in > libgnome-desktop is a good idea tho -- which makes me think I've been being too > CM specific. How about the attached patch which just adds > gnome_rr_screen_change_property() and gnome_rr_screen_remove_property() -- that > seems like a good compromise to me. Comments please. * We already have gdk_property_change(). Any reason not to use this instead of reinventing that function? If it's about having a nice GError, I'm sure we can put such a function in GDK and deprecate the old one. * What about that "if (rc == BadRequest)" quirk? Does it only happen for the ICC-specific properties, or for any property change? (If the latter, that should *definitely* be encapsulated away in GDK). Again - I'd like to keep GnomeRR dealing with things related to RANDR outputs, not for things which just happen to require a screen and a root window :) If you keep ICC profiles per-output, go ahead; otherwise, please put this code somewhere else. Little exercise - I don't know how color apps deal with things right now - would the GIMP fetch the ICC profile for the output of its image window by calling XRRBlahBlah() and figuring out the output for the window, or would it rather have a convenience function close to GnomeRR? (Would the GIMP use the GnomeRR API (in which case we really need to document it and stabilize it), or should we start thinking of pushing this down to GTK+?) Or is the GIMP content to XGetProperty (the_root_window, "ICC...") and be done with it?
(In reply to comment #8) > How about this - we maintain a > > gboolean > gnome_rr_output_set_icc_profile (GnomeRROutput *output, > guchar *icc, > gsize icc_size, > GError **error); This might work. > * We already have gdk_property_change(). Any reason not to use this instead of > reinventing that function? If it's about having a nice GError, I'm sure we can > put such a function in GDK and deprecate the old one. Ahh, I forgot about gdk_property_change(). > * What about that "if (rc == BadRequest)" quirk? Does it only happen for the > ICC-specific properties, or for any property change? (If the latter, that > should *definitely* be encapsulated away in GDK). I'll do some experimenting and see what triggers it, I don't remember all the details now. > Again - I'd like to keep GnomeRR dealing with things related to RANDR outputs, > not for things which just happen to require a screen and a root window :) If > you keep ICC profiles per-output, go ahead; otherwise, please put this code > somewhere else. Fair enough :-) At the moment, I'm thinking of just using gdk_property_change in the color plugin, as the per-output stuff isn't in spec and I'm not sure it's worth spending lots of time on a solution that's going to be obsolete soon anyway. > Little exercise - I don't know how color apps deal with things right now - > would the GIMP fetch the ICC profile for the output of its image window by > calling XRRBlahBlah() and figuring out the output for the window In an ideal world it would, but i'm not sure it's terribly easy to get the xrandr output a window is (mostly) on. >, or would it > rather have a convenience function close to GnomeRR? Seems like the wrong layer. I did provide some convenience API for this once in g-c-m (gets the icc profile for a window xid) but I'm probably going to remove that for 3.2. > Or is the GIMP content to XGetProperty (the_root_window, "ICC...") and be done > with it? Yup, of the 7 or 8 apps that actually use _ICC_PROFILE, only 1 KDE app does the correct per-output atom tricks (using the xinerama suffix ID no less, urgh) and the other 6 (gimp included) just use XGetProperty(window, "ICC_PROFLE"). As my teacher would have said at school: "D- = MUST TRY HARDER" which is why I'm hoping to fix this properly and upset all the graphics applications in 3.4 using the GPU GLSL shader in mutter. I'll add this to the color plugin for now, and we can revisit later, thanks.