Created attachment 145097 [details] [review]
gnome-desktop patch to add xrandr gamma support

patch to gnome desktop to add some crtc interfaces along with gamma interface.

Created attachment 145098 [details] [review]
gnome-screensaver randr fade patch.

This adds fading via the randr 1.2 protocol using gnome-desktop APIs.

It'll fall back to old vidmode in non randr screens.

Created attachment 145101 [details] [review]
v2 of gamma support for g-s

this fixes a bug where we re-enter the fade and lose the restore gamma.

It would probably be a good idea to check it with the color conversion people (argyllcms, etc). They've complained in the past screensavers messed up their color adjustments

well this stuff won't mess it up any different than previous stuff its just an alternate method that works on dual head.

Created attachment 145263 [details] [review]
simpler gnome-desktop patch

Not sure what I was doing there, this is all that is required in the gnome desktop component, much simpler.

Also reflects ssp comments on irc.

Created attachment 145264 [details] [review]
g-s patch to go with latest gnome-desktop patch.

This is reworked using the crtcs interface correctly, sleepy Friday coding failure.

Some comments:

- It looks like the crtc->gamma field is only ever used during setting/getting
of gamma, so it could just be a local variable in those functions.

- The gamma_size field is remembered, but I don't think it changes for any particular CRTC, so it could just be initialized once in crtc_initialize().

- g_new0() can never return NULL. If malloc() fails, then glib will abort the program. Also, convention is that the application can pass NULL for "out" parameters, so in get_gamma() the code could be something like this:

      copy_size = crtc->gamma_size * sizeof(unsigned short);

      if (red)
      {
          unsigned short *r = g_new0 (unsigned short, crtc->gamma_size);
          memcpy (r, crtc->gamma->red, copy_size);
          *red = r;
      }

for each of the components. And then  if (size) *size = crtc->gamma_size;

- The boolean return from get_gamma() may be unnecessary then, if it can only fail due to out-of-memory.

- A couple of style issues:
  - Blank line after a block of variables
  - Space between function names and parentheses (both calls and definitions)

Created attachment 145460 [details] [review]
gnome-desktop patch v3.

fixes as per ssp comment,

still need the gboolean since the XRRGetCrtcGamma call can fail.

otherwise hopefully this should follow any coding insanity^Wstandard.

Created attachment 145461 [details] [review]
gnome-screensaver randr fade patch v4

Updates for coding style.

Comment on attachment 145461 [details] [review]
gnome-screensaver randr fade patch v4

Still a few coding style problems I'd like to fix before we commit.  I've tested this and it seems to work nicely.  We can commit after branching.

This

        while (*crtcs)
        {
                crtc = *crtcs;
                
                /* if no mode ignore crtc */
                if (!gnome_rr_crtc_get_current_mode (crtc))
                        continue;


make g-s-s get suck in a busy loop if you ever have a crtc without a current mode (as I do :-( ). Simply skipping the crtc here will be problematic though, since that will leave the info array member uninitialized. Other loops assume that they can just loop over the whole info array, so thats a bad idea. The next alternative is to set info->size to 0, but then it looks like we'll trigger a size==0 special case in xf86_wack_gamma ? Ah, no. There is a separate xrandr_crtc_whack_gamma which looks like it will do the right thing with size==0.

The loop should probably look like this ?

        while (*crtcs)
+        {
+                crtc = *crtcs;
+
+                info = &screen_priv->info[crtc_count];
+
+                /* if no mode ignore crtc */
+                if (!gnome_rr_crtc_get_current_mode (crtc)) {
+                       info->size = 0;
+                       info->r = NULL;
+                       info->g = NULL;
+                       info->b = NULL;
+                }
+               else {
+                       res = gnome_rr_crtc_get_gamma (crtc, &info->size,
+                                                      &info->r, &info->g,
+                                                      &info->b);
+                       if (res == FALSE)
+                               goto fail;
+               }
+
+                crtcs++;
+                crtc_count++;
+        }


I'll try this one

Created attachment 146756 [details] [review]
the gnome-desktop patch we're actually shipping

Committed modified patch from Matthias.  Fixed in master (2.29.x).  Thanks!

*** Bug 566857 has been marked as a duplicate of this bug. ***