bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Benjamin Riefenstahl]

When I resize images, using image-transform-fit-to-height or
image-transform-fit-to-width, the scaling is performed but than
image-transform-check-size is called and signals an error.  This uses
ImageMagick 6.9.7.4 as installed by Debian GNUS/Linux 9.6 (stretch).

Also, because the scale factor is kept globally after that operation (is
that intentional BTW?), loading other images afterwards also runs into
that error, thus image-mode fails to execute and instead in new image
buffers the raw bytes are shown.

I can reproduce this on emacs-26 as well as master with "emacs -Q" with
PBM and with JPEG images.


In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2019-01-03 built on arrian
Repository revision: f7ccac9f9d82019d4d5ba3182200422fbc9c7e67
Repository branch: harfbuzz
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description: Debian GNU/Linux 9 (stretch)

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS GLIB
NOTIFY INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ LIBOTF
XFT ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM THREADS LCMS2 GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  global-magit-file-mode: t
  magit-file-mode: t
  global-git-commit-mode: t
  async-bytecomp-package-mode: t
  shell-dirtrack-mode: t
  desktop-save-mode: t
  display-time-mode: t
  diff-auto-refine-mode: t
  delete-selection-mode: t
  cua-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Features:
(shadow sort mail-extr emacsbug sendmail pcmpl-unix dirtrack dired-aux
whitespace compile ert pp ewoc man noutline outline tramp-archive
tramp-gvfs zeroconf dbus xml tramp trampver tramp-compat tramp-loaddefs
ucs-normalize parse-time magit-find-file edebug pulse xref project debug
backtrace tabify misearch multi-isearch rect cl-print thingatpt
eieio-opt speedbar sb-image ezimage dframe find-func help-fns
bug-reference magit-obsolete magit-blame magit-stash magit-bisect
magit-remote magit-commit magit-sequence magit-notes magit-worktree
magit-tag magit-merge magit-branch magit-reset magit-collab ghub-graphql
treepy graphql ghub url-http url-gw nsm url-auth url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-domsuf
url-util mailcap magit-files magit-refs magit-status magit magit-repos
magit-apply magit-wip magit-log which-func magit-diff smerge-mode
magit-core magit-autorevert autorevert filenotify magit-process
magit-margin magit-mode git-commit magit-git magit-section magit-utils
crm magit-popup log-edit message rmc puny format-spec rfc822 mml mml-sec
epa gnus-util rmail rmail-loaddefs time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev
mail-utils gmm-utils mailheader pcvs-util add-log with-editor
async-bytecomp async shell pcomplete dash rng-xsd xsd-regexp rng-cmpct
rng-nxml rng-valid rng-loc rng-uri rng-parse nxml-parse rng-match rng-dt
rng-util rng-pttrn nxml-ns nxml-mode nxml-outln nxml-rap sgml-mode dom
nxml-util nxml-enc xmltok make-mode jka-compr image-mode tcl elec-pair
imenu sh-script smie executable desktop frameset highline
benny-calendar-cfg ange-ftp comint ansi-color ring benny-unicode
generic-x cl autoinsert cc-mode cc-fonts cc-guess cc-menus cc-styles
cc-align cc-cmds cc-engine cc-vars cc-defs ps-print ps-print-loaddefs
ps-def lpr advice dired dired-loaddefs benny-x-clipboard disp-table
mm-util mail-prsvr time server protbuf cal-china lunar solar cal-dst
cal-bahai cal-islam cal-hebrew holidays hol-loaddefs vc-git diff-mode
easy-mmode diary-lib diary-loaddefs cal-menu calendar cal-loaddefs
delsel cua-base .loaddefs benny-tools browse-url autoload radix-tree
lisp-mnt mule-util cus-edit cus-start cus-load wid-edit info finder-inf
package let-alist derived pcase cl-extra help-mode easymenu url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json map url-vars seq byte-opt gv bytecomp byte-compile
cconv epg epg-config subr-x cl-loaddefs cl-lib tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Date: Sat, 05 Jan 2019 18:26:06 +0100
> 
> When I resize images, using image-transform-fit-to-height or
> image-transform-fit-to-width, the scaling is performed but than
> image-transform-check-size is called and signals an error.  This uses
> ImageMagick 6.9.7.4 as installed by Debian GNUS/Linux 9.6 (stretch).

Please show the error message and the Lisp backtrace from the error,
and also the values of the relevant variables.

Thanks.

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Benjamin Riefenstahl]

Eli Zaretskii writes:
> Please show the error message and the Lisp backtrace from the error,
> and also the values of the relevant variables.

Values:

  size -> (995 . 1068) ; From image-display-size, i.e. the image dimensions 
  (window-inside-pixel-edges) -> (8 0 933 970)

