search

jonls / redshift

Redshift adjusts the color temperature of your screen according to your surroundings. This may help your eyes hurt less if you are working in front of the screen at night.
http://jonls.dk/redshift
GNU General Public License v3.0
5.89k stars 428 forks source link

"stdin" manual provider #690

Open martinetd opened 5 years ago

martinetd commented 5 years ago

Is your feature request related to a problem? Please describe.

The new wlr-gamma-control protocol (used by wlroots based wayland compositors like sway, and implemented in https://github.com/minus7/redshift/tree/wayland ) has a "feature" that the gamma adjustment resets when the client exits like Quartz.

I'm exclusively using redshift manually with a script that lets me bump the temperature up/down on key bindings, like brightness, and while it is easy to make redshift not exit until it is killed there is a small flicker when I replace an instance of redshift with another.

Describe the solution you'd like I'd like to be able to implement a trivial "stdin" or "manual control" provider - something simple that would just read from stdin continuously (blocking), try to convert each line of input to a temperature and if it could just apply it. The problem is that providers aren't meant to provide a temperature directly, but a location that will let the main loop compute where the sun is and what temperature should be used -- the plugin model would need to be adapted to have the provider give either location or temp.

The least bad approach I can think of for that would be to not have the provider return location but something that would be an union of location or temperature, and the available bool could be changed to an enum going unavailable/location/temperature, that the main loop could act on accordingly.

Describe alternatives you've considered Complain loudly to wlroots folks to have an option to not reset the ramp on exit? But I already did that, not sure it'd have much chance of working :P

I don't think it's possible to backcompute a location that would get the desired temp, that might be another way to go if it were.

A more plausible alternative to both would be to just implement that alongside the other modes, e.g. a PROGRAM_MODE_MANUAL_STDIN that would just do that -- it's trivial to do and doesn't need anything changed, just not as elegant.

Additional context I'm more than willing to implement whatever direction you prefer - I just don't want the "wayland" fork to drift further apart and would like to submit it here even if there is no real reason to have this mode for X11. It would still be useful for people using Quartz I suppose. Would this make sense for you? What solution do you prefer?

Thanks!

martinetd commented 5 years ago

bump, any comment please? I'm really open to anything on the technical side, and am happy to do the grunt work as required; I'd just like to keep my usage of redshift simple and controlled. Time of the day/position of the sun doesn't mean anything to me.

Thanks.

piater commented 5 years ago

I am also interested in talking directly to a running redshift process. My use case is the brightness setting. I have an OLED display; none of the backlight-adjusting tools work because, well, I don't have a backlight. I have been using xrandr --brightness, which adjusts brightness persistently, but there is no such tool for Wayland. As a generic interface, dbus looks like a good way to go. A simpler but less elegant solution might be to use a signal to make redshift reread its config file. Either would probably also address @martinetd's concerns. For my use case, redshift might read /sys/class/backlight/backlight/brightness. I could lift code from minus7's redshift wayland branch and add just what I need, but I think it makes sense to add more generic functionality upstream. I'm willing to contribute code, but would like to settle on a technical solution first.

On a related note, I frequently invert the colors of my screen (also using xrandr; keeping most pixels dark saves considerable energy on OLED displays and is sometimes easier on the eyes). @jonls, would you accept code implementing such functionality for redshift? If not, I'll write my own tool after all.

martinetd commented 5 years ago

I'd rather not depend on having a redshift config to reload if it can be helped but I guess that could work... I'm not really a fan of dbus either, to be honest, but at this point we're stuck with it so why not (although I find just reading stdin much simpler) - technically it's not very different from what I suggested though, the way providers are coded a provider has no way to directly provide temperature/brightness to the redshift backend, so that is what's going to need changing first.

I'm still open to helping as well, but, yeah, need to decide on something first :)

piater commented 5 years ago

OK, I have forged ahead. Here are three patches on top of @martinetd's wayland branch commit 420d0d5 (and not on top of @jonls' upstream because I need this for wayland):

  1. 0001 originated while working on 0002. It enables type checking of function pointers in gamma_method_t by properly defining the opaque type struct gamma_state in each of the gamma-*.c.
  2. 0002 adds a new -i option to invert colors by inverting gamma ramps.
  3. 0003 adds a new -C option to continually read commands from stdin or a fifo.

I compiled and tested them only under Linux using randr and wayland.

The first two patches should be uncontroversial; they are simple and unobtrusive.

