Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WiP: Add get_gamma to gamma-control #1059

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@agx
Copy link
Contributor

agx commented Jun 13, 2018

This adds get_gamma to get the current gamma lut to wlr_gamma_control. This is needed since some things like gnome-desktop (as used by gnome-settings-daemon) want to read out the gamme lut.

TODOs:

  • figure out what to do with gamma-control protocol. It's not marked as unstable but e.g. has no docs and isn't in wlr-protocols or even wayland-protocols either. Is if fit for wlroots 1.0? Idea would be to rename to wlr-gamma-control and mark it as unstable. Old gamma-control could be kept for backward compat but are there any users atm?
  • don't always use legacy drm interface

Test Plan

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 13, 2018

Why do you need to read gamma?

Yeah, the gamma protocol is one of these Orbital protocols, so just like KDE protocols they don't have any stability promise and don't follow wayland-protocols conventions. It's used by the Wayland branch of redshift.

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jun 13, 2018

@emersion see above: the gnome-desktop library uses it (gnome_rr_crtc_get_gamma) and that again is used by gnome-settings-daemon (which is e.g. responsible for nightlight aka redshift) among other things.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 13, 2018

Sure, but why do these need to read gamma?

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Jun 13, 2018

I'm guessing it's for showing the user what the current gamma is so you can set some sliders or something up

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jun 13, 2018

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 13, 2018

I'm guessing it's for showing the user what the current gamma is so you can set some sliders or something up

I don't think clients reverse the gamma lut to get back values for their sliders. Usually the workflow for redshift-like clients is:

  • Get a temperature from a user preference or from the current time
  • With this temperature, compute gamma luts
  • Send gamma luts to the compositor

A client should never need to read the gamma luts. If it lets the user set something with a slider, it needs to save it somewhere anyway.

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jun 13, 2018

Well it apparently can be done on X and there's nothing wrong with allowing it IMHO and some abstractions like gnome-desktop expect it. I could mock that up in phosh to make these clients not fail but reporting sane values is IMHO better.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 13, 2018

Well it apparently can be done on X and there's nothing wrong with allowing it

"It works like this on X" won't convince me. Wayland's policy is usually "if the client doesn't need it, better not to give it otherwise it'll do stupid stuff with it".

Since you're building a bridge, I imagine you can just save in memory the last gamma lut?

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 13, 2018

Also, this protocol needs work. For instance:

  • What happens if two clients want to manage gamma luts at the same time?
  • What happens when the client disconnects or crashes?
@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jun 13, 2018

Also, this protocol needs work. For instance:
What happens if two clients want to manage gamma luts at the same time?
What happens when the client disconnects or crashes?

Are you refering to gamma-control in general or the parts of the PR here? We're talking about reading here so two clients at the same time won't interfere or am I missing s.th.? Writing is a different matter but that's not part of the PR. IMHO Not every client should be allowed to bind as with surface layers, input methods and other things.

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jun 13, 2018

I'm not arguing along the lines of X11 but rather that if we allow writing we should allow readind as well.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jun 13, 2018

Are you refering to gamma-control in general or the parts of the PR here?

No this comment ("this protocol needs work") was about the protocol in general, unrelated to this PR.

I'm not arguing along the lines of X11 but rather that if we allow writing we should allow readind as well.

Unless clients have a good reason to read gamma lut, I wouldn't add it to the protocol. If there isn't a real use-case for this, I don't see why it should be part of the protocol (being compatible with the internal GNOME D-Bus protocol isn't a use-case).

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Jun 14, 2018

@subdiff may have some comments on this

@agx agx force-pushed the agx:gamma branch from 3fd5ece to f20c934 Jun 14, 2018

agx added some commits Jun 14, 2018

gamma-control: add gamma event and allow to trigger it via get_gamma()
This sends the current gamma ramps.

There's currently no indication about the stability of the protocol and
it's not upstream either so it might be best to rename to
wlr-gamma-control-untable-v1.
examples: Add gamma control test client
Able to read gamma and gamma size send by the compositor

@agx agx force-pushed the agx:gamma branch from f20c934 to c55e11f Jun 27, 2018

@emersion
Copy link
Member

emersion left a comment

Needs real use-case

@agx

This comment has been minimized.

Copy link
Contributor Author

agx commented Jul 16, 2018

Let's drop this for now since all users of gnome_rr_crtc_get_gamma only seem to want the size of the gamma table:

https://codesearch.debian.net/search?q=gnome_rr_crtc_get_gamma

So it's enough for phoshto return an empty table of the appropriate size:

https://source.puri.sm/Librem5/phosh/merge_requests/58/diffs#370563a8cb744218352eee39b7d945673c1272b3_151_177

@agx agx closed this Jul 16, 2018

@subdiff

This comment has been minimized.

Copy link

subdiff commented Jul 16, 2018

Don't know much about this specific implementation here, but for Night Color I only send a temperature value to KWin and then KWin calculates the gamma ramp from the linear default case with a suitable multiplication as redshift does it.

For gamma control (not yet merged) the workspace needs to know current red, green, blue values of an output, but these are not the ones received through DRM interface (since these have been additionally multiplied with temperature value of Night Color).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.