And yes, the image is actually higher than the window, it can be
scrolled.  The original image is rather small, 75x80, and the
difference, 1068 - 970 -> 98, is about the size of an original pixel
after transformation, so this looks like a rounding problem.  Maybe the
code should adjust by one original pixel when it calculates the target
height in image-transform-properties?

Stacktrace:

  Debugger entered--Lisp error: (cl-assertion-failed ((= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) nil 1068 970))
    cl--assertion-failed((= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) nil (1068 970) nil)
    (or (= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) (cl--assertion-failed '(= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) nil (list (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) (list)))
    (progn (or (= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) (cl--assertion-failed '(= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) nil (list (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) (list))) nil)
    (cl-assert (= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) t)
    (cond ((eq image-transform-resize 'fit-width) (cl-assert (= (car size) (- (nth 2 (window-inside-pixel-edges)) (nth 0 (window-inside-pixel-edges)))) t)) ((eq image-transform-resize 'fit-height) (cl-assert (= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) t)))
    (let ((size (image-display-size (image-get-display-property) t))) (cond ((eq image-transform-resize 'fit-width) (cl-assert (= (car size) (- (nth 2 (window-inside-pixel-edges)) (nth 0 (window-inside-pixel-edges)))) t)) ((eq image-transform-resize 'fit-height) (cl-assert (= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) t))))
    (progn (let ((size (image-display-size (image-get-display-property) t))) (cond ((eq image-transform-resize 'fit-width) (cl-assert (= (car size) (- (nth 2 ...) (nth 0 ...))) t)) ((eq image-transform-resize 'fit-height) (cl-assert (= (cdr size) (- (nth 3 ...) (nth 1 ...))) t)))))
    (if (and (not (numberp image-transform-resize)) (boundp 'image-type) (eq image-type 'imagemagick)) (progn (let ((size (image-display-size (image-get-display-property) t))) (cond ((eq image-transform-resize 'fit-width) (cl-assert (= (car size) (- ... ...)) t)) ((eq image-transform-resize 'fit-height) (cl-assert (= (cdr size) (- ... ...)) t))))))
    (when (and (not (numberp image-transform-resize)) (boundp 'image-type) (eq image-type 'imagemagick)) (let ((size (image-display-size (image-get-display-property) t))) (cond ((eq image-transform-resize 'fit-width) (cl-assert (= (car size) (- (nth 2 ...) (nth 0 ...))) t)) ((eq image-transform-resize 'fit-height) (cl-assert (= (cdr size) (- (nth 3 ...) (nth 1 ...))) t)))))
    image-transform-check-size()
    image-toggle-display-image()
    image-mode()
    set-auto-mode-0(image-mode nil)
    set-auto-mode()
    normal-mode(t)
    after-find-file(nil t)
    find-file-noselect-1(#<buffer att-fin-199.03.14.pbm> "~/Projects/Mandaic-Font/src/199/att-fin-199.03.14...." nil nil "~/Projects/Mandaic-Font/src/199/att-fin-199.03.14...." (17829146 66305))
    find-file-noselect("/home/benny/Projects/Mandaic-Font/src/199/att-fin-..." nil nil nil)
    find-file("/home/benny/Projects/Mandaic-Font/src/199/att-fin-...")
    dired-find-file()
    funcall-interactively(dired-find-file)
    call-interactively(dired-find-file nil nil)
    command-execute(dired-find-file)
    recursive-edit()
    debug(error (cl-assertion-failed ((= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) nil 1067 970)))
    cl--assertion-failed((= (cdr size) (- (nth 3 (window-inside-pixel-edges)) (nth 1 (window-inside-pixel-edges)))) nil (1067 970) nil)
    image-transform-check-size()
    image-toggle-display-image()
    image-transform-fit-to-height()
    funcall-interactively(image-transform-fit-to-height)
    call-interactively(image-transform-fit-to-height nil nil)
    command-execute(image-transform-fit-to-height)

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 33990@debbugs.gnu.org
> Date: Sun, 06 Jan 2019 10:12:13 +0100
> 
> Eli Zaretskii writes:
> > Please show the error message and the Lisp backtrace from the error,
> > and also the values of the relevant variables.
> 
> Values:
> 
>   size -> (995 . 1068) ; From image-display-size, i.e. the image dimensions 
>   (window-inside-pixel-edges) -> (8 0 933 970)
> 
> And yes, the image is actually higher than the window, it can be
> scrolled.  The original image is rather small, 75x80, and the
> difference, 1068 - 970 -> 98, is about the size of an original pixel
> after transformation, so this looks like a rounding problem.  Maybe the
> code should adjust by one original pixel when it calculates the target
> height in image-transform-properties?

Looks like your analysis is correct, thanks.  Can you propose a patch
that would implement your suggestion?

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Benjamin Riefenstahl]

Hi Eli,

Eli Zaretskii writes:
> Looks like your analysis is correct, thanks.  Can you propose a patch
> that would implement your suggestion?

Just to let you know, I am on it, though slowly.

I just found out that the problem reproduces on my laptop, but not on my
desktop machine.  Either it's the difference in display size or some
different library version.  Although last I looked at it on my laptop,
it seemed to be a problem in the Emacs code somewhere between image-size
and compute_image_size.  To be continued...

benny

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 33990@debbugs.gnu.org
> Date: Mon, 14 Jan 2019 19:40:58 +0100
> 
> Eli Zaretskii writes:
> > Looks like your analysis is correct, thanks.  Can you propose a patch
> > that would implement your suggestion?
> 
> Just to let you know, I am on it, though slowly.

Thank you.

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Benjamin Riefenstahl]

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

Eli Zaretskii writes:
> Can you propose a patch that would implement your suggestion?

It looks like my analysis was wrong.  The problem is rather that
create-image applies a default scaling, when :scale is not given.  This
depends on the font size (only if char-width > 10 pixels), which
explains why it happens on my laptop but not on my desktop machine.  I
guess create-image is targeting image resources like toolbar buttons,
where it may be a good idea to scale the images with the fonts for
so-called "hidpi" displays.  But for image-mode the situation is
different and I at least do not want that behaviour.

I attach three patches, the first is a minimal change that fixes the
problem for me.  The second adds to the doc string of create-image,
because I think that its behaviour is unexpected enough to warrant
highlighting.  The third makes the image-mode transform parameters
buffer-local, because that seems more appropriate and even complies with
the doc string form image-transform-rotation better.  This third patch
changes behaviour, though, so maybe a second opinion is in order.

The patches are against master, I haven't tested if they apply against
emacs-26.  Let me know if there is something that needs to be changed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-image-mode-Do-not-use-default-scaling-bug-33990.patch --]
[-- Type: text/x-diff, Size: 1235 bytes --]

From 951eadeb77c665c9d419e982b3651f401ad5240a Mon Sep 17 00:00:00 2001
From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
Date: Mon, 14 Jan 2019 23:37:03 +0100
Subject: [PATCH 1/4] image-mode: Do not use default scaling (bug#33990)

* lisp/image-mode.el (image-toggle-display-image): Set :scale == 1 so
that create-image does not apply additional scaling.
---
 lisp/image-mode.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index 3570c7cba4..7f290719bb 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -741,9 +741,11 @@ image-toggle-display-image
 	 (type (if (image--imagemagick-wanted-p filename)
 		   'imagemagick
 		 (image-type file-or-data nil data-p)))
+         ;; :scale 1: If we do not set this, create-image will apply
+         ;; default scaling based on font size.
 	 (image (if (not edges)
-		    (create-image file-or-data type data-p)
-		  (create-image file-or-data type data-p
+		    (create-image file-or-data type data-p :scale 1)
+		  (create-image file-or-data type data-p :scale 1
 				:max-width (- (nth 2 edges) (nth 0 edges))
 				:max-height (- (nth 3 edges) (nth 1 edges)))))
 	 (inhibit-read-only t)
-- 
2.11.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-create-image-Expand-documentation-bug-33990.patch --]
[-- Type: text/x-diff, Size: 1579 bytes --]

From ab0bb7674a702a2d142c6e7790c26fb880484591 Mon Sep 17 00:00:00 2001
From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
Date: Tue, 15 Jan 2019 17:00:04 +0100
Subject: [PATCH 2/4] create-image: Expand documentation (bug#33990)

* lisp/image.el (create-image): Discuss default for :scale in the doc
string.
---
 lisp/image.el | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lisp/image.el b/lisp/image.el
index 2e84e47b5c..3aa3b0aa24 100644
--- a/lisp/image.el
+++ b/lisp/image.el
@@ -415,13 +415,20 @@ image-type-auto-detected-p
 (defun create-image (file-or-data &optional type data-p &rest props)
   "Create an image.
 FILE-OR-DATA is an image file name or image data.
+
 Optional TYPE is a symbol describing the image type.  If TYPE is omitted
 or nil, try to determine the image type from its first few bytes
 of image data.  If that doesn't work, and FILE-OR-DATA is a file name,
 use its file extension as image type.
+
 Optional DATA-P non-nil means FILE-OR-DATA is a string containing image data.
+
 Optional PROPS are additional image attributes to assign to the image,
-like, e.g. `:mask MASK'.
+like, e.g. `:mask MASK'.  If the property `:scale' is not given and the
+display has a high resolution (more exactly, when the average width of a
+character in the default font is more than 10 pixels), the image is
+automatically scaled up in proportion to the default font.
+
 Value is the image created, or nil if images of type TYPE are not supported.
 
 Images should not be larger than specified by `max-image-size'.
-- 
2.11.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-image-mode-Make-parameters-buffer-local.patch --]
[-- Type: text/x-diff, Size: 1605 bytes --]

From aa78a077b2eaf5d844f0c8e65f7ac3354c07cfb4 Mon Sep 17 00:00:00 2001
From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
Date: Sat, 12 Jan 2019 15:36:24 +0100
Subject: [PATCH 3/4] image-mode: Make parameters buffer-local

Image parameters were treated as image specific, but because they
actually were global variables, their behaviour transfered to new
images.

* lisp/image-mode.el (image-transform-resize, image-transform-scale)
(image-transform-rotation): Declare with defvar-local.
---
 lisp/image-mode.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/image-mode.el b/lisp/image-mode.el
index 7f290719bb..fa1362c471 100644
--- a/lisp/image-mode.el
+++ b/lisp/image-mode.el
@@ -53,7 +53,7 @@ image-mode-new-window-functions
 It is called with one argument, the initial WINPROPS.")
 
 ;; FIXME this doesn't seem mature yet. Document in manual when it is.
-(defvar image-transform-resize nil
+(defvar-local image-transform-resize nil
   "The image resize operation.
 Its value should be one of the following:
  - nil, meaning no resizing.
@@ -61,10 +61,10 @@ image-transform-resize
  - `fit-width', meaning to fit the image to the window width.
  - A number, which is a scale factor (the default size is 1).")
 
-(defvar image-transform-scale 1.0
+(defvar-local image-transform-scale 1.0
   "The scale factor of the image being displayed.")
 
-(defvar image-transform-rotation 0.0
+(defvar-local image-transform-rotation 0.0
   "Rotation angle for the image in the current Image mode buffer.")
 
 (defvar image-transform-right-angle-fudge 0.0001
-- 
2.11.0

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 33990@debbugs.gnu.org
> Date: Tue, 15 Jan 2019 22:38:40 +0100
> 
> Eli Zaretskii writes:
> > Can you propose a patch that would implement your suggestion?
> 
> It looks like my analysis was wrong.  The problem is rather that
> create-image applies a default scaling, when :scale is not given.  This
> depends on the font size (only if char-width > 10 pixels), which
> explains why it happens on my laptop but not on my desktop machine.  I
> guess create-image is targeting image resources like toolbar buttons,
> where it may be a good idea to scale the images with the fonts for
> so-called "hidpi" displays.  But for image-mode the situation is
> different and I at least do not want that behaviour.
> 
> I attach three patches, the first is a minimal change that fixes the
> problem for me.  The second adds to the doc string of create-image,
> because I think that its behaviour is unexpected enough to warrant
> highlighting.  The third makes the image-mode transform parameters
> buffer-local, because that seems more appropriate and even complies with
> the doc string form image-transform-rotation better.  This third patch
> changes behaviour, though, so maybe a second opinion is in order.
> 
> The patches are against master, I haven't tested if they apply against
> emacs-26.  Let me know if there is something that needs to be changed.

Thanks, this LGTM.  I will wait for a few days for comments, and pushd
these if no objections are heard.

I understand these are not recent regressions?  If so, I think the
master branch is where they should go.  But if you think the bug is
grave enough, I could consider them for the emacs-26 branch instead.

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Benjamin Riefenstahl]

Hi Eli,
 
Eli Zaretskii writes:
> I understand these are not recent regressions?  If so, I think the
> master branch is where they should go.  But if you think the bug is
> grave enough, I could consider them for the emacs-26 branch instead.

It seems that automatic scaling was added to create-image in february
2016, so the bug may have been there since than.

How about fixing the bug and documentation on emacs-26 (patches 1 and 2)
and making the behaviour change (patch 3) on master?  I think the bug
fix (patch 1) is obviously right and not likely to cause problems.
OTOH, without it, once I had invoked an image transform I could not use
image-mode for any new images any more in that same Emacs session.

Thanks, benny

bug#33990: 27.0.50; image-transform-fit-to-height/-width violates assert

[Eli Zaretskii]

> From: Benjamin Riefenstahl <b.riefenstahl@turtle-trading.net>
> Cc: 33990@debbugs.gnu.org
> Date: Sat, 19 Jan 2019 18:21:06 +0100
> 
> How about fixing the bug and documentation on emacs-26 (patches 1 and 2)
> and making the behaviour change (patch 3) on master?

Done, thanks.