Subject: Re: bug#37755: Logic in init_fringe_bitmap should be moved to backends (maybe rif->define_fringe_bitmap)

From: Carlos Pita <[email-masked]>
Date: Wed, 16 Oct 2019 19:07:26 -0300

Ok, I found the bug. The init part was in general iterating along bits
by advancing the pointer (i.e. bits++). When I juxtaposed this part to
the define part, that also used bits, then bits was pointing to one
past the end of the bitmap and not to the bitmap itself, as expected
by the define code. Once diagnosed, it was an easy fix. In detail:

1. For Cairo, I followed my own suggestion above of not modifying bits
at all. Both the previous init and define loops are now merged into a
single loop that directly writes to the surface data, instead of
modifying bits.
2. For X11, bits is the only state propagated between init/define and
draw and there was no specific define part before my changes, so the
new define is simply the old init. Here bits is obviously modified in
order to prepare it for the drawing routine.
3. For Win32, I replaced bits++ with bits[i]. Bits is also modified,
although here it's easy to avoid that if so desired because it's only
locally needed to initialize the platform-specific bitmap structure
then used by the drawing routine.
4. For MacOS/NS, there is nothing to be done since there was no
specific init part, as I observed a few posts above.

I've cursorily tested this patch both alone and merged with my "hidpi
fringe" patches and seems to be working fine for the Cairo backend.

The new change amounts to yet another code simplification and an
overall ~40 reduction in LOCs.

