Patch: fix broken temperature setting on Quartz

[07151129]

In 1.11 release, CGSetDisplayTransferByTable aborts with:

Client is attempting to access a display by index (1) instead of display ID. abort() called

at least on OSX 10.9. Here is a fix:

--- /tmp/redshift-1.11/src/gamma-quartz.c   2016-01-02 20:01:16.000000000 +0100
+++ ./gamma-quartz.c    2016-10-21 10:21:39.000000000 +0200
@@ -23,6 +23,7 @@

 #include <stdio.h>
 #include <stdlib.h>
+#include <stdbool.h>

 #include <ApplicationServices/ApplicationServices.h>

@@ -36,6 +37,87 @@
 #include "gamma-quartz.h"
 #include "colorramp.h"

+static quartz_display_state_t
+init_state(CGDirectDisplayID display) {
+   quartz_display_state_t ret;
+
+   ret.display = display;
+   ret.valid = true;
+
+   uint32_t ramp_size = CGDisplayGammaTableCapacity(display);
+   if (ramp_size == 0) {
+       fprintf(stderr, _("Gamma ramp size too small: %i\n"),
+           ramp_size);
+       abort();
+   }
+
+   ret.ramp_size = ramp_size;
+
+   /* Allocate space for saved ramps */
+   ret.saved_ramps =
+       malloc(3 * ramp_size * sizeof(float));
+   if (ret.saved_ramps == NULL) {
+       perror("malloc");
+       abort();
+   }
+
+   float *gamma_r = &ret.saved_ramps[0*ramp_size];
+   float *gamma_g = &ret.saved_ramps[1*ramp_size];
+   float *gamma_b = &ret.saved_ramps[2*ramp_size];
+
+   /* Copy the ramps to allocated space */
+   uint32_t sample_count;
+   CGError
+   error = CGGetDisplayTransferByTable(display, ramp_size,
+                       gamma_r, gamma_g, gamma_b,
+                       &sample_count);
+   if (error != kCGErrorSuccess ||
+       sample_count != ramp_size) {
+       fputs(_("Unable to save current gamma ramp.\n"),
+             stderr);
+       abort();
+   }
+
+   return ret;
+}
+
+static void
+reconfig_cb(CGDirectDisplayID display, CGDisplayChangeSummaryFlags flags, void* data) {
+   quartz_state_t* state = (quartz_state_t*)data;
+
+   if (!(flags & (kCGDisplayAddFlag | kCGDisplayRemoveFlag)))
+       return;
+
+   quartz_display_state_t* disp = NULL;
+   for (size_t i = 0; i < state->display_count; i++) {
+       if (state->displays->display == display) {
+           disp = &state->displays[i];
+           break;
+       }
+   }
+
+   if (disp) { /* Operating on an existing display */
+       if (flags & kCGDisplayAddFlag)
+           disp->valid = true;
+       else
+           disp->valid = false;
+   } else {
+       if (flags & kCGDisplayRemoveFlag) {
+           fprintf(stderr, _("Removing a non-existing display\n"));
+           abort();
+       }
+
+       quartz_display_state_t dstate = init_state(display);
+       dstate.valid = true;
+
+       state->display_count++;
+       state->displays = reallocf(state->displays, state->display_count * sizeof(quartz_display_state_t));
+       if (!state->displays) {
+           perror("realloc");
+           abort();
+       }
+   }
+}

 int
 quartz_init(quartz_state_t *state)
@@ -83,50 +165,18 @@ quartz_start(quartz_state_t *state)
        return -1;
    }

-   /* Copy display indentifiers to display state */
-   for (int i = 0; i < display_count; i++) {
-       state->displays[i].display = displays[i];
-       state->displays[i].saved_ramps = NULL;
-   }
-
    free(displays);

    /* Save gamma ramps for all displays in display state */
    for (int i = 0; i < display_count; i++) {
-       CGDirectDisplayID display = state->displays[i].display;
-
-       uint32_t ramp_size = CGDisplayGammaTableCapacity(display);
-       if (ramp_size == 0) {
-           fprintf(stderr, _("Gamma ramp size too small: %i\n"),
-               ramp_size);
-           return -1;
-       }
-
-       state->displays[i].ramp_size = ramp_size;
-
-       /* Allocate space for saved ramps */
-       state->displays[i].saved_ramps =
-           malloc(3 * ramp_size * sizeof(float));
-       if (state->displays[i].saved_ramps == NULL) {
-           perror("malloc");
-           return -1;
-       }
+       state->displays[i] = init_state(displays[i]);
+       state->displays[i].valid = true;
+   }

