eXeC64 / imv

Image viewer for X11/Wayland
MIT License
799 stars 57 forks source link

Calling mogrify on larger image causes endless loop #207

Open mb720 opened 4 years ago

mb720 commented 4 years ago

Hi!

I'm using imv 4.0.1 on Sway 1.2 and I think I've encountered a bug:

I got this mapping in my imv config:

<Shift+R> = exec mogrify -rotate 90 "$imv_current_file"

This works without a hitch for smaller images, below one MiB for example.

But when trying to rotate a larger image like this one, the processes imv-wayland and mogrify never stop. A temporary image file with a ~ suffix keeps getting generated and deleted in the current directory.

pkill imv-wayland makes it stop.

I first suspected it was a bug with mogrify but mogrify -rotate 90 theFile.jpg works.

mogrify -v produces

ImageMagick 7.0.9-2

eXeC64 commented 4 years ago

I'm unable to reproduce with ImageMagick 7.0.9-2 on my Arch install. Both imv-wayland and imv-x11 are able to rotate the linked image without going into a loop. I'm using imv v4.0.1-15-g0c0d26a on sway version 1.2-rc1-58-g00c4c7e8 (Sep 28 2019, branch 'master')

Can you try to reproduce with the latest tip of master?

mb720 commented 4 years ago

Thanks for looking into this.

I cloned the repo, set WINDOWS=wayland in config.mk, and ran make. This is the error I got:

Package cmocka was not found in the pkg-config search path.
Perhaps you should add the directory containing `cmocka.pc'
to the PKG_CONFIG_PATH environment variable
Package 'cmocka', required by 'virtual:world', not found
cc -o build/imv-wayland build/main.o build/binds.o build/bitmap.o build/canvas.o build/commands.o build/console.o build/image.o build/imv.o build/ini.o build/ipc.o build/ipc_common.o build/keyboard.o build/list.o build/log.o build/navigator.o build/source.o build/viewport.o build/backend_freeimage.o build/backend_librsvg.o build/wl_window.o build/xdg-shell-protocol.o -lGL -lpthread -lxkbcommon -lpangocairo-1.0 -lpango-1.0 -lgobject-2.0 -lglib-2.0 -lharfbuzz -lcairo  -licuio -licui18n -licuuc -licudata  -lfreeimage -lrsvg-2 -lm -lgio-2.0 -lgdk_pixbuf-2.0 -lgobject-2.0 -lglib-2.0 -lcairo  -lwayland-client -lwayland-egl -lEGL -lrt 
/usr/bin/ld: build/backend_librsvg.o: in function `load_image':
backend_librsvg.c:(.text+0xc5): undefined reference to `imv_image_create_from_svg'
collect2: error: ld returned 1 exit status
make: *** [Makefile:120: build/imv-wayland] Error 1
eXeC64 commented 4 years ago

Can you do a make clean and build again? Sounds like image.c was compiled without SVG support enabled, and then backend_librsvg.c was later added without recompiling image.c

mb720 commented 4 years ago

Sure. After git pull and make clean I get this error:

Package cmocka was not found in the pkg-config search path.
Perhaps you should add the directory containing `cmocka.pc'
to the PKG_CONFIG_PATH environment variable
Package 'cmocka', required by 'virtual:world', not found

git remote -v gives:

origin  https://github.com/eXeC64/imv.git (fetch)
origin  https://github.com/eXeC64/imv.git (push)

I'm on the master branch.

eXeC64 commented 4 years ago

It's missing the cmocka package, which is used for unit tests. Seems like a bug in the makefile since you should only be required to have it to run make test. For now can you install cmocka?

mb720 commented 4 years ago

After pacman -S cmocka I get:

cc -o build/imv-msg build/imv_msg.o build/ipc_common.o  
a2x --no-xmllint --doctype manpage --format manpage doc/imv.1.txt
make: a2x: Command not found
make: *** [Makefile:152: doc/imv.1] Error 127

I installed a2x with sudo pacman -S asciidoc

This worked afterwards:

make
make install

imv -v: Version: v4.0.1-19-gc9b8ec8-dirty

Then I tried rotating the image again with R which caused an endless loop.

After killing mogrify there's this error message in the terminal:

mogrify: insufficient image data in file `/home/mb/experiments/ada_images/ada.jpg' @ error/jpeg.c/ReadJPEGImage/1183.
Killed
mb720 commented 4 years ago

By the way: I remember having to install pkg-config as well to get the build going.

cinghiopinghio commented 4 years ago

Hi, I have a similar behaviour when calling external programs like rofi dmenu or wofi. when I define the following binding:

[binds]
p = exec notify-send $(lpstat -s | dmenu)

after pressing p, imv enters in a loop (whenever I choose something from dmenu I got the notification and another instance of dmenu and so on).

This works as described with wofi and rofi and also when notify-send is not involved. I'm in sway, archlinux.

rmsacks commented 4 years ago

I am experiencing the same issue on Arch Linux, sway version 1.4, and imv Version: v4.1.0-3-g04ef9440fb07-dirty. After doing some debugging, I found that the issue is due to a conflict between long-running external commands executed by imv and the key repeat timer for wayland windows. If the external process runs for long enough, the key repeat timer will fire, the repeat key will be "pressed" again, and a new external process will be created before the first one was able to finish (causing an endless loop).

I was able to mitigate this issue by disabling the key repeat timer like so:

diff --git a/src/wl_window.c b/src/wl_window.c
index 3a1caed77c91..fc0cd3db5aee 100644
--- a/src/wl_window.c
+++ b/src/wl_window.c
@@ -767,7 +767,7 @@ struct imv_window *imv_window_create(int width, int height, const char *title)
   create_window(window, width, height, title);

   struct sigevent timer_handler = {
-    .sigev_notify = SIGEV_THREAD,
+    .sigev_notify = SIGEV_NONE,
     .sigev_value.sival_ptr = window,
     .sigev_notify_function = on_timer,
   };

Now that on_timer() is never actually called, I am able to rotate images with mogrify just fine. This is less than ideal, though, since key repeating on wayland windows no longer works.

I'm assuming that some people aren't able to reproduce this bug because their computers are able to completely finish the image rotation before the repeat timer is fired. You may be able to reproduce it by checking how long the mogrify command takes,

$ time mogrify -rotate 90 test.jpg

real    0m0.739s
user    0m0.567s
sys     0m0.170s

and then changing the repeat delay in your wayland session to be shorter than it takes to run that command. On sway, you can do that by adding this to your config file:

input * repeat_delay 300

I'm working on a fix for this bug, but I'm new to the codebase and it may take longer than expected. If anyone is able to get a fix working before I do, please send a patch.

mb720 commented 4 years ago

Wow, @rmsacks, excellent catch!

How did you find out that it's due to the key repeater?

cinghiopinghio commented 4 years ago

For the time being, I found an easy workaround: send the long lasting command to the background.

<Shift+R> = exec long --lasting --command &