GNU bug report logs - #69992
Minor improvement to image map transformation logic

Previous Next

Full log

Message #23 received at 69992 <at> debbugs.gnu.org (full text, mbox):

From: David Ponce <da_vid <at> orange.fr>
To: 69992 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, Joseph Turner <joseph <at> breatheoutbreathe.in>
Subject: Re: bug#69992: Minor improvement to image map transformation logic
Date: Thu, 28 Mar 2024 23:22:10 +0100

[Message part 1 (text/plain, inline)]

Re-sent to all (sorry)

On 27/03/2024 23:17, Joseph Turner wrote:
[...]
> On my machine, not all tests pass with the patch.  Please be sure that
> these three new tests pass:
> 
> image-create-image-with-map
> image--compute-map-and-original-map
> image-transform-map
> 
> Personally, I find it easier to understand image map transformation when
> the logic is split into multiple functions.  However, the benefit of
> readability could certainly be outweighed by a noticeable improvement to
> user experience.  Please share some benchmarks.
> 
> Please keep in mind that `image--delayed-change-size' already debounces
> image transformation, so this code may not be so performance-critical.
  
Hello,

After more work, testing and benchmarks, I can finally confirm that my
proposed version of `image--compute-*map' without the logic splits
into multiple functions is not significantly faster than the current
version with the logic splits into multiple functions :-)

What I found interesting after profiling both current and proposed
functions is that most of the time is consumed by the call to
`image-size'!

I also found that the current implementation is not correct when
rotation is not a multiple of 90 degrees.  In this case, Emacs still
scales the image if specified, but ignores rotation and flipping.
However, in this case, the `image--compute-*map' functions do not
recompute map.

The attached new patch fixes the logic to be consistent with Emacs
internal implementation, plus some other tweaks to check if a
transformation apply before to call the transformation function.
I also updated some tests according to functions changes.
Here is a possible change log:

2024-03-28  David Ponce  <da_vid <at> orange.fr>

	* lisp/image.el (image--compute-scaling)
	(image--compute-rotation): New functions.
	(image--compute-map, image--compute-original-map): Use them.
	Ensure all transformations are applied or undone according to what
	Emacs does internally.  Call a transformation function only when
	needed.  Fix doc string.
	(image--scale-map): Assume effective scale argument.
	(image--rotate-map): Assume effective rotation argument.
	(image--rotate-coord): Improve doc string.
	(image--flip-map): Remove no more used flip argument.

	* test/lisp/image-tests.el (image-create-image-with-map): Use a
	valid SVG image otherwise `image-size' will not return a valid
	value and calculation of scale could fail.
	(image-transform-map): Update according to changed signature of
	image--flip-map.

This new version passes the `image-create-image-with-map' and
`image-transform-map' tests.  But on my laptop, the
`image--compute-map-and-original-map' fails the same for both the
current and proposed version of the functions:

F image--compute-map-and-original-map
     Test ‘image--compute-map’ and ‘image--compute-original-map’.
     (ert-test-failed
      ((should
        (image-tests--map-equal (image--compute-map image) rotated-map))
       :form
       (image-tests--map-equal
        (((circle ... . 24) "a" (help-echo "A"))
         ((rect ... 127 . 77) "b" (help-echo "B"))
         ((poly . [199 161 206 160 213 154 218 146 221 136 ...]) "c"
          (help-echo "C")))
        (((circle ... . 24) "a" (help-echo "A"))
         ((rect ... 54 . 77) "b" (help-echo "B"))
         ((poly . [126 161 133 160 140 154 145 146 148 136 ...]) "c"
          (help-echo "C"))))
       :value nil))

Thanks!

[image-compute-map-V0.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.