Best regards
--
Carlos
From 16428d7e32fa531a226d02578f275c6340d984e5 Mon Sep 17 00:00:00 2001
From: memeplex <[email-masked]>
Date: Tue, 15 Oct 2019 16:37:24 -0300
Subject: [PATCH] Fringe refactor: move platform-specific code into rif
 (Bug#37755)

* src/fringe.c (init_fringe_bitmap): Remove it.
* src/w32term.c (w32_define_fringe_bitmap): Add NTGUI init part.
* src/xterm.c (x_define_fringe_bitmap): New interface function for X11,
add XWINDOWS init part.
(x_cr_define_fringe_bitmap): Add CAIRO init part.
---
 src/fringe.c  | 128 +++++---------------------------------------------
 src/w32term.c |  15 +++++-
 src/xterm.c   |  60 +++++++++++++++++++++--
 3 files changed, 81 insertions(+), 122 deletions(-)

diff --git a/src/fringe.c b/src/fringe.c
index 22f3bdc..a17b684 100644
--- a/src/fringe.c
+++ b/src/fringe.c
@@ -1388,112 +1388,6 @@ DEFUN ("destroy-fringe-bitmap", Fdestroy_fringe_bitmap, Sdestroy_fringe_bitmap,
   return Qnil;
 }
 
-
-/* Initialize bitmap bit.
-
-   On X, we bit-swap the built-in bitmaps and reduce bitmap
-   from short to char array if width is <= 8 bits.
-
-   On MAC with big-endian CPU, we need to byte-swap each short.
-
-   On W32 and MAC (little endian), there's no need to do this.
-*/
-
-#if defined (HAVE_X_WINDOWS)
-static const unsigned char swap_nibble[16] = {
-  0x0, 0x8, 0x4, 0xc,           /* 0000 1000 0100 1100 */
-  0x2, 0xa, 0x6, 0xe,           /* 0010 1010 0110 1110 */
-  0x1, 0x9, 0x5, 0xd,           /* 0001 1001 0101 1101 */
-  0x3, 0xb, 0x7, 0xf};          /* 0011 1011 0111 1111 */
-#endif                          /* HAVE_X_WINDOWS */
-
-static void
-init_fringe_bitmap (int which, struct fringe_bitmap *fb, int once_p)
-{
-  if (once_p || fb->dynamic)
-    {
-#if defined (HAVE_X_WINDOWS)
-      unsigned short *bits = fb->bits;
-      int j;
-
-#ifdef USE_CAIRO
-      for (j = 0; j < fb->height; j++)
-	{
-	  unsigned short b = *bits;
-#ifdef WORDS_BIGENDIAN
-	  *bits++ = (b << (16 - fb->width));
-#else
-	  b = (unsigned short)((swap_nibble[b & 0xf] << 12)
-			       | (swap_nibble[(b>>4) & 0xf] << 8)
-			       | (swap_nibble[(b>>8) & 0xf] << 4)
-			       | (swap_nibble[(b>>12) & 0xf]));
-	  *bits++ = (b >> (16 - fb->width));
-#endif
-	}
-#else  /* not USE_CAIRO */
-      if (fb->width <= 8)
-	{
-	  unsigned char *cbits = (unsigned char *)fb->bits;
-	  for (j = 0; j < fb->height; j++)
-	    {
-	      unsigned short b = *bits++;
-	      unsigned char c;
-	      c = (unsigned char)((swap_nibble[b & 0xf] << 4)
-				  | (swap_nibble[(b>>4) & 0xf]));
-	      *cbits++ = (c >> (8 - fb->width));
-	    }
-	}
-      else
-	{
-	  for (j = 0; j < fb->height; j++)
-	    {
-	      unsigned short b = *bits;
-	      b = (unsigned short)((swap_nibble[b & 0xf] << 12)
-				   | (swap_nibble[(b>>4) & 0xf] << 8)
-				   | (swap_nibble[(b>>8) & 0xf] << 4)
-				   | (swap_nibble[(b>>12) & 0xf]));
-	      b >>= (16 - fb->width);
-#ifdef WORDS_BIGENDIAN
-	      b = bswap_16 (b);
-#endif
-	      *bits++ = b;
-	    }
-	}
-#endif /* not USE_CAIRO */
-#endif /* HAVE_X_WINDOWS */
-
-#ifdef HAVE_NTGUI
-      unsigned short *bits = fb->bits;
-      int j;
-      for (j = 0; j < fb->height; j++)
-	{
-	  unsigned short b = *bits;
-	  b <<= (16 - fb->width);
-	  /* Windows is little-endian, so the next line is always
-	     needed.  */
-	  b = ((b >> 8) | (b << 8));
-	  *bits++ = b;
-	}
-#endif
-    }
-
-  if (!once_p)
-    {
-      /* XXX Is SELECTED_FRAME OK here? */
-      struct redisplay_interface *rif = FRAME_RIF (SELECTED_FRAME ());
-
-      destroy_fringe_bitmap (which);
-
-      if (rif && rif->define_fringe_bitmap)
-	rif->define_fringe_bitmap (which, fb->bits, fb->height, fb->width);
-
-      fringe_bitmaps[which] = fb;
-      if (which >= max_used_fringe_bitmap)
-	max_used_fringe_bitmap = which + 1;
-    }
-}
-
-
 DEFUN ("define-fringe-bitmap", Fdefine_fringe_bitmap, Sdefine_fringe_bitmap,
        2, 5, 0,
        doc: /* Define fringe bitmap BITMAP from BITS of size HEIGHT x WIDTH.
@@ -1625,7 +1519,17 @@ list (ALIGN PERIODIC) where PERIODIC non-nil specifies that the bitmap
 
   *xfb = fb;
 
-  init_fringe_bitmap (n, xfb, 0);
+  /* XXX Is SELECTED_FRAME OK here? */
+  struct redisplay_interface *rif = FRAME_RIF (SELECTED_FRAME ());
+
+  destroy_fringe_bitmap (n);
+
+  if (rif && rif->define_fringe_bitmap)
+    rif->define_fringe_bitmap (n, xfb->bits, xfb->height, xfb->width);
+
+  fringe_bitmaps[n] = xfb;
+  if (n >= max_used_fringe_bitmap)
+    max_used_fringe_bitmap = n + 1;
 
   return bitmap;
 }
@@ -1743,19 +1647,9 @@ mark_fringe_data (void)
 
 /* Initialize this module when Emacs starts.  */
 
-static void init_fringe_once_for_pdumper (void);
-
 void
 init_fringe_once (void)
 {
-  pdumper_do_now_and_after_load (init_fringe_once_for_pdumper);
-}
-
-static void
-init_fringe_once_for_pdumper (void)
-{
-  for (int bt = NO_FRINGE_BITMAP + 1; bt < MAX_STANDARD_FRINGE_BITMAPS; bt++)
-    init_fringe_bitmap (bt, &standard_bitmaps[bt], 1);
 }
 
 void
diff --git a/src/w32term.c b/src/w32term.c
index 9da0845..888b736 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -835,9 +835,22 @@ w32_draw_fringe_bitmap (struct window *w, struct glyph_row *row,
 static void
 w32_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
 {
+#ifdef HAVE_NTGUI
+  int i;
+
+  for (i = 0; i < h; i++)
+    {
+      unsigned short b = bits[i];
+      b <<= (16 - wd);
+      /* Windows is little-endian, so the next line is always
+         needed.  */
+      b = ((b >> 8) | (b << 8));
+      bits[i] = b;
+    }
+#endif
   if (which >= max_fringe_bmp)
     {
-      int i = max_fringe_bmp;
+      i = max_fringe_bmp;
       max_fringe_bmp = which + 20;
       fringe_bmp = (HBITMAP *) xrealloc (fringe_bmp, max_fringe_bmp * sizeof (HBITMAP));
       while (i < max_fringe_bmp)
diff --git a/src/xterm.c b/src/xterm.c
index 5d8b148..5cb1b28 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -548,6 +548,11 @@ x_end_cr_xlib_drawable (struct frame *f, GC gc)
 
 static int max_fringe_bmp = 0;
 static cairo_pattern_t **fringe_bmp = 0;
+static const unsigned char swap_nibble[16] = {
+  0x0, 0x8, 0x4, 0xc,           /* 0000 1000 0100 1100 */
+  0x2, 0xa, 0x6, 0xe,           /* 0010 1010 0110 1110 */
+  0x1, 0x9, 0x5, 0xd,           /* 0001 1001 0101 1101 */
+  0x3, 0xb, 0x7, 0xf};          /* 0011 1011 0111 1111 */
 
 static void
 x_cr_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
@@ -572,10 +577,18 @@ x_cr_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
   stride = cairo_image_surface_get_stride (surface);
   data = cairo_image_surface_get_data (surface);
 
-  for (i = 0; i < h; i++)
+  for (i = 0; i < h; i++, data += stride)
     {
-      *((unsigned short *) data) = bits[i];
-      data += stride;
+      unsigned short b = bits[i];
+#ifdef WORDS_BIGENDIAN
+      *((unsigned short *) data) = (b << (16 - wd));
+#else
+      b = (unsigned short)((swap_nibble[b & 0xf] << 12)
+                           | (swap_nibble[(b>>4) & 0xf] << 8)
+                           | (swap_nibble[(b>>8) & 0xf] << 4)
+                           | (swap_nibble[(b>>12) & 0xf]));
+      *((unsigned short *) data) = (b >> (16 - wd));
+#endif
     }
 
   cairo_surface_mark_dirty (surface);
@@ -1393,6 +1406,45 @@ x_after_update_window_line (struct window *w, struct glyph_row *desired_row)
 #endif
 }
 
+#ifndef USE_CAIRO
+static void
+x_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
+{
+  /* On X, we bit-swap the built-in bitmaps and reduce bitmap */
+  /* from short to char array if width is <= 8 bits. */
+  int j;
+
+  if (wd <= 8)
+    {
+      unsigned char *cbits = (unsigned char *)bits;
+      for (j = 0; j < h; j++)
+        {
+          unsigned short b = *bits++;
+          unsigned char c;
+          c = (unsigned char)((swap_nibble[b & 0xf] << 4)
+                              | (swap_nibble[(b>>4) & 0xf]));
+          *cbits++ = (c >> (8 - wd));
+        }
+    }
+  else
+    {
+      for (j = 0; j < h; j++)
+        {
+          unsigned short b = *bits;
+          b = (unsigned short)((swap_nibble[b & 0xf] << 12)
+                               | (swap_nibble[(b>>4) & 0xf] << 8)
+                               | (swap_nibble[(b>>8) & 0xf] << 4)
+                               | (swap_nibble[(b>>12) & 0xf]));
+          b >>= (16 - wd);
+#ifdef WORDS_BIGENDIAN
+          b = bswap_16 (b);
+#endif
+          *bits++ = b;
+        }
+    }
+}
+#endif /* USE_CAIRO */
+
 static void
 x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fringe_bitmap_params *p)
 {
@@ -13361,7 +13413,7 @@ x_activate_timeout_atimer (void)
     x_cr_define_fringe_bitmap,
     x_cr_destroy_fringe_bitmap,
 #else
-    0, /* define_fringe_bitmap */
+    x_define_fringe_bitmap,
     0, /* destroy_fringe_bitmap */
 #endif
     x_compute_glyph_string_overhangs,
-- 
2.20.1