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

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

[Carlos Pita]

In fringe.c you have init_fringe_bitmap with this structure:

```
#if defined (HAVE_X_WINDOWS)
...
#ifdef USE_CAIRO
...
#endif
...
#endif
#ifdef HAVE_NTGUI
...
#endif
  if (!once_p)
    {
    ....
    rif->define_fringe_bitmap (...)
    ....
    }
```

Now, this is backend specific code that should be moved behind the
redisplay_interface. It seems to me that the obvious candidate to host
that code is define_fringe_bitmap. The once_p clause is related to
pdumper which I ignore all about.

Best regards
--
Carlos

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

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 14 Oct 2019 23:30:08 -0300
> 
> In fringe.c you have init_fringe_bitmap with this structure:
> 
> ```
> #if defined (HAVE_X_WINDOWS)
> ...
> #ifdef USE_CAIRO
> ...
> #endif
> ...
> #endif
> #ifdef HAVE_NTGUI
> ...
> #endif
>   if (!once_p)
>     {
>     ....
>     rif->define_fringe_bitmap (...)
>     ....
>     }
> ```
> 
> Now, this is backend specific code that should be moved behind the
> redisplay_interface.

Yes, it should.

> It seems to me that the obvious candidate to host that code is
> define_fringe_bitmap.

No, I think we need another interface, as define_fringe_bitmap is used
directly from other places.

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

[Carlos Pita]

(Sorry, forgot to CC debbugs, resending all together below)

Hi Eli,

I've sketched the tree of define/init calls below and I believe I
pretty much understand how the initialization sequence works now and
I'm ready to think about how to properly extend the redisplay
interface in order to include init_fringe_bitmap.

But could you check that my understanding of the flow is sound?


# On emacs startup initialize all standard bitmaps but
# postponing rif->define_fringe_bitmap until frame creation

init_fringe_once
    init_fringe_once_for_pdumper
        for each standard bitmap
            init_fringe_bitmap (oncep = 1)
                since oncep: do not try to destroy previous
                since !rif: do not rif->define_fringe_bitmap

# When a frame is created, actually call rif->define_fringe_bitmap
# for each standard bitmap and also for lisp defined
# bitmaps that were created in a daemon with no frame / rif

gui_init_fringe (I assume this is called once per frame)
    for each standard bitmap
        rif->define_fringe_bitmap
    for each additional bitmap (recently introduced by [1])
        rif->define_fringe_bitmap

# When defined from lisp do both steps at once (init and rif->define)
# except that we're in daemon with no frame / rif

define-fringe-bitmap (lisp)
   init_fringe_bitmap (oncep = 0)
      when rif && !oncep:
           since !oncep: do destroy previous (if any)
           since rif: do rif->define_fringe_bitmap (if not in daemon)

Also, I understand the roles of init_fringe_bitmap and define_fringe_bitmap as:

init_fringe_bitmap
   Initialize bitmap as far as possible without assuming there is a rif/frame
   available. For example, bit shuffling, endianness stuff.

define_fringe_bitmap
   Do the remaining, backend specific, initialization, which is now possible
   because we have a rif/frame. For example, create Cairo surfaces.

Now I see a problem here: abstracting from the platform/backend by
encapsulating platform/backend related logic in the rif is only
possible when there is a rif at hand! And there is no rif when there
is no frame. So that's probably the reason why quite a few
platform-specific assumptions have leaked into fringe.c. The sad part
is how coupled are those bit manipulations to those backends, it's not
like there is some platform-specific part first and then a
backend-specific part, because both are too intermingled and they
logically belong together.

So I'm again tempted to move init into define, I don't think there is
any real gain in splitting the initialization this way, it's not like
the initial part is time consuming or resource intensive so we better
do it ASAP or whatever and, as I've said, it neither provides an
orthogonal abstraction, there is no cartesian product of platforms and
backends here, so to speak. And part of the complexity of the
initialization sequence above is due to this split.

What do you think?

I'll be sending a patch quite soon.

---

[1] [PATCH] Fix initialization of user-defined fringe bitmaps in daemon

