Bug fix: GLX: ARGB texture too dark & Jitter when resize & others

- GLX backend: Fix a bug that ARGB windows / shadows are rendered too dark. Thanks to derhass in FreeNode/##opengl for help. - GLX backend: Fix a problem that during window resize the content looks jittering, by letting compton fetch pixmap sizes with XGetGeometry() instead of relying on window width/height, which could be inaccurate during window resize. Negative effect on performance. Thanks to M4he for reporting. (#7) - Add .desktop file. Thanks to quequotion for providing it. (#97) - Avoid checking presence of window pixmap, because they may not exist with very old X Composite implementations. - Add workaround for a strange window restack issue when compton receieves a ConfigureNotify with non-existent new above window. - Add debugging function hexdump(). Extra sanity checks on various places.
2013-03-18 11:48:28 +08:00 · 2013-03-18 11:48:28 +08:00 · 1a88e3d0c5
parent b6a99334ce
commit 1a88e3d0c5
6 changed files with 128 additions and 42 deletions
--- a/.gitignore
+++ b/.gitignore
@ -43,3 +43,4 @@ man/*.1
 doxygen/
 .clang_complete
 /src/backtrace-symbols.[ch]
+/compton*.trace
--- a/compton.desktop
+++ b/compton.desktop
@ -0,0 +1,10 @@
+[Desktop Entry]
+Version=1.0
+Type=Application
+Name=compton
+Comment=A X compositor
+Categories=Utility;
+TryExec=compton
+Exec=compton
+Icon=xcompmgr
+# Thanks to quequotion for providing this file!
--- a/src/common.h
+++ b/src/common.h
@ -297,9 +297,9 @@ struct _glx_texture {
  GLuint texture;
  GLXPixmap glpixmap;
  Pixmap pixmap;
-  int width;
-  int height;
-  int depth;
+  unsigned width;
+  unsigned height;
+  unsigned depth;
  bool y_inverted;
 };
 #endif
@ -1573,7 +1573,7 @@ glx_init_blur(session_t *ps);

 bool
 glx_bind_pixmap(session_t *ps, glx_texture_t **pptex, Pixmap pixmap,
-    int width, int height, int depth);
+    unsigned width, unsigned height, unsigned depth);

 void
 glx_release_pixmap(session_t *ps, glx_texture_t *ptex);
@ -1686,3 +1686,39 @@ c2_match(session_t *ps, win *w, const c2_lptr_t *condlst,
 ///@}

 #endif
+
+/**
+ * @brief Dump raw bytes in HEX format.
+ *
+ * @param data pointer to raw data
+ * @param len length of data
+ */
+static inline void
+hexdump(const char *data, int len) {
+  static const int BYTE_PER_LN = 16;
+
+  if (len <= 0)
+    return;
+
+  // Print header
+  printf("%10s:", "Offset");
+  for (int i = 0; i < BYTE_PER_LN; ++i)
+    printf(" %2d", i);
+  putchar('\n');
+
+  // Dump content
+  for (int offset = 0; offset < len; ++offset) {
+    if (!(offset % BYTE_PER_LN))
+      printf("0x%08x:", offset);
+
+    printf(" %02hhx", data[offset]);
+
+    if ((BYTE_PER_LN - 1) == offset % BYTE_PER_LN)
+      putchar('\n');
+  }
+  if (len % BYTE_PER_LN)
+    putchar('\n');
+
+  fflush(stdout);
+}
+
--- a/src/compton.c
+++ b/src/compton.c
@ -474,6 +474,7 @@ win_build_shadow(session_t *ps, win *w, double opacity) {

  w->shadow_paint.pixmap = shadow_pixmap_argb;
  w->shadow_paint.pict = shadow_picture_argb;
+
  bool success = paint_bind_tex(ps, &w->shadow_paint, shadow_image->width, shadow_image->height, 32, true);

  XFreeGC(ps->dpy, gc);
@ -863,8 +864,7 @@ get_root_tile(session_t *ps) {
  ps->root_tile_paint.pixmap = pixmap;
 #ifdef CONFIG_VSYNC_OPENGL
  if (BKEND_GLX == ps->o.backend)
-    return glx_bind_pixmap(ps, &ps->root_tile_paint.ptex, ps->root_tile_paint.pixmap,
-        ps->root_width, ps->root_height, ps->depth);
+    return glx_bind_pixmap(ps, &ps->root_tile_paint.ptex, ps->root_tile_paint.pixmap, 0, 0, 0);
 #endif

  return true;
@ -1437,8 +1437,10 @@ win_paint_win(session_t *ps, win *w, XserverRegion reg_paint) {
    }
  }
  // GLX: Build texture
-  if (!paint_bind_tex(ps, &w->paint, w->widthb, w->heightb,
-        w->pictfmt->depth, w->pixmap_damaged)) {
+  // Let glx_bind_pixmap() determine pixmap size, because if the user
+  // is resizing windows, the width and height we get may not be up-to-date,
+  // causing the jittering issue M4he reported in #7.
+  if (!paint_bind_tex(ps, &w->paint, 0, 0, 0, w->pixmap_damaged)) {
    printf_errf("(%#010lx): Failed to bind texture. Expect troubles.", w->id);
  }
  w->pixmap_damaged = false;
@ -2633,21 +2635,33 @@ restack_win(session_t *ps, win *w, Window new_above) {
  }

  if (old_above != new_above) {
-    win **prev;
+    win **prev = NULL, **prev_old = NULL;

-    /* unhook */
+    // unhook
    for (prev = &ps->list; *prev; prev = &(*prev)->next) {
      if ((*prev) == w) break;
    }

-    *prev = w->next;
+    prev_old = prev;

-    /* rehook */
+    bool found = false;
+
+    // rehook
    for (prev = &ps->list; *prev; prev = &(*prev)->next) {
-      if ((*prev)->id == new_above && !(*prev)->destroyed)
+      if ((*prev)->id == new_above && !(*prev)->destroyed) {
+        found = true;
        break;
+      }
    }

+    if (!found) {
+      printf_errf("(%#010lx, %#010lx): "
+          "Failed to found new above window.", w->id, new_above);
+      return;
+    }
+
+    *prev_old = w->next;
+
    w->next = *prev;
    *prev = w;

@ -2668,7 +2682,9 @@ restack_win(session_t *ps, win *w, Window new_above) {

        desc = "";
        if (c->destroyed) desc = "(D) ";
-        printf("%#010lx \"%s\" %s-> ", c->id, window_name, desc);
+        printf("%#010lx \"%s\" %s", c->id, window_name, desc);
+        if (c->next)
+          printf("-> ");

        if (to_free) {
          XFree(window_name);
--- a/src/compton.h
+++ b/src/compton.h
@ -162,7 +162,9 @@ free_wincondlst(c2_lptr_t **pcondlst) {
 */
 static inline bool
 paint_isvalid(session_t *ps, const paint_t *ppaint) {
-  if (!ppaint || !ppaint->pixmap)
+  // Don't check for presence of Pixmap here, because older X Composite doesn't
+  // provide it
+  if (!ppaint)
    return false;

  if (BKEND_XRENDER == ps->o.backend && !ppaint->pict)
@ -179,13 +181,15 @@ paint_isvalid(session_t *ps, const paint_t *ppaint) {
 * Bind texture in paint_t if we are using GLX backend.
 */
 static inline bool
-paint_bind_tex(session_t *ps, paint_t *ppaint, int wid, int hei, int depth,
-    bool force) {
+paint_bind_tex(session_t *ps, paint_t *ppaint,
+    unsigned wid, unsigned hei, unsigned depth, bool force) {
 #ifdef CONFIG_VSYNC_OPENGL
-  // TODO: Make sure we have the same Pixmap binded?
-  if (BKEND_GLX == ps->o.backend
-      && (force || !glx_tex_binded(ppaint->ptex, ppaint->pixmap))) {
-    return glx_bind_pixmap(ps, &ppaint->ptex, ppaint->pixmap, wid, hei, depth);
+  if (BKEND_GLX == ps->o.backend) {
+    if (!ppaint->pixmap)
+      return false;
+
+    if (force || !glx_tex_binded(ppaint->ptex, ppaint->pixmap))
+      return glx_bind_pixmap(ps, &ppaint->ptex, ppaint->pixmap, wid, hei, depth);
  }
 #endif

--- a/src/opengl.c
+++ b/src/opengl.c
@ -107,7 +107,6 @@ glx_init(session_t *ps, bool need_render) {

    // glEnable(GL_DEPTH_TEST);
    glTexEnvi(GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_REPLACE);
-    glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);
    glDisable(GL_BLEND);

    if (!ps->o.glx_no_stencil) {
@ -322,15 +321,10 @@ glx_cmp_fbconfig(session_t *ps,
 */
 bool
 glx_bind_pixmap(session_t *ps, glx_texture_t **pptex, Pixmap pixmap,
-    int width, int height, int depth) {
-  if (depth > OPENGL_MAX_DEPTH) {
-    printf_errf("(%d): Requested depth higher than %d.", depth,
-        OPENGL_MAX_DEPTH);
-    return false;
-  }
-  const glx_fbconfig_t *pcfg = ps->glx_fbconfigs[depth];
-  if (!pcfg) {
-    printf_errf("(%d): Couldn't find FBConfig with requested depth.", depth);
+    unsigned width, unsigned height, unsigned depth) {
+  if (!pixmap) {
+    printf_errf("(%#010lx): Binding to an empty pixmap. This can't work.",
+        pixmap);
    return false;
  }

@ -359,8 +353,7 @@ glx_bind_pixmap(session_t *ps, glx_texture_t **pptex, Pixmap pixmap,
  glEnable(target);

  // Release pixmap if parameters are inconsistent
-  if (ptex->texture && !(width == ptex->width && height == ptex->height
-        && ptex->pixmap == pixmap && depth == ptex->depth)) {
+  if (ptex->texture && ptex->pixmap != pixmap) {
    glx_release_pixmap(ps, ptex);
  }

@ -380,7 +373,6 @@ glx_bind_pixmap(session_t *ps, glx_texture_t **pptex, Pixmap pixmap,
    glBindTexture(target, 0);

    ptex->texture = texture;
-    ptex->y_inverted = pcfg->y_inverted;
  }
  if (!ptex->texture) {
    printf_errf("(): Failed to allocate texture.");
@ -393,6 +385,29 @@ glx_bind_pixmap(session_t *ps, glx_texture_t **pptex, Pixmap pixmap,
  if (!ptex->glpixmap) {
    need_release = false;

+    // Retrieve pixmap parameters, if they aren't provided
+    if (!(width && height && depth)) {
+      Window rroot = None;
+      int rx = 0, ry = 0;
+      unsigned rbdwid = 0;
+      if (!XGetGeometry(ps->dpy, pixmap, &rroot, &rx, &ry,
+            &width, &height, &rbdwid, &depth)) {
+        printf_errf("(%#010lx): Failed to query Pixmap info.", pixmap);
+        return false;
+      }
+      if (depth > OPENGL_MAX_DEPTH) {
+        printf_errf("(%d): Requested depth higher than %d.", depth,
+            OPENGL_MAX_DEPTH);
+        return false;
+      }
+    }
+
+    const glx_fbconfig_t *pcfg = ps->glx_fbconfigs[depth];
+    if (!pcfg) {
+      printf_errf("(%d): Couldn't find FBConfig with requested depth.", depth);
+      return false;
+    }
+
    // Determine texture target, copied from compiz
    GLint tex_tgt = 0;
    if (GLX_TEXTURE_2D_BIT_EXT & pcfg->texture_tgts
@ -419,6 +434,11 @@ glx_bind_pixmap(session_t *ps, glx_texture_t **pptex, Pixmap pixmap,
    };

    ptex->glpixmap = glXCreatePixmap(ps->dpy, pcfg->cfg, pixmap, attrs);
+    ptex->pixmap = pixmap;
+    ptex->width = width;
+    ptex->height = height;
+    ptex->depth = depth;
+    ptex->y_inverted = pcfg->y_inverted;
  }
  if (!ptex->glpixmap) {
    printf_errf("(): Failed to allocate GLX pixmap.");
@ -436,11 +456,6 @@ glx_bind_pixmap(session_t *ps, glx_texture_t **pptex, Pixmap pixmap,
  glBindTexture(target, 0);
  glDisable(target);

-  ptex->width = width;
-  ptex->height = height;
-  ptex->depth = depth;
-  ptex->pixmap = pixmap;
-
  return true;
 }

@ -493,7 +508,7 @@ glx_set_clip(session_t *ps, XserverRegion reg) {
      GLint z = 0;

 #ifdef DEBUG_GLX
-      printf_dbgf("(): Rect %d: %f, %f, %f, %f\n", i, rx, ry, rxe, rye);
+      printf_dbgf("(): Rect %d: %d, %d, %d, %d\n", i, rx, ry, rxe, rye);
 #endif

      glVertex3i(rx, ry, z);
@ -532,10 +547,14 @@ glx_render(session_t *ps, const glx_texture_t *ptex,
  if (opacity < 1.0 || GLX_TEXTURE_FORMAT_RGBA_EXT ==
      ps->glx_fbconfigs[ptex->depth]->texture_fmt) {
    glEnable(GL_BLEND);
-    glBlendFunc(GL_SRC_ALPHA, GL_ONE_MINUS_SRC_ALPHA);
+
    // Needed for handling opacity of ARGB texture
    glTexEnvi(GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_MODULATE);
-    glColor4f(1.0f, 1.0f, 1.0f, opacity);
+
+    // This is all weird, but X Render is using a strange ARGB format, and
+    // we need to use those things to correct it. Thanks to derhass for help.
+    glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA);
+    glColor4f(opacity, opacity, opacity, opacity);
  }

  // Color negation