bug#28824: 26.0.90; display of pbm images broken?

[Andy Moreton <andrewjmoreton@gmail.com>]

On Sat 14 Oct 2017, Roland Winkler wrote:

> On Sat Oct 14 2017 Eli Zaretskii wrote:
>> Works for me in Emacs 26.0.90 on MS-Windows.  I used PBM images from
>> etc/images/ in the Emacs tree: does this fail to work for you with
>> those images as well?
>
> I need to apologize: the bug is not with pbm images, but with ppm
> images.  (The tool I use to create them calls them pbm, but it gives
> ppm, pgm or pbm files depending on the content of the image.)

No problem - it lead me to find a different bug that also needed fixing.

> $ identify home*
> home-1bit.ppm PPM 24x24 24x24+0+0 1-bit sRGB 1.84KB 0.000u 0:00.000
> home-8bit.ppm PPM 24x24 24x24+0+0 8-bit sRGB 1.78KB 0.000u 0:00.000
>
> PS: Visiting these files display them fine (according to the
> modeline, this uses imagemagick).  The problem occurs with
> insert-image which appears to process these files differently

Thanks for the example files. On Windows with a 64bit MSYS2 build of
emacs 26 I also see the problems loading the home-8bit.ppm file.

After some debugging, it appears that the problem was introduced in:

commit 3c2c50260e19deff2a0a054882eaea4049f25a2f
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Thu Sep 29 20:09:37 2016 -0700

This changed the raster data from using "unsigned char" to "char" which
cannot be correct, as this is binary data. The trouble occurs when
reading an 0xff byte from the raster data, which gets sign extended to
r, g and b values (of type int) of -1.

The following patch works for me (including the earlier fix):

diff --git a/lisp/image.el b/lisp/image.el
index 1d0776180b..32df508bc8 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -34,8 +34,8 @@ 'image-refresh
 (defconst image-type-header-regexps
   `(("\\`/[\t\n\r ]*\\*.*XPM.\\*/" . xpm)
     ("\\`P[1-6]\\(?:\
-\\(?:\\(?:#[^\r\n]*[\r\n]\\)?[[:space:]]\\)+\
-\\(?:\\(?:#[^\r\n]*[\r\n]\\)?[0-9]\\)+\
+\\(?:\\(?:#[^\r\n]*[\r\n]\\)*[[:space:]]\\)+\
+\\(?:\\(?:#[^\r\n]*[\r\n]\\)*[0-9]\\)+\
 \\)\\{2\\}" . pbm)
     ("\\`GIF8[79]a" . gif)
     ("\\`\x89PNG\r\n\x1a\n" . png)
diff --git a/src/image.c b/src/image.c
index 3dac7086cb..1d81da2663 100644
--- a/src/image.c
+++ b/src/image.c
@@ -5523,23 +5523,23 @@ pbm_load (struct frame *f, struct image *img)
 
 	    if (type == PBM_GRAY && raw_p)
 	      {
-		r = g = b = *p++;
+		r = g = b = *(uint8_t *) p++;
 		if (max_color_idx > 255)
-		  r = g = b = r * 256 + *p++;
+		  r = g = b = r * 256 + *(uint8_t *) p++;
 	      }
 	    else if (type == PBM_GRAY)
 	      r = g = b = pbm_scan_number (&p, end);
 	    else if (raw_p)
 	      {
-		r = *p++;
+		r = *(uint8_t *) p++;
 		if (max_color_idx > 255)
-		  r = r * 256 + *p++;
-		g = *p++;
+		  r = r * 256 + *(uint8_t *) p++;
+		g = *(uint8_t *) p++;
 		if (max_color_idx > 255)
-		  g = g * 256 + *p++;
-		b = *p++;
+		  g = g * 256 + *(uint8_t *) p++;
+		b = *(uint8_t *) p++;
 		if (max_color_idx > 255)
-		  b = b * 256 + *p++;
+		  b = b * 256 + *(uint8_t *) p++;
 	      }
 	    else
 	      {

Eli can decide if reverting the previous "unsigned char" -> "char"
changes is a better fix.

    AndyM