-       float *gamma_r = &state->displays[i].saved_ramps[0*ramp_size];
-       float *gamma_g = &state->displays[i].saved_ramps[1*ramp_size];
-       float *gamma_b = &state->displays[i].saved_ramps[2*ramp_size];
-
-       /* Copy the ramps to allocated space */
-       uint32_t sample_count;
-       error = CGGetDisplayTransferByTable(display, ramp_size,
-                           gamma_r, gamma_g, gamma_b,
-                           &sample_count);
-       if (error != kCGErrorSuccess ||
-           sample_count != ramp_size) {
-           fputs(_("Unable to save current gamma ramp.\n"),
-                 stderr);
-           return -1;
-       }
+   error = CGDisplayRegisterReconfigurationCallback(reconfig_cb, state);
+   if (error != kCGErrorSuccess) {
+       free(displays);
+       return -1;
    }

    return 0;
@@ -141,6 +191,7 @@ quartz_restore(quartz_state_t *state)
 void
 quartz_free(quartz_state_t *state)
 {
+   CGDisplayRemoveReconfigurationCallback(reconfig_cb, state);
    if (state->displays != NULL) {
        for (int i = 0; i < state->display_count; i++) {
            free(state->displays[i].saved_ramps);
@@ -177,10 +228,11 @@ quartz_set_option(quartz_state_t *state,
 }

 static void
-quartz_set_temperature_for_display(quartz_state_t *state, int display,
+quartz_set_temperature_for_display(quartz_state_t *state, int disp_idx,
                   const color_setting_t *setting)
 {
-   uint32_t ramp_size = state->displays[display].ramp_size;
+   CGDirectDisplayID display = state->displays[disp_idx].display;
+   uint32_t ramp_size = state->displays[disp_idx].ramp_size;

    /* Create new gamma ramps */
    float *gamma_ramps = malloc(3*ramp_size*sizeof(float));
@@ -195,7 +247,7 @@ quartz_set_temperature_for_display(quart

    if (state->preserve) {
        /* Initialize gamma ramps from saved state */
-       memcpy(gamma_ramps, state->displays[display].saved_ramps,
+       memcpy(gamma_ramps, state->displays[disp_idx].saved_ramps,
               3*ramp_size*sizeof(float));
    } else {
        /* Initialize gamma ramps to pure state */
@@ -226,7 +278,8 @@ quartz_set_temperature(quartz_state_t *s
               const color_setting_t *setting)
 {
    for (int i = 0; i < state->display_count; i++) {
-       quartz_set_temperature_for_display(state, i, setting);
+       if (state[i].displays->valid)
+           quartz_set_temperature_for_display(state, i, setting);
    }

    return 0;

Update: now the patch adds display hotplug support. I did not verify if CGDirectDisplayID is consistent across display removals, though.

[07151129]

Oh, the reconfiguration callback will not work as it seems to require a CFRunLoop. Still, the temperature setting fix applies:

--- /tmp/redshift-1.11/src/gamma-quartz.c   2016-01-02 20:01:16.000000000 +0100
+++ ./gamma-quartz.c    2016-10-21 11:14:19.000000000 +0200
@@ -23,6 +23,7 @@

 #include <stdio.h>
 #include <stdlib.h>
+#include <stdbool.h>

 #include <ApplicationServices/ApplicationServices.h>

@@ -36,6 +37,49 @@
 #include "gamma-quartz.h"
 #include "colorramp.h"

+static quartz_display_state_t
+init_state(CGDirectDisplayID display) {
+   quartz_display_state_t ret;
+
+   ret.display = display;
+   ret.valid = true;
+
+   uint32_t ramp_size = CGDisplayGammaTableCapacity(display);
+   if (ramp_size == 0) {
+       fprintf(stderr, _("Gamma ramp size too small: %i\n"),
+           ramp_size);
+       abort();
+   }
+
+   ret.ramp_size = ramp_size;
+
+   /* Allocate space for saved ramps */
+   ret.saved_ramps =
+       malloc(3 * ramp_size * sizeof(float));
+   if (ret.saved_ramps == NULL) {
+       perror("malloc");
+       abort();
+   }
+
+   float *gamma_r = &ret.saved_ramps[0*ramp_size];
+   float *gamma_g = &ret.saved_ramps[1*ramp_size];
+   float *gamma_b = &ret.saved_ramps[2*ramp_size];
+
+   /* Copy the ramps to allocated space */
+   uint32_t sample_count;
+   CGError
+   error = CGGetDisplayTransferByTable(display, ramp_size,
+                       gamma_r, gamma_g, gamma_b,
+                       &sample_count);
+   if (error != kCGErrorSuccess ||
+       sample_count != ramp_size) {
+       fputs(_("Unable to save current gamma ramp.\n"),
+             stderr);
+       abort();
+   }
+
+   return ret;
+}

 int
 quartz_init(quartz_state_t *state)
@@ -83,50 +127,10 @@ quartz_start(quartz_state_t *state)
        return -1;
    }

-   /* Copy display indentifiers to display state */
-   for (int i = 0; i < display_count; i++) {
-       state->displays[i].display = displays[i];
-       state->displays[i].saved_ramps = NULL;
-   }
-
-   free(displays);
-
    /* Save gamma ramps for all displays in display state */
    for (int i = 0; i < display_count; i++) {
-       CGDirectDisplayID display = state->displays[i].display;
-
-       uint32_t ramp_size = CGDisplayGammaTableCapacity(display);
-       if (ramp_size == 0) {
-           fprintf(stderr, _("Gamma ramp size too small: %i\n"),
-               ramp_size);
-           return -1;
-       }
-
-       state->displays[i].ramp_size = ramp_size;
-
-       /* Allocate space for saved ramps */
-       state->displays[i].saved_ramps =
-           malloc(3 * ramp_size * sizeof(float));
-       if (state->displays[i].saved_ramps == NULL) {
-           perror("malloc");
-           return -1;
-       }
-
-       float *gamma_r = &state->displays[i].saved_ramps[0*ramp_size];
-       float *gamma_g = &state->displays[i].saved_ramps[1*ramp_size];
-       float *gamma_b = &state->displays[i].saved_ramps[2*ramp_size];
-
-       /* Copy the ramps to allocated space */
-       uint32_t sample_count;
-       error = CGGetDisplayTransferByTable(display, ramp_size,
-                           gamma_r, gamma_g, gamma_b,
-                           &sample_count);
-       if (error != kCGErrorSuccess ||
-           sample_count != ramp_size) {
-           fputs(_("Unable to save current gamma ramp.\n"),
-                 stderr);
-           return -1;
-       }
+       state->displays[i] = init_state(displays[i]);
+       state->displays[i].valid = true;
    }

    return 0;
@@ -177,10 +181,11 @@ quartz_set_option(quartz_state_t *state,
 }

 static void
-quartz_set_temperature_for_display(quartz_state_t *state, int display,
+quartz_set_temperature_for_display(quartz_state_t *state, int disp_idx,
                   const color_setting_t *setting)
 {
-   uint32_t ramp_size = state->displays[display].ramp_size;
+   CGDirectDisplayID display = state->displays[disp_idx].display;
+   uint32_t ramp_size = state->displays[disp_idx].ramp_size;

    /* Create new gamma ramps */
    float *gamma_ramps = malloc(3*ramp_size*sizeof(float));
@@ -195,7 +200,7 @@ quartz_set_temperature_for_display(quart

    if (state->preserve) {
        /* Initialize gamma ramps from saved state */
-       memcpy(gamma_ramps, state->displays[display].saved_ramps,
+       memcpy(gamma_ramps, state->displays[disp_idx].saved_ramps,
               3*ramp_size*sizeof(float));
    } else {
        /* Initialize gamma ramps to pure state */
@@ -226,7 +231,8 @@ quartz_set_temperature(quartz_state_t *s
               const color_setting_t *setting)
 {
    for (int i = 0; i < state->display_count; i++) {
-       quartz_set_temperature_for_display(state, i, setting);
+       if (state[i].displays->valid)
+           quartz_set_temperature_for_display(state, i, setting);
    }

    return 0;

[jonls]

Thanks :+1: I will look into this when I have time to convert your patches into a pull request. If you could do this, though, it would be a great help.

[jonls]

Incorrect use of CGDirectDisplayID is fixed in #534.