Summary: | digiKam crashes when clicking on image in thumbnails [patch] | ||
---|---|---|---|
Product: | [Applications] digikam | Reporter: | Simon Munton <simon.j.munton> |
Component: | Thumbs-Engine | Assignee: | Digikam Developers <digikam-bugs-null> |
Status: | RESOLVED FIXED | ||
Severity: | crash | CC: | caulier.gilles |
Priority: | NOR | ||
Version: | 3.1.0 | ||
Target Milestone: | --- | ||
Platform: | Compiled Sources | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: | 4.0.0 | |
Sentry Crash Report: | |||
Attachments: | Patch to detach image data before replacing the image data in rotate function |
Description
Simon Munton
2013-04-25 20:50:03 UTC
Added a call to detach() just before all occurences of delete [] m_priv->data; in the DImg::rotate() function; so far I haven't been able to reproduce the crash. Simon, Can you provide a patch please. http://www.digikam.org/contrib Thanks in advance Gilles Caulier Created attachment 79453 [details]
Patch to detach image data before replacing the image data in rotate function
Attached patch adds the call to detach just before the old data is deleted.
This is not the most efficient way, as the image data is copied (if necessary) in the detach function, and then immediately discarded after the return from detach.
There is also the chance that between the call to detach and the data being replaced that another thread could make a copy of the Dimg and have a pointer to the deleted data .
Question : - When you clic on thumbnail, do you press rotation button as well ? - Do you have any process in background which change image data (as from Batch Queue Manager) ? Gilles Caulier No, I'm not pressing the rotation button No, there is no background process changing image data. The images were imported from my camera a week ago, some were taken in portrait mode. I start digikam, scroll down the thumbnail page and click on the image. Digikam crashes. What I have just noticed is that after clicking on the rotate left and then rotate right in the thumbnail page, the crash doesn't occur for that image, even after restarting digikam. It does still crash on other images though. If you don't press on Rotation action, why image is rotated in background ? I would to identify the condition where concurrent threads appear using image data between rotation and histogram. I would to reproduce the problem here. Currently i cannot. Gilles Caulier It is true the DImg is not implicitly but explicitly shared (=> does not automatically detach when calling a non-const method) so the scenario is indeed one we need to care for. My question is: from where it rotate() called? For exif-rotation of previews, it is called from the loading thread before it is distributed (previewtask.cpp 117) For the image editor, it is also called before being distributed (editorcore.cpp 292). I would like to fully understand this bug. I assumed it was rotating the image to display it in the preview image page. The exif data for an image that crashes shows: Orientation : Rotate 270 CW After clicking the rotate buttons it changes to: Orientation : Horizontal (normal) and all the width/heights are swapped, and it doesn't crash any more. If I put the crashing image in a subdirectory of it's own, it doesn't crash when clicked. If I then add another image to that directory, it does crash on click. >I assumed it was rotating the image to display it in the preview image page. If you is in preview mode, you cannot use icon-view to scroll down. I don't understand. by preview do you mean Image Editor openned in a separated window ? >If I put the crashing image in a subdirectory of it's own, it doesn't crash when clicked. If I then >add another image to that directory, it does crash on click. I'm confuse. This behavior is done with your patch or not ? You click on what ? Rotation button ? Gilles Caulier (In reply to comment #9) > >I assumed it was rotating the image to display it in the preview image page. > > If you is in preview mode, you cannot use icon-view to scroll down. I don't > understand. by preview do you mean Image Editor openned in a separated > window ? Click on the image in thumbnail mode, which should result in the preview mode opening and displaying image, but it crashes > >If I put the crashing image in a subdirectory of it's own, it doesn't crash when clicked. If I then >add another image to that directory, it does crash on click. > > I'm confuse. This behavior is done with your patch or not ? You click on > what ? Rotation button ? > This is without the patch. Two images in a directory in thumbnail mode, click on the second image to show in preview mode, crash. >Click on the image in thumbnail mode, which should result in the preview mode opening and >displaying image, but it crashes Ok, now it's clear... I remember few other entries in this context. So your Histogram view is open on the right side of preview. right ? >This is without the patch. Two images in a directory in thumbnail mode, click on the second >image to show in preview mode, crash. ok. Can you share on the web file which crash, to reproduce the problem on my computer ? Q : which computer you use ? Which distro ? how many CPU and memory ? Gilles Caulier (In reply to comment #11) > So your Histogram view is open on the right side of preview. right ? No, histogram view is closed > >This is without the patch. Two images in a directory in thumbnail mode, click on the second >image to show in preview mode, crash. > > ok. Can you share on the web file which crash, to reproduce the problem on > my computer ? The two images here are the ones I had in a folder on their own: https://www.dropbox.com/sh/zm9m92mqyqj4n6f/7XYty4SrNf > Q : which computer you use ? Which distro ? how many CPU and memory ? Home built, Slackware 64, Core i7, 8 cpu, 16GB. KDE 4.10.2 built from source. (In reply to comment #7) > My question is: from where it rotate() called? For exif-rotation of > previews, it is called from the loading thread before it is distributed > (previewtask.cpp 117) Seems to be called from previewtask.cpp 186: #0 0x00007ffff4aecc98 in Digikam::DImg::rotate (this=0x5b34cd0, angle=<optimized out>) at /home/simon/build/kde4/digikam-3.1.0/core/libs/dimg/dimg.cpp:2419 #1 0x00007ffff4aecf1a in Digikam::DImg::rotateAndFlip (this=0x5b34cd0, orientation=-1580573) at /home/simon/build/kde4/digikam-3.1.0/core/libs/dimg/dimg.cpp:2635 #2 0x00007ffff4c9c116 in Digikam::LoadSaveThread::exifRotate (image=..., filePath=...) at /home/simon/build/kde4/digikam-3.1.0/core/libs/threadimageio/loadsavethread.cpp:329 #3 0x00007ffff4cac9ee in Digikam::PreviewLoadingTask::execute (this=0x5b34ad0) at /home/simon/build/kde4/digikam-3.1.0/core/libs/threadimageio/previewtask.cpp:186 #4 0x00007ffff4c9beae in Digikam::LoadSaveThread::run (this=0x120aca0) at /home/simon/build/kde4/digikam-3.1.0/core/libs/threadimageio/loadsavethread.cpp:136 #5 0x00007ffff4cd495e in Digikam::DynamicThread::DynamicThreadPriv::run (this=0x120ada0) at /home/simon/build/kde4/digikam-3.1.0/core/libs/threads/dynamicthread.cpp:186 #6 0x00007ffff0112fad in QThreadPoolThread::run (this=0x3964550) at concurrent/qthreadpool.cpp:107 #7 0x00007ffff011f33c in QThreadPrivate::start (arg=0x3964550) at thread/qthread_unix.cpp:338 #8 0x00007fffefe7eeae in start_thread () from /lib64/libpthread.so.0 #9 0x00007fffeec05fed in clone () from /lib64/libc.so.6 for this (rotate) thread: print m_priv.d->data $20 = (unsigned char *) 0x7fff25dc3010 While in another thread: #0 Digikam::ImageHistogram::calculate (this=0x5a72ef0) at /home/simon/build/kde4/digikam-3.1.0/core/libs/dimg/filters/levels/imagehistogram.cpp:164 #1 0x00007ffff4cd495e in Digikam::DynamicThread::DynamicThreadPriv::run (this=0x4692bc0) at /home/simon/build/kde4/digikam-3.1.0/core/libs/threads/dynamicthread.cpp:186 #2 0x00007ffff0112fad in QThreadPoolThread::run (this=0x39460b0) at concurrent/qthreadpool.cpp:107 #3 0x00007ffff011f33c in QThreadPrivate::start (arg=0x39460b0) at thread/qthread_unix.cpp:338 #4 0x00007fffefe7eeae in start_thread () from /lib64/libpthread.so.0 #5 0x00007fffeec05fed in clone () from /lib64/libc.so.6 in this (histogram) thread: print d.img.m_priv.d->data $21 = (unsigned char *) 0x7fff25dc3010 So both threads are processing the same image data. If I let the program continue, I end up with a total of four threads all calculating the histogram on the same image data. But typically, as i remember, histogram are only computed if right sidebar is open and histogram view is selected. Why ? To reduce CPU consumption... No need to compute an histogram is this one no need to be displayed... Why histogram is computed here in this case ? Of course the dysfunction still valid if all conditions are true... Gilles Caulier (In reply to comment #14) > But typically, as i remember, histogram are only computed if right sidebar > is open and histogram view is selected. Why ? To reduce CPU consumption... > No need to compute an histogram is this one no need to be displayed... > > Why histogram is computed here in this case ? After clicking on the image in the thumbnail mode, Digikam::ImagePropertiesColorsTab::slotLoadImageFromUrlComplete is called which then recomputes the histogram (imagepropertiescolorstab.cpp:498) 4 times, once for each histogram widget. Marcel, In all case, this patch is not too intrusive and easy to review. What do you think about to apply against git /master if this one can reduce crashes ? It's acceptable or not ? Best Gilles Caulier Simon, are you available for a small test (it is you who can reliably reproduce this crash) i have an interesting idea: I believe the histogram loads the image, then the histogram is computed. During that time the preview task takes the data from the cache but may not copy it. The preview is rotated in the PreviewTask. That fits well with your backtraces. This could be a subtle, difficult to spot bug explaining some of our crashes. Can you test if this fixes your problem instead of your proposed patch? diff --git a/libs/threadimageio/previewtask.cpp b/libs/threadimageio/previewtask.cpp index 5bb39fa..9763a34 100644 --- a/libs/threadimageio/previewtask.cpp +++ b/libs/threadimageio/previewtask.cpp @@ -104,10 +104,10 @@ void PreviewLoadingTask::execute() // image is found in image cache, loading is successful m_img = *cachedImg; - if (accessMode() == LoadSaveThread::AccessModeReadWrite) - { + //if (accessMode() == LoadSaveThread::AccessModeReadWrite) + //{ m_img = m_img.copy(); - } + //} // rotate if needed - images are unrotated in the cache, // except for RAW images, which are already rotated by dcraw. Marcel, I've tried to reproduce the failure, so far with no success. I tried latest git, 3.5.0 and 3.1.0 (which is where I first saw the problem). Since reporting the bug, I am now running later versions of KDE(4.11.2), Qt (4.8.5), Kernel (3.11.6) so perhaps there was some subtle interaction there. Git commit 1872bd95d4d3d706b31b5039e679d6529923859d by Marcel Wiesweg. Committed on 04/11/2013 at 18:37. Pushed by mwiesweg into branch 'master'. Ensure to use a copy of an image in a load thread if the thread only requests read-only, but exif rotation / post processing require modification of data from the cache. This is meant to fix all mysterious crashes in the ImageHistogram calculation. (as I cannot reproduce the bug, I cannot prove it is a fix) Thanks to Simon Munton for his investigations. M +8 -2 libs/threadimageio/loadsavethread.cpp M +1 -0 libs/threadimageio/loadsavethread.h M +9 -3 libs/threadimageio/previewtask.cpp http://commits.kde.org/digikam/1872bd95d4d3d706b31b5039e679d6529923859d Simon, Can you checkout code from git/master including last commit from Marcel (see comment #19) and check if crash still reproducible or not ? Thanks in advance Gilles Caulier I've tried latest git, I'm unable to reproduce crash. |