On Tue, Oct 15, 2019 at 6:33 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Carlos Pita <carlosjosepita@gmail.com>
> > Date: Mon, 14 Oct 2019 23:30:08 -0300
> >
> > In fringe.c you have init_fringe_bitmap with this structure:
> >
> > ```
> > #if defined (HAVE_X_WINDOWS)
> > ...
> > #ifdef USE_CAIRO
> > ...
> > #endif
> > ...
> > #endif
> > #ifdef HAVE_NTGUI
> > ...
> > #endif
> >   if (!once_p)
> >     {
> >     ....
> >     rif->define_fringe_bitmap (...)
> >     ....
> >     }
> > ```
> >
> > Now, this is backend specific code that should be moved behind the
> > redisplay_interface.
>
> Yes, it should.
>
> > It seems to me that the obvious candidate to host that code is
> > define_fringe_bitmap.
>
> No, I think we need another interface, as define_fringe_bitmap is used
> directly from other places.

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

[Carlos Pita]

[-- Attachment #1: Type: text/plain, Size: 2251 bytes --]

This is the modified initialization logic implemented by the attached patch:

# When a frame is created, actually call rif->define_fringe_bitmap
# for each standard bitmap and also for lisp defined
# bitmaps that were created in a daemon with no frame / rif

gui_init_fringe (I assume this is called once per frame)
    for each standard bitmap
        rif->define_fringe_bitmap
    for each additional bitmap (recently introduced by [1])
        rif->define_fringe_bitmap

# When defined from lisp call rif->define_fringe_bitmap
# except that we're in daemon with no frame / rif

define-fringe-bitmap (lisp)
      when rif: rif->define_fringe_bitmap

Much simpler, don't you think? Also removed ~30 LOCs.

Except that I'm overlooking something obvious it seems to do exactly
the same, only that the bit shuffling part is postponed until the
frame is actually created (that is, moved into
rif->define_fringe_bitmap).

Some remarks:

1. There was this comment on top of init_fringe_bitmap: "On MAC with
big-endian CPU, we need to byte-swap each short". But all the code
there were either specific to NTGUI, specific to XWINDOWS or related
to bitmap destruction. So nothing remained that could be actually
moved into the NS backend.

2. I left the HAVE_NTGUI guard even after moving the relevant code
into w32term.c because I'm not sure whether all w32 code share the
NTGUI path or not.

3. The previous implementation modified bits as a way of connecting
init with define, this was the state passed from one stage to the
other. Now, since both parts are done together, there is no need to
modify the passed bits array, the array is only required in order to
initialize backend-specific structures. Nevertheless, I decided to
keep that as it was in order to prevent regressions, but I believe an
implementation that preserves the original value of bits would be
preferable now that it is indeed possible. In particular, for X the
only "backend-specific structure" is an utterly modified bits array
(in some cases shorts are converted into chars, that's why I said in
#37689 that it's difficult for the upper layer to "split rows" after
this kind of manipulations had already taken place).

Any reviews would be much appreciated.

Best regards
--
Carlos

[-- Attachment #2: 0001-Fringe-refactor-move-platform-specific-code-into-red.patch --]
[-- Type: text/x-patch, Size: 8538 bytes --]

From c4792b1d7ad7957bb07107d6e461173ab42d87a7 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Tue, 15 Oct 2019 16:37:24 -0300
Subject: [PATCH] Fringe refactor: move platform-specific code into redisplay
 interface

* src/fringe.c: remove init_fringe_bitmap
* src/w32term.c: add NTGUI parts to w32_define_fringe_bitmap
* src/xterm.c:
  - New interface function for X11: x_define_fringe_bitmap
  - add XWINDOWS parts to x_define_fringe_bitmap
  - add CAIRO parts to x_cr_define_fringe_bitmap

Detailed discussion: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37755
---
 src/fringe.c  | 128 +++++---------------------------------------------
 src/w32term.c |  12 +++++
 src/xterm.c   |  58 ++++++++++++++++++++++-
 3 files changed, 80 insertions(+), 118 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..c43ad81 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -835,6 +835,18 @@ 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 j;
+  for (j = 0; j < h; j++)
+    {
+      unsigned short b = *bits;
+      b <<= (16 - wd);
+      /* Windows is little-endian, so the next line is always
+         needed.  */
+      b = ((b >> 8) | (b << 8));
+      *bits++ = b;
+    }
+#endif
   if (which >= max_fringe_bmp)
     {
       int i = max_fringe_bmp;
diff --git a/src/xterm.c b/src/xterm.c
index 5d8b148..a8c2944 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)
@@ -557,6 +562,20 @@ x_cr_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
   cairo_pattern_t *pattern;
   unsigned char *data;
 
+  for (i = 0; i < h; i++)
+    {
+      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 - wd));
+#endif
+    }
+
   if (which >= max_fringe_bmp)
     {
       i = max_fringe_bmp;
@@ -1393,6 +1412,43 @@ x_after_update_window_line (struct window *w, struct glyph_row *desired_row)
 #endif
 }
 
+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;
+        }
+    }
+}
+
 static void
 x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fringe_bitmap_params *p)
 {
@@ -13361,7 +13417,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

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

[Carlos Pita]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

* Improved commit message according to CONTRIBUTE rules.

* Made some minor changes to avoid conditional compilation warnings.

* I'm aware of some issues when rendering fringe bitmaps (nothing
fatal, though) and I'm still debugging these changes, but it would be
great if you could review them meanwhile.

[-- Attachment #2: 0001-Fringe-refactor-move-platform-specific-code-into-rif.patch --]
[-- Type: text/x-patch, Size: 8465 bytes --]

From 7613e7908f9918726aa4aefce13d10f0e2c5fb90 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
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 part.
* src/xterm.c (x_define_fringe_bitmap): New interface function for X11,
add XWINDOWS part.
(x_cr_define_fringe_bitmap): Add CAIRO part.
---
 src/fringe.c  | 128 +++++---------------------------------------------
 src/w32term.c |  12 +++++
 src/xterm.c   |  60 ++++++++++++++++++++++-
 3 files changed, 82 insertions(+), 118 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..c43ad81 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -835,6 +835,18 @@ 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 j;
+  for (j = 0; j < h; j++)
+    {
+      unsigned short b = *bits;
+      b <<= (16 - wd);
+      /* Windows is little-endian, so the next line is always
+         needed.  */
+      b = ((b >> 8) | (b << 8));
+      *bits++ = b;
+    }
+#endif
   if (which >= max_fringe_bmp)
     {
       int i = max_fringe_bmp;
diff --git a/src/xterm.c b/src/xterm.c
index 5d8b148..95fb25e 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)
@@ -557,6 +562,20 @@ x_cr_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
   cairo_pattern_t *pattern;
   unsigned char *data;
 
+  for (i = 0; i < h; i++)
+    {
+      unsigned short b = *bits;
+#ifdef WORDS_BIGENDIAN
+      *bits++ = (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]));
+      *bits++ = (b >> (16 - wd));
+#endif
+    }
+
   if (which >= max_fringe_bmp)
     {
       i = max_fringe_bmp;
@@ -1393,6 +1412,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 +13419,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

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

[Carlos Pita]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

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

[-- Attachment #2: 0001-Fringe-refactor-move-platform-specific-code-into-rif.patch --]
[-- Type: text/x-patch, Size: 8848 bytes --]

From 16428d7e32fa531a226d02578f275c6340d984e5 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
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

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

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 16 Oct 2019 19:07:26 -0300
> Cc: 37755@debbugs.gnu.org
> 
> The new change amounts to yet another code simplification and an
> overall ~40 reduction in LOCs.

Sorry, I think I'm missing something here.  It looks like you removed
the call to init_fringe_bitmap during dumping, and left its equivalent
only in define-fringe-bitmap, is that right?  If so, I cannot see how
this could work, because Emacs needs to have the standard fringe
bitmaps (for line truncation, continuation, etc.) be defined even
without a call to define-fringe-bitmap.  If you start the current
master under a debugger after putting a breakpoint in
Fdefine_fringe_bitmap, the breakpoint doesn't break, and yet the
bitmaps are known and will be displayed as needed.

What did I miss?

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

[Carlos Pita]

Hi Eli,

thank you for the review.

>It looks like you removed
> the call to init_fringe_bitmap during dumping, and left its equivalent
> only in define-fringe-bitmap, is that right? >

> What did I miss?

The call to gui_init_fringe I guess. Also, notice that
define_fringe_bitmap is quite different than Fdefine_fringe_bitmap.

I suggest you take a look at the modified pseudo code I posted quite a
few message above.

> Emacs needs to have the standard fringe
> bitmaps (for line truncation, continuation, etc.) be defined even
> without a call to define-fringe-bitmap.

This is indeed the case after applying the patch. Some bit shuffling
has been postponed from init_fringe_once to gui_init_fringe, but
that's all.

Now, regarding the dumping stuff you mention, TBH I'm completely
ignorant. So maybe this innocent looking delay of bit shuffling could
have some effect, I don't now, but it's a very different thing from
not initializing standard bitmaps until define-fringe-bitmap is first
called from elisp world.

Besides, whatever is missing after the C static initialization part is
just this *platform dependent* bit shuffling, which I seriously doubt
emacs could make sense of without the appropriate rif at hand, so
quite late in the initialization sequence. I even suggested to avoid
this destructive manipulation of platform independent bitmaps from the
part of the rifs, although I've only followed my suggestion in the
case of cairo, which was quite natural and convenient.

Best regards
--
Carlos

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

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sun, 20 Oct 2019 12:47:13 -0300
> Cc: 37755@debbugs.gnu.org
> 
> > What did I miss?
> 
> The call to gui_init_fringe I guess.

I don't see that call in the patch, nor any changes in gui_init_fringe
that would modify its current effect.

If you mean the existing calls, then they are only made at run time,
which would mean Emacs is dumped without the standard bitmaps?  Why is
that?

> Also, notice that define_fringe_bitmap is quite different than
> Fdefine_fringe_bitmap.

Sure, but I said define-fringe-bitmap, which is the Lisp name of
Fdefine_fringe_bitmap.

> I suggest you take a look at the modified pseudo code I posted quite a
> few message above.

I will, but I'd like to see the full patch as well.

> Besides, whatever is missing after the C static initialization part is
> just this *platform dependent* bit shuffling, which I seriously doubt
> emacs could make sense of without the appropriate rif at hand, so
> quite late in the initialization sequence. I even suggested to avoid
> this destructive manipulation of platform independent bitmaps from the
> part of the rifs, although I've only followed my suggestion in the
> case of cairo, which was quite natural and convenient.

If RIF is the problem, we could make each terminal backend do this
initialization unconditionally at dump time.

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

[Carlos Pita]

> I don't see that call in the patch, nor any changes in gui_init_fringe
> that would modify its current effect.

Because nothing changed in gui_init_fringe itself. It did and does:

  for (bt = NO_FRINGE_BITMAP + 1; bt < MAX_STANDARD_FRINGE_BITMAPS; bt++)
      rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
  for ( ; bt < max_used_fringe_bitmap; bt++)
    rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);

The change affects rif->define_fringe_bitmap instead. It now does:

- Create platform-dependent structures from platform-independent bitmaps.

Previously this was divided between init and define as:

- Init: manipulate platform-independent bitmaps in a platform-dependent way.
- Define: use this platform-dependently shuffled bitmaps to create
platform-dependent structures.

So the only thing that have moved down the initialization sequence is
the bit-shuffling gymnastics which, if any, are done in
gui_init_fringe now.

> Sure, but I said define-fringe-bitmap, which is the Lisp name of
> Fdefine_fringe_bitmap.

I meant to remark that they do quite different things not that you
mistake one for the other, sorry if I wasn't clear.

> > I suggest you take a look at the modified pseudo code I posted quite a
> > few message above.
>
> I will, but I'd like to see the full patch as well.

You have already seen it :)

> If RIF is the problem, we could make each terminal backend do this
> initialization unconditionally at dump time.

According to my rationale above, I don't see any problem at all. But,
as I have said, I ignore everything about the dumper. Yet, I find it
hard to believe that whatever this  dumper thing is, it needs the bits
to be in little-endian, 8-bit per row format, or any other
rif-specific pattern.

Hope it's clearer now.

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

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sun, 20 Oct 2019 13:32:36 -0300
> Cc: 37755@debbugs.gnu.org
> 
> > If RIF is the problem, we could make each terminal backend do this
> > initialization unconditionally at dump time.
> 
> According to my rationale above, I don't see any problem at all. But,
> as I have said, I ignore everything about the dumper. Yet, I find it
> hard to believe that whatever this  dumper thing is, it needs the bits
> to be in little-endian, 8-bit per row format, or any other
> rif-specific pattern.

Sorry, we cannot just ignore the dumping issue.  We don't want to
waste CPU cycles each startup to regenerate these standard bitmaps.
So the fringe bit patterns need to be initialized at dump time and
dumped together with all the other stuff we prepare at that time.

If I understand correctly, the difficulty you had with doing this at
dump time was that frame's RIF was not yet set (because dumping works
in batch mode, where redisplay interface is not set to the correct GUI
frame type).  If so, my suggestion is to call the window-system
specific initialization function directly.  For example, in the X
build, you can add code to syms_of_xterm code to call
x_define_fringe_bitmap, and similarly for Cairo, w32, etc.

Does this proposal resolve the difficulty?  If not, please point out
what else is missing.

Thanks.

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

[Carlos Pita]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

>
>
> Sorry, we cannot just ignore the dumping issue.  We don't want to
> waste CPU cycles each startup to regenerate these standard bitmaps.
> So the fringe bit patterns need to be initialized at dump time and
> dumped together with all the other stuff we prepare at that time.
>

Ok, I'm not sure about how dumping works, but the patterns that you store
are backend specific. Does the dumper take that into account?

>
>
> Does this proposal resolve the difficulty?  If not, please point out
> what else is missing.
>

I just thought that the change decoupled and simplified the code, but I can
always add an initialization step to the rif, previous to the definition of
the actual bitmaps used by the backend.

[-- Attachment #2: Type: text/html, Size: 1184 bytes --]

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

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Sat, 26 Oct 2019 12:46:08 -0300
> Cc: 37755@debbugs.gnu.org
> 
>  Sorry, we cannot just ignore the dumping issue.  We don't want to
>  waste CPU cycles each startup to regenerate these standard bitmaps.
>  So the fringe bit patterns need to be initialized at dump time and
>  dumped together with all the other stuff we prepare at that time.
> 
> Ok, I'm not sure about how dumping works, but the patterns that you store are backend specific. Does the
> dumper take that into account?

The pdump file, like the Emacs binary, is architecture-specific.  So
yes, this is inherently taken into account.

>  Does this proposal resolve the difficulty?  If not, please point out
>  what else is missing.
> 
> I just thought that the change decoupled and simplified the code, but I can always add an initialization step to
> the rif, previous to the definition of the actual bitmaps used by the backend.

I don't think you can do that with a RIF, for the reason already
mentioned: Emacs is dumped in batch mode, where we have a frame type
that doesn't support fringes, and doesn't implement the RIF interfaces
you will need.

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

[Carlos Pita]

[-- Attachment #1: Type: text/plain, Size: 448 bytes --]

> I don't think you can do that with a RIF, for the reason already
> mentioned: Emacs is dumped in batch mode, where we have a frame type
> that doesn't support fringes, and doesn't implement the RIF interfaces
> you will need.
>

Ok, I'm not eager to rework this, so if you think that avoiding repeating
that bit shuffling at the beginning is worth the additional coupling and
complexity I trust in your criterion and close this issue. Thanks.

>

[-- Attachment #2: Type: text/html, Size: 901 bytes --]

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

[Carlos Pita]

[-- Attachment #1: Type: text/plain, Size: 2666 bytes --]

>
>
>
> > Ok, I'm not sure about how dumping works, but the patterns that you
> store are backend specific. Does the
> > dumper take that into account?
>
> The pdump file, like the Emacs binary, is architecture-specific.  So
> yes, this is inherently taken into account.
>

I kept thinking about this and there is also the fact that is not only the
architecture (I mean x, w32, ns, endianness) that is assumed in that bit
shuffling but also, for example, if we have cairo or pure xlib backend, and
that's because I'm quite sure that code was written with the input format
assumed by the rif (even if the rif still doesn't exist at that point) in
mind. Again, I don't know about the dumper but my intuition says there is
something potentially wrong in this arrangement.

What I proposed is:

1. Static initialization of fringe rif/platform-independent structures,
that I guess will be dumped.

2. Prepare. Per-rif initialization of the rif/platform-dependent
structures. This shouldn't affect the independent structures, although in
some cases the original bit pattern is still destructively changed because
it was simpler to keep the existing implementation.

3. Draw. Rendering of the rif/platform-dependent structures to the screen.

What you argue for is:

1. Idem.

1'. Init. Initialization of the platform-dependent but rif->independent
structures.

2'. Prepare. Initialization of the rif/platform-dependent structures from
the platform-dependent but rif->independent structures.

3. Idem.

Now 1' is just cheap bit shuffling of some twenty or so standard bitmaps
having an average grid of 8x8 bits. Also there are might be bugs if output
patterns are not rif specific and the dumper is unaware of that (again I
can't say for sure because of my lack of knowledge of the dumper).

Moreover, having 1' and 2' merged in 2 may actually speed things up,
because there is no need for two separate iterations over the bitmaps, the
first one producing the bit pattern for the second one. It's natural to
simply iterate over the original pattern and directly produce the input
expected by the particular rendering backend.

So, having exposed my reasoning as detailed as I could, and once again, are
you sure that you want to keep that phase 1' just to save some milli
(micro?) seconds, if any? There is a price in code complexity and the risk
of coupling fringe.c too much with backend specific logic. Also, suppose
that there is a problem with this cairo vs xlib decision hardcoded there,
before the dumping happens. One option is to move that xlib vs cairo
decision to the rif (that is to 2 or 2' above). And this way you will be
converging to an empty 1' and 2'->2.

[-- Attachment #2: Type: text/html, Size: 4031 bytes --]