Add gamma control

[emersion]

This uses file descriptors instead of arrays to fix swaywm/wlroots#1135.

It also adds descriptions and clearer semantics. reset_gamma is removed in favour of using the destroy request, and a failed event is added.

As always, comments are welcome. Do we want anything else in this protocol?

[ddevault]

I think you should be able to disconnect and leave gamma as it was, no reason to leave a process hanging around. Consider that gamma ramps might be used for color tuning as well.

[emersion]

I think you should be able to disconnect and leave gamma as it was, no reason to leave a process hanging around. Consider that gamma ramps might be used for color tuning as well.

I really don't think it's a good idea. Setting an invalid gamma table can make your output unusable. In general Wayland clients cannot set permanent state and I think this is a good thing.

Color management will be more involved, so it'll most likely require a separate protocol (see https://lists.freedesktop.org/archives/wayland-devel/2016-November/031729.html).

[martinetd]

fwiw I run redshift in one-shot mode from key bindings, having it carrying on running from there is definitely not impossible but smooth transition from one step to the next might suck a bit... (would need to start the next redshift, wait until it applied its ramp, then kill the previous one; otherwise killing the previous one would reset the gamma ramp once and flicker the screen)

However I do agree that in general not being able to leave a broken state behind is a good thing, so I guess it's just a matter of fixing the usage pattern e.g. giving redshift a proper remote-control protocol instead.

[ddevault]

Can't they just leave it broken by fucking up the gamma and then not disconnecting?

[emersion]

giving redshift a proper remote-control protocol instead

Yeah this is the intended behaviour.

Can't they just leave it broken by fucking up the gamma and then not disconnecting?

Except it's way easier to debug and to fix. You know exactly who's responsible for this mess, and you can kill it to get a sane state back.

[ddevault]

How are you going to debug it, kill it, etc if your screens are hosed?

[martinetd]

giving redshift a proper remote-control protocol instead

Yeah this is the intended behaviour.

I meant a remote-control protocol to control redshift, not to adjust the gamma ramp. There's currently no way to manually adjust the screen temperature other than one-shot mode, which exits immediately after changing the temperature. I wonder why people figure time and location are any good to decide how red the screen should be in the days of artificial light -_-
But anyway, I'm just ranting at this point, I'll deal with it when this comes out however that turns out to be.

[agx]

LGTM. I'd also go for not resetting the gamma ramp when he client disconnects, since (as you wrote) it's for privileged clients.

[Ongy]

How are you going to debug it, kill it, etc if your screens are hosed?

ssh in? At least you got an offender, and can fix it without digging out a potentially new client that then just needs to do some repairs

[emersion]

This mechanism also ensures that there aren't two clients setting gamma ramps at the same time.

I meant a remote-control protocol to control redshift

Yeah, that's what I understood. I think redshift should allow this (doesn't it already allow this via its traybar icon menu?).

How are you going to debug it, kill it, etc if your screens are hosed?

If you know the offender, you can blindly open a terminal and pkill <bad-client>. If you don't, you can switch to another tty or ssh in and kill it.

I'd also go for not resetting the gamma ramp when he client disconnects, since (as you wrote) it's for privileged clients.

Privileged isn't an excuse to allow anything. For instance layer-shell is privileged but constrains a lot what the client is able to do.

When I discussed this protocol on #wayland a while ago ensuring there's no dirty state after a client exits was a concern. And I think it's a valid one.

[martinetd]

This mechanism also ensures that there aren't two clients setting gamma ramps at the same time.

Yeah that actually makes it so I won't be able to hack in two "set-and-daemonize" one-shot instance of redshifts to set next gradiant of gamma before killing the previous one, I almost complained when I realized that I would actually have to give this a proper control implementation instead :P

tbh I think this would be fine with a "latest who gives an order wins" design like it does on X11, would just need to be careful on reset to reset the previous last state on exit (and not reset anything if exiting client wasn't the last one), but both are valid designs so I don't really care either way.

I meant a remote-control protocol to control redshift

Yeah, that's what I understood. I think redshift should allow this (doesn't it already allow this via its traybar icon menu?).

My bad, I had read your answer with "this" being "this new protocol".

You just made me install redshift-gtk to try the tray icon, it doesn't seem to have any gimmick other than disabling redshift altogether temporarily, but I might be missing out. It does this with SIGUSR1 == toggle, so that's not really exactly extensible; the report info given in the widget come from stdout of the redshift daemon.
So no, will have to do something from scratch.

FWIW though, Quartz (OSX) has the same behaviour of resetting the gamma ramp when redshift exits, so you can pretend we're being cool by doing like Apple (sorry, couldn't resist)
Anyway, it's not overly difficult to make manual mode read from stdin and adjust everytime it gets a line and use a pipe or something, it's just one more thing to do -- not changing grumpy people's habits isn't a valid reason to not do things you think are right.

[minus7]

Maybe define it here in the protocol that it's not possible to obtains a second object for the same output (so it's not implementation-defined whether the second one will invalidate the first one or not). Or the other way around if that makes more sense (I think it doesn't)

[minus7]

Oh, I see, the decision what to do should lie with the compositor. Maybe change the wording to "There can only be.." so it's clear that it's not the user's duty to check if they're the only one.

[emersion]

Right.

[minus7]

Oh, I see, it's the failed event, but what about the error enum? That doesn't seem to be used anywhere

[emersion]

No. Protocol errors are reserved for situations in which the client does something wrong. Protocol errors are suitable for a client passing a gamma with an invalid size, but are unsuitable for signaling that an output doesn't support gamma control.

[agx]

Maybe it would be good to reset the gamma table not when gamma_control is destroyed but when the gamma_control_manager is destroyed? This would

• avoid having the app to hold the gamma_control all the time (can release it when after updating the table)
• would still give a sane output when the client disconnects

[emersion]

• I'm not sure what the upside is, since you'll need to hold the gamma_control_manager instead of the gamma_control?
• Clients are supposed to keep the gamma_control around to keep a "lock" on the output and prevent other clients from controlling the same output. How would this mechanism work with your proposal?
• A common Wayland idiom is to create the manager, create the control, and immediately destroy the manager. This is done with wl_region for instance (clients create a region, assign it to a surface, and then immediately destroy it).

[agx]

The upside is since the manager is the singleton global the client is unlikely to dismiss it since it will need it anyway to handle monitor hotplug. (in your region example the wl_compositor is the singleton global not the region). I also wonder how much sense it makes to have different clients to be able to keep a lock for different outputs.

[minus7]

Oh, I see, it's the failed event, but what about the error enum? That doesn't seem to be used anywhere

[emersion]

what about the error enum? That doesn't seem to be used anywhere

It's used by wl_resource_post_error in the compositor.