SUMMARY Generating the thumbnail for Comics Manager takes a long time and consumes a large amount of memory that is not released. STEPS TO REPRODUCE 1. Create a comic project in Comics Manager. 2. Add pages into it. 3. Open the project again in Krita. OBSERVED RESULT Comics Manager takes a long time to generate the thumbnails and consumes a large amount of memory (proportional to the size and number of pages in the project). EXPECTED RESULT Loading the thumbnails is near instantaneous and consumes imperceptible amount of memory. SOFTWARE/OS VERSIONS Ubuntu: 20.04 Tested both the appimage and flatpak versions. ADDITIONAL INFORMATION When loading an existing comicConfig.json comic (e.g. 56 pages, 4962x7014 pixels), Krita displays a “Loading pages” dialogue for ~10 seconds before it loads the thumbnails. When that is done, Krita uses 7.5GB more RAM than before opening the json file and it doesn’t seem to be released. Opening any of the pages uses additional memory still. Adding a new page also keeps increasing the memory usage. This happens both with the appimage and flatpak for 4.4.2. When using the 4.4.1 appimage, opening comicConfig.json happens instantly and it takes a negligible amount of memory. Also quoting some additional information from Tiar on krita-artists (https://krita-artists.org/t/potential-memory-leak-for-comics-manager-in-krita-4-4-2/17391): I believe it might be because Krita loads those files as KisDocuments (which means it loads it fully) and then it doesn’t release it even though it removes those documents because Krita doesn’t release memory unless it’s needed (technicalities of using TileManager). Suspicious lines: /comics_project_management_tools/comics_project_manager_docker.py:913: /comics_project_management_tools/comics_project_manager_docker.py:720: I think in other places Krita just loaded mergedimage.png, I think it would be better if it used it everywhere for thumbnails unless it’s for actual loading the file when the user clicks on it.
Er, those two lines are for when you are actually opening the krita file and when you are resizing batch resizing all files. Neither is happening when you just open the config file, and we did not change anything in the python code for that docker for 4.4.1. It uses fill_pages when loading the data, and there it loads it from the zip files, and those get shutdown too, so I don't know what is causing this.
The bug seems to still be present in Krita 4.4.3-beta2.
I managed to find some free time to investigate this and I believe I have found the cause of the issue, and it isn't a memory leak but rather an (unforseen?) change in behaviour. Looking at the history, I think the culprit is this commit: https://invent.kde.org/graphics/krita/-/commit/b8b657d2ebf323a2007e870950c131ce5ee1e6f9. In order to have higher resolution images to display, comics_project_manager_docker.fill_pages() was changed to read the mergedimage.png rather than preview.png. The problem is, that when loaded in memory (and as such decompressed), a merged image can take a significant amount of RAM. In my case, that's ~130MiB per file. Given that a comic/graphic novel has dozens of images, this quickly balloons to multiple gigabytes of ram just for page previews. Unsurprisingly, loading that many big images also takes a lot of time, which causes the delay I described before. I think this can be seen as a regression in functionality, as it uses up valuable RAM that would be otherwise be used for actual drawing. If the comic book is large enough, it might even exhaust all of the available RAM just by opening the project json file! In terms of solutions to this, I see two possible avenues to fix this: 1) Revert back to the old behaviour of loading preview.png instead of mergedimage.png. This will fix the memory/loading time issues, but will lead to smaller page previews (in my opinion not a huge issue). 2) Alternatively, if larger size page previews are a necessity, the mergedimage could be resized on the fly to something smaller before being used as an icon. This would make memory usage more sensible without sacrificing much in terms of visual quality, but it would increase the loading time even more given that we now would need to also resize multiple large images. Any thoughts on this? This might be a niche of a bug, but it prevents me from upgrading from 4.4.1, so I'd be more than willing to propose an MR if you are happy with one of the two options :)
Thanks! I had totally not seen this particular commit had changed the _source_ of the images, so I was completely puzzled, as I couldn't figure out which commit would've changed anything significantly. Guess I'll be semi-reverting tiar's commit.
Looks like it's inevitable: performance is more important... you're welcome to make a MR, but please: 1) add a comment explaining why preview.png is used instead of mergedimage.png (so no one else makes the same mistake again), 2) keep other changes; for example the devicePixelRatioF() stuff (it keeps it looking at least somewhat nice on 4k screens). (I'm afraid the pixel art won't look good there anyway, so it might be a good idea to remove that part that scales them up using FastTransformation; it will be blurred terribly anyway, unfortunately... SmoothTransformation at least will have only one kind of artifacts). 3) It would be good if you checked if after your changes the pages in the page viewer are still rendered nicely with high resolution. Thank you :)
(In reply to Tiar from comment #5) > Looks like it's inevitable: performance is more important... you're welcome > to make a MR, but please: > 1) add a comment explaining why preview.png is used instead of > mergedimage.png (so no one else makes the same mistake again), > 2) keep other changes; for example the devicePixelRatioF() stuff (it keeps > it looking at least somewhat nice on 4k screens). (I'm afraid the pixel art > won't look good there anyway, so it might be a good idea to remove that part > that scales them up using FastTransformation; it will be blurred terribly > anyway, unfortunately... SmoothTransformation at least will have only one > kind of artifacts). > 3) It would be good if you checked if after your changes the pages in the > page viewer are still rendered nicely with high resolution. > > Thank you :) I have created a (quite minimal) MR: https://invent.kde.org/graphics/krita/-/merge_requests/920 I just changed the source image for the thumbnails in the docker, so the page viewer is unaffected as far as I can tell. Unfortunately I don't own any HiDPI displays, so I wouldn't be able to tell the difference if HiDPI support regressed. I don't anticipate the change to break anything, but if anyone with a HiDPI monitor could double check it would be greatly appreciated. As a future improvement, I think it would be possible to have higher resolution thumbnails if we allowed some preprocessing and additional storage in the comic project folder. We could generate dedicated higher res previews (512px) from the merged image every time we detected a change in a .kra file, and store it in a dedicated folder (e.g. <project_root>/thumbnails). This could then be used to populate the thumbnail list instead of loading from the .kra files. I did not attempt to do that yet, because it would add quite a bit of complexity to do it right, and I first wanted a fix for the current problem. The main issue is that there is a nasty corner case when the .kra file gets modified without loading the project in the comic manager first, which can lead to stale thumbnails (this is fixable but a proper solution is complex). That and the fact that we'd need to create an additional folder on disk, which might be undesirable. However, if you are OK with the idea I could try to implement that once I get some free time.
Git commit 33330f5329b6bdbf8b960677b9154ec749d86810 by Dmitry Kazakov, on behalf of Anestis Papazoglou. Committed on 25/06/2021 at 09:36. Pushed by dkazakov into branch 'master'. Fix excessive memory usage in Comics Manager docker Reverts back to using preview.png for thumbnails instead of mergedimage.png to avoid excessive memory usage and processing time when dealing with high resolution images. This partially reverts behaviour introduced in: b8b657d2ebf323a2007e870950c131ce5ee1e6f9 M +3 -3 plugins/python/comics_project_management_tools/comics_project_manager_docker.py https://invent.kde.org/graphics/krita/commit/33330f5329b6bdbf8b960677b9154ec749d86810