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

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

[Markus Triska]

Please start Emacs with "$ emacs -Q", and insert the form:

    (let* ((width 100)
           (height 100)
           (data (make-bool-vector (* width height) t)))
      (insert "\n")
      (insert-image `(image :type xbm
                            :data ,data
                            :width ,width
                            :height ,height) "t")
      (insert "\n"))

Please move point to the end of the form, and evaluate it with C-x C-e.

This displays a 100x100 XBM image. However, the bottom of the image is
not displayed as intended: I see a mix of black and white pixels at the
bottom of the image, whereas I intend it to be filled with black pixels.

For comparison, it works correctly when I change both width and height
from 100 to 200, or both to 400, and also for several other values.

In GNU Emacs 26.1 (build 1, x86_64-apple-darwin15.3.0, X toolkit, Xaw scroll bars)
 of 2018-09-22
Windowing system distributor 'The X.Org Foundation', version 11.0.11502000

Configured features:
XPM JPEG TIFF GIF PNG GSETTINGS NOTIFY ACL GNUTLS LIBXML2 FREETYPE XFT
ZLIB TOOLKIT_SCROLL_BARS LUCID X11 MODULES THREADS LCMS2

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

[Pip Cet]

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

The code does indeed assume that it is passed a bool vector which is
padded to a multiple of 8 bits per line, but doesn't verify that the
bool vector it is passed indeed matches this format, so it displays
past the end of the bool vector.

This is easy enough to fix by rewriting the bool vector, but that's
potentially very slow (on debug builds), so maybe we shouldn't do
that?

Does the attached patch work for you?



On Sun, Jun 23, 2019 at 7:16 AM Markus Triska <triska@metalevel.at> wrote:
>
>
> Please start Emacs with "$ emacs -Q", and insert the form:
>
>     (let* ((width 100)
>            (height 100)
>            (data (make-bool-vector (* width height) t)))
>       (insert "\n")
>       (insert-image `(image :type xbm
>                             :data ,data
>                             :width ,width
>                             :height ,height) "t")
>       (insert "\n"))
>
> Please move point to the end of the form, and evaluate it with C-x C-e.
>
> This displays a 100x100 XBM image. However, the bottom of the image is
> not displayed as intended: I see a mix of black and white pixels at the
> bottom of the image, whereas I intend it to be filled with black pixels.
>
> For comparison, it works correctly when I change both width and height
> from 100 to 200, or both to 400, and also for several other values.
>
> In GNU Emacs 26.1 (build 1, x86_64-apple-darwin15.3.0, X toolkit, Xaw scroll bars)
>  of 2018-09-22
> Windowing system distributor 'The X.Org Foundation', version 11.0.11502000
>
> Configured features:
> XPM JPEG TIFF GIF PNG GSETTINGS NOTIFY ACL GNUTLS LIBXML2 FREETYPE XFT
> ZLIB TOOLKIT_SCROLL_BARS LUCID X11 MODULES THREADS LCMS2
>
>
>
>

[-- Attachment #2: 0001-Don-t-assume-the-width-of-xbm-images-is-divisible-by.patch --]
[-- Type: text/x-patch, Size: 1174 bytes --]

From eb46f70db5c79bd3f711e4958b78cbf9ae91bc98 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sun, 23 Jun 2019 08:02:18 +0000
Subject: [PATCH] Don't assume the width of xbm images is divisible by 8.

---
 src/image.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/image.c b/src/image.c
index 866323ba6e..7ca6033697 100644
--- a/src/image.c
+++ b/src/image.c
@@ -3814,7 +3814,24 @@ xbm_load (struct frame *f, struct image *img)
 	  else if (STRINGP (data))
 	    bits = SSDATA (data);
 	  else
-	    bits = (char *) bool_vector_data (data);
+	    {
+	      if (img->width & 7)
+		{
+		  int nbytes = (img->width + CHAR_BIT - 1) / CHAR_BIT;
+		  Lisp_Object newdata =
+		    Fmake_bool_vector (make_fixnum (img->height * nbytes * CHAR_BIT), Qnil);
+
+		  for (int y = 0; y < img->height; y++)
+		    {
+		      int i = y * nbytes * CHAR_BIT;
+		      for (int j = y * img->width; j < (y+1) * img->width; i++, j++)
+			bool_vector_set (newdata, i, bool_vector_ref (data, j));
+		    }
+
+		  data = newdata;
+		}
+	      bits = (char *) bool_vector_data (data);
+	    }
 
 #ifdef HAVE_NTGUI
           {
-- 
2.20.1

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

[Markus Triska]

Pip Cet <pipcet@gmail.com> writes:

> Does the attached patch work for you?

Yes, it works, thank you very much!

All the best,
Markus

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 23 Jun 2019 08:05:55 +0000
> Cc: 36337@debbugs.gnu.org
> 
> The code does indeed assume that it is passed a bool vector which is
> padded to a multiple of 8 bits per line, but doesn't verify that the
> bool vector it is passed indeed matches this format, so it displays
> past the end of the bool vector.
> 
> This is easy enough to fix by rewriting the bool vector, but that's
> potentially very slow (on debug builds), so maybe we shouldn't do
> that?
> 
> Does the attached patch work for you?

Thanks, but I really hope there's a more elegant solution.  If not,
maybe we should simply require that both width and height be an
integral multiple of 8 in this case.

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

[Pip Cet]

On Sun, Jun 23, 2019 at 2:29 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sun, 23 Jun 2019 08:05:55 +0000
> > Cc: 36337@debbugs.gnu.org
> > This is easy enough to fix by rewriting the bool vector, but that's
> > potentially very slow (on debug builds), so maybe we shouldn't do
> > that?
> >
> > Does the attached patch work for you?
>
> Thanks, but I really hope there's a more elegant solution.

I thought there had to be, but I've yet to think of anything that's
unequivocally better.

My suggestion would be to expand `substring' to work on bool vectors,
then building a vector of bool vectors and using the existing code for
that case. Less code in image.c, plus a new utility function that
might be generally useful. (However, do we want to encourage people to
use bool vectors?)

I don't think performance is an issue, though, and you might disagree.

> If not,
> maybe we should simply require that both width and height be an
> integral multiple of 8 in this case.

Why would you require the height to be a multiple of 8?

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 23 Jun 2019 16:26:16 +0000
> Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> 
> My suggestion would be to expand `substring' to work on bool vectors,
> then building a vector of bool vectors and using the existing code for
> that case. Less code in image.c, plus a new utility function that
> might be generally useful.

Or maybe we should have a variant of make-bool-vector that accepts 2
dimension s instead of just one?

> (However, do we want to encourage people to use bool vectors?)

Why not?  Evidently, it's convenient in this particular use case.

> > If not,
> > maybe we should simply require that both width and height be an
> > integral multiple of 8 in this case.
> 
> Why would you require the height to be a multiple of 8?

You are right, width is enough.

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

[Pip Cet]

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

On Sun, Jun 23, 2019 at 4:41 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sun, 23 Jun 2019 16:26:16 +0000
> > Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> >
> > My suggestion would be to expand `substring' to work on bool vectors,
> > then building a vector of bool vectors and using the existing code for
> > that case. Less code in image.c, plus a new utility function that
> > might be generally useful.
>
> Or maybe we should have a variant of make-bool-vector that accepts 2
> dimension s instead of just one?

I don't really see how that would be generally useful, to be honest.
In fact, I just played around with removing bool vector support
entirely.

> > (However, do we want to encourage people to use bool vectors?)
> Why not?

We seem to lack even very basic functions for interacting with bool
vectors, and hardly anyone appears to be using them. Even the :stipple
face property doesn't. Emacs starts up fine with bool vector support
removed. We can use vectors of nil/t (in most cases) or unibyte
strings or bignums (which have arbitrary size limits now, but
bigbignums would be just a few lines of code, I think).

And people _think_ bool vectors have a natural presentation as bytes,
but they don't, because some people start with the most significant
bit.

So I just don't see where bool vectors fit in.

> Evidently, it's convenient in this particular use case.

Is the convenience worth a thousand lines of code (much of it C) and
documentation?

[-- Attachment #2: 0001-Don-t-assume-the-width-of-xbm-images-is-divisible-by.patch --]
[-- Type: text/x-patch, Size: 3655 bytes --]

From 2fbff32843dceb6a903ca98a03b2f981c076d3b3 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sun, 23 Jun 2019 08:02:18 +0000
Subject: [PATCH] Don't assume the width of xbm images is divisible by 8.

---
 src/alloc.c |  2 +-
 src/data.c  | 20 ++++++++++++++++++++
 src/fns.c   |  3 +++
 src/image.c | 14 +++++++++++---
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 64aaa8acdf..a80fce078f 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -2210,7 +2210,7 @@ make_uninit_bool_vector (EMACS_INT nbits)
 
 DEFUN ("make-bool-vector", Fmake_bool_vector, Smake_bool_vector, 2, 2, 0,
        doc: /* Return a new bool-vector of length LENGTH, using INIT for each element.
-LENGTH must be a number.  INIT matters only in whether it is t or nil.  */)
+LENGTH must be a number.  INIT matters only in whether it is true or nil.  */)
   (Lisp_Object length, Lisp_Object init)
 {
   Lisp_Object val;
diff --git a/src/data.c b/src/data.c
index c1699aeae7..76b1c6ca4c 100644
--- a/src/data.c
+++ b/src/data.c
@@ -3783,6 +3783,25 @@ DEFUN ("bool-vector-count-consecutive", Fbool_vector_count_consecutive,
   return make_fixnum (count);
 }
 
+DEFUN ("bool-vector-extract", Fbool_vector_extract, Sbool_vector_extract, 1, 3, 0,
+       doc: /* Return a new bool vector which consists of the bits
+between index FROM (inclusive) and index TO (exclusive) of VECTOR.  */)
+  (Lisp_Object vector, Lisp_Object from, Lisp_Object to)
+{
+  Lisp_Object res;
+  ptrdiff_t size, ifrom, ito;
+
+  CHECK_BOOL_VECTOR (vector);
+  size = bool_vector_size (vector);
+  validate_subarray (vector, from, to, size, &ifrom, &ito);
+
+  res = make_uninit_bool_vector (ito - ifrom);
+
+  for (ptrdiff_t i = ifrom; i < ito; i++)
+    bool_vector_set (res, i - ifrom, bool_vector_bitref (vector, i));
+
+  return res;
+}
 \f
 void
 syms_of_data (void)
@@ -4064,6 +4083,7 @@ #define PUT_ERROR(sym, tail, msg)			\
   defsubr (&Sbool_vector_subsetp);
   defsubr (&Sbool_vector_count_consecutive);
   defsubr (&Sbool_vector_count_population);
+  defsubr (&Sbool_vector_extract);
 
   set_symbol_function (Qwholenump, XSYMBOL (Qnatnump)->u.s.function);
 
diff --git a/src/fns.c b/src/fns.c
index fd0c7fc71a..6bf469d1e9 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -1326,6 +1326,9 @@ DEFUN ("substring", Fsubstring, Ssubstring, 1, 3, 0,
   Lisp_Object res;
   ptrdiff_t size, ifrom, ito;
 
+  if (BOOL_VECTOR_P (string))
+    return Fbool_vector_extract (string, from, to);
+
   size = CHECK_VECTOR_OR_STRING (string);
   validate_subarray (string, from, to, size, &ifrom, &ito);
 
diff --git a/src/image.c b/src/image.c
index 7b648c46ae..79c8ac0df8 100644
--- a/src/image.c
+++ b/src/image.c
@@ -3242,7 +3242,7 @@ xbm_image_p (Lisp_Object object)
 	}
       else if (BOOL_VECTOR_P (data))
 	{
-	  if (bool_vector_size (data) / height < width)
+	  if (height > 0 && bool_vector_size (data) / height < width)
 	    return 0;
 	}
       else
@@ -3794,6 +3794,16 @@ xbm_load (struct frame *f, struct image *img)
 	{
 	  USE_SAFE_ALLOCA;
 
+	  if (BOOL_VECTOR_P (data))
+	    {
+	      Lisp_Object newdata = Fmake_vector (make_fixnum (img->height), Qnil);
+	      for (int y = 0; y < img->height; y++)
+		ASET (newdata, y, Fsubstring (data,
+					      make_fixnum (y * img->width),
+					      make_fixnum ((y + 1) * img->width)));
+	      data = newdata;
+	    }
+
 	  if (VECTORP (data))
 	    {
 	      int i;
@@ -3813,8 +3823,6 @@ xbm_load (struct frame *f, struct image *img)
 	    }
 	  else if (STRINGP (data))
 	    bits = SSDATA (data);
-	  else
-	    bits = (char *) bool_vector_data (data);
 
 #ifdef HAVE_NTGUI
           {
-- 
2.20.1

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 23 Jun 2019 19:16:04 +0000
> Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> 
> > Or maybe we should have a variant of make-bool-vector that accepts 2
> > dimension s instead of just one?
> 
> I don't really see how that would be generally useful, to be honest.

For one, it would be useful in this particular case.  Or would you
rather require the width of XBM be an integral multiple of 8?

> In fact, I just played around with removing bool vector support
> entirely.
> 
> > > (However, do we want to encourage people to use bool vectors?)
> > Why not?
> 
> We seem to lack even very basic functions for interacting with bool
> vectors, and hardly anyone appears to be using them. Even the :stipple
> face property doesn't. Emacs starts up fine with bool vector support
> removed. We can use vectors of nil/t (in most cases) or unibyte
> strings or bignums (which have arbitrary size limits now, but
> bigbignums would be just a few lines of code, I think).
> 
> And people _think_ bool vectors have a natural presentation as bytes,
> but they don't, because some people start with the most significant
> bit.
> 
> So I just don't see where bool vectors fit in.

I'm sorry, but I object to removing a feature that has been with us
since Emacs 19, for which we installed new operations just recently in
Emacs 24.  Emacs is too stable a program to remove such basic features
because we cannot immediately see where they fit in.  Please consider
them as "fitting in" by definition; we can only remove them if there
are very good _positive_ reasons for removal, not because we cannot
find reasons _not_ to remove them.

> > Evidently, it's convenient in this particular use case.
> 
> Is the convenience worth a thousand lines of code (much of it C) and
> documentation?

Not necessarily; it might mean that the proposed solution is not the
best one.

What you propose is not what I think I had in mind.  I meant to extend
make-bool-vector (or make a new function, if extending proves
inconvenient or inelegant) that generates a bool-vector given 2
dimensions, not one.  Then such vectors could be used to create XBM
images of arbitrary dimensions.  We could even call this new function
something like make-xbm-data or somesuch, if its utility is limited to
XBM images.

Thanks.

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

[Pip Cet]

On Fri, Jun 28, 2019 at 7:58 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sun, 23 Jun 2019 19:16:04 +0000
> > Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> >
> > > Or maybe we should have a variant of make-bool-vector that accepts 2
> > > dimension s instead of just one?
> >
> > I don't really see how that would be generally useful, to be honest.
>
> For one, it would be useful in this particular case.  Or would you
> rather require the width of XBM be an integral multiple of 8?

Having thought about it, I would rather require the bool vector passed
to XBM to be in the right format: the width can be 100, but the
stride, specified by a new :stride argument, must be the smallest
multiple of 8 greater or equal to the width. If you don't want that,
pass a vector of bool vectors which is copied together, but let's not
pretend we can take a 10,000-bit bool vector and display it
efficiently.

I think my mistake was not to distinguish between stride and width,
but we can easily do so. Shall I prepare a patch?

> > In fact, I just played around with removing bool vector support
> > entirely.

> I'm sorry, but I object to removing a feature that has been with us
> since Emacs 19

Okay. Thanks for taking the time to explain. One serious question,
though unrelated to the current bug: do you think it is just the Lisp
API that we're stuck with for eternity, or is the C representation of
bool vectors forbidden territory as well?

> , for which we installed new operations just recently in Emacs 24.

If we add new features for bool vectors, they might stick out less.
People might actually start using them. That would invalidate my
earlier argument.

> What you propose is not what I think I had in mind.  I meant to extend
> make-bool-vector (or make a new function, if extending proves
> inconvenient or inelegant) that generates a bool-vector given 2
> dimensions, not one.  Then such vectors could be used to create XBM
> images of arbitrary dimensions.  We could even call this new function
> something like make-xbm-data or somesuch, if its utility is limited to
> XBM images.

How is a vector of bool vectors different from a 2-dimensional bool
vector, from the point of view of Lisp? If I understand you correctly,
you would like two-dimensional bool vectors to be represented in
memory in the XBM format.

As for the problem at hand, how would you feel about adding a :stride
argument which must be a multiple of 8 and ensures that the bool
vector passed to the XBM code has the right memory layout?

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 28 Jun 2019 08:29:04 +0000
> Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> 
> On Fri, Jun 28, 2019 at 7:58 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > > From: Pip Cet <pipcet@gmail.com>
> > > Date: Sun, 23 Jun 2019 19:16:04 +0000
> 
> Having thought about it, I would rather require the bool vector passed
> to XBM to be in the right format: the width can be 100, but the
> stride, specified by a new :stride argument, must be the smallest
> multiple of 8 greater or equal to the width. If you don't want that,
> pass a vector of bool vectors which is copied together, but let's not
> pretend we can take a 10,000-bit bool vector and display it
> efficiently.

I'm not sure I understand what you mean, exactly.  Can you show an
example of the new API?

> I think my mistake was not to distinguish between stride and width,
> but we can easily do so. Shall I prepare a patch?

Maybe a patch will be the best means to explain what you mean, but if
you'd like to show it before you invest too much effort, please show
an example.

> > I'm sorry, but I object to removing a feature that has been with us
> > since Emacs 19
> 
> Okay. Thanks for taking the time to explain. One serious question,
> though unrelated to the current bug: do you think it is just the Lisp
> API that we're stuck with for eternity, or is the C representation of
> bool vectors forbidden territory as well?

The external public APIs are the primary concern; they should be
changed in backward-compatible manner (e.g., by adding &optional
arguments or adding compatible interpretations of existing arguments).
C-level implementation details matter only inasmuch as they affect the
provided features visible by callers (e.g., signaling errors in some
cases or producing certain results in specific use cases), and also if
they affect other related functionalities.

> > What you propose is not what I think I had in mind.  I meant to extend
> > make-bool-vector (or make a new function, if extending proves
> > inconvenient or inelegant) that generates a bool-vector given 2
> > dimensions, not one.  Then such vectors could be used to create XBM
> > images of arbitrary dimensions.  We could even call this new function
> > something like make-xbm-data or somesuch, if its utility is limited to
> > XBM images.
> 
> How is a vector of bool vectors different from a 2-dimensional bool
> vector, from the point of view of Lisp?

I didn't mean to introduce a 2-dimensional bool-vector, I meant to be
able to create a unidimensional vector in a way that it could be then
used as data for XBM.  In practice, that means the number of bits in
the vector will be more than strictly required by multiplying the two
dimensions.

> As for the problem at hand, how would you feel about adding a :stride
> argument which must be a multiple of 8 and ensures that the bool
> vector passed to the XBM code has the right memory layout?

Could be fine, but I'd like to see an example of using such an API
before I make up my mind.

Thanks.

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

[Pip Cet]

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

On Fri, Jun 28, 2019 at 12:45 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > Having thought about it, I would rather require the bool vector passed
> > to XBM to be in the right format: the width can be 100, but the
> > stride, specified by a new :stride argument, must be the smallest
> > multiple of 8 greater or equal to the width. If you don't want that,
> > pass a vector of bool vectors which is copied together, but let's not
> > pretend we can take a 10,000-bit bool vector and display it
> > efficiently.
>
> I'm not sure I understand what you mean, exactly.  Can you show an
> example of the new API?

No major changes, just that :stride is passed in along with :height
and :width (optional for now, but strongly recommended by the
documentation). See attached patch.

[-- Attachment #2: 0001-Allow-a-stride-argument-so-XBM-boolvecs-are-in-the-r.patch --]
[-- Type: text/x-patch, Size: 3762 bytes --]

From 2b8afbef133edb9947332b114f37582ae96ef1af Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
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.
---
 doc/lispref/display.texi | 18 ++++++++++++------
 src/image.c              | 14 ++++++++++++--
 2 files changed, 24 insertions(+), 8 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/src/image.c b/src/image.c
index f3d6508f46..f628fe46db 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,14 @@ 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);
+
+      if (height > 1 && stride != (width + CHAR_BIT - 1) / CHAR_BIT * CHAR_BIT)
+	return 0;
+
       /* Check type of data, and width and height against contents of
 	 data.  */
       if (VECTORP (data))
@@ -3242,7 +3252,7 @@ xbm_image_p (Lisp_Object object)
 	}
       else if (BOOL_VECTOR_P (data))
 	{
-	  if (bool_vector_size (data) / height < width)
+	  if (bool_vector_size (data) / height < stride)
 	    return 0;
 	}
       else
-- 
2.20.1

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 29 Jun 2019 07:20:26 +0000
> Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> 
> > I'm not sure I understand what you mean, exactly.  Can you show an
> > example of the new API?
> 
> No major changes, just that :stride is passed in along with :height
> and :width (optional for now, but strongly recommended by the
> documentation). See attached patch.

Fine with me, but please indent with indent-tabs-mode set to non-nil,
to be consistent with the surrounding indentation.

Also, this change needs to be called out in NEWS.

Thanks.

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

[Pip Cet]

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

On Sat, Jun 29, 2019 at 7:56 AM Eli Zaretskii <eliz@gnu.org> wrote:
> Fine with me, but please indent with indent-tabs-mode set to non-nil,
> to be consistent with the surrounding indentation.

Oops. I actually have indent-tabs-mode set to t, so I hope whatever
caused that didn't go wrong in other places.

> Also, this change needs to be called out in NEWS.

How's this?

[-- Attachment #2: 0001-Allow-a-stride-argument-so-XBM-boolvecs-are-in-the-r.patch --]
[-- Type: text/x-patch, Size: 4396 bytes --]

From 5b761e53d7de798fd5d93bb620be682a009bcf66 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
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                 |  6 ++++++
 src/image.c              | 14 ++++++++++++--
 3 files changed, 30 insertions(+), 8 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..b7165cd1dd 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2262,6 +2262,12 @@ 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.
+This helps with bitmaps generated from Lisp.  Also, the XBM image
+handler no longer reads past the end of a bool vector that is a few
+bytes too short.
+
 \f
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/src/image.c b/src/image.c
index f3d6508f46..2c29c7b5f2 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,14 @@ 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);
+
+      if (height > 1 && stride != (width + CHAR_BIT - 1) / CHAR_BIT * CHAR_BIT)
+	return 0;
+
       /* Check type of data, and width and height against contents of
 	 data.  */
       if (VECTORP (data))
@@ -3242,7 +3252,7 @@ xbm_image_p (Lisp_Object object)
 	}
       else if (BOOL_VECTOR_P (data))
 	{
-	  if (bool_vector_size (data) / height < width)
+	  if (bool_vector_size (data) / height < stride)
 	    return 0;
 	}
       else
-- 
2.20.1

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 29 Jun 2019 08:25:21 +0000
> Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> 
> > Also, this change needs to be called out in NEWS.
> 
> How's this?

It's fine, but I wonder whether the NEWS entry should contain a more
clear message about this new attribute.  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?  The text you
wrote doesn't spell that out, and also its last sentence is about
internal implementation details, which I'm not sure matter to the
reader.

Thanks.

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

[Pip Cet]

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

On Sat, Jun 29, 2019 at 9:54 AM Eli Zaretskii <eliz@gnu.org> 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.

[-- Attachment #2: 0001-Allow-a-stride-argument-so-XBM-boolvecs-are-in-the-r.patch --]
[-- Type: text/x-patch, Size: 4801 bytes --]

From 48d9faa8a0774c5dbef0b5907142ab1a32871d66 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
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.
+
 \f
 * 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

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 30 Jun 2019 09:48:31 +0000
> Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> 
> +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}
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This should be @w{@code{@var{stride} * @code{height}}}, sorry I didn't
notice this before.

Otherwise, this LGTM, thanks.

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

[Pip Cet]

On Sun, Jun 30, 2019 at 2:34 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > +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}
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This should be @w{@code{@var{stride} * @code{height}}}, sorry I didn't
> notice this before.

Thanks, but maybe @w{@code{@var{stride} * @var{height}}} would be more correct?

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 30 Jun 2019 14:53:24 +0000
> Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> 
> Thanks, but maybe @w{@code{@var{stride} * @var{height}}} would be more correct?

Yes, of course.

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

[Pip Cet]

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

On Sun, Jun 30, 2019 at 3:15 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > Thanks, but maybe @w{@code{@var{stride} * @var{height}}} would be more correct?
>
> Yes, of course.

Okay, I wasn't sure. Updated patch attached.

[-- Attachment #2: 0001-Allow-a-stride-argument-so-XBM-boolvecs-are-in-the-r.patch --]
[-- Type: text/x-patch, Size: 4809 bytes --]

From a10f0e6d67521ca1598ea0dd18cca0829eded517 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
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..38f16fe180 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 @code{@var{stride} * @var{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 should
+be specified in image specs representing the entire bitmap as a single
+bool vector.
+
 \f
 * 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

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 30 Jun 2019 15:36:17 +0000
> Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> 
> On Sun, Jun 30, 2019 at 3:15 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > > Thanks, but maybe @w{@code{@var{stride} * @var{height}}} would be more correct?
> >
> > Yes, of course.
> 
> Okay, I wasn't sure. Updated patch attached.

You removed the @w{...}, which IMO is important with long
expressions.  Other than that, looks good.

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

[Pip Cet]

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

> You removed the @w{...}, which IMO is important with long
> expressions.  Other than that, looks good.

You're right. I got it right in the email, so I don't know what happened there.

How's this?

[-- Attachment #2: 0001-Allow-a-stride-argument-so-XBM-boolvecs-are-in-the-r.patch --]
[-- Type: text/x-patch, Size: 4813 bytes --]

From a10f0e6d67521ca1598ea0dd18cca0829eded517 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
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..38f16fe180 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 @w{@code{@var{stride} * @var{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 should
+be specified in image specs representing the entire bitmap as a single
+bool vector.
+
 \f
 * 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

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

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sun, 30 Jun 2019 17:12:53 +0000
> Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> 
> From a10f0e6d67521ca1598ea0dd18cca0829eded517 Mon Sep 17 00:00:00 2001
> From: Pip Cet <pipcet@gmail.com>
> 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.

This is fine, thanks.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> 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.
>
> This is fine, thanks.

This was 12 weeks ago, but the patch hadn't been applied (and looked
good to me, too), so I've now done so.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no