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

Implement wlr-gamma-control-unstable-v1 #1157

Merged
merged 13 commits into from Aug 2, 2018

Conversation

Projects
None yet
9 participants
@emersion
Copy link
Member

emersion commented Jul 22, 2018

See swaywm/wlr-protocols#25

  • Reset gamma ramps
  • Consider setting O_NONBLOCK on the fd before reading

Fixes #1135

if (!ok) {
zwlr_gamma_control_v1_send_failed(gamma_control->resource);
gamma_control_destroy(gamma_control);
return;

This comment has been minimized.

@ddevault

ddevault Jul 22, 2018

Member

This return is not necessary. Maybe there should be a few gotos somewhere?

@ddevault ddevault changed the title Implement wlr-gamma-control-unstable-v1 [WIP] Implement wlr-gamma-control-unstable-v1 Jul 22, 2018

@emersion emersion changed the title [WIP] Implement wlr-gamma-control-unstable-v1 Implement wlr-gamma-control-unstable-v1 Jul 22, 2018

@emersion emersion force-pushed the emersion:wlr-gamma-control branch from eb541af to 43261bc Jul 22, 2018

static void fill_empty_gamma_table(uint32_t size,
uint16_t *r, uint16_t *g, uint16_t *b) {
for (uint32_t i = 0; i < size; ++i) {
uint16_t val = (uint32_t)0xffff * (uint32_t)i / (uint32_t)(size - 1);

This comment has been minimized.

@ascent12

ascent12 Jul 22, 2018

Collaborator

Why does this have so much casting? i and size are uint32_ts anyway.

This comment has been minimized.

@emersion

emersion Jul 22, 2018

Member

Yeah, true. Fixed.

I kept (uint32_t)0xffff though.

@agx

This comment has been minimized.

Copy link
Contributor

agx commented Jul 23, 2018

A example client would be totally awesome.

@martinetd

This comment has been minimized.

Copy link
Contributor

martinetd commented Jul 23, 2018

@minus7 implemented it for redshift there minus7/redshift@b980009

@agx

This comment has been minimized.

Copy link
Contributor

agx commented Jul 23, 2018

@martinetd thanks! I was actually thinking about a separate client in examples. If wlroots "drives" the protocols (like also with layer-surfaces) it's great to have a test client in wlroots as well so others wanting to implement that protocol (say weston, mutter, ...) don't have to run around and grab different sources. If not that's perfectly fine but having it might help adoption.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 23, 2018

A example client would be totally awesome.

I considered doing that, but I've realized computing the gamma ramps for a given temperature is non-trivial, so I wonder if we want this in examples/. See https://github.com/jonls/redshift/blob/master/src/colorramp.c

@agx

This comment has been minimized.

Copy link
Contributor

agx commented Jul 23, 2018

I noticed that while working on the client for reading gamma ramps:

https://github.com/swaywm/wlroots/pull/1059/files#diff-ed82c9cad3a00609d7ecb9d9eca3c333

but wondered if a sample client couldn't e.g. just half all the values. The main thing is to have something different than the default color ramp. Now that values get reset when the client exits we're sure to get a reasonable display again.

@agx

This comment has been minimized.

Copy link
Contributor

agx commented Jul 23, 2018

That said I'm not trying to shove any more work your way. I can just write that client at a later point when I have some more time for compositor work again.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 23, 2018

Right, I could just go with a simple client that changes the brightness. Good idea, I'll do that.

@progandy

This comment has been minimized.

Copy link

progandy commented Jul 23, 2018

You could even add contrast, and a simple gamma value
A = c * E^y + b

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 23, 2018

@progandy Thanks, this seems easy enough for an example :)

@atomnuker

This comment has been minimized.

Copy link
Contributor

atomnuker commented Jul 23, 2018

Could you make an example client to force linear gamma? For hdmi, which some drivers force limited range rgb for (something which shouldn't exist). Should be even simpler.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 23, 2018

Could you make an example client to force linear gamma?

Actually that's how I reset gamma tables. Maybe we should just always set a linear gamma?

I've been searching for other ways to reset gamma, but haven't found one (setting an empty table is a no-op).

@agx

This comment has been minimized.

Copy link
Contributor

agx commented Jul 23, 2018

@emersion liniear looks good when you don't have any other color temperature to match against:

https://github.com/GNOME/gnome-settings-daemon/blob/master/plugins/color/gsd-color-state.c#L551

@atomnuker

This comment has been minimized.

Copy link
Contributor

atomnuker commented Jul 23, 2018

Actually that's how I reset gamma tables. Maybe we should just always set a linear gamma?

Yeah, I think wlroots should always init the gamma table as linear rather than whatever the driver thinks it should be (which is pretty much incorrect most of the time). Pretty much all TVs I've used have had settings to enable full-range rather than limited range, and some even properly autodetect. If the user should encounter one that doesn't, they can use a client to modify it.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Jul 23, 2018

Added an example client. There's a pretty high probability I messed up the function filling the gamma tables, @progandy thoughts?

@emersion emersion force-pushed the emersion:wlr-gamma-control branch from 42f6b1b to 51f1c2f Jul 23, 2018

@progandy
Copy link

progandy left a comment

I like it, just don't forget to clamp the calculation result.

uint16_t *b = table + 2 * ramp_size;
for (uint32_t i = 0; i < ramp_size; ++i) {
double val = (double)i / (ramp_size - 1);
val = contrast * pow(val, 1.0 / gamma) + brightness;

This comment has been minimized.

@progandy

progandy Jul 24, 2018

You have to clamp the value to the range 0 .. 1 after the calculation
val = (val > 1.0) ? 1.0 : ( (val < 0.0) ? 0.0 : val );

static const char usage[] = "usage: gamma-control [options]\n"
" -h show this help message\n"
" -c <value> set contrast (default: 1)\n"
" -b <value> set brightness (default: 0)\n"

This comment has been minimized.

@progandy

progandy Jul 24, 2018

maybe use 1 as default for the brightness parameter and subtract 1 later?

@@ -115,21 +115,26 @@ static void fill_gamma_table(uint16_t *table, uint32_t ramp_size,
uint16_t *b = table + 2 * ramp_size;
for (uint32_t i = 0; i < ramp_size; ++i) {
double val = (double)i / (ramp_size - 1);
val = contrast * pow(val, 1.0 / gamma) + brightness;
val = contrast * pow(val, 1.0 / gamma) + (1 - brightness);

This comment has been minimized.

@progandy

progandy Jul 25, 2018

(brightness - 1) would be better.
In my opinion brightness = 0 should result in a black image, 2 pure white.

This comment has been minimized.

@emersion

emersion Jul 25, 2018

Member

Oh right


struct wl_listener output_destroy_listener;

void* data;

This comment has been minimized.

@minus7

minus7 Jul 27, 2018

Nitpick: placement of *. Is there no auto formatter?

"The gamma ramps don't have the correct size");
goto error_fd;
}
lseek(fd, 0, SEEK_SET);

This comment has been minimized.

@minus7

minus7 Jul 27, 2018

I'd just skip the whole seeking because you test whether enough data was read later anyway. Also, why use a different error here than after failed read()?

This comment has been minimized.

@emersion

emersion Jul 29, 2018

Member

This check was originally here because I wasn't setting O_NONBLOCK, but I guess now it can be merged with the read check. The error is different because a read can fail for other reasons than the file being too short (in which case the client did nothing wrong and we shouldn't send a protocol error), but I can handle both cases.

@minus7

This comment has been minimized.

Copy link

minus7 commented Jul 27, 2018

Otherwise looks good to me, but gamma changes don't apply instantly. They only apply after switching TTY or DPMS off/on'ing.

Additionally, after switching TTY, resetting gamma ramps there and then switching back, gamma is back to normal; redshift continues calling set_gamma with success but the gamma stays reset (even after switching TTY back and forth). I need to restart redshift and switch TTY to make it work.

wl_list_remove(&manager->display_destroy.link);
struct wlr_gamma_control_v1 *gamma_control, *tmp;
wl_list_for_each_safe(gamma_control, tmp, &manager->controls, link) {
wl_resource_destroy(gamma_control->resource);

This comment has been minimized.

@minus7

minus7 Jul 27, 2018

Does this destroy the gamma_controls the clients still keeping a handle on?

This comment has been minimized.

@emersion

@emersion emersion force-pushed the emersion:wlr-gamma-control branch 2 times, most recently from 8bb1a4c to 13cb312 Jul 29, 2018

@minus7

This comment has been minimized.

Copy link

minus7 commented Jul 30, 2018

Can anyone with amdgpu try if the gamma curve applies instantly?

@Emantor

This comment has been minimized.

Copy link
Contributor

Emantor commented Jul 30, 2018

Yep, I'll test when I get home.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Jul 30, 2018

On it.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Jul 30, 2018

I can reproduce it like @minus7 specified, only after tty switch (rootston).

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Aug 2, 2018

Works with legacy DRM. I think we should use legacy DRM for setting the gamma ramps for now and file a driver bug, maybe check our implementation against weston first.

@minus7

This comment has been minimized.

Copy link

minus7 commented Aug 2, 2018

Confirming that it works with legacy. If working around the problem causes the driver not to fix the problem then I'm against working around it. Preferrably I'd enable the workaround with an env var, so affected users can work around the problem for the time being.

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Aug 2, 2018

Yeah, I'd prefer not to add a workaround for a bug in one driver too. In the meantime, you can already use the env var WLR_DRM_NO_ATOMIC=1 if you're affected and want to use redshift.

@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Aug 2, 2018

Can we get a rebase? Then I'm prepared to merge this.

@minus7

This comment has been minimized.

Copy link

minus7 commented Aug 2, 2018

In the meantime, you can already use the env var WLR_DRM_NO_ATOMIC=1 if you're affected and want to use redshift.

I'd prefer not to set everything to the old backend, but okay

@ddevault

This comment has been minimized.

@emersion emersion force-pushed the emersion:wlr-gamma-control branch from 13cb312 to c3afe4f Aug 2, 2018

@emersion

This comment has been minimized.

Copy link
Member

emersion commented Aug 2, 2018

Rebased.

@ddevault ddevault merged commit 1654fc8 into swaywm:master Aug 2, 2018

1 check passed

builds.sr.ht builds.sr.ht job completed successfully
Details
@ddevault

This comment has been minimized.

Copy link
Member

ddevault commented Aug 2, 2018

Thanks!

@emersion emersion deleted the emersion:wlr-gamma-control branch Aug 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment