all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#28824: 26.0.90; display of pbm images broken?
@ 2017-10-14  2:10 Roland Winkler
  2017-10-14  7:01 ` Eli Zaretskii
  0 siblings, 1 reply; 16+ messages in thread
From: Roland Winkler @ 2017-10-14  2:10 UTC (permalink / raw)
  To: 28824


start emacs -Q
evaluate (arg FILE-OR-DATA of create-image should point to a pbm image)

(defun foo ()
  (interactive)
  (insert-image (create-image "~/foo.pbm")))

M-x foo

This displays the image with emacs 25, but not with emacs 26.0.90:
there is just a small bounding box (smaller than the image) where
the image should be displayed.  It is my understanding that pbm
images do not depend on external libraries.  (Anyway, I built emacs
25 on the same machine without problems.)


In GNU Emacs 26.0.90 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.18.9)
 of 2017-10-13 built on regnitz
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description:	Ubuntu 16.04.3 LTS

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT
ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 LCMS2

Important settings:
  value of $LC_COLLATE: C
  value of $LANG: en_US.utf8
  value of $XMODIFIERS: 
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  display-time-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t





^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14  2:10 bug#28824: 26.0.90; display of pbm images broken? Roland Winkler
@ 2017-10-14  7:01 ` Eli Zaretskii
  2017-10-14 15:01   ` Andy Moreton
  2017-10-14 21:52   ` Roland Winkler
  0 siblings, 2 replies; 16+ messages in thread
From: Eli Zaretskii @ 2017-10-14  7:01 UTC (permalink / raw)
  To: Roland Winkler; +Cc: 28824

> Date: Fri, 13 Oct 2017 21:10:50 -0500
> From: "Roland Winkler" <winkler@gnu.org>
> 
> start emacs -Q
> evaluate (arg FILE-OR-DATA of create-image should point to a pbm image)
> 
> (defun foo ()
>   (interactive)
>   (insert-image (create-image "~/foo.pbm")))
> 
> M-x foo
> 
> This displays the image with emacs 25, but not with emacs 26.0.90:

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?

> It is my understanding that pbm images do not depend on external
> libraries.

Yes, no external libraries should be needed.





^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14  7:01 ` Eli Zaretskii
@ 2017-10-14 15:01   ` Andy Moreton
  2017-10-14 15:31     ` Eli Zaretskii
  2017-10-14 21:52   ` Roland Winkler
  1 sibling, 1 reply; 16+ messages in thread
From: Andy Moreton @ 2017-10-14 15:01 UTC (permalink / raw)
  To: 28824

On Sat 14 Oct 2017, Eli Zaretskii wrote:

>> Date: Fri, 13 Oct 2017 21:10:50 -0500
>> From: "Roland Winkler" <winkler@gnu.org>
>> 
>> start emacs -Q
>> evaluate (arg FILE-OR-DATA of create-image should point to a pbm image)
>> 
>> (defun foo ()
>>   (interactive)
>>   (insert-image (create-image "~/foo.pbm")))
>> 
>> M-x foo
>> 
>> This displays the image with emacs 25, but not with emacs 26.0.90:
>
> 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?
>
>> It is my understanding that pbm images do not depend on external
>> libraries.
>
> Yes, no external libraries should be needed.

I see problems with some PBM files from etc/images with emacs 26.0.90 on
Windows. The ones that don't work appear to be those that contain a
Netpbm header, e.g. back-arrow.pbm close.pbm cut.pbm fwd-arrow.pbm
home.pbm

    AndyM






^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14 15:01   ` Andy Moreton
@ 2017-10-14 15:31     ` Eli Zaretskii
  2017-10-14 18:16       ` Andy Moreton
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-10-14 15:31 UTC (permalink / raw)
  To: Andy Moreton; +Cc: 28824

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 14 Oct 2017 16:01:20 +0100
> 
> I see problems with some PBM files from etc/images with emacs 26.0.90 on
> Windows. The ones that don't work appear to be those that contain a
> Netpbm header, e.g. back-arrow.pbm close.pbm cut.pbm fwd-arrow.pbm
> home.pbm

I see no problems with those images you mention as problematic.

Could it be a problem specific to 64-bit builds?





^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14 15:31     ` Eli Zaretskii
@ 2017-10-14 18:16       ` Andy Moreton
  2017-10-14 19:03         ` Andy Moreton
  2017-10-14 19:14         ` Eli Zaretskii
  0 siblings, 2 replies; 16+ messages in thread
From: Andy Moreton @ 2017-10-14 18:16 UTC (permalink / raw)
  To: 28824

On Sat 14 Oct 2017, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 14 Oct 2017 16:01:20 +0100
>> 
>> I see problems with some PBM files from etc/images with emacs 26.0.90 on
>> Windows. The ones that don't work appear to be those that contain a
>> Netpbm header, e.g. back-arrow.pbm close.pbm cut.pbm fwd-arrow.pbm
>> home.pbm
>
> I see no problems with those images you mention as problematic.
>
> Could it be a problem specific to 64-bit builds?

I've tried this in a 32bit MinGW build with the same results. It appears
to be a problem with the pbm entry in image-type-header-regexps, as
doing (setq image-type-header-regexps '(("\\`P[1-6]" . pbm))) results in
the problematic images displaying correctly.

    AndyM






^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14 18:16       ` Andy Moreton
@ 2017-10-14 19:03         ` Andy Moreton
  2017-10-14 19:16           ` Eli Zaretskii
  2017-10-14 19:14         ` Eli Zaretskii
  1 sibling, 1 reply; 16+ messages in thread
From: Andy Moreton @ 2017-10-14 19:03 UTC (permalink / raw)
  To: 28824

On Sat 14 Oct 2017, Andy Moreton wrote:

> On Sat 14 Oct 2017, Eli Zaretskii wrote:
>
>>> From: Andy Moreton <andrewjmoreton@gmail.com>
>>> Date: Sat, 14 Oct 2017 16:01:20 +0100
>>> 
>>> I see problems with some PBM files from etc/images with emacs 26.0.90 on
>>> Windows. The ones that don't work appear to be those that contain a
>>> Netpbm header, e.g. back-arrow.pbm close.pbm cut.pbm fwd-arrow.pbm
>>> home.pbm
>>
>> I see no problems with those images you mention as problematic.
>>
>> Could it be a problem specific to 64-bit builds?
>
> I've tried this in a 32bit MinGW build with the same results. It appears
> to be a problem with the pbm entry in image-type-header-regexps, as
> doing (setq image-type-header-regexps '(("\\`P[1-6]" . pbm))) results in
> the problematic images displaying correctly.
>
>     AndyM

The file format is described at http://netpbm.sourceforge.net/doc/pbm.html

Further testing with the images from etc/images in emacs 26 shows that
this patch appears to fix the problem:

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)






^ permalink raw reply related	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14 18:16       ` Andy Moreton
  2017-10-14 19:03         ` Andy Moreton
@ 2017-10-14 19:14         ` Eli Zaretskii
  1 sibling, 0 replies; 16+ messages in thread
From: Eli Zaretskii @ 2017-10-14 19:14 UTC (permalink / raw)
  To: Andy Moreton; +Cc: 28824

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 14 Oct 2017 19:16:11 +0100
> 
> > I see no problems with those images you mention as problematic.
> >
> > Could it be a problem specific to 64-bit builds?
> 
> I've tried this in a 32bit MinGW build with the same results. It appears
> to be a problem with the pbm entry in image-type-header-regexps, as
> doing (setq image-type-header-regexps '(("\\`P[1-6]" . pbm))) results in
> the problematic images displaying correctly.

Then why does it work here with the unmodified PBM entry?





^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14 19:03         ` Andy Moreton
@ 2017-10-14 19:16           ` Eli Zaretskii
  2017-10-14 19:50             ` Andy Moreton
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-10-14 19:16 UTC (permalink / raw)
  To: Andy Moreton; +Cc: 28824

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 14 Oct 2017 20:03:27 +0100
> 
> Further testing with the images from etc/images in emacs 26 shows that
> this patch appears to fix the problem:
> 
> 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)

Thanks.  However, I'm puzzled by the fact that it works here without
any changes.

Can you step with a debugger through the code in image.c and tell what
exactly fails with the unmodified regexp?





^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14 19:16           ` Eli Zaretskii
@ 2017-10-14 19:50             ` Andy Moreton
  2017-10-14 20:15               ` Andy Moreton
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Moreton @ 2017-10-14 19:50 UTC (permalink / raw)
  To: 28824

On Sat 14 Oct 2017, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 14 Oct 2017 20:03:27 +0100
>> 
>> Further testing with the images from etc/images in emacs 26 shows that
>> this patch appears to fix the problem:
>> 
>> 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)
>
> Thanks.  However, I'm puzzled by the fact that it works here without
> any changes.

Loading the troublesome images from "emacs -Q" also works in unmodified
emacs 26, so something in my init.el changes the behaviour.

> Can you step with a debugger through the code in image.c and tell what
> exactly fails with the unmodified regexp?

The problem is in image.el, where string-match-p in image-type-from-data
fails to match the regexp.

    AndyM







^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14 19:50             ` Andy Moreton
@ 2017-10-14 20:15               ` Andy Moreton
  0 siblings, 0 replies; 16+ messages in thread
From: Andy Moreton @ 2017-10-14 20:15 UTC (permalink / raw)
  To: 28824

On Sat 14 Oct 2017, Andy Moreton wrote:

> On Sat 14 Oct 2017, Eli Zaretskii wrote:
>> Thanks.  However, I'm puzzled by the fact that it works here without
>> any changes.
>
> Loading the troublesome images from "emacs -Q" also works in unmodified
> emacs 26, so something in my init.el changes the behaviour.

Further testing shows that my init.el has:
    (auto-image-file-mode 1)

Removing that line fixes the problem.

    AndyM






^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14  7:01 ` Eli Zaretskii
  2017-10-14 15:01   ` Andy Moreton
@ 2017-10-14 21:52   ` Roland Winkler
  2017-10-15 17:42     ` Andy Moreton
  1 sibling, 1 reply; 16+ messages in thread
From: Roland Winkler @ 2017-10-14 21:52 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 28824

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1176 bytes --]

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.)

Then I realized there seem to be different versions of ppm files.
If I use the imagemagick tool `convert' to convert
etc/images/home.pbm to a ppm file it gives me the attached file
home-1bit.ppm.  This one still displays fine.  But if I export this
file with gimp, it gives me the attached file home-8bit.ppm.  This
file illustrates the bug I am talking about.

The file names refer to the output from the imagemagick tool
`identify':

$ 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


[-- Attachment #2: home-1bit.ppm --]
[-- Type: application/octet-stream, Size: 1845 bytes --]

[-- Attachment #3: home-8bit.ppm --]
[-- Type: application/octet-stream, Size: 1780 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-14 21:52   ` Roland Winkler
@ 2017-10-15 17:42     ` Andy Moreton
  2017-10-15 18:49       ` Eli Zaretskii
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Moreton @ 2017-10-15 17:42 UTC (permalink / raw)
  To: 28824

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






^ permalink raw reply related	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-15 17:42     ` Andy Moreton
@ 2017-10-15 18:49       ` Eli Zaretskii
  2017-10-16  8:30         ` Paul Eggert
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-10-15 18:49 UTC (permalink / raw)
  To: Andy Moreton, Paul Eggert; +Cc: 28824

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sun, 15 Oct 2017 18:42:27 +0100
> 
> Eli can decide if reverting the previous "unsigned char" -> "char"
> changes is a better fix.

Looks like going back to unsigned should be cleaner, but I'd like to
hear what Paul thinks.





^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-15 18:49       ` Eli Zaretskii
@ 2017-10-16  8:30         ` Paul Eggert
  2017-10-16 15:11           ` Eli Zaretskii
  0 siblings, 1 reply; 16+ messages in thread
From: Paul Eggert @ 2017-10-16  8:30 UTC (permalink / raw)
  To: Eli Zaretskii, Andy Moreton; +Cc: 28824-done, Roland Winkler

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

Eli Zaretskii wrote:
>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sun, 15 Oct 2017 18:42:27 +0100
>>
>> Eli can decide if reverting the previous "unsigned char" -> "char"
>> changes is a better fix.
> 
> Looks like going back to unsigned should be cleaner, but I'd like to
> hear what Paul thinks.

Andy's fixes look good to me. Going back to unsigned would result in several 
pointer casts that are more dangerous than converting to unsigned. They can be 
further improved by encapsulating this stuff into a function, and I installed 
the attached (the second one is Andy's other fix, which I installed in his name).

I looked for related bugs in image.c (i.e., caused by my earlier patch) and did 
not find any.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-regression-in-display-of-PPM-images.patch --]
[-- Type: text/x-patch; name="0001-Fix-regression-in-display-of-PPM-images.patch", Size: 2311 bytes --]

From cac318a1fce0a4c8c5c1f81f715c06c49c117592 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 16 Oct 2017 01:14:58 -0700
Subject: [PATCH 1/2] Fix regression in display of PPM images

Problem reported by Roland Winkler (Bug#28824#35).
Based on a patch proposed by Andy Moreton (Bug#28824#38).
* src/image.c (pbm_scan_index): New function.
(pbm_load): Use it to decode raw data correctly when its top bit
is set.
---
 src/image.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/src/image.c b/src/image.c
index cd4901b..335a43e 100644
--- a/src/image.c
+++ b/src/image.c
@@ -5277,6 +5277,25 @@ pbm_scan_number (char **s, char *end)
   return val;
 }
 
+/* Scan an index from *S and return it.  It is a one-byte unsigned
+   index if !TWO_BYTE, and a two-byte big-endian unsigned index if
+   TWO_BYTE.  */
+
+static int
+pbm_scan_index (char **s, bool two_byte)
+{
+  char *p = *s;
+  unsigned char c0 = *p++;
+  int n = c0;
+  if (two_byte)
+    {
+      unsigned char c1 = *p++;
+      n = (n << 8) + c1;
+    }
+  *s = p;
+  return n;
+}
+
 
 /* Load PBM image IMG for use on frame F.  */
 
@@ -5499,7 +5518,8 @@ pbm_load (struct frame *f, struct image *img)
   else
     {
       int expected_size = height * width;
-      if (max_color_idx > 255)
+      bool two_byte = 255 < max_color_idx;
+      if (two_byte)
 	expected_size *= 2;
       if (type == PBM_COLOR)
 	expected_size *= 3;
@@ -5522,24 +5542,14 @@ pbm_load (struct frame *f, struct image *img)
 	    int r, g, b;
 
 	    if (type == PBM_GRAY && raw_p)
-	      {
-		r = g = b = *p++;
-		if (max_color_idx > 255)
-		  r = g = b = r * 256 + *p++;
-	      }
+	      r = g = b = pbm_scan_index (&p, two_byte);
 	    else if (type == PBM_GRAY)
 	      r = g = b = pbm_scan_number (&p, end);
 	    else if (raw_p)
 	      {
-		r = *p++;
-		if (max_color_idx > 255)
-		  r = r * 256 + *p++;
-		g = *p++;
-		if (max_color_idx > 255)
-		  g = g * 256 + *p++;
-		b = *p++;
-		if (max_color_idx > 255)
-		  b = b * 256 + *p++;
+		r = pbm_scan_index (&p, two_byte);
+		g = pbm_scan_index (&p, two_byte);
+		b = pbm_scan_index (&p, two_byte);
 	      }
 	    else
 	      {
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Don-t-reject-PBM-header-whitespace-unnecessarily.patch --]
[-- Type: text/x-patch; name="0002-Don-t-reject-PBM-header-whitespace-unnecessarily.patch", Size: 1010 bytes --]

From 000abe3c89b1934c23377a792a838f289d4bbf8a Mon Sep 17 00:00:00 2001
From: Andy Moreton <andrewjmoreton@gmail.com>
Date: Mon, 16 Oct 2017 01:23:32 -0700
Subject: [PATCH 2/2] Don't reject PBM header whitespace unnecessarily

* lisp/image.el (image-type-header-regexps):
Allow two or more CRs or LFs in initial whitespace sequences.  See:
http://netpbm.sourceforge.net/doc/pbm.html
---
 lisp/image.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/image.el b/lisp/image.el
index 1d07761..32df508 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)
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-16  8:30         ` Paul Eggert
@ 2017-10-16 15:11           ` Eli Zaretskii
  2017-10-16 15:22             ` Roland Winkler
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2017-10-16 15:11 UTC (permalink / raw)
  To: Paul Eggert; +Cc: andrewjmoreton, winkler, 28824

> Cc: 28824-done@debbugs.gnu.org, Roland Winkler <winkler@gnu.org>
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 16 Oct 2017 01:30:07 -0700
> 
> > Looks like going back to unsigned should be cleaner, but I'd like to
> > hear what Paul thinks.
> 
> Andy's fixes look good to me. Going back to unsigned would result in several 
> pointer casts that are more dangerous than converting to unsigned. They can be 
> further improved by encapsulating this stuff into a function, and I installed 
> the attached (the second one is Andy's other fix, which I installed in his name).
> 
> I looked for related bugs in image.c (i.e., caused by my earlier patch) and did 
> not find any.

OK, thanks.





^ permalink raw reply	[flat|nested] 16+ messages in thread

* bug#28824: 26.0.90; display of pbm images broken?
  2017-10-16 15:11           ` Eli Zaretskii
@ 2017-10-16 15:22             ` Roland Winkler
  0 siblings, 0 replies; 16+ messages in thread
From: Roland Winkler @ 2017-10-16 15:22 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Paul Eggert, andrewjmoreton, 28824

On Mon Oct 16 2017 Eli Zaretskii wrote:
> > Cc: 28824-done@debbugs.gnu.org, Roland Winkler <winkler@gnu.org>
> > From: Paul Eggert <eggert@cs.ucla.edu> Date: Mon, 16 Oct 2017
> > 01:30:07 -0700
> > 
> > I looked for related bugs in image.c (i.e., caused by my earlier
> > patch) and did not find any.
> 
> OK, thanks.

Thanks for the quick fix.  Proper handling of these images is so
important for my everyday usage of emacs I'll have to wait for the
next pretest to come out.





^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-10-16 15:22 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-14  2:10 bug#28824: 26.0.90; display of pbm images broken? Roland Winkler
2017-10-14  7:01 ` Eli Zaretskii
2017-10-14 15:01   ` Andy Moreton
2017-10-14 15:31     ` Eli Zaretskii
2017-10-14 18:16       ` Andy Moreton
2017-10-14 19:03         ` Andy Moreton
2017-10-14 19:16           ` Eli Zaretskii
2017-10-14 19:50             ` Andy Moreton
2017-10-14 20:15               ` Andy Moreton
2017-10-14 19:14         ` Eli Zaretskii
2017-10-14 21:52   ` Roland Winkler
2017-10-15 17:42     ` Andy Moreton
2017-10-15 18:49       ` Eli Zaretskii
2017-10-16  8:30         ` Paul Eggert
2017-10-16 15:11           ` Eli Zaretskii
2017-10-16 15:22             ` Roland Winkler

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.