Thumbnails #1262
Thumbnails #1262
Conversation
leejuyuu
force-pushed
the
thumbs
branch
2 times, most recently
from
dac42a0 to
bee51e0
Compare
|
Please rebase the branch so we have fresh Windows build on appveyor. |
There was a problem hiding this comment.
This is a lot slower for me (reading from NAS which tops out around 300MB/s). Which is not very fast by today's standards (nvme etc well over 1GB/s).
I'm not sure what to do about this but I think there needs to be a multi-threaded option. Maybe we can scale the number of threads based on how much time is spent in I/O vs decoding/scaling.
If the computed thumbs were cached somewhere then maybe we don't need multi-threading.
|
@novomesk This branch has been rebased. @scrubbbbs Did you observe slow down when loading files without EXIF thumbnail, or was it slow even for those with EXIF thumbnail? |
It is quite a bit slower in both cases, of course not as much for EXIF since it can load much faster. However much more noticeable when loading thousands of files in the grid view. I've just noticed the embedded thumb is also not transformed correctly anymore, orientation "2" is not flipped anymore. Easy to see in formats_testset/orientations/with-thumb |
Do you mean scrolling down the grid view? I think it should only load what is currently visible, which is ~50 files for my screen. It's strange that EXIF thumbnails loads almost instantly for me (on NVME SSD), so they should not be compute bound...
Thanks! I've fixed that. |
|
Scrolling is where this is most apparent, resorting is not too bad. I prepared a couple of clips to show the difference. This folder has about 5000 files, they are small jpgs, most have exif thumbs but many do not. I tried to scroll both at the same rate. Clip 1 new version] thumbs.mp4Clip 2 git master thumbs-threads.mp4If the grid view could reschedule items when scrolling, the difference would be a lot less. Maybe after the view scrolls we set a timer to reschedule provided there is not another scroll in the meantime? I think I would prefer not reordering EXIF vs computed thumbs, just seems weird having gaps in the thumbs that fill in later. I guess I don't understand the benefit of rescheduling based on missing EXIF thumbs or not. |
|
I sorry but I haven't seen the videos. |
Thanks, reencoded to fit the github limit now. |
|
I seems to me from the videos, that no-longer visible thumbnails are still being loaded before those active on the screen. |
This is legacy behavior AFAIK and yes I agree it is probably most of the apparent slowdown. I would suggest fixing this first as it will have a bigger impact. However, this clearly shows that loading thumbnails is not always going to be I/O bound as @leejuyuu has asserted here with a single-threaded implementation. Given it the difference is around 2x in this testcase, multi-threaded thumbnail support should probably be retained. |
|
@scrubbbbs @novomesk I added some concurrency back to address the compute bound part, and the loading is now cancelable for the no longer visible thumbnails. The scrolling experience should be better now. |
|
Thanks for the update. The speed seems about the same as previously and the cancelling is a huge improvement. I haven't finished a code review but I did find a segfault when using DkFilePreview and also noticed that DkFilePreview is no longer working for zip files, it no longer scrolls and remains at its initial state with the first few thumbs loaded. The crash is a bit difficult to reproduce. I am unlocking my scroll wheel and changing scroll directions a lot on small image files so there are perhaps a 100 of images per second going by. It took me quite a long time of doing this and loading different folders etc to reproduce so I only have this one trace for now, see attachment (the first time I had no debugger attached). That's as far as I've gotten, and one interesting thing is that the image being painted seems to be valid (at least Qtcreator debugger displays it .. it has this cool feature that shows all QImages in local variables). So a use-after-free doesn't make much sense unless the debugger can read invalid memory locations. |
|
|
|
It's weird. It seems that |
|
This patch seems to fix the segfault, but I can't explain why. Also it skips an extra conversion (rgba->gray8) in setAlphaChannel .. which is also where this has been faulting. --- ImageLounge/src/DkGui/DkThumbsWidgets.cpp
+++ ImageLounge/src/DkGui/DkThumbsWidgets.cpp
@@ -477,8 +477,10 @@ void DkFilePreview::drawFadeOut(QLinearGradient gradient, QRectF imgRect, QImage
- QImage mask = *img;
- QPainter painter(&mask);
- painter.fillRect(img->rect(), Qt::black);
- painter.fillRect(img->rect(), imgGradient);
- painter.end();
-
+ // setAlphaChannel converts the mask to 8-bit gray otherwise
+ QImage mask(img->size(), QImage::Format_Grayscale8);
+ {
+ QPainter painter(&mask);
+ painter.fillRect(img->rect(), Qt::black);
+ painter.fillRect(img->rect(), imgGradient);
+ }
+ // converts image to rgba
img->setAlphaChannel(mask); |
It's not the paintEvent that was called in multiple threads, notice the "Block alloc'd by thread 1" at the end. The weird part is that is says the allocation happened (according to helgrind) in thread 1, and that makes no sense since we know it is allocated in the worker thread. However, with the patch above, the helgrind message goes away, so something must be up. This is not specific to Qt5/6. |
- Pass references - Return modified image instead of modifying pointer
Fix DkFilePreview current index not found when viewing zip files. Images inside zip files originally have encoded file path. However, `DkImageContainerT::checkForFileUpdates` later replace the file path with the path inside the archive, causing the path comparison to always fail. DkThumbLabel is also affected. Record a copy of the original path that is not mutated inside DkImageContainer to fix this.
Check if the thumbnail load was failed before requesting to avoid infinite loop.
These booleans control whether the thumbnail is to be requested. This fixes a weird bug that some thumbnails are never loaded and the gray boxes in the DkThumbScrollWidget.
- Remove unused members - Make constant members static constexpr - Reorder member variables by size - Inline the weirdly-named setThumb()
The original implementation did not scale full image thumbnail from dispatchFullImage to prevent blocking the main thread. Add another queue so these images can be scaled asynchronously.
Add a cache in DkThumbLoader, so the thumbnail consumers do not need to store a copy of thumbnail anymore. Also, toggling the thumbnail preview widget does not necessary trigger a reload.
- Create pixmap for drawing on receiving the thumbnail. Cache them for simple bounded memory management. - Reduce the lambdas to save more memory. - Regroup the updates on receiving a thumbnail. This allows us to store less data. - Cache the tooltip so there is no repeated QFileInfo creation.
This crash is due to use after free. I've dropped 0261212. Should no longer be crashing. |
|
@scrubbbbs Sorry for the late response. I've added a cache layer so that disk i/o could be reduced at the expense of slightly more memory usage. However, the memory consumption of DkThumbScene should now be capped by the cache size, instead of growing linearly by the number of thumbnails. One thing that's left is that the Pixmap cache could be used in DkFilePreview to reduce memory and increase performance. However, this seems not necessary for this PR to work and could be left as future work. Please test and see if you have further questions. Thanks! |
|
Hi @novomesk , @scrubbbbs , if there are no further issues, I plan to merge this during this weekend. |
There was a problem hiding this comment.
It seems we still have a segfault. It happened after clicking a few folders in explorer, then it crashed when double-clicking on a thumbnail.
Still trying to find a sequence to trigger it. But here is one trace from Qt5 version, release build.
Update: I seem to be completely unable to reproduce this at the moment. I guess we need some theory as to what's happening.
[INFO] [Thumbnail] /home/test/mnt/hd4/tmp/imbdb-wiki/imdb400/imdb/05/nm0001505_rm399476736_1947-11-13_2005.jpg exif=yes size=107x158 38ms
Warning: Unsupported date format
Warning: Unsupported time format
Warning: Unsupported date format
Warning: Unsupported date format
[INFO] [Thumbnail] /home/test/mnt/hd4/tmp/imbdb-wiki/imdb400/imdb/05/nm0001505_rm1223527168_1947-11-13_2005.jpg exif=yes size=160x105 16ms
Warning: Unsupported date format
Warning: Unsupported time format
Warning: Unsupported date format
Warning: Unsupported date format
[INFO] [Thumbnail] /home/test/mnt/hd4/tmp/imbdb-wiki/imdb400/imdb/05/nm0001505_rm1257081600_1947-11-13_2005.jpg exif=yes size=160x114 18ms
[INFO] [DkImageLoader] 5673 containers created in 269 ms
[INFO] [DkImageLoader] after sorting: 1944 ms
Thread 1 "nomacs" received signal SIGSEGV, Segmentation fault.
0x00007ffff63e385d in QDir::fromNativeSeparators(QString const&) () from /lib/x86_64-linux-gnu/libQt5Core.so.5
(gdb) bt
#0 0x00007ffff63e385d in QDir::fromNativeSeparators(QString const&) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#1 0x00007ffff63f834c in QFileInfo::QFileInfo(QString const&) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#2 0x00007ffff7e22103 in nmc::DkImageLoader::load (this=0x555555c1bd70, filePath=...)
at /home/test/sw/nomacs/ImageLounge/src/DkCore/DkImageLoader.cpp:862
#3 0x00007ffff6512e16 in ??? () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#4 0x00007ffff7cb1aa7 in nmc::DkThumbScene::loadFileSignal (this=<optimized out>, _t1=<optimized out>, _t2=<optimized out>)
at /home/test/sw/nomacs-testing/build/nomacsCore_autogen/XJ2BUHI2UY/moc_DkThumbsWidgets.cpp:610
#5 0x00007ffff6512e16 in ??? () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#6 0x00007ffff7cb19d7 in nmc::DkThumbLabel::loadFileSignal (this=<optimized out>, _t1=<optimized out>, _t2=<optimized out>)
at /home/test/sw/nomacs-testing/build/nomacsCore_autogen/XJ2BUHI2UY/moc_DkThumbsWidgets.cpp:365
#7 0x00007ffff749ea86 in QGraphicsItem::sceneEvent(QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#8 0x00007ffff74c2a99 in ??? () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#9 0x00007ffff74cb67f in ??? () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#10 0x00007ffff74d5903 in QGraphicsScene::event(QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#11 0x00007ffff716bd45 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#12 0x00007ffff64d8118 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#13 0x00007ffff74f395f in QGraphicsView::mouseDoubleClickEvent(QMouseEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
|
I cannot reproduce this. Which Qt5 version are you using? |
The previous trace (I have yet to produce another one) was: 7b53202, 2025-05-25, custom |
|
I have a sequence now, in both Qt5/6 same issue.
Also I have a new trace, "double free" is printed to console sometimes [INFO] [Thumbnail] /home/test/mnt/hd4/tmp/imbdb-wiki/imdb400/imdb/07/nm0000107_rm1736571392_1953-12-8_1989.jpg exif=no size=400x264 5ms
[INFO] [DkImageLoader] 3872 containers created in 115 ms
[INFO] [DkImageLoader] after sorting: 1123 ms
double free or corruption (out)
Thread 1 "nomacs" received signal SIGABRT, Aborted.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
warning: 44 ./nptl/pthread_kill.c: No such file or directory
(gdb) bt
#0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1 __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2 __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3 0x00007ffff5e4527e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4 0x00007ffff5e288ff in __GI_abort () at ./stdlib/abort.c:79
#5 0x00007ffff5e297b6 in __libc_message_impl (fmt=fmt@entry=0x7ffff5fce8d7 "%s\n") at ../sysdeps/posix/libc_fatal.c:134
#6 0x00007ffff5ea8ff5 in malloc_printerr (str=str@entry=0x7ffff5fd1ac0 "double free or corruption (out)") at ./malloc/malloc.c:5772
#7 0x00007ffff5eab120 in _int_free_merge_chunk (av=0x7ffff6003ac0 <main_arena>, p=0x55555780fff0, size=93825028652752) at ./malloc/malloc.c:4676
#8 0x00007ffff5eab43a in _int_free (av=0x7ffff6003ac0 <main_arena>, p=<optimized out>, have_lock=<optimized out>) at ./malloc/malloc.c:4646
#9 0x00007ffff5eaddae in __GI___libc_free (mem=0x555557810000) at ./malloc/malloc.c:3398
#10 0x00007ffff63f845f in QFileInfo::QFileInfo(QString const&) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#11 0x00007ffff7e22103 in nmc::DkImageLoader::load (this=0x555555c0b350, filePath=...) at /home/test/sw/nomacs/ImageLounge/src/DkCore/DkImageLoader.cpp:862
#12 0x00007ffff6512e16 in ??? () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#13 0x00007ffff7cb1aa7 in nmc::DkThumbScene::loadFileSignal (this=<optimized out>, _t1=<optimized out>, _t2=<optimized out>)
at /home/test/sw/nomacs-testing/build/nomacsCore_autogen/XJ2BUHI2UY/moc_DkThumbsWidgets.cpp:610
#14 0x00007ffff6512e16 in ??? () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#15 0x00007ffff7cb19d7 in nmc::DkThumbLabel::loadFileSignal (this=<optimized out>, _t1=<optimized out>, _t2=<optimized out>)
at /home/test/sw/nomacs-testing/build/nomacsCore_autogen/XJ2BUHI2UY/moc_DkThumbsWidgets.cpp:365
#16 0x00007ffff749ea86 in QGraphicsItem::sceneEvent(QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#17 0x00007ffff74c2a99 in ??? () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#18 0x00007ffff74cb67f in ??? () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#19 0x00007ffff74d5903 in QGraphicsScene::event(QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#20 0x00007ffff716bd45 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5
#21 0x00007ffff64d8118 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /lib/x86_64-linux-gnu/libQt5Core.so.5
#22 0x00007ffff74f395f in QGraphicsView::mouseDoubleClickEvent(QMouseEvent*) () at /lib/x86_64-linux-gnu/libQt5Widgets.so.5 |
|
It is use-after-free on the --- ImageLounge/src/DkGui/DkThumbsWidgets.cpp
+++ ImageLounge/src/DkGui/DkThumbsWidgets.cpp
@@ -991,3 +991,3 @@ void DkThumbLabel::mouseDoubleClickEvent(QGraphicsSceneMouseEvent *event)
{
- emit loadFileSignal(mFilePath, event->modifiers() == Qt::ControlModifier);
+ emit loadFileSignal(QString(mFilePath), event->modifiers() == Qt::ControlModifier);
} |
|
I cannot reproduce the crash. Do you have a screen recording? The steps are unclear (click where?). The problem is that the event handlers are called by Qt. The DkThumbLabel should be owned by QGraphicsScene. I don't see why the event handlers are called after/during the DkThumbLabel is destroyed. |
|
Never mind. I just got a crash after posting the last comment... 🤦 |
|
Record some logs. Something else other than DkThumbScene::updateThumbLabels is destroying the labels. |
|
More logs. This is indeed a use after free. |
|
I think I understand what is going on now. The call to |
It should only happen when switching from zip to non-zip folder, then double-clicking. This is the line that causes the directory update when double-clicking. I can imagine there are other cases that can also cause this as there are several paths ending up in void DkImageLoader::setCurrentImage(QSharedPointer<DkImageContainerT> newImg)
{
// force index folder if we dir out of the zip
if (mCurrentImage && newImg && mCurrentImage->isFromZip() && !newImg->isFromZip())
mFolderUpdated = true; |
The loadFileSignal is connected to slots that updates the current directory and could potentially delete the DkThumbLabel during the process, making `mFilePath` a dangling reference. See the discussions in nomacs#1262 This temporarily bypasses the possible segfault by copying the `mFilePath`, so the value outlives the signal. We should fix the lifetime of DkThumbLabel in the future.
There was a crash at setAlphaChannel call that was probably due to a race condition when converting the mask image to grayscale8. See #1262 (comment) Specify the format when constructing the mask to bypass this conversion.
No description provided.