The third is also simple and unobtrusive, but the way it is implemented may be up for debate. I took the following decisions:

There is room for improvement here, but I think this should be done by a major refactoring of redshift.c under the premise that any option can change at any time.

As it stands, it works for me, and I will soon start using it in production. I'd love these patches - or something better - to be applied upstream (by @martinetd and/or @jonls).

martinetd commented 5 years ago

Neat, I'm definitely ok with something that just read commands continuously, it's just a matter of formatting and does solve the provider issue I described neatly.

I'll give this a spin and update my scripts to use this mode in the next few days.

Just reading the commits though a few comments:

  1. looks good to me, can be either taken together or separately so I'd suggest opening this in a separate PR to get it early on if @jonls is happy with it
  2. haven't looked much into it but is definitely unrelated, I'd also recommend getting this in separately.
  3. definitely has some issues, e.g. the g_strfreev you commented out would really need understanding why it doesn't work, even if the leak is not "serious" :] I'd also think stuff like EOF should be handled more gracefully, I don't think it should be an error like it currently is. hard limit on line length is probably fine though I have no opinion there.

Anyway, will give this a try; if it works I don't mind taking the patches in my branch but my tree definitely is not 'upstream'; don't expect too much from this alone.

piater commented 5 years ago

  1. looks good to me, can be either taken together or separately so I'd suggest opening this in a separate PR to get it early on if @jonls is happy with it

  2. haven't looked much into it but is definitely unrelated, I'd also recommend getting this in separately.

Patch 2 depends on Patch 1, but the two are unrelated to Patch 3. @jonls and @martinetd, do you need me to fork your repos on github and create pull requests (dreaded enforced github workflow), or can you pick up the patches directly?

  1. definitely has some issues, e.g. the g_strfreev you commented out would really need understanding why it doesn't work, even if the leak is not "serious" :]

Absolutely :-) It looks like getopt()'s pointer mangling interferes with glib's internal data structures. I replaced g_shell_parse_argv() by strtok() (losing the automatic handling of quoted arguments containing spaces, but I don't think this hurts us here). Valgrind is happy now. I updated the patch.

I'd also think stuff like EOF should be handled more gracefully, I don't think it should be an error like it currently is.

Pipes are weird beasts. Once the last writer quits, the reader receives an EOF, and the connection is severed. If the pipe was connected to stdin, you're screwed. A fifo you could close and reopen, but fopen() would block until someone opens it for writing again. With open() one would have some more freedom, but using that would require a different mix of open() and fgets(), which is also hairy. As I understand, fopen() in "r+" mode is the canonical way to handle this: Just open it for writing yourself. Then you don't block waiting for a writer, and there is always a writer (yourself) who keeps you from receiving an EOF. Alternatively, you can tail -f the fifo into the reader, but this requires extra setup and one more process. Lots of interesting stuff I learned while coding this up :-)

my tree definitely is not 'upstream'; don't expect too much from this alone.

Someone packaged your wayland branch for the Arch User Repository; this is where I found it. So your tree is a sort of de-facto "upstream" for some number of users out there, whether you want it to be or not :-)

martinetd commented 5 years ago

do you need me to fork your repos on github and create pull requests (dreaded enforced github workflow), or can you pick up the patches directly?

I hate github for that as well and have no problem with the gists, personally. 1 applies fine on both master and my branch; 2 only applies on my branch because you touched gamma-wl.c (obviously) so I'll need to carry part of it anyway; it should merge just fine if @jonls picks it though. I don't know how he prefers to work though, git am is a bit of a pain for unclean patches, I'll let him comment :)

Back to the 3rd patch, it doesn't apply on either branch and it took me a while to apply manually because the format isn't exactly the same as what my patch program likes (the 'modified xxx' part, it's not what I get with git format-patch?); anyway, it was just some 3 lines offset in redshift.c but I did say git am was a pain...

So now I have this checked out (branch here for convenience), a few more comments:

Someone packaged your wayland branch for the Arch User Repository; this is where I found it. So your tree is a sort of de-facto "upstream" for some number of users out there, whether you want it to be or not :-)

Eeep, first time I've heard of this. . . Well, I guess it's AUR as usual :)

piater commented 5 years ago

Back to the 3rd patch, it doesn't apply on either branch and it took me a while to apply manually because the format isn't exactly the same as what my patch program likes

Sorry about that. I wanted one patch that spans two commits instead of a two-patch series. I won't do that again...

  • redshift still inits a location provider immediately after starting, which on my setup kills redshift after a few seconds

Hmm... I did not try that; I hard-coded my location in the config file. I'll check that out.

  • re: EOF, yeah, it's a bit tricky. Actually now I ran the program I noticed you went out of your way to pass an argument for what file to open

Our use cases differ. You want a single feeder process, which, when it exits, might as well take redshift with it. Here, a fifo does not really buy you anything. I, in contrast, want to feed something into redshift at any time, by any means. For example, I'll configure my brightness keys to launch a little utility that writes the respective command into redshift. So, for this utility to know where to write to, I need something like a fifo anyway.

I put it the explicit file opening because it's only a few lines of code and avoids extra plumbing with a separate tail -f process. And it's quite common for UNIX utilities to accept filenames on the command line, with - meaning standard in/output.

  • re: argument parsing and corruption on free: yeah, actually, it's not getopt but the processing itself that has some issues... For example options->provider_args is not copied but the pointer is used directly,

Good catch. I'll take a look.

Not sure when I'll get to it though. This brief, fun programming interlude may be over for a while...

martinetd commented 5 years ago

Sorry about that. I wanted one patch that spans two commits instead of a two-patch series. I won't do that again...

Hmm, out of curiosity how did you do that? Even something like git diff HEAD^^ doesn't give me that format.

Hmm... I did not try that; I hard-coded my location in the config file. I'll check that out.

Yeah I don't have any config file, so it tries the default geoclue provider; I guess I can just pass a -l 0:0 for now but it's not required in one-shot mode so it's a bit of a downside.

Anyway, even then I can't get -O to work at all; I hadn't read the patch thoroughly enough and was convinced the continual mode was a new one, but it's not and the way it works really is too closely related to location / automatic adjustments for gamma to work for oneshot type commands so my use-case is pretty much screwed there... Ah, I can work around this whole thing by forcing the range interval, e.g. instead of -O 2200 I can pass -t 2200:2200 and get that applied by force; hmm. Feels somewhat of an overkill to have redshift continuously monitor time for naughts but I guess I can live with that... Prehaps hacking the timeout from 5s to a few hours to prevent redshift from waking up the cpu all the time on battery if gamma/brightness ranges are empty...

Our use cases differ. You want a single feeder process, which, when it exits, might as well take redshift with it. Here, a fifo does not really buy you anything. I, in contrast, want to feed something into redshift at any time, by any means. For example, I'll configure my brightness keys to launch a little utility that writes the respective command into redshift. So, for this utility to know where to write to, I need something like a fifo anyway.

I put it the explicit file opening because it's only a few lines of code and avoids extra plumbing with a separate tail -f process. And it's quite common for UNIX utilities to accept filenames on the command line, with - meaning standard in/output.

I actually want the same thing (binding some script to gamma keys, e.g. shift + brightness up/down); but it's more of a "standard unixy thing" than anything else; in my head it should behave like cat or similar programs. As long as the fifo is open for writing as you commented we won't get eof (that's different from cat admitedly) so I think exiting on eof makes sense; in case of stdin, e.g. a pipe (e.g. someone running tail -f | redshift -C -) I really don't see any reason not to quit when the tail process is killed or ^D is pressed.

argument parsing

No hurry as far as I'm concerned, I'm not sure what kind of option you were passing to get memory corruptions with g_strfreev but at least with the current version asan does not seem to trip on any use after scope with what I tried so I guess it's "good enough". I'll give this a more thorough check when I get around to proper testing and fixing my scripts.

(This also was a good occasion for me to play with the brightness setting of redshift, it's fun to see it behave quite differently from the built-in brightness of my screen, I might be able to get some interesting usage out of this as well by combining the two somehow; I wonder if battery usage cares about which way the brightness is tuned... Well, that's for later anyway)

piater commented 5 years ago

Hmm, out of curiosity how did you do that? Even something like git diff HEAD^^ doesn't give me that format.

I used magit-diff from emacs, specifying a 2-commit range. It should map to git diff commitA..commitB. No idea why this appears to be different...

Ah, I can work around this whole thing by forcing the range interval, e.g. instead of -O 2200 I can pass -t 2200:2200

This is exactly what I had in mind for this use case :-)

Feels somewhat of an overkill to have redshift continuously monitor time for naughts but I guess I can live with that... Prehaps hacking the timeout from 5s to a few hours to prevent redshift from waking up the cpu all the time on battery if gamma/brightness ranges are empty...

How about this: If -C is given and temperature/brightness ranges are empty, select() is called without a timeout?

I think exiting on eof makes sense; in case of stdin, e.g. a pipe (e.g. someone running tail -f | redshift -C -) I really don't see any reason not to quit when the tail process is killed or ^D is pressed.

I don't have a strong opinion on this; if you or someone else prefers the filename argument of -C to be ripped out, then so be it.

I'm not sure what kind of option you were passing to get memory corruptions

Independently of what caused the memory corruption or whether such still happen: How about declaring cmdbuf and argv in options_parse_continual_cmds() static? Then, this argv would behave just like people would expect something called argv to behave. Principle of least surprise. Defensive programming.

martinetd commented 5 years ago

How about this: If -C is given and temperature/brightness ranges are empty, select() is called without a timeout?

Yeah that was more or less my idea; I said a few hours because I just hard-coded that for now for my own use :p

I don't have a strong opinion on this; if you or someone else prefers the filename argument of -C to be ripped out, then so be it.

I don't mind having to pass a filename or -, it's the "does not exit on EOF" that doesn't feel natural to me :)

Independently of what caused the memory corruption or whether such still happen: How about declaring cmdbuf and argv in options_parse_continual_cmds() static? Then, this argv would behave just like people would expect something called argv to behave. Principle of least surprise. Defensive programming.

Hmm, I don't really like static variables in general but I guess there is little harm here; no opinion. We're already non-reentrant with strtok anyway... I'd only change cmdbuf though, if you're going with that, argv does not really matter as things will point to the substring in cmdbuf and not argv anyway.

Anyway, my script has been updated to use this, I'm pretty happy with how it turned out. I'll push this as my wayland branch once the last few points open here settle down a bit. Would still prefer to get both your first two patches and this properly integrated into @jonls tree eventually; If it's controversial I'd rather not keep it in my tree at least. For whenever he has time to look at this :)

piater commented 5 years ago

How about this: If -C is given and temperature/brightness ranges are empty, select() is called without a timeout?

Or, since it waits only on a single fd, skip it altogether and have fgets() block.

I don't mind having to pass a filename or -, it's the "does not exit on EOF" that doesn't feel natural to me :)

Just pipe your commands directly into redshift -C -; then it will exit on EOF. Or, send it a q :-P

How about declaring cmdbuf and argv in options_parse_continual_cmds() static?

I'd only change cmdbuf though, if you're going with that, argv does not really matter as things will point to the substring in cmdbuf and not argv anyway.

This was my first thought too, but nothing prevents options_parse_args() or its callees from keeping a pointer to argv itself around and stick it into something persistent like options. Better play it safe.

Anyway, my script has been updated to use this, I'm pretty happy with how it turned out. I'll push this as my wayland branch

Great, then the AUR package will pick it up :-)

once the last few points open here settle down a bit.

If you trust me enough to give me commit access to your repo, we can streamline the settling-down a bit. I'd only ever push to your piater branch, and only after discussing with you.

piater commented 5 years ago

I discovered the reason for some confusion above: I was confused. My patches above applied to @minus7's wayland branch, not @martinetd's. Both are largely the same though, fortunately; the wayland code in @martinetd's tree is from @minus7. Also, the AUR packages @minus7's wayland branch, not @martinetd's. Sorry...

I'll continue to interact here, working on @martinetd's tree from now on, since everything should go upstream to @jonls anyway.

piater commented 5 years ago

I created four new patches. 0001 and 0003 are trivial and address consistency; 0004 declares cmdbuf and argv static as suggested above.

The interesting one is 0002; it fixes the issue that dynamic location prevented commands from being read.

zikaeroh commented 4 years ago

This is an old issue (unfortunate), but I really have to thank you for these patches. I finally have a working backlight setup for my X1 Yoga's OLED screen after more than a year going without it. PKGBUILD and a script to run from sway here: https://gist.github.com/zikaeroh/b687938443a59d3f0645ed7dabe67aea

jonls commented 4 years ago

I think D-Bus or similar is what I would consider (#54). I don't think I'm willing to maintain a custom protocol for this.

martinetd commented 4 years ago

Sure; I don't particularily like dbus but it would work for my purposes with EnforceTemperature if they also add something equivalent for brightness (I assume that won't be too much work once #54 gets in, I can probably do this much)

The patches @piater pushed don't really implement a new protocol for that though, he just loops over stdin and parses it as if it were argv -- I'm happy with that for now :)

Feel free to close this issue when #54 gets merged.