Wayland support PR (updated Lourens rich master) #568
Conversation
This uses a protocol extension, not in core Wayland.
…gamma A compositor may want to restrict the access to the gamma_control_manager interface, to avoid clients changing the gamma without the user knowing. With the new protocol extension redshift request permission to the compositor to bind the gamma control global, so that the compositor can grant/deny it, or ask the user for input.
Try to test Sway redshift support
…Lourens-Rich-master
|
Known issues (please help):
|
src/gamma-wl.c
Outdated
| state->outputs = malloc(sizeof(struct output)); | ||
| } else { | ||
| state->outputs = realloc(state->outputs, state->num_outputs * sizeof(struct output)); | ||
| } |
There was a problem hiding this comment.
This would be clearer:
num_outputs++;
state->outputs = realloc(...);
|
I'd like to test locally, but can't get this to compile (on Linux). Bootstrap and configure steps work fine, but the actual build fails with the compiler errors below. Am I doing something silly? |
|
@atheriel I have a fixed version of this PR available at https://github.com/t-8ch/redshift/tree/wayland |
|
@t-8ch Your branch does indeed compile and work for me. I can confirm that normal invocation works (i.e. |
|
@breznak Wayland on Windows will not work in any case, it should be disabled in |
@t-8ch just merged the changes, the Windows CI is still failing, as expected, as you said. My point is, if redshift was used on Windows (was it ever?), then we should fallback to the "non-wayland" paths there. |
configure.ac
Outdated
| @@ -67,6 +67,12 @@ PKG_CHECK_MODULES([XF86VM], [xxf86vm], [have_xf86vm=yes], [have_xf86vm=no]) | |||
| PKG_CHECK_MODULES([XCB], [xcb], [have_xcb=yes], [have_xcb=no]) | |||
| PKG_CHECK_MODULES([XCB_RANDR], [xcb-randr], | |||
| [have_xcb_randr=yes], [have_xcb_randr=no]) | |||
| PKG_CHECK_MODULES([WAYLAND], [wayland-client wayland-scanner], [have_wayland=yes], [have_wayland=no]) | |||
| AC_PATH_PROG([wayland_scanner], [wayland-scanner]) | |||
There was a problem hiding this comment.
These 5 lines appear to be causing the build to fail on Travis (for non-Wayland Linux builds, macOs builds, and Windows builds), which I think is a legitimate issue. The configuration script should only look for WAYLAND_SCANNER if Wayland is available. It is not doing that at present, but I'm not familiar enough with autoconf syntax to know precisely why.
There was a problem hiding this comment.
Thanks for finding the culpit and fixing that, guys! Merged the update and triggering a new CI run here...
| @@ -67,6 +67,8 @@ PKG_CHECK_MODULES([XF86VM], [xxf86vm], [have_xf86vm=yes], [have_xf86vm=no]) | |||
| PKG_CHECK_MODULES([XCB], [xcb], [have_xcb=yes], [have_xcb=no]) | |||
| PKG_CHECK_MODULES([XCB_RANDR], [xcb-randr], | |||
| [have_xcb_randr=yes], [have_xcb_randr=no]) | |||
| PKG_CHECK_MODULES([WAYLAND], [wayland-client wayland-scanner], [have_wayland=yes], [have_wayland=no]) | |||
| PKG_CHECK_VAR(WAYLAND_SCANNER, wayland-scanner, wayland_scanner) | |||
There was a problem hiding this comment.
@t-8ch there must be a bug it the precent PR merged, as now both Lin & Win CI fail on wayland detection... Could you have a look please?
[wayland] fix build with old pkg-config or without wayland
|
Can you print the stacktrace? |
|
Dear @t-8ch , BT and BT full below: and |
|
@breznak |
@t-8ch I did put the guard, but that's not the problem..the issue seems Why is it NULL?
Well, I have "system" redshift (xrandr), which I killed. The redshift from this branch "worked" in a sense that I had the backtrace... The compiled redshift works OK on X. |
|
@breznak The guard leads at least to a correct diagnosis. |
Yup. So there goes down ... #55 (comment) Can we at least steal the gamma correction code from Gnome,KDE to make them supported here as well, as an alternative? |
|
AFAIK the compositors of Gnome and KDE have this included directly and don't expose any interface (yet alone a stable one) to change it from the outside. |
|
Thanks for keeping interest in this PR, redshift is awesome and I wouldn't live without it anymore 👍 This isn't 100% reliable for me in one-shot mode, I get this trace on the client: But server sometimes takes the destroy event before set_gamma (I sometimes never see set_gamma in logs on server side) The problem is that when we call wayland_free, we don't wait for the callback to finish but just destroy it to get on with it -- and then destroy the gamma control before set_gamma got through. diff --git a/src/gamma-wl.c b/src/gamma-wl.c
index b0617d6..8e7a427 100644
--- a/src/gamma-wl.c
+++ b/src/gamma-wl.c
@@ -207,8 +207,11 @@ wayland_restore(wayland_state_t *state)
static void
wayland_free(wayland_state_t *state)
{
-
- if (state->callback) {
+ int ret = 0;
+ while (state->callback && ret >= 0) {
+ ret = wl_display_dispatch(state->display);
+ }
+ if (state->callback) { /* ret < 0, log error? */
wl_callback_destroy(state->callback);
}
An alternative would be to not try to be asynchronous and just use wl_display_roundtrip() at the end of wayland_set_temperature, to ensure that when this function returns the temperature has indeed been set, instead of the promise that it will be set that we currently have. |
|
@martinetd thanks for the report and fix! Could you please make a commit in your branch, so I can easily bring it here? |
This prevents destroying gamma control before gamma_control_set_gamma() has been effectively handled by the wayland server, otherwise we risk not setting gamma at all in one-shot mode (or miss the last set temperature call)
|
Eh, I don't particularily like forking a project for a single commit, but I guess that's the github way... :) |
gamma-wl: wait for sync callback on destroy
|
Which window managers are supported? As far as I know this is not generic Wayland support, right? Can we give the adjustment method a name that reflects this? Some questions:
|
|
|
@jonls Would you be open for a |
|
This implementation doesn't work if your gamma ramps are larger than wayland's internal buffer of 4096 bytes (swaywm/wlroots#1135). @emersion has updated the protocol to use a fd to pass the gamma ramps instead of passing them as arguments (swaywm/wlr-protocols#24; see changes e.g. here). I adapted the changed protocol in the wayland branch in my fork if anyone wants to give it a try. Related PRs for wlroots and sway: swaywm/wlr-protocols#24 swaywm/wlroots#1157 swaywm/sway#2325 |
FYI GNOME and KDE probably won't ever support a standard protocol because they now have this functionality built-in in the compositor. |
|
@emersion Do you have confirmation that those projects won't accept a common protocol to allow third party implementations? I don't see why they wouldn't per se. |
Installed new redshift version from here: jonls/redshift#568 Tweaked systemd service files and now it's finally working as I wanted it to.
|
What's the state of this PR ? I'd love to use redshift on wayland/sway! |
btw, wayland (or atleast on KDE) has it's way of "Night mode" |
Fixes #55
Replaces: #558