Subject: Re: bug#36337: 26.1; XBM images are sometimes not displayed correctly

From: Pip Cet <[email-masked]>
Date: Sun, 30 Jun 2019 09:48:31 +0000

On Sat, Jun 29, 2019 at 9:54 AM Eli Zaretskii <[email-masked]> wrote:
> It's fine, but I wonder whether the NEWS entry should contain a more
> clear message about this new attribute.

It should :-)

> Don't we want to say that
> this attribute _must_ be used if :width is not an integral multiple of
> 8, or else the XBM image will come out slightly garbled?

It won't come out at all (I'm not sure the worst-case scenario of
shifting each successive line by an extra 7 pixels counts as
"slightly" garbled).

Slightly changed patch attached.
From 48d9faa8a0774c5dbef0b5907142ab1a32871d66 Mon Sep 17 00:00:00 2001
From: Pip Cet <[email-masked]>
Date: Sat, 29 Jun 2019 07:15:52 +0000
Subject: [PATCH] Allow a :stride argument so XBM boolvecs are in the right
 format.

Bug#36337

* src/image.c (xbm_image_p): Explicitly specify the right stride if a
  bool vector is used as argument.
* doc/lispref/display.texi (XBM Images): Describe bool vectors
  accurately.
* etc/NEWS: Document the change.
---
 doc/lispref/display.texi | 18 ++++++++++++------
 etc/NEWS                 |  5 +++++
 src/image.c              | 21 +++++++++++++++------
 3 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 217df3b2cc..8e7d621b41 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -5409,12 +5409,14 @@ XBM Images
 XBM file.  The file contents specify the height and width of the image.
 
 @item
-A string or a bool-vector containing the bits of the image (plus perhaps
-some extra bits at the end that will not be used).  It should contain at
-least @var{width} * @code{height} bits.  In this case, you must specify
-@code{:height} and @code{:width}, both to indicate that the string
-contains just the bits rather than a whole XBM file, and to specify the
-size of the image.
+A string or a bool-vector containing the bits of the image (plus
+perhaps some extra bits at the end that will not be used).  It should
+contain at least @var{stride} * @code{height} bits, where @var{stride}
+is the smallest multiple of 8 greater than or equal to the width of
+the image.  In this case, you should specify @code{:height},
+@code{:width} and @code{:stride}, both to indicate that the string
+contains just the bits rather than a whole XBM file, and to specify
+the size of the image.
 @end itemize
 
 @item :width @var{width}
@@ -5422,6 +5424,10 @@ XBM Images
 
 @item :height @var{height}
 The value, @var{height}, specifies the height of the image, in pixels.
+
+@item :stride @var{stride}
+The number of bool vector entries stored for each row; the smallest
+multiple of 8 greater than or equal to @var{width}.
 @end table
 
 @node XPM Images
diff --git a/etc/NEWS b/etc/NEWS
index 864eb8c110..44545d5bfe 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2262,6 +2262,11 @@ argument is 'iec' and the empty string otherwise.  We recomment a
 space or non-breaking space as third argument, and "B" as fourth
 argument, circumstances allowing.
 
++++
+** The XBM image handler now accepts a ':stride' argument, which must
+be specified in image specs representing the entire bitmap as a single
+bool vector.
+
 
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/src/image.c b/src/image.c
index f3d6508f46..cdb910deed 100644
--- a/src/image.c
+++ b/src/image.c
@@ -3095,6 +3095,7 @@ slurp_file (int fd, ptrdiff_t *size)
   XBM_FILE,
   XBM_WIDTH,
   XBM_HEIGHT,
+  XBM_STRIDE,
   XBM_DATA,
   XBM_FOREGROUND,
   XBM_BACKGROUND,
@@ -3116,6 +3117,7 @@ slurp_file (int fd, ptrdiff_t *size)
   {":file",		IMAGE_STRING_VALUE,			0},
   {":width",		IMAGE_POSITIVE_INTEGER_VALUE,		0},
   {":height",		IMAGE_POSITIVE_INTEGER_VALUE,		0},
+  {":stride",		IMAGE_POSITIVE_INTEGER_VALUE,		0},
   {":data",		IMAGE_DONT_CHECK_VALUE_TYPE,		0},
   {":foreground",	IMAGE_STRING_OR_NIL_VALUE,		0},
   {":background",	IMAGE_STRING_OR_NIL_VALUE,		0},
@@ -3191,7 +3193,7 @@ xbm_image_p (Lisp_Object object)
   else
     {
       Lisp_Object data;
-      int width, height;
+      int width, height, stride;
 
       /* Entries for `:width', `:height' and `:data' must be present.  */
       if (!kw[XBM_WIDTH].count
@@ -3203,6 +3205,11 @@ xbm_image_p (Lisp_Object object)
       width = XFIXNAT (kw[XBM_WIDTH].value);
       height = XFIXNAT (kw[XBM_HEIGHT].value);
 
+      if (!kw[XBM_STRIDE].count)
+	stride = width;
+      else
+	stride = XFIXNAT (kw[XBM_STRIDE].value);
+
       /* Check type of data, and width and height against contents of
 	 data.  */
       if (VECTORP (data))
@@ -3221,8 +3228,7 @@ xbm_image_p (Lisp_Object object)
 
 	      if (STRINGP (elt))
 		{
-		  if (SCHARS (elt)
-		      < (width + CHAR_BIT - 1) / CHAR_BIT)
+		  if (SCHARS (elt) < stride / CHAR_BIT)
 		    return 0;
 		}
 	      else if (BOOL_VECTOR_P (elt))
@@ -3236,13 +3242,16 @@ xbm_image_p (Lisp_Object object)
 	}
       else if (STRINGP (data))
 	{
-	  if (SCHARS (data)
-	      < (width + CHAR_BIT - 1) / CHAR_BIT * height)
+	  if (SCHARS (data) < stride / CHAR_BIT * height)
 	    return 0;
 	}
       else if (BOOL_VECTOR_P (data))
 	{
-	  if (bool_vector_size (data) / height < width)
+	  if (height > 1 && stride != (width + CHAR_BIT - 1)
+	      / CHAR_BIT * CHAR_BIT)
+	    return 0;
+
+	  if (bool_vector_size (data) / height < stride)
 	    return 0;
 	}
       else
-- 
2.20.1