GNOME Bugzilla – Bug 742149
Integrate redshift filter in GNOME
Last modified: 2017-03-08 14:48:10 UTC
Created attachment 293518 [details] [review] The initial patch Redshift is a software which changes the color temperature of the connected displays based on the current light conditions. It uses the computers location to calculate the sun rise/set times and lowers the color temperature from the white point (6500K) down to e.g. 3700K at night as most artificial light sources have a lower color temperature. Both redshift and the color management system need to adjust the gamma curve. This is why I think that it makes sense to integrate such a feature into the color plugin of gnome-settings-daemon. The attached patch is an initial draft for such a feature. I have not implemented any brightness change support (the original redshift has such a feature). The general idea is to just add a couple of settings keys which will be managed by a small gnome-shell extension. This patch adds two settings keys to org.gnome.settings-daemon.plugins.color: * adjust-color-temperature: Whether the color temperature of attached monitors should be adjusted * color-temperature: The requrested color temperature. 6500K is an identity transform. I copied the adjustment table from the original redshift implementation. Any thoughts?
The original redshift implementation is at https://github.com/jonls/redshift
Oh, with this patch some more changes in gnome-control-center might be required to force a linear gamma curve during display calibration.
Another thing I failed to mention is that I tried to get the white point from lcms2. This did not work for me for two reasons: * The neutral point (1,1,1) in sRGB had a color temperature of D50/5000K in my tests. But as I read the standard it should be at 6504 Kelvin. * It would not return valid sRGB colors below 4000K
An experimental extension is now available at https://github.com/benzea/gnome-shell-extension-redshift. It is a bit broken (i.e. sometimes the geoclue connection does not work), but it does work fine for me already. It either uses geoclue2 + gweather to get sunrise/sunset and calculate a color temperature from that, or it allows the user to slide the color temperature between night and day.
So, something we also want to consider is for ambient light sensors; so we now have four things wanting to control the backlight and gamma curves: * User brightness level (brightness) * Selected color profile (RGB, perhaps brightness) * Redshift (RGB) * Ambient light sensor (brightness, perhaps RGB) We also want to consider non-primary panels in the future, of which the color profile might also be different. Benjamin, if you don't have a ALS sensor in your laptop and want a free ColorHugALS device, send me your address on email and I'll get one to you when they're ready. Richard
Hey, actually just bought the device :) So, I simply updated the patch to instead read an RGB white point from gsettings. What I am now wondering about is what the colorhug ASL actually gives me, as it is reporting an insanely large values for red in the UI.
Now that Gnome got automatic brightness adjustment, it would also be great to integrate functionality like redshift, or f.lux in the standard system. It's already in Windows and Apple. I think it really helps for late work sessions. Redshift is great but is often behind on the updating to new gnome versions making it unusable. F.lux works really well, but you need to know about it to add the repo. I figure it would be great to include this in Gnome as standard.
We wouldn't be able to implement this in gnome-settings-daemon in Wayland, so duplicating the bug against the gnome-shell bug asking the same feature. *** This bug has been marked as a duplicate of bug 741224 ***
Created attachment 344236 [details] [review] Patch using libcolord This is using libcolord to generate the RGB factors rather than hardcoding a table directly.
Review of attachment 344236 [details] [review]: How does this interact with the power plugin's automatic brightness, or even manual brightness? ::: plugins/color/gsd-color-manager.c @@ +148,3 @@ + if (manager->priv->bus_cancellable != NULL) { + g_cancellable_cancel (manager->priv->bus_cancellable); + g_object_unref (manager->priv->bus_cancellable); g_clear_object(); @@ +153,3 @@ + + if (manager->priv->introspection_data) { + g_dbus_node_info_unref (manager->priv->introspection_data); g_clear_pointer(); @@ +160,3 @@ + + if (manager->priv->name_id != 0) + g_bus_unown_name (manager->priv->name_id); set to 0 as well. @@ +170,3 @@ +static void +handle_method_call (GDBusConnection *connection, This is a no-op, remove it. @@ +214,3 @@ + g_set_error (error, G_DBUS_ERROR, G_DBUS_ERROR_FAILED, + "Failed to get property: %s", property_name); + } linefeed afterwards @@ +229,3 @@ + GsdColorManager *manager = GSD_COLOR_MANAGER (user_data); + GsdColorManagerPrivate *priv = manager->priv; + gint32 temperature; Move to within the branch that uses it. @@ +239,3 @@ + if (g_strcmp0 (property_name, "Temperature") == 0) { + g_variant_get (value, "i", &temperature); + if (temperature < 1000 || temperature > 10000) { Why? This should be a proper dbus error instead, that mentions that the error is due to invalid args. Should move the hard-coded values to constants (#defines) as well. @@ +255,3 @@ +static const GDBusInterfaceVTable interface_vtable = +{ + handle_method_call, Should be NULL. @@ +282,3 @@ + + infos = priv->introspection_data->interfaces; + for (i = 0; infos[i] != NULL; i++) { Does infos ever contain more than 1 entry? No need for the loop there. ::: plugins/color/gsd-color-state.c @@ +1490,3 @@ NULL); + /* default color temperature (==unity) */ unity? This might need a better explanation. ::: plugins/color/gsd-color-state.h @@ +50,3 @@ void gsd_color_state_stop (GsdColorState *state); +void gsd_color_state_set_temperature (GsdColorState *state, + gint temperature); Why is this not a guint? You only allow a minimum of 1000 for the value, so you can use 0 as an error.
Created attachment 344302 [details] [review] Patch using colord v2 (In reply to Bastien Nocera from comment #10) > How does this interact with the power plugin's automatic brightness, or even > manual brightness? It doesn't, and can't. Doing brightness using the gamma table would result in horrible posterisation of images, and using the hardware backlight level would only work for internal panels. We're not really trying to limit the brightness at all with the Redshift functionality, just the display color temperature. All of the other review comments: agreed, and fixed, thanks for the speedy review.
Review of attachment 344302 [details] [review]: ::: plugins/color/gsd-color-manager.c @@ +154,3 @@ + } + + if (manager->priv->introspection_data) { No, just use g_clear_pointer, it does the NULL check. @@ +192,3 @@ + + if (g_strcmp0 (property_name, "Temperature") == 0) { + gint temperature; gint32 @@ +230,3 @@ + G_IO_ERROR, + G_IO_ERROR_INVALID_ARGUMENT, + "%" G_GUINT32_FORMAT "K is smaller than " On one line. @@ +239,3 @@ + G_IO_ERROR, + G_IO_ERROR_INVALID_ARGUMENT, + "%" G_GUINT32_FORMAT "K is larger than " Ditto. @@ +290,3 @@ + GSD_COLOR_DBUS_NAME, + G_BUS_NAME_OWNER_FLAGS_NONE, + NULL, It will need to handle the name being acquired and lost as well. This is especially important as you'll need to avoid poking at anything until you own the name. @@ +299,3 @@ +register_manager_dbus (GsdColorManager *manager) +{ + GsdColorManagerPrivate *priv = manager->priv; Add a linefeed here. ::: plugins/color/gsd-color-state.h @@ +50,3 @@ void gsd_color_state_stop (GsdColorState *state); +void gsd_color_state_set_temperature (GsdColorState *state, + guint temperature); Why do you get an int32 from D-Bus but pass a guint? Wouldn't it be better to use a uint32 in the D-Bus API? ("u" type)
Created attachment 344413 [details] [review] v3, with fixes This does the exit-on-name-lost as requested which means you can't have more than one instance running.
Review of attachment 344413 [details] [review]: ::: plugins/color/gsd-color-manager.c @@ +191,3 @@ + gint temperature; + temperature = gsd_color_state_get_temperature (priv->state); + retval = g_variant_new_int32 (temperature); uint32. @@ +222,3 @@ + if (g_strcmp0 (property_name, "Temperature") == 0) { + guint32 temperature; + g_variant_get (value, "i", &temperature); I'm guessing this wasn't tested ;) ::: plugins/color/gsd-color-state.c @@ +1491,3 @@ + /* default color temperature of D65 is a RGB value of [1.0,1.0,1.0] */ + priv->color_temperature = 6500; make that a define somewhere? ::: plugins/color/gsd-color-state.h @@ +50,3 @@ void gsd_color_state_stop (GsdColorState *state); +void gsd_color_state_set_temperature (GsdColorState *state, + guint temperature); Would have been better to ensure a guint32, but that's good enough, most likely.
Created attachment 344533 [details] [review] v4, with fixes Mea culpa. New patch with issues fixed. This time tested too. Thanks
Review of attachment 344533 [details] [review]: Looks good otherwise. ::: plugins/color/gsd-color-manager.c @@ +176,3 @@ +{ + GVariant *retval = NULL; + GsdColorManager *manager = GSD_COLOR_MANAGER (user_data); No need for the cast here. @@ +208,3 @@ + GError **error, gpointer user_data) +{ + GsdColorManager *manager = GSD_COLOR_MANAGER (user_data); Nor here. ::: plugins/color/gsd-color-state.c @@ +174,3 @@ +void +gsd_color_state_set_temperature (GsdColorState *state, guint temperature) This... @@ +182,3 @@ + +guint +gsd_color_state_get_temperature (GsdColorState *state) ...and this are semi-public APIs, can you please add guards to it? @@ +408,3 @@ + /* get the color temperature */ + cd_color_get_blackbody_rgb (color_temperature, &temp); Do you need to check for errors here? @@ +1490,3 @@ NULL); + /* default color temperature is RGB value of [1.0,1.0,1.0] */ Move the comment to where you define the constant.
Comment on attachment 344533 [details] [review] v4, with fixes Great, thanks!
Created attachment 345453 [details] [review] add extra property for the shell This makes the shell UI easy to implement without hardcoding 6500K = inactive.
Review of attachment 345453 [details] [review]: Looks fine otherwise. ::: plugins/color/gsd-natural-light.c @@ +44,3 @@ gdouble cached_sunset; gdouble cached_temperature; + gboolean cached_active; This isn't a cache. Just "active" would do fine.
Can you please not post patches in closed bugs in the future. Learn to use git-bz to open your bugs with patches.
Comment on attachment 293518 [details] [review] The initial patch Marking as needs-work as we've updated it.
As it currently is, the filter would break when launching certain kinds of applications such as NVIDIA-Settings and fullscreen applications under wine. Switching night mode off and back on enables the filter again until the focus is given back to the fullscreen app, which makes the filter break once again. As such, it is impossible, for instance, to play games with wine while keeping the filter. Redshift-gtk's filter also breaks but enables itself again after a fixed number of seconds and thus overrides the full screen app. I think Redshift-gtk's behavior is more consistent and hurts the eyes less.
Why are NVIDIA-Settings and wine touching the system gamma tables? This (thankfuly) isn't going to work under Wayland. I don't think polling every few seconds setting the values over and over and over is the correct fix.
Comment on attachment 345453 [details] [review] add extra property for the shell Was fixed in. commit 1e1fccb80e576b313f6730b26f385e0d6426ed4d Author: Richard Hughes <richard@hughsie.com> Date: Fri Feb 10 12:46:17 2017 +0000 color: Add a NaturalLightActive property to the D-Bus interface This makes the shell UI easy to write as we don't need to compare the temperature property against 6500K. https://bugzilla.gnome.org/show_bug.cgi?id=742149
Please file other problems with the feature as separate bugs.