Bug 426175 - Segmentation fault while Faces detection
Summary: Segmentation fault while Faces detection
Status: RESOLVED FIXED
Alias: None
Product: digikam
Classification: Applications
Component: Faces-Detection (show other bugs)
Version: 7.2.0
Platform: Ubuntu Linux
: NOR crash
Target Milestone: ---
Assignee: Digikam Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-04 08:36 UTC by Marcel
Modified: 2020-10-04 05:42 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 7.2.0
Sentry Crash Report:


Attachments
Backtrace (1.45 KB, text/plain)
2020-09-04 08:36 UTC, Marcel
Details
Run without debug output (539 bytes, text/plain)
2020-09-04 08:37 UTC, Marcel
Details
Run with debug output (17.39 KB, text/plain)
2020-09-04 08:38 UTC, Marcel
Details
Error with skipping already scanned images (7.52 KB, text/plain)
2020-09-04 08:38 UTC, Marcel
Details
Memory consumption of the process before it crashs (29.45 KB, image/png)
2020-09-05 05:11 UTC, Marcel
Details
endless loop without crash (308.41 KB, image/png)
2020-09-05 05:27 UTC, Marcel
Details
Valgrind output (85.54 KB, text/plain)
2020-09-05 07:09 UTC, Marcel
Details
Memory of digikam-7.2.0-beta1-20200905T070522-x86-64-debug.appimage (199.43 KB, image/png)
2020-09-06 07:19 UTC, Marcel
Details
Memory of digikam-7.2.0-beta1-20200905T070220-x86-64.appimage (231.51 KB, image/png)
2020-09-06 07:20 UTC, Marcel
Details
Seg fault in virtual Box running Kubuntu (7.84 KB, text/plain)
2020-09-06 11:15 UTC, Marcel
Details
valgrind session Kubuntu 20.04 7.2.0-beta1 scan new items (21.23 KB, application/gzip)
2020-09-06 16:05 UTC, caulier.gilles
Details
heaptrack.jpg (526.54 KB, image/jpeg)
2020-09-06 16:32 UTC, Maik Qualmann
Details
valgrind session Kubuntu 20.04 7.2.0-beta1 scan faces (7.17 KB, application/gzip)
2020-09-06 17:41 UTC, caulier.gilles
Details
BackTrace after fixing FaceExtractor memory consumtion (1.58 KB, text/plain)
2020-09-08 21:25 UTC, Minh Nghia Duong
Details
Face detection process in htop (365.10 KB, image/png)
2020-09-21 16:19 UTC, Marcel
Details
heap-use-after-free error (7.91 KB, text/plain)
2020-09-22 05:08 UTC, Marcel
Details
Address Sanitizer error on PreviewLoadingTask (9.82 KB, text/plain)
2020-09-22 13:42 UTC, Marcel
Details
Again address Sanitizer error on PreviewLoadingTask (7.88 KB, text/plain)
2020-09-28 14:36 UTC, Marcel
Details
clang tidy patch for override fixes in theadimageio (7.98 KB, patch)
2020-09-29 06:17 UTC, caulier.gilles
Details
clang tidy patch for override fixes in theads (4.70 KB, patch)
2020-09-29 06:19 UTC, caulier.gilles
Details
threadimageio clang-tidy patch about pass by value (8.18 KB, patch)
2020-10-01 18:46 UTC, caulier.gilles
Details
asan error in CPGFMemoryStream (8.30 KB, text/plain)
2020-10-02 05:47 UTC, Marcel
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel 2020-09-04 08:36:44 UTC
Created attachment 131414 [details]
Backtrace

SUMMARY
I can't run the current face detection. There is a seg fault. In previous digikam version I scanned everything. So I tried with complete new can (with merge) an skip already scanned. Both end up in a seg fault.
By the way it is a big image library with around 20'000 pics.

STEPS TO REPRODUCE
1. Just ran the face detection (I did not try with a smaller set)

OBSERVED RESULT


EXPECTED RESULT


SOFTWARE/OS VERSIONS
Digikam: Commit 2fdab922ec229c29be1698e9dce59b80dd6cfda6
Linux/KDE Plasma: Kubuntu 20.04
(available in About System)
KDE Plasma Version: 5.18.5
KDE Frameworks Version: 5.68.0
Qt Version: 5.12.8

ADDITIONAL INFORMATION
Comment 1 Marcel 2020-09-04 08:37:30 UTC
Created attachment 131415 [details]
Run without debug output
Comment 2 Marcel 2020-09-04 08:38:03 UTC
Created attachment 131416 [details]
Run with debug output
Comment 3 Marcel 2020-09-04 08:38:30 UTC
Created attachment 131417 [details]
Error with skipping already scanned images
Comment 4 caulier.gilles 2020-09-04 09:03:03 UTC
The backtrace has already seen somewhere and Maik tried already to fix the problem i think...

Gilles
Comment 5 caulier.gilles 2020-09-04 09:04:05 UTC
NGhia,

Can you reproduce the crash on your computer ?

Gilles
Comment 6 Minh Nghia Duong 2020-09-04 09:12:55 UTC
Hi Gilles,

Yes, I can reproduce it with a huge trunk of images. I am investigating it. I suspect that somewhere in faces pipeline where I changed QImage to QImage* could cause the problem. As I remember, it does not involve face detection, but it might link somewhere. With the backtrace, I think the crash might be during the rendering of cropped faces to Widget.

Nghia
Comment 7 caulier.gilles 2020-09-04 14:22:15 UTC
Hi Nghia,

Do you use also a Ubuntu like Linux ?

Gilles
Comment 8 Minh Nghia Duong 2020-09-04 14:50:24 UTC
(In reply to caulier.gilles from comment #7)
> Hi Nghia,
> 
> Do you use also a Ubuntu like Linux ?
> 
Yes, I use Ubuntu 18.4.

> Gilles
Comment 9 caulier.gilles 2020-09-04 14:55:09 UTC
Here under Mageia and Centos, i cannot reproduce the crash. As you know i parsed my huge family collection of photo.

So, the difference can be something in the kernel about memory allocation or a different ulimit settings.

Gilles
Comment 10 Minh Nghia Duong 2020-09-04 14:58:02 UTC
(In reply to caulier.gilles from comment #9)

How large is your collection? I found that it crashes after found about 3000 faces.

Nghia
Comment 11 caulier.gilles 2020-09-04 15:00:14 UTC
Around 200 000 photo, with JPEG (mostly), RAW, PNG, and TIFF.

I use 32 Gb of RAM, so if there is a memory leak, i cannot see the dysfunction, but i already check the system memory while scanning, and it's very stable.

Gilles
Comment 12 Minh Nghia Duong 2020-09-04 15:23:08 UTC
I will go through this problem while reform faces management in preparation for the portage to DPlugin. I hope I will fund somethings
Comment 13 Marcel 2020-09-04 16:37:15 UTC
I checked my library and there are around 60'000 pics and 3'500 unknown faces and about the same number tagged. The error shows up around 10 to 20 seconds after starting the process. It works on a smaller set of pics. 

If I can check anything just say it.
Comment 14 Maik Qualmann 2020-09-04 18:18:38 UTC
The problem is a race condition in the preview loading thread. I've spent a lot of time with it and think I have a good overview of the function. It is programmed very amboned. I have a new idea how the loading notification can be exchanged between the threads safely. I'll be programming a major change for testing. Why the problem mainly occurs under Ubuntu is a mystery to me.

Maik
Comment 15 Maik Qualmann 2020-09-04 18:20:42 UTC
Marcel, a GDB backtrace would be good to see in which function it crashes.

https://www.digikam.org/contribute/

Maik
Comment 16 caulier.gilles 2020-09-04 18:37:44 UTC
Maik,

Can you remember me which Preview engine call is in fault from the Face detection process ? I want mean, if the face engine call to quickly the preview engine and create race conditions, perhaps we can lock the process step ?

Creating a unit test to run Preview Engine in multi-thread with plenty of images to render can help to debug this code.

Gilles
Comment 17 Maik Qualmann 2020-09-04 18:50:32 UTC
I think a unit test won't help. The problem is that all processes are brought together via the static loading cache, including the thumbnail threads / tasks. Unfortunately, I and apparently you cannot reproduce the crash. All loading tasks use a non-recursive mutex because a QWaitCondition is used. But this also means that all processes start when the mutex is unlocked. I hope that the problem with calling the wrong overloaded function due to my change to explicit float type in the progress info function is a thing of the past, so far this crash has not been reported again.

Maik
Comment 18 Maik Qualmann 2020-09-04 18:59:22 UTC
Another problematic feature of the preview loader is the loading notification. This is only used by a single function in digiKam. It's the color properties tab. The idea is good, if in the meantime another thread has already loaded the image completely, the current request is canceled and the new image requested. I debugged it. The chance of it entering was zero after 1 hour of intensive use. But thousands of notifications were sent about the addresses stored in the loading cache, if one of these taks no longer exists in the meantime, it would crash.

Maik
Comment 19 Maik Qualmann 2020-09-04 19:08:22 UTC
Git commit bd222b146bc7516378646efd3fff8ac5bf7bb737 by Maik Qualmann.
Committed on 04/09/2020 at 19:07.
Pushed by mqualmann into branch 'master'.

disable loading notification for a test

M  +1    -1    core/libs/threadimageio/fileio/loadsavetask.cpp
M  +1    -1    core/libs/threadimageio/preview/previewtask.cpp
M  +1    -1    core/libs/threadimageio/thumb/thumbnailtask.cpp

https://invent.kde.org/graphics/digikam/commit/bd222b146bc7516378646efd3fff8ac5bf7bb737
Comment 20 Maik Qualmann 2020-09-04 19:09:16 UTC
Marcel, it would be good if you would test the change.

Maik
Comment 21 caulier.gilles 2020-09-04 19:32:18 UTC
See my face detection session with 7.2.0-beta2 Appimage bundle:

https://youtu.be/KnJVSsu0nX8

There is no memory leak here, no crash at all.

Best

Gilles
Comment 22 caulier.gilles 2020-09-04 19:34:18 UTC
Note the HD version of the session is under processing by YouTube. Please wait...

Gilles
Comment 23 Marcel 2020-09-05 04:49:08 UTC
(In reply to Maik Qualmann from comment #15)
> Marcel, a GDB backtrace would be good to see in which function it crashes.
> 
> https://www.digikam.org/contribute/
> 
> Maik

The backtrace was attached from the beginning on. See Backtrace.txt
Comment 24 Marcel 2020-09-05 05:11:29 UTC
Created attachment 131427 [details]
Memory consumption of the process before it crashs

I checked the new commit from Maik. It looks much better. Anyway I still have a seg fault later in time. So it takes longer until it crashes. 

But now I think it is a memory leak. See the attached picture MemoryConsumption.png
Comment 25 Marcel 2020-09-05 05:27:48 UTC
Created attachment 131428 [details]
endless loop without crash

On another try with gdb the behavior was even more strange. There is no seg fault after running out of memory but digikam is still running but does not check any pictures for faces. You can see this on the network history as all my pics are on a network drive. See MemoryConsumptionSlowDown.png
Comment 26 caulier.gilles 2020-09-05 05:53:17 UTC
Marcel,

If there is a memory leak somewhere with your Linux box (i see nothing with mine)
valgrind can certainly help to identify the dirty code.

Look here to see how to run digiKam in valgrind (running speed is decreased by 20).

https://www.digikam.org/api/index.html#memleak

I full valgrind console trace can be instructive as the memory leak can be located outside digiKam...


Gilles
Comment 27 Marcel 2020-09-05 07:09:18 UTC
Created attachment 131429 [details]
Valgrind output

Here is the output of valgrind. As far as I understand this output, there is a problem in dnnfacedetectorssd.cpp.
Comment 28 caulier.gilles 2020-09-05 07:26:27 UTC
Hi Marcel,

The problem is not directly in dnnfacedetectorssd. Look well the back-trace. All continue in low level inside... OpenCV API. The memory problem is at this place.

This is what we suspect since a while: the big puzzle named OpenCV as too many way to optimize computation / process/ data management, using hardware, as GPU.

I'm in contact with team from KDenlive project (video montage application) which use also OpenCV in background with the same problem.

The solution is to disable optimizations at OpenCV compilation. I do it for all bundles, but it's not possible for native digiKam version where we don't have control of OpenCV configuration.

So the ultimate solution is to find a real solution in digiKam code to prevent this memory leak. The problem is to find the context where this dysfunction appear, and it's certainly sue to compilation option in OpenCV, as, i never reproduce this memory leak on all Linux version installed in my computer since many year. This want mean that OpenCV here is compiled without a specific optimization. The Q is which one?

If you look in OpenCV CMake top level options, you will see a very huge list of possibility. It's completely crazy...   

This is what's i use for the bundles :

https://invent.kde.org/graphics/digikam/-/blob/master/project/bundles/3rdparty/ext_opencv/CMakeLists.txt#L11

Another possibility, is a problem with a specific kernel option or a GPU driver bug, used indirectly by OpenCV. Here again, the complexity to found the reasons is similar to the distance which separate the sun and the earth.

Gilles
Comment 29 caulier.gilles 2020-09-05 07:42:55 UTC
Marcel,

The video of face detection session is done with the last AppImage bundle computed yesterday with master code. Can you check to see if you reproduce the dysfunction with this version ?

https://files.kde.org/digikam/

Gilles
Comment 30 caulier.gilles 2020-09-05 07:47:14 UTC
Marcel, Nghia,

Another difference between AppImage and the native OpenCV is the version used.

AppImage : 3.4.11 LTS
Marcel computer : 4.2.0

The last stable OpenCV is 4.4.0.

Which OpenCV version we need to use ? With the new DNN code, do we need to switch definitevely to 4.4.0 or i can stay on 3.x LTS ?

Gilles
Comment 31 caulier.gilles 2020-09-05 08:15:46 UTC
Nghia,

Another to take a care in digiKam code is the way to wrap all OpenCV code with C++ exception catcher.

OpenCV can prevent a crash if client application is able to capture and exception due to an identified problematic condition in OpenCV API.

I already patch few month ago all OpenCV use in face engine, but i don't yet check if your new code repect this rule.

Have you take a care about this coding rule ?

Gilles
Comment 32 Marcel 2020-09-05 09:25:14 UTC
(In reply to caulier.gilles from comment #29)
> Marcel,
> 
> The video of face detection session is done with the last AppImage bundle
> computed yesterday with master code. Can you check to see if you reproduce
> the dysfunction with this version ?
> 
> https://files.kde.org/digikam/
> 
> Gilles

Gilles, 

I tested it with the AppImage bundle from yesterday. I guess we have at least two errors. The fix from Maik is not included in that bundle. I get again the original bug (actually I got it 2 times of 3). The time it worked, I didn't see any increase in memory. So I guess you are right with the openCv version, but as I mentioned, I can't really test it.
Comment 33 caulier.gilles 2020-09-05 09:29:29 UTC
MArcel,

You are right, Maik patches are not yet includes in AppImage.

I start Appimage 64 build now. Files will be published in few hours.

Gilles
Comment 34 Marcel 2020-09-05 10:16:25 UTC
(In reply to caulier.gilles from comment #28)
> Hi Marcel,
> 
> The problem is not directly in dnnfacedetectorssd. Look well the back-trace.
> All continue in low level inside... OpenCV API. The memory problem is at
> this place.
> 
> This is what we suspect since a while: the big puzzle named OpenCV as too
> many way to optimize computation / process/ data management, using hardware,
> as GPU.
> 
> I'm in contact with team from KDenlive project (video montage application)
> which use also OpenCV in background with the same problem.
> 
> The solution is to disable optimizations at OpenCV compilation. I do it for
> all bundles, but it's not possible for native digiKam version where we don't
> have control of OpenCV configuration.
> 
> So the ultimate solution is to find a real solution in digiKam code to
> prevent this memory leak. The problem is to find the context where this
> dysfunction appear, and it's certainly sue to compilation option in OpenCV,
> as, i never reproduce this memory leak on all Linux version installed in my
> computer since many year. This want mean that OpenCV here is compiled
> without a specific optimization. The Q is which one?
> 
> If you look in OpenCV CMake top level options, you will see a very huge list
> of possibility. It's completely crazy...   
> 
> This is what's i use for the bundles :
> 
> https://invent.kde.org/graphics/digikam/-/blob/master/project/bundles/
> 3rdparty/ext_opencv/CMakeLists.txt#L11
> 
> Another possibility, is a problem with a specific kernel option or a GPU
> driver bug, used indirectly by OpenCV. Here again, the complexity to found
> the reasons is similar to the distance which separate the sun and the earth.
> 
> Gilles

Gilles, 

you are right about the memory leak in openCV. For me it's the first time reading a valgrind output and I want to understand it. From my understanding there are more problems then just the one in openCV. Whats about the one QRect::operator& or QRect::isValid() or qRound(double) or Digikam::DNNFaceDetectorBase::correctBbox. I guess in most examples it's just because of an uninitialized variable, but shouldn't that be fixed anyway. Can't this lead to undefined behavior in same strange cases?(In reply to caulier.gilles from comment #28)
> Hi Marcel,
> 
> The problem is not directly in dnnfacedetectorssd. Look well the back-trace.
> All continue in low level inside... OpenCV API. The memory problem is at
> this place.
> 
> This is what we suspect since a while: the big puzzle named OpenCV as too
> many way to optimize computation / process/ data management, using hardware,
> as GPU.
> 
> I'm in contact with team from KDenlive project (video montage application)
> which use also OpenCV in background with the same problem.
> 
> The solution is to disable optimizations at OpenCV compilation. I do it for
> all bundles, but it's not possible for native digiKam version where we don't
> have control of OpenCV configuration.
> 
> So the ultimate solution is to find a real solution in digiKam code to
> prevent this memory leak. The problem is to find the context where this
> dysfunction appear, and it's certainly sue to compilation option in OpenCV,
> as, i never reproduce this memory leak on all Linux version installed in my
> computer since many year. This want mean that OpenCV here is compiled
> without a specific optimization. The Q is which one?
> 
> If you look in OpenCV CMake top level options, you will see a very huge list
> of possibility. It's completely crazy...   
> 
> This is what's i use for the bundles :
> 
> https://invent.kde.org/graphics/digikam/-/blob/master/project/bundles/
> 3rdparty/ext_opencv/CMakeLists.txt#L11
> 
> Another possibility, is a problem with a specific kernel option or a GPU
> driver bug, used indirectly by OpenCV. Here again, the complexity to found
> the reasons is similar to the distance which separate the sun and the earth.
> 
> Gilles


Gilles, 

you are right about the memory leak in openCV. For me it's the first time reading a valgrind output and I want to understand it. From my understanding there are more problems then just the one in openCV. Whats about the one QRect::operator& or QRect::isValid() or qRound(double) or Digikam::DNNFaceDetectorBase::correctBbox. I guess in most examples it's just because of an uninitialized variable, but shouldn't that be fixed anyway. Can't this lead to undefined behavior in same strange cases?
Comment 35 Marcel 2020-09-05 11:53:24 UTC
(In reply to caulier.gilles from comment #33)
> MArcel,
> 
> You are right, Maik patches are not yet includes in AppImage.
> 
> I start Appimage 64 build now. Files will be published in few hours.
> 
> Gilles

Gilles,

I tested the new build. For the test I used optimized build (not debug). There it took longer for the error (around an hour) but the memory leak still exists.
Comment 36 caulier.gilles 2020-09-05 18:07:14 UTC
So the way to build AppImage with an optimized OpenCV improve the situation but do not solve it.

How the memory allocation grow with AppImage version ? less faster than the navive digiKam version ?

It can be usefull to compare the backtrace with the debug version of AppImage bundle. run it with "debug" argument to caught crash with GDB.

Other Q : which kind of image format do you parse while face detection ? Do you see a memory leak difference with JPEG, PNG, RAW, TIFF for ex ?

Gilles
Comment 37 caulier.gilles 2020-09-06 04:28:32 UTC
Marcel,

Looking your full valgrind trace do not show any memory leak, only uninitialized values.

As explain here : https://stackoverflow.com/questions/2612447/pinpointing-conditional-jump-or-move-depends-on-uninitialized-values-valgrin

This can be considerate as false positive and in all cases not the real case of the leak of memory from face detection.

How to investigate better :

1/ always use a digiKam compiled with debug symbols using valgrind.
2/ The whole memory leak trace is generally reported at end of valgrind session, when digiKam is down. Valgrind will take time to list leak sorted by size. This part of valgrind report is missing in your trace.

Gilles
Comment 38 caulier.gilles 2020-09-06 07:15:30 UTC
*** Bug 426236 has been marked as a duplicate of this bug. ***
Comment 39 Marcel 2020-09-06 07:19:46 UTC
Created attachment 131443 [details]
Memory of digikam-7.2.0-beta1-20200905T070522-x86-64-debug.appimage
Comment 40 Marcel 2020-09-06 07:20:16 UTC
Created attachment 131444 [details]
Memory of digikam-7.2.0-beta1-20200905T070220-x86-64.appimage
Comment 41 Marcel 2020-09-06 07:30:50 UTC
I just uploaded the comparison of two appimages (normal and debug). I stopped digikam searching for faces manually before running out of memory because I was not able to take a screen shot after running out of memory. My computer was not reacting any more. That is nasty. Both were taken after a restart of the computer because after the crash the swap is still in use. So I got a cleaner record

Personally I can't see any significant difference. 

Why and when should I take a backtrace?
Where can provide the debug argument? There is no such argument on appimages. 

I have the following pics in my lib:
BMP: 13
GIF: 4
JPEG: 1
JPG: 59875
PNG: 758
PSD: 2
TIFF: 1
So I guess the leak must come at least with JPG or png pictures.
Comment 42 caulier.gilles 2020-09-06 07:56:29 UTC
About the Valgrind trace, all unititialized memory entries are like this :

==11721==    at 0x56FDAA1: int const& qMin<int>(int const&, int const&) (qglobal.h:594)
==11721==    by 0x5EB28CE: Digikam::DNNFaceDetectorBase::correctBbox(cv::Rect_<int>&, cv::Size_<int> const&) const (dnnfacedetectorbase.cpp:127)
==11721==    by 0x5EAE704: Digikam::DNNFaceDetectorSSD::postprocess(cv::Mat, cv::Size_<int> const&, std::vector<cv::Rect_<int>, std::allocator<cv::Rect_<int> > >&) const (dnnfacedetectorssd.cpp:144)
==11721==    by 0x5EAE18F: Digikam::DNNFaceDetectorSSD::detectFaces(cv::Mat const&, cv::Size_<int> const&, std::vector<cv::Rect_<int>, std::allocator<cv::Rect_<int> > >&) (dnnfacedetectorssd.cpp:78)
==11721==    by 0x5EAD201: Digikam::OpenCVDNNFaceDetector::cvDetectFaces(cv::Mat const&, cv::Size_<int> const&) (opencvdnnfacedetector.cpp:274)
==11721==    by 0x5EAD09C: Digikam::OpenCVDNNFaceDetector::detectFaces(cv::Mat const&, cv::Size_<int> const&) (opencvdnnfacedetector.cpp:245)
==11721==    by 0x5E8C7D5: Digikam::FaceDetector::detectFaces(Digikam::DImg const&, QSize const&) (facedetector.cpp:289)
==11721==    by 0x4E2E481: Digikam::DetectionWorker::process(QExplicitlySharedDataPointer<Digikam::FacePipelineExtendedPackage>) (detectionworker.cpp:55)
==11721==    by 0x4E0CD4B: Digikam::DetectionWorker::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (moc_detectionworker.cpp:87)

All are relevant of a use of qMax/qMin from Qt in code using OpenCV API. I will use cv::max/cv::min instead, just to be homogeneous, but it's not the source of the real memory leak problem.

Gilles
Comment 43 caulier.gilles 2020-09-06 08:00:30 UTC
Marcel,

There is another possible test to do : use a VirtualBox instance, running the same version of Ubuntu, and check if memory leak appear.

As VirtualBox use an emulated basic video card, and as OpenCV try to use optimisations from GPU, is there is a link between video driver and the memory leak, we will see imediatly. 

Q: which real video card do you use in your computer ? 

Gilles
Comment 44 Marcel 2020-09-06 08:06:30 UTC
(In reply to caulier.gilles from comment #43)
> Marcel,
> 
> There is another possible test to do : use a VirtualBox instance, running
> the same version of Ubuntu, and check if memory leak appear.
> 
> As VirtualBox use an emulated basic video card, and as OpenCV try to use
> optimisations from GPU, is there is a link between video driver and the
> memory leak, we will see imediatly. 
> 
> Q: which real video card do you use in your computer ? 
> 
> Gilles

I'm checking that with virtualbox. I'm using a Intel Corporation HD Graphics 620 (rev 02)
Comment 45 caulier.gilles 2020-09-06 08:17:07 UTC
Marcel, from your comment #41 :

>I just uploaded the comparison of two appimages (normal and debug). I stopped >digikam searching for faces manually before running out of memory because I was >not able to take a screen shot after running out of memory. My computer was not >reacting any more. That is nasty. Both were taken after a restart of the >computer because after the crash the swap is still in use. So I got a cleaner >record

Your system do not have enough free meory to run properly. It's normal. You reach the limits.

>Personally I can't see any significant difference. 

As expected.

>Why and when should I take a backtrace?

The backtrace with GDb can be done with Appimage to pass "debug" as argument in command line when you start the AppImage bundle. This will run digiKam in GDB. When it crash, enter "bt" in GDB shell.

>Where can provide the debug argument? There is no such argument on appimages. 

In the bundle, there is a bash script to catch arguments. I written the script as AppImage SDK do not provide this kind of feature, even if i talk with AppImage lead developer, but he won't to support this as i know.

Note : just for info : the official AppImage target builder tool from the SDK is unable to compile the bundle. I reported this problem to the AppImage project, which responded that the target application is to huge to be processed. Seriouly ! AppImage has used bash scripts in the pass and all work as expected. They convert all bash script to C program, and now nothing work: the program crash. As usual, developer use this kind of excuse to not fix the real problem...

Read the story here: https://github.com/probonopd/linuxdeployqt/issues/359  

This is why all the digiKam AppImage build process run through bash scripts that i written. There is no stupid limit.

>I have the following pics in my lib:
>BMP: 13
>GIF: 4
>JPEG: 1
>JPG: 59875
>PNG: 758
>PSD: 2
>TIFF: 1
>So I guess the leak must come at least with JPG or png pictures.

JPG probably, i think. It can be also a problem with libjpeg-turbo which try to optimize also the JPEG processing using GPU. If it true, decoding/encoding JPEG images with BQM for ex can be a try to check if the problem exists. 

Gilles
Comment 46 caulier.gilles 2020-09-06 08:24:39 UTC
Git commit a98a9d0a5a8a6e69ae60ed8045d1bf6e885fc84a by Gilles Caulier.
Committed on 06/09/2020 at 08:23.
Pushed by cgilles into branch 'master'.

qMin => cv::min / qMax => cv::max

M  +18   -14   core/libs/facesengine/detection/opencv-dnn/dnnfacedetectorbase.cpp

https://invent.kde.org/graphics/digikam/commit/a98a9d0a5a8a6e69ae60ed8045d1bf6e885fc84a
Comment 47 caulier.gilles 2020-09-06 09:43:46 UTC
Marcel,

As you are developer (:=))... I can give you some pointer to AppImage build script used by digiKam.

https://invent.kde.org/graphics/digikam/-/tree/master/project/bundles/appimage

I written all of them, since many years now. So i learn in-deep core AppImage to understand all the functionalities, the limits, the capabilities, etc.

The lead README explain all the stuff. Just read an ask question. One note: the 01 script require Mageia6 system. I use this Linux box because i know very well, but another ones can be used. The package names just need to be changed.

In all case, never use Ubuntu for that. This Linux is just a pain and a waste of time (to develop). Centos, Suse, Debian, etc, are fine.

The startup up scrip used internally in the bundle to launch digiKam is this one :

https://invent.kde.org/graphics/digikam/-/blob/master/project/bundles/appimage/data/AppRun

Voilà.

Gilles
Comment 48 Marcel 2020-09-06 11:15:40 UTC
Created attachment 131445 [details]
Seg fault in virtual Box running Kubuntu

I started digikam in a virtualBox running Kubuntu. It was hard to start digikam with a total of 8 GB of RAM. Digikam run not before a total of 6 GB of my memory was given. 

So first the good news. During the scan (approximately 1 minutes). The memory did not noticeable increase. Digikam crashed with the seg fault in segfault_virtualBox.txt. I hope I can give with this valuable information. 


How can I switch my JPEG decoding/encoding to BQM that is not using my GPU?
Comment 49 caulier.gilles 2020-09-06 11:46:22 UTC
>I started digikam in a virtualBox running Kubuntu. It was hard to start digikam >with a total of 8 GB of RAM. Digikam run not before a total of 6 GB of my memory >was given. 

6Gb at minimum to start ? Impossible. I run digiKam with 3Gb computer (laptop), and it work. It's definitively a Ubuntu problem, i'm sure...

This is why Ubuntu is a weird, dirty, wrong system at all...

>So first the good news. During the scan (approximately 1 minutes). The memory >did not noticeable increase. Digikam crashed with the seg fault in >segfault_virtualBox.txt. I hope I can give with this valuable information. 

Well, 6Gb or ram is definitively abnormal. Don't forget that we compile digiKam for 32 bits target, where only 4Gb is available, and it work...

>How can I switch my JPEG decoding/encoding to BQM that is not using my GPU?

You cannot. libjpeg-turbo is compiled with optimization as well.

Just create a BQM queue, fill with few JPEG, in the workflow convert JPEG to another JPEG, and look the memory allocation.

Another Q: try showfoto and look if memory allocation is also huge. AppImage provide also showfoto. Just pass "showfoto" as argument to the command line.

With digiKam there is also a possibility that database interface leak memory while operating. This will operate at startup and while scanning faces. In both case, database commits are done. As Showfoto do not use a database, this will be an indicator.

Gilles
Comment 50 caulier.gilles 2020-09-06 12:07:10 UTC
Marcel,

I installed a Kubuntu 20.04 under VirtualBox with 8Gb of Ram. This is the memory allocation while startup with Appimage 7.2.0-beta1. There is just a single SQlite managed collection with no images. So scan do nothing.

https://i.imgur.com/V0iocMJ.png

Gilles
Comment 51 caulier.gilles 2020-09-06 12:11:56 UTC
Marcel,

I forget to said that memory allocation from comment #50 and done under Kubuntu is measured with AppImage bundle including debug symbols. So binary objects in memory are really more important than without debug symbols, of course.

Gilles Caulier
Comment 52 caulier.gilles 2020-09-06 12:16:53 UTC
Arf. It's clear. The problem is not located in face scan under Kubuntu.

It's a memory leak done by the system, because it do not release memory at all. When i close my digiKam session, ksysguard still showing the memory allocated as well. If i restart to another digiKam session, the system continue to allocate over and other and it finish to dead, OF COURSE.

Definitively, Ubuntu done a worse memory management in the background. How a system like can exists, seriously !

Gilles Caulier
Comment 53 caulier.gilles 2020-09-06 12:21:57 UTC
I tried to run Showfoto from the AppImage. It start quickly and memory allocation is ridiculous, and resumed well at end of Showfoto session.

So this problem of memory leak is :

1/ not relevant at all of faces scanning.
2/ not releavant of plugins loaded in memory. Showfoto use same plugins list (excepted ones from BQM), and there is no side effect in memory.
3/ probably relevant of database init and use as it's the main difference with Showfoto.

Gilles
Comment 54 caulier.gilles 2020-09-06 12:43:06 UTC
Forget my last comment #53. 


Wth AppImage 7.2.0-beta1, I compared the memory allocation between Kubuntu and Mageia7, and the behaviour is exactly the same : 4 Gb used by digiKam at startup.

With or without debug symbols, it's the same (here i don't understand why).

And at end of digiKam session, both system the memory is free. ouf...

So yes, the memory leak is well located in Face detection...

Note : i disabled all plugin (generic and editor), and restarted digiKam. This is have no effect on memory allocation : always 4 Gb when digiKam has completed the startup stage.

Gilles
Comment 55 caulier.gilles 2020-09-06 13:40:36 UTC
Marcel,

Kubuntu 20.04 installed and ready to compile digiKam from git/master, of course in a VM. Kubuntu package is really a puzzle and lots of inter-dependencies are not resolved automatically or set by default. It's really a pain...

Q: did you enable the 3D acceleration in VM video settings ?

Gilles
Comment 56 Marcel 2020-09-06 14:40:16 UTC
(In reply to caulier.gilles from comment #55)
> Marcel,
> 
> Kubuntu 20.04 installed and ready to compile digiKam from git/master, of
> course in a VM. Kubuntu package is really a puzzle and lots of
> inter-dependencies are not resolved automatically or set by default. It's
> really a pain...
> 
> Q: did you enable the 3D acceleration in VM video settings ?
> 
> Gilles

Of course as we are looking for a memory leaks I tried to disable all hw accelerations. Glade you experience the same as I do with the memory demand. So does Digikam still run on your 3GB RAM Machine? If you want I have an ansible playbook to install all dependencies for compiling digikam (or just read it from there)
Comment 57 caulier.gilles 2020-09-06 14:51:51 UTC
digiKam is compiled and run on my Kubuntu VM.

I playing with valgrind.

3D acceleration is disabled.

I imported 20 Gb of family photo. I will start a scan faces and see.

Note : typically, digiKam must start on a 32 bits system so with less than 4Gb or RAM. I suspect that huge memory allocation at startup is due the problem listed on file #426185.

Gilles
Comment 58 caulier.gilles 2020-09-06 15:57:06 UTC
Marcel, Maik,

The native compiled version of digiKam from git/master running on my Kubuntu 20.04 VM run since more than 1 hour in valgrind. it scan for new items. After the startup stage and a memory allocated up to 6.4 Gb (all the system). It's stable since a while. Scanning for new items do not leak memory.

All digiKAm settings are all defaults. Database is a sqlite one hosted in ~/Pictures directory.

I uploaded 20 Gb or photos from my lead computer for testing, all containing more and less 1 face.

digikam version 7.2.0-beta1
Images: 
JPG: 1108
PGF: 1
PNG: 16
RAW-ARW: 143
RAW-RAF: 9
total: 1277
: 
Videos: 
MOV: 1
total: 1
: 
Total Items: 1278
Albums: 36
Tags: 37
: 
Database backend: QSQLITE
Database Path: /home/gilles/Pictures/

!thz sc
Comment 59 caulier.gilles 2020-09-06 15:58:04 UTC
MArcel, Maik,

The scan for new items is now complete. I will close the valgrind session to see if memory leak are reported by the analyzer.

Gilles Caulier
Comment 60 Maik Qualmann 2020-09-06 16:00:00 UTC
The cause of the high memory consumption at the beginning in digiKam-7.2.0 is bug 426185. We load the file "/usr/share/digikam/facesengine/openface_nn4.small2.v1.t7" into memory 40 times.

Maik
Comment 61 caulier.gilles 2020-09-06 16:05:47 UTC
Created attachment 131448 [details]
valgrind session Kubuntu 20.04 7.2.0-beta1 scan new items
Comment 62 caulier.gilles 2020-09-06 16:12:23 UTC
Maik,

yes, this what i have suspected.

I start now the valgrind session with face scan... Please wait.

Gilles
Comment 63 Minh Nghia Duong 2020-09-06 16:21:24 UTC
(In reply to Maik Qualmann from comment #60)
> The cause of the high memory consumption at the beginning in digiKam-7.2.0
> is bug 426185. We load the file
> "/usr/share/digikam/facesengine/openface_nn4.small2.v1.t7" into memory 40
> times.
> 
> Maik

Hi Maik,

I find that we create 3 different FacialRecognitionWrapper instances in FaceManement. Can I change this class into singleton to assure the coherence of faces engine and avoid memory consumtion?

Nghia
Comment 64 Maik Qualmann 2020-09-06 16:32:38 UTC
Created attachment 131449 [details]
heaptrack.jpg

It even has to be FacialRecognitionWrapper 4 times, because I see the 10 block of the debug output 4 times. Here is an analysis from the start with Heaptrack.

Maik
Comment 65 Minh Nghia Duong 2020-09-06 16:45:59 UTC
Yes, it has to be 4 times but I found only 3 instances in FacesManagement. But either way, the solution to this problem can be making this class a singleton since it is nothing but an interface to the recognition of faces engine. 

Nghia
Comment 66 Maik Qualmann 2020-09-06 17:19:13 UTC
The main problem is the "shapepredictor.dat", this file is also loaded 40 times into the memory.

Maik
Comment 67 caulier.gilles 2020-09-06 17:28:53 UTC
Maik, 

Nghia proposal from coment #63 is the right to go i think.

At least if data model files must be loaded and shared in memory by read only, a singleton with public accessors can be optimum the solution.

Gilles
Comment 68 caulier.gilles 2020-09-06 17:41:01 UTC
Created attachment 131452 [details]
valgrind session Kubuntu 20.04 7.2.0-beta1 scan faces

This valgrind trace start when scan face process is launch an stop at end of digiKam session. The huge memory leak is not really visible here.
Comment 69 Minh Nghia Duong 2020-09-06 17:43:07 UTC
(In reply to Maik Qualmann from comment #66)
> The main problem is the "shapepredictor.dat", this file is also loaded 40
> times into the memory.
> 
> Maik

shapepredictor.dat is loaded with FaceExtractor. Therefore, when we avoid the duplicate of this object, we can reduce the memory consumption. 

For now, I used 10 FaceExtractor objects to x3 speed of facial recognition. If you like I can reverse it back to use only 1 object.

Nghia
Comment 70 caulier.gilles 2020-09-06 17:54:26 UTC
This is a desktop video capture under Kubuntu 20.04 VM session running compiled digiKam 7.2.0-beta1 while face detection inside valgrind. Look well the memory allocation with ksysguard. Yes, we allocate memory and clear step by step. There is not cumulative memory leak visible.

Of course, using valgrind, this decrease running speed by 20 but the behaviour is similar than under Mageia7 tested yesterday.

Nghia, there is no active clustering while faces detection. Right ?
If yes, well it's coherent to see no memory allocated to group similar faces before recognition processing.

If the Marcel dysfunction exists, how to reproduce exactly ? Outside the fact of cumulative model data files loaded at startup that will reduce the fingerprint at when it will be fixed, i cannot see a memory dysfunction at face scan.

Gilles
Comment 71 caulier.gilles 2020-09-06 17:54:59 UTC
I miss the video link (:=)))

https://youtu.be/iK3Ueh1TAng
Comment 72 Minh Nghia Duong 2020-09-06 17:58:31 UTC
(In reply to caulier.gilles from comment #70)
> This is a desktop video capture under Kubuntu 20.04 VM session running
> compiled digiKam 7.2.0-beta1 while face detection inside valgrind. Look well
> the memory allocation with ksysguard. Yes, we allocate memory and clear step
> by step. There is not cumulative memory leak visible.
> 
> Of course, using valgrind, this decrease running speed by 20 but the
> behaviour is similar than under Mageia7 tested yesterday.
> 
> Nghia, there is no active clustering while faces detection. Right ?
> If yes, well it's coherent to see no memory allocated to group similar faces
> before recognition processing.
> 
> If the Marcel dysfunction exists, how to reproduce exactly ? Outside the
> fact of cumulative model data files loaded at startup that will reduce the
> fingerprint at when it will be fixed, i cannot see a memory dysfunction at
> face scan.
> 
> Gilles

They aren't active but the instances are pre-created along with Workers of FaceManagement.
Comment 73 caulier.gilles 2020-09-06 18:07:45 UTC
https://youtu.be/87gFl0VJVRA

desktop video recording running digiKam 7.2.0-beta1 without valgrind this time and using single core to faces scan. There is no memory leak at all.

Gilles
Comment 74 caulier.gilles 2020-09-06 18:14:54 UTC
https://youtu.be/zaiWDqSIyUQ

desktop video recording running digiKam 7.2.0-beta1 without valgrind and and using multi-core to faces scan. There is no memory leak at all.

Gilles
Comment 75 caulier.gilles 2020-09-06 18:28:26 UTC
https://youtu.be/rR0V_CrIRsQ

desktop video recording running digiKam 7.2.0-beta1 to faces scan:

+ without valgrind
+ using multi-core
+ 3D acceleration from Virtual Box

This time a small memory leak is visible. This will not crash the system as it increase very slowly, and i don't have enough items to scan to overload memory.

In all cases, OpenCV + GPU stuff introduce a dysfunction.

Note that OpenCV come from the system I don't recommpile myself the library with usual optimization include in AppImage bundle.

Other point : It's an emultated 3D acceleration. What's about a real 3D card + proprietary driver, as NVidia or AMD stuff...

Gilles

Gilles
Comment 76 caulier.gilles 2020-09-06 20:20:34 UTC
After all, I found this entry :

https://github.com/opencv/opencv/issues/17154

Gilles
Comment 77 caulier.gilles 2020-09-06 20:22:17 UTC
And another one :

https://github.com/opencv/opencv/issues/10101

Gilles
Comment 78 caulier.gilles 2020-09-06 21:06:31 UTC
OpenCV provide a solution to disable 3D aceleration by API call:

https://answers.opencv.org/question/123097/how-to-stop-using-gpu-in-opencv-32/?comment=123114

So typically, for each thread using OpenCV API, we need to call cv::ocl::setUseOpenCL(false)

But it's problematic to use :

https://github.com/opencv/opencv/issues/8035

In all case, we must not use cv::Umat which is optimized for OpenCV and use cv::Mat instead everywhere.

Gilles
Comment 79 Marcel 2020-09-07 05:32:46 UTC
So, I think it's time to summarize all findings:

1. Disable loading notification
This solved the original problem

2. Huge memory demand at startup
For this problem is another bug (https://bugs.kde.org/show_bug.cgi?id=426185). Some memory can be reduced anyway bug some is used to gain a speed advantage in digikam. The Question is what is more important. Speed or memory consumption. 
Personally, this memory for speed gain should definitely not be allocated at startup time as most digikam usages will not scan for faces each time they use digikam. It should be allocated at the start of face detection.

3. Memory leak on face detection
There is a bug in openCV  with the use of GPU causing that leak. More memory leaks on bigger picture collections. To solve that problem cv::Mat can be used instead of cv::UMat, but again that will decrease the scanning speed. Probably a checkbox "use GPU (experimental)" could help.
Comment 80 Minh Nghia Duong 2020-09-07 07:11:47 UTC
I implement singleton on FacialRecognitionWrapper so that it initiated only once. I also reduce the number of FaceExtractor to 1, which is the same as the previous version of faces engine. However, this error still occurs, and I notice that it only occurs when the Unknown face tab is opened and the cropped faces are rendered.
Comment 81 caulier.gilles 2020-09-07 09:21:32 UTC
To Marcel from Comment #72:

By chance, we do not use c::Umat in digiKam opencv source code. Only cv:Mat is used. So the memory leak can be only located internally in OpenCV libraries.

Gilles
Comment 82 caulier.gilles 2020-09-07 09:45:53 UTC
Hi all,

Reading the OpenCV document, that i try to understand, we have this envirronement variable which can be changed at run time:

OPENCV_OPENCL_DEVICE

... as explained here:

https://docs.opencv.org/2.4/modules/ocl/doc/introduction.html

... and if i'm not too wrong, if i set 

export OPENCV_OPENCL_DEVICE=CPU

this will only use CPU and never GPU optimization internally with OpenCV.

I'm right ?

Gilles
Comment 83 Minh Nghia Duong 2020-09-07 09:49:12 UTC
(In reply to caulier.gilles from comment #82)
> Hi all,
> 
> Reading the OpenCV document, that i try to understand, we have this
> envirronement variable which can be changed at run time:
> 
> OPENCV_OPENCL_DEVICE
> 
> ... as explained here:
> 
> https://docs.opencv.org/2.4/modules/ocl/doc/introduction.html
> 
> ... and if i'm not too wrong, if i set 
> 
> export OPENCV_OPENCL_DEVICE=CPU
> 
> this will only use CPU and never GPU optimization internally with OpenCV.
> 
> I'm right ?
> 
> Gilles

In my knowledge, currently, OpenCV DNN is using CPU as default. For GPU, there is only NVIDIA CUDA is supported and it's required further configuration. So for now in digikam faces engine, all has been processed on CPU.
Comment 84 caulier.gilles 2020-09-07 09:53:40 UTC
I respond to myself:

This is the right syntax: export OPENCV_OPENCL_DEVICE=:CPU:

See comment from this OpenCV issue:

https://github.com/opencv/opencv/issues/8275

Gilles
Comment 85 Maik Qualmann 2020-09-07 09:53:52 UTC
Look in main.cpp, we already have a system setting option for it and is set by default. Further environment variables can be added.

Maik
Comment 86 caulier.gilles 2020-09-07 09:58:31 UTC
Nghia,

well, this is not what's a read. depending of OpenCV version, especially the 4.x, opencv try to use GPU optimizations everywhere if possible. 

Perhaps DNN code as well is not optimized, but i'm sure that DNN code call internal OpenCV API which are optimized, for ex about matrix management.

My test under Kubuntu VM customized with and without 3D acceleration setting demonstrate the dysfunction about memory allocation is lost at run time.

Gilles
Comment 87 caulier.gilles 2020-09-07 10:03:21 UTC
Right Maik,

I forget this part.

https://invent.kde.org/graphics/digikam/-/blob/master/core/app/main/main.cpp#L123

... but the export is set to:

OPENCV_OPENCL_DEVICE=null

instead of:

OPENCV_OPENCL_DEVICE=:CPU:

...as explained in the OpenCV documentation to disable GPU usage.

I'm wrong ?

Gilles
Comment 88 Minh Nghia Duong 2020-09-07 10:04:36 UTC
(In reply to caulier.gilles from comment #86)
> Nghia,
> 
> well, this is not what's a read. depending of OpenCV version, especially the
> 4.x, opencv try to use GPU optimizations everywhere if possible. 
> 
> Perhaps DNN code as well is not optimized, but i'm sure that DNN code call
> internal OpenCV API which are optimized, for ex about matrix management.
> 
> My test under Kubuntu VM customized with and without 3D acceleration setting
> demonstrate the dysfunction about memory allocation is lost at run time.
> 
> Gilles

OpenCV provides GPU optimization for matrix operations but I'm sure that we can actually choose the processing unit for OpenCV::dnn::net via their API. And currently, the only GPU driver supported for OpenCV's neural network is NVIDIA CUDA. By default, all the internal backend of OpenCV::dnn::net are handled by CPU
Comment 89 Maik Qualmann 2020-09-07 11:04:33 UTC
Gilles, that's a good question. I hadn't read ":CPU:" anywhere so far, rather "null". At the end of the thread someone writes that he was successful with "disable"...OpenCV is a puzzle...

Maik
Comment 90 caulier.gilles 2020-09-07 11:40:09 UTC
I will said more: it's a big puzzle...

To be sure, this evening is will try again with the VM Kubuntu, to set the environment variable to see if there is a difference with the memory allocation.

Gilles
Comment 91 caulier.gilles 2020-09-07 11:43:55 UTC
Nghia,

It still a pending Q in my mind. Currently, all digiKAm bundle still compiled with OpenCV LTS version 3.4.11.

There is an advantage to switch definitively to OpenCV 4.x ?

Gilles
Comment 92 Minh Nghia Duong 2020-09-07 16:35:29 UTC
(In reply to caulier.gilles from comment #91)
> Nghia,
> 
> It still a pending Q in my mind. Currently, all digiKAm bundle still
> compiled with OpenCV LTS version 3.4.11.
> 
> There is an advantage to switch definitively to OpenCV 4.x ?
> 
> Gilles

I doubt that. I saw that OpenCV 4.x has added recent trending models, but most of the current code base is nearly the same. I already tried it out when I tried YOLOv4 on digiKam since YOLOv4 required OpenCV 4.x. The performance is not much different, in my opinion.
Comment 93 Maik Qualmann 2020-09-07 18:03:51 UTC
Git commit 5102e32b4e9e308df760a45624832caa204d82c0 by Maik Qualmann.
Committed on 07/09/2020 at 18:02.
Pushed by mqualmann into branch 'master'.

enable loading notification again and change stop function

M  +1    -11   core/libs/threadimageio/fileio/loadingcache.cpp
M  +16   -19   core/libs/threadimageio/fileio/loadsavetask.cpp
M  +2    -2    core/libs/threadimageio/preview/previewtask.cpp
M  +2    -2    core/libs/threadimageio/thumb/thumbnailtask.cpp

https://invent.kde.org/graphics/digikam/commit/5102e32b4e9e308df760a45624832caa204d82c0
Comment 94 caulier.gilles 2020-09-08 02:54:05 UTC
To Maik comment #89:

Reading OpenCV source code give the solution:

https://github.com/opencv/opencv/blob/master/modules/core/src/ocl.cpp#L2068

OPENCV_OPENCL_DEVICE=null has no effect.

OPENCV_OPENCL_DEVICE=disable must work.

OPENCV_OPENCL_DEVICE=:CPU: must work (see the parser https://github.com/opencv/opencv/blob/master/modules/core/src/ocl.cpp#L2034)

Gilles
Comment 95 caulier.gilles 2020-09-08 03:08:52 UTC
Maik, 

Setting the 2nd environment variable OPENCV_OPENCL_RUNTIME=disabled is fine:

https://github.com/opencv/opencv/blob/master/modules/core/src/ocl.cpp#L1151

For this one it's really "disabled" where the other one is "disable". Take a care to the typo with ending by 'd' or not. This is clearly an error prone syntax...

Gilles
Comment 96 caulier.gilles 2020-09-08 03:14:09 UTC
Git commit b5e3c5987a7380fdc5ce9a59ee5e62e6acbaf51b by Gilles Caulier.
Committed on 08/09/2020 at 03:11.
Pushed by cgilles into branch 'master'.

Fix typo with OpenCV environnement variable value used to disable OpenCL device for 3D accelerations
Related: bug 423632

M  +2    -1    core/app/main/main.cpp

https://invent.kde.org/graphics/digikam/commit/b5e3c5987a7380fdc5ce9a59ee5e62e6acbaf51b
Comment 97 caulier.gilles 2020-09-08 03:46:36 UTC
Marcel,

See my comment:

https://bugs.kde.org/show_bug.cgi?id=426185#c4

The huge memory allocation at startup is now fixed by Nghia commit: 

https://invent.kde.org/graphics/digikam/-/commit/28027b21d9fa4368eb2f2ad26dbb1cb33e274cce

It still to check if the OpenCL memory leak from OpenCV is fixed with my commit:

https://invent.kde.org/graphics/digikam/commit/b5e3c5987a7380fdc5ce9a59ee5e62e6acbaf51b

And if the another memory glitch is fixed by Maik commit:

https://invent.kde.org/graphics/digikam/commit/5102e32b4e9e308df760a45624832caa204d82c0

I will rebuild AppImage 64 bits this morning.

Gilles
Comment 98 Minh Nghia Duong 2020-09-08 04:00:49 UTC
(In reply to caulier.gilles from comment #97)
> Marcel,
> 
> See my comment:
> 
> https://bugs.kde.org/show_bug.cgi?id=426185#c4
> 
> The huge memory allocation at startup is now fixed by Nghia commit: 
> 
> https://invent.kde.org/graphics/digikam/-/commit/
> 28027b21d9fa4368eb2f2ad26dbb1cb33e274cce
> 
> It still to check if the OpenCL memory leak from OpenCV is fixed with my
> commit:
> 
> https://invent.kde.org/graphics/digikam/commit/
> b5e3c5987a7380fdc5ce9a59ee5e62e6acbaf51b
> 
> And if the another memory glitch is fixed by Maik commit:
> 
> https://invent.kde.org/graphics/digikam/commit/
> 5102e32b4e9e308df760a45624832caa204d82c0
> 
> I will rebuild AppImage 64 bits this morning.
> 
> Gilles

Hi Gilles,

Even with this version, the crash during face detection still occurs on my machine

Nghia
Comment 99 Maik Qualmann 2020-09-08 05:46:14 UTC
@Nghia, do you have a backtrace?

Maik
Comment 100 caulier.gilles 2020-09-08 07:25:57 UTC
Git commit f813d64af40c5ea342ab9da340f12acc4b19023d by Gilles Caulier.
Committed on 08/09/2020 at 07:23.
Pushed by cgilles into branch 'master'.

I needs new glasses : disable => disabled
Related: bug 423632

M  +1    -1    core/app/main/main.cpp

https://invent.kde.org/graphics/digikam/commit/f813d64af40c5ea342ab9da340f12acc4b19023d
Comment 101 Maik Qualmann 2020-09-08 10:40:26 UTC
Git commit cde3403938b82e0cfcccb1b557b6c2319ac8557e by Maik Qualmann.
Committed on 08/09/2020 at 10:37.
Pushed by mqualmann into branch 'master'.

add base classes initialization explicit in the constructor
Related: bug 423632, bug 425723

M  +2    -0    core/libs/threadimageio/fileio/loadsavetask.cpp
M  +2    -0    core/libs/threadimageio/fileio/loadsavetask.h

https://invent.kde.org/graphics/digikam/commit/cde3403938b82e0cfcccb1b557b6c2319ac8557e
Comment 102 Maik Qualmann 2020-09-08 20:06:14 UTC
Git commit a06b3c5dcc32a2f95c5fbf0ac2fa898931524cea by Maik Qualmann.
Committed on 08/09/2020 at 20:05.
Pushed by mqualmann into branch 'master'.

add static cast for loading notifikation
Related: bug 423632, bug 425723

M  +2    -1    core/libs/threadimageio/fileio/loadingcache.cpp

https://invent.kde.org/graphics/digikam/commit/a06b3c5dcc32a2f95c5fbf0ac2fa898931524cea
Comment 103 Minh Nghia Duong 2020-09-08 21:25:33 UTC
Created attachment 131496 [details]
BackTrace after fixing FaceExtractor memory consumtion

Hi Maik,

Here the backtrace. It's still the same error.

Nghia
Comment 104 Maik Qualmann 2020-09-09 04:29:42 UTC
This backreace is not from the current head of master. Please try the current master version.

Maik
Comment 105 Minh Nghia Duong 2020-09-09 05:03:42 UTC
(In reply to Maik Qualmann from comment #104)
> This backreace is not from the current head of master. Please try the
> current master version.
> 
> Maik

This is the new back trace : 

Thread 86 "Thread (pooled)" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff0bfff700 (LWP 16177)]
0x00007ffff7077a32 in Digikam::PreviewLoadingTask::execute (this=0x55558f9f3240) at /home/minhnghiaduong/Documents/Projects/digikam/core/libs/threadimageio/preview/previewtask.cpp:443
443             m_thread->taskHasFinished();
(gdb) bt
#0  0x00007ffff7077a32 in Digikam::PreviewLoadingTask::execute() (this=0x55558f9f3240) at /home/minhnghiaduong/Documents/Projects/digikam/core/libs/threadimageio/preview/previewtask.cpp:443
#1  0x00007ffff708a0f5 in Digikam::LoadSaveThread::run() (this=0x5555606f79a0) at /home/minhnghiaduong/Documents/Projects/digikam/core/libs/threadimageio/fileio/loadsavethread.cpp:134
#2  0x00007ffff70b2fe8 in Digikam::DynamicThread::Private::run() (this=0x55557aff5ba0) at /home/minhnghiaduong/Documents/Projects/digikam/core/libs/threads/dynamicthread.cpp:191
#3  0x00007ffff3eaf2b2 in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#4  0x00007ffff3eb217d in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007ffff2cab6db in start_thread (arg=0x7fff0bfff700) at pthread_create.c:463
#6  0x00007ffff3595a3f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Nghia
Comment 106 caulier.gilles 2020-09-09 05:40:34 UTC
Maik, Nghia,

The crash under previewtask due to a call of pure virtual method from parent class can by due to a call of pure method in constructor. This is the only case that i know. It explained here:

https://stackoverflow.com/questions/99552/where-do-pure-virtual-function-call-crashes-come-from

I don't check preview code to see if something like this is code somewhere...

Gilles
Comment 107 caulier.gilles 2020-09-09 05:44:47 UTC
Another point is the time life of the object where gcc call pure virtual method at run time. It's a race condition in destructor. It's explained here :

https://tombarta.wordpress.com/2008/07/10/gcc-pure-virtual-method-called/

Gilles
Comment 108 caulier.gilles 2020-09-09 06:01:52 UTC
Hi all,

I just tested again this morning under Kubuntu if memory leak while scanning faces is fixed, and i can confirm. Look the desktop video :

https://youtu.be/VtRgENoJeNA

Here i re-scan and merge results in database, as all image was previously scanned from collection. I set KSysguard to only show the application memory allocation from user space with a large graph. You cannot see a cumulative memory hole.

Gilles
Comment 109 caulier.gilles 2020-09-09 06:02:45 UTC
Maik,

Why under a VM running Kubuntu, i cannot reproduce the crash from preview task?

Gilles
Comment 110 Maik Qualmann 2020-09-09 06:09:01 UTC
Yes, pure virtual functions are not allowed to called in the constructor because they have not yet been created, but that is not the case here in the loader task. The other thing about the deconstructor is interesting. But I suspect that a deleted task remains in the QMap due to a race condition...due to non-recursive mutex...

Maik
Comment 111 Maik Qualmann 2020-09-09 06:12:04 UTC
Yes, that is the question, why can some people reproduce it after just a few seconds and we cannot reproduce it at all. Assumption: Hardware dependent, processor / cache / memory / mainboard? Or the host Linux system?

Maik
Comment 112 caulier.gilles 2020-09-09 07:10:24 UTC
I have a theory: Look my desktop video. There is no tab open in right sidebar.

Perhaps crash from users is due to an extra operation done in right sidebar while processing face detection. Look well the icon view. It's constantly refresh when a new face is found, if you take the focus on the unknown virtual album. This want mean the first item in icon view is never the same when new items are appended. So, the right sidebar is updated accordingly, and dependeing of the active tab, processing in background is started (for ex, in colors view, the image is loaded to compute histogram.

This is an example. We can imagine the end users opening image editor to start to fix images while long faces detection, or to compute panorama, or to export images on web services. 

This can have side effects.

In my case, when i test faces management, i lets the job alone. It's long, i will do a dog walk and go back later to check...

Gilles
Comment 113 Marcel 2020-09-09 10:45:54 UTC
Just thinking loud...
I don't think the theorie of gilles is right that there are other tasks runnung. Of course this may lead the other problems but not to the one we are looking for. When I look to the video of the scan I think the main difference is the precondition. I have a library with tousends of taged people. Some with a real tag others just in the db as unknown So in my case the new engine must do a huge merge process. Probably there is the error we are looking for.
Comment 114 caulier.gilles 2020-09-09 10:53:04 UTC
Marcel,

The Preview Task code where Maik try to fix the crash since a very long time is not relevant of the database access and the merge operation between tags. And we talk about face detection, not face recognition, so the face area identification and ready to use for face tagging (manual or automatic). 

The Preview Task is only used to get the image contents and pass it to the face engine for analysis. The result is a list or area where faces are located and stored in main database.

So my theory still valid, but you are right about face recognition and possible side effects with the rest of the program. It's another point to check later.

Gilles
Comment 115 Maik Qualmann 2020-09-09 11:05:31 UTC
And my theory is that we have a mutex problem. I have a new idea and I will implement it tonight.

Maik
Comment 116 Maik Qualmann 2020-09-09 19:57:54 UTC
Git commit 901227fa96db807e02b71a84c933d34b97ce3ec3 by Maik Qualmann.
Committed on 09/09/2020 at 19:56.
Pushed by mqualmann into branch 'master'.

changes to setStatus() function
Related: bug 423632, bug 425723

M  +1    -1    core/libs/threadimageio/fileio/loadingcache.cpp
M  +19   -12   core/libs/threadimageio/fileio/loadsavetask.cpp
M  +1    -2    core/libs/threadimageio/thumb/thumbnailtask.cpp

https://invent.kde.org/graphics/digikam/commit/901227fa96db807e02b71a84c933d34b97ce3ec3
Comment 117 caulier.gilles 2020-09-12 09:44:42 UTC
MAik,

i append a rules to cppcheck this morning to pass all includes pathes from digiKam/core while analysis. This add more than 700 entries in the reports. 

https://www.digikam.org/reports/cppcheck/master/

A lots of entries can be dropped as it's relevant of code that we don't maintain (dng sdk, libraw, libpgf, etc.). I will try to fix the analysis to remove these entries automatically.

I don't yet check all entries, but perhaps you will found interesting items, as  virtual calls in constructors, null pointer deference, missing override, missing explicit constructor, etc.

Gilles
Comment 118 Marcel 2020-09-19 09:04:48 UTC
Hi all. I'm back from holiday. 

I tried the process again with the latest version of Digikam (aa4f7ce6522c1c9559619c2dd3af5207a8a451ac).

About the huge memory demand:
Great job, this is solved.

About the memory leak:
Hard to say, I think it still leaks a bit (not as much as before). Anyway, for the moment I have enough memory that it does not crash at least because of this leak.

About the seg fault:
Still the same. Sometimes after a few minutes and sometimes after an hour or more. Here is the most resent backtrace:
Thread 54 "Thread (pooled)" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff81ffb700 (LWP 22073)]
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00007ffff6af3896 in Digikam::PreviewLoadingTask::execute (this=0x555560845610)
    at /home/marcel/Repos/digikam/core/libs/threadimageio/preview/previewtask.cpp:362
#2  0x00007ffff6b06f6e in Digikam::LoadSaveThread::run (this=0x7fffd400bf80)
    at /home/marcel/Repos/digikam/core/libs/threadimageio/fileio/loadsavethread.cpp:134
#3  0x00007ffff6b305fa in Digikam::DynamicThread::Private::run (this=0x555557b3b590)
    at /home/marcel/Repos/digikam/core/libs/threads/dynamicthread.cpp:191
#4  0x00007ffff5301f82 in ?? () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007ffff52fe9d2 in ?? () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007ffff37f8609 in start_thread (arg=<optimized out>) at pthread_create.c:477
#7  0x00007ffff4f72103 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb) 

Is there anything else I could test that helps finding this issue?
Comment 119 caulier.gilles 2020-09-19 10:29:19 UTC
Git commit 2601c0a8adbd51a3828353e24f5bc169260eb850 by Gilles Caulier.
Committed on 19/09/2020 at 10:27.
Pushed by cgilles into branch 'master'.

Use heap to create DRawInfo instance to prevent stack overflow
Related: bug 426597

M  +3    -3    core/libs/threadimageio/preview/previewtask.cpp

https://invent.kde.org/graphics/digikam/commit/2601c0a8adbd51a3828353e24f5bc169260eb850
Comment 120 caulier.gilles 2020-09-19 10:50:22 UTC
Git commit 2ed30b67282e725671e65f5499c6293dc0de7070 by Gilles Caulier.
Committed on 19/09/2020 at 10:50.
Pushed by cgilles into branch 'master'.

Use heap to create DRawInfo instance to prevent stack overflow
Related: bug 426597

M  +20   -18   core/dplugins/generic/tools/htmlgallery/generator/galleryelementfunctor.cpp

https://invent.kde.org/graphics/digikam/commit/2ed30b67282e725671e65f5499c6293dc0de7070
Comment 121 caulier.gilles 2020-09-19 10:52:41 UTC
Git commit edf82fbaf85e4131b3e39e4f787e9820a1e273ee by Gilles Caulier.
Committed on 19/09/2020 at 10:52.
Pushed by cgilles into branch 'master'.

Use heap to create DRawInfo instance to prevent stack overflow
Related: bug 426597

M  +5    -5    core/dplugins/dimg/raw/dimgrawloader.cpp

https://invent.kde.org/graphics/digikam/commit/edf82fbaf85e4131b3e39e4f787e9820a1e273ee
Comment 122 caulier.gilles 2020-09-19 10:54:52 UTC
Git commit f7521ff595caaee5a85fdd75354ac78a753e39dd by Gilles Caulier.
Committed on 19/09/2020 at 10:54.
Pushed by cgilles into branch 'master'.

Use heap to create DRawInfo instance to prevent stack overflow
Related: bug 426597

M  +80   -80   core/libs/metadataengine/dmetadata/dmetadata_libraw.cpp

https://invent.kde.org/graphics/digikam/commit/f7521ff595caaee5a85fdd75354ac78a753e39dd
Comment 123 caulier.gilles 2020-09-19 10:59:01 UTC
Git commit e662dbc0caea17d673cd84966655fe29fcc50828 by Gilles Caulier.
Committed on 19/09/2020 at 10:58.
Pushed by cgilles into branch 'master'.

Use heap to create DRawInfo instance to prevent stack overflow
Related: bug 426597

M  +1    -1    core/libs/rawengine/drawdecoder.cpp
M  +12   -8    core/libs/rawengine/drawdecoder_p.cpp

https://invent.kde.org/graphics/digikam/commit/e662dbc0caea17d673cd84966655fe29fcc50828
Comment 124 caulier.gilles 2020-09-19 11:18:48 UTC
Git commit 605e6875ca4386291a0b681ec26db9db0b305757 by Gilles Caulier.
Committed on 19/09/2020 at 11:18.
Pushed by cgilles into branch 'master'.

Use heap to create DRawInfo instance to prevent stack overflow
Related: bug 426597

M  +99   -99   core/libs/dngwriter/dngwriter_convert.cpp
M  +2    -0    core/libs/dngwriter/dngwriter_p.h

https://invent.kde.org/graphics/digikam/commit/605e6875ca4386291a0b681ec26db9db0b305757
Comment 125 caulier.gilles 2020-09-19 11:26:03 UTC
Git commit 96bc53a0682968ad3df03c698bf3e9bacb3a665d by Gilles Caulier.
Committed on 19/09/2020 at 11:25.
Pushed by cgilles into branch 'master'.

Use heap to create DRawInfo instance to prevent stack overflow
Related: bug 426597

M  +2    -1    core/dplugins/dimg/raw/dimgrawloader.cpp
M  +4    -2    core/dplugins/generic/tools/htmlgallery/generator/galleryelementfunctor.cpp
M  +2    -1    core/libs/metadataengine/dmetadata/dmetadata_libraw.cpp
M  +2    -1    core/libs/rawengine/drawdecoder_p.cpp
M  +2    -1    core/libs/threadimageio/preview/previewtask.cpp
M  +16   -13   core/tests/rawengine/raw2png.cpp

https://invent.kde.org/graphics/digikam/commit/96bc53a0682968ad3df03c698bf3e9bacb3a665d
Comment 126 caulier.gilles 2020-09-19 11:36:44 UTC
Git commit a1deb248f5e457b5abef17d207cb16daef1237b9 by Gilles Caulier.
Committed on 19/09/2020 at 11:36.
Pushed by cgilles into branch 'master'.

Use heap to create DRawDecoder instance to prevent stack overflow
Related: bug 426597

M  +1    -1    core/libs/progressmanager/workingwidget.cpp
M  +3    -2    core/libs/rawengine/drawdecoder.cpp
M  +1    -0    core/libs/threadimageio/preview/previewtask.cpp
M  +19   -6    core/tests/multithreading/myactionthread.cpp

https://invent.kde.org/graphics/digikam/commit/a1deb248f5e457b5abef17d207cb16daef1237b9
Comment 127 caulier.gilles 2020-09-19 11:43:20 UTC
Git commit 0507f9712e4cd76bcbe3aa4cec690ecb25e3fe9e by Gilles Caulier.
Committed on 19/09/2020 at 11:42.
Pushed by cgilles into branch 'master'.

Use heap to create DMetadata instance to prevent stack overflow
Related: bug 426597

M  +2    -2    core/app/main/digikamapp.cpp
M  +1    -0    core/app/main/digikamapp_p.h
M  +11   -10   core/app/views/stack/mapwidgetview.cpp

https://invent.kde.org/graphics/digikam/commit/0507f9712e4cd76bcbe3aa4cec690ecb25e3fe9e
Comment 128 caulier.gilles 2020-09-19 11:55:15 UTC
Marcel, 

My last commits instanciate DRawInfo and DRawDecoder on heap, not on the stack. If ulimit fix low value on your system to allocate stack memory, this can crash the system (see report #426597 for details).

I plan to patch all DMetadata and DMetaengine instances in the same way. But there are plenty ones everywhere, so this will take time.

Gilles
Comment 129 Marcel 2020-09-19 13:19:40 UTC
Gilles, great work. This solved the problem of the sigfault. Anyway, I'm still not able to finish a full scan of my library. Again I'm running out of memory. It turned out there is still a leak or some resources that are not released before the scan is finished. I tried it twice.
Comment 130 caulier.gilles 2020-09-19 13:32:26 UTC
Ah, <fr> "enfin" </fr>, some progress in this room (:=)))...

Ok, let's me some time to check the static analyzer results. perhaps in my last commits i forget something to clear objects from heap memory. The changes are important and the error is always an human stuff.

Gilles
Comment 131 caulier.gilles 2020-09-19 13:34:01 UTC
MArcel, look the comment 14 here :

https://bugs.kde.org/show_bug.cgi?id=426597#c14

It's clear, there is a heap memory corruption in libraw::tiff processor...

Gilles
Comment 132 caulier.gilles 2020-09-19 14:30:55 UTC
Git commit c3e43795e4bbe3be6b59692916b494a90d97c172 by Gilles Caulier.
Committed on 19/09/2020 at 14:30.
Pushed by cgilles into branch 'master'.

Use heap to create DMetadata instance to prevent stack overflow

M  +6    -6    core/libs/threadimageio/preview/previewtask.cpp
M  +9    -5    core/libs/threadimageio/thumb/thumbnailcreator_engine.cpp
M  +1    -0    core/libs/threadimageio/thumb/thumbnailcreator_p.h

https://invent.kde.org/graphics/digikam/commit/c3e43795e4bbe3be6b59692916b494a90d97c172
Comment 133 caulier.gilles 2020-09-19 14:45:44 UTC
Git commit ac89e0f9a60ddde563c23e8db64da99cf5262d76 by Gilles Caulier.
Committed on 19/09/2020 at 14:45.
Pushed by cgilles into branch 'master'.

Use heap to create DMetadata instance to prevent stack overflow

M  +24   -24   core/libs/dimg/dimg_metadata.cpp
M  +1    -0    core/libs/dimg/dimg_p.h
M  +2    -2    core/libs/dimg/dimg_transform.cpp
M  +4    -3    core/libs/dimg/filters/hotpixels/blackframelistviewitem.cpp
M  +4    -3    core/libs/dimg/filters/lens/lensfunfilter.cpp
M  +21   -16   core/libs/dimg/loaders/dimgloader.cpp

https://invent.kde.org/graphics/digikam/commit/ac89e0f9a60ddde563c23e8db64da99cf5262d76
Comment 134 caulier.gilles 2020-09-19 14:55:09 UTC
Git commit 45092f74196f6b97054dbeda6139b653bfc1c2f7 by Gilles Caulier.
Committed on 19/09/2020 at 14:54.
Pushed by cgilles into branch 'master'.

Use heap to create DMetadata instance to prevent stack overflow

M  +21   -20   core/libs/dplugins/iface/dmetainfoiface.cpp
M  +6    -5    core/libs/jpegutils/jpegutils.cpp
M  +5    -4    core/libs/widgets/history/itemfiltershistorymodel.cpp

https://invent.kde.org/graphics/digikam/commit/45092f74196f6b97054dbeda6139b653bfc1c2f7
Comment 135 Marcel 2020-09-19 15:00:32 UTC
Gilles are you sure? I don't have a single raw image and just one tiff. How could this leak come from a part I'm not using?I guess the the problem now must come from somewhere else.
Comment 136 caulier.gilles 2020-09-19 16:20:18 UTC
yes, i 'm sure that libraw make glitch with memory, but i never said that it's the origin of your problem (:=)))...

Gilles
Comment 137 caulier.gilles 2020-09-19 16:52:53 UTC
Git commit 33b43acb76fff9c36925518bbb7bd27f23b5ff5b by Gilles Caulier.
Committed on 19/09/2020 at 16:51.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +6    -0    core/dplugins/dimg/heif/dimgheifloader.cpp
M  +10   -5    core/dplugins/dimg/heif/dimgheifloader_save.cpp
M  +9    -5    core/dplugins/generic/tools/calendar/print/calpainter.cpp
M  +16   -15   core/libs/metadataengine/dmetadata/dmetadata_generic.cpp

https://invent.kde.org/graphics/digikam/commit/33b43acb76fff9c36925518bbb7bd27f23b5ff5b
Comment 138 caulier.gilles 2020-09-19 21:00:31 UTC
Git commit 765701869ec1b3506407bb6f3b790e43f3763a64 by Gilles Caulier.
Committed on 19/09/2020 at 21:00.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +4    -3    core/dplugins/bqm/colors/iccconvert/iccconvert.cpp
M  +3    -2    core/dplugins/bqm/enhance/lensautofix/lensautofix.cpp
M  +10   -9    core/dplugins/bqm/metadata/assigntemplate/assigntemplate.cpp
M  +31   -30   core/dplugins/bqm/metadata/removemetadata/removemetadata.cpp
M  +64   -63   core/dplugins/bqm/metadata/timeadjust/timeadjust.cpp

https://invent.kde.org/graphics/digikam/commit/765701869ec1b3506407bb6f3b790e43f3763a64
Comment 139 caulier.gilles 2020-09-20 03:09:12 UTC
Git commit 1893c2a88b6d8ea40095c2ec05ea6a533a15924a by Gilles Caulier.
Committed on 20/09/2020 at 03:08.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +3    -2    core/dplugins/dimg/jpeg2000/dimgjpeg2000loader_load.cpp
M  +39   -22   core/dplugins/dimg/tiff/dimgtiffloader_save.cpp
M  +14   -13   core/dplugins/generic/import/dscanner/saveimgthread.cpp
M  +10   -10   core/dplugins/generic/metadata/timeadjust/timeadjustdialog.cpp
M  +56   -55   core/dplugins/generic/metadata/timeadjust/timeadjusttask.cpp
M  +36   -36   core/dplugins/generic/tools/htmlgallery/generator/galleryelementfunctor.cpp
M  +3    -3    core/dplugins/generic/tools/printcreator/manager/advprinttask.cpp
M  +6    -7    core/dplugins/generic/tools/printcreator/wizard/advprintcaptionpage.cpp
M  +10   -9    core/dplugins/generic/tools/sendbymail/manager/imageresizejob.cpp
M  +3    -2    core/dplugins/generic/view/glviewer/glviewertexture.cpp
M  +11   -8    core/dplugins/generic/webservices/unified/manager/wsauthentication.cpp

https://invent.kde.org/graphics/digikam/commit/1893c2a88b6d8ea40095c2ec05ea6a533a15924a
Comment 140 caulier.gilles 2020-09-20 03:54:41 UTC
Git commit 86b911ae10d0f24ba5f2c9c637348605d8b53029 by Gilles Caulier.
Committed on 20/09/2020 at 03:54.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +1    -0    core/libs/database/collection/collectionscanner_p.h
M  +8    -8    core/libs/database/collection/collectionscanner_utils.cpp
M  +148  -146  core/libs/dngwriter/dngwriter_convert.cpp
M  +10   -6    core/libs/fileactionmanager/fileworkeriface.cpp
M  +20   -19   core/libs/fileactionmanager/metadatahub.cpp
M  +7    -3    core/utilities/advancedrename/parser/options/cameranameoption.cpp
M  +6    -5    core/utilities/advancedrename/parser/options/metadataoption.cpp
M  +4    -3    core/utilities/imageeditor/core/editorcore.cpp
M  +4    -4    core/utilities/imageeditor/main/imagewindow.cpp
M  +1    -0    core/utilities/imageeditor/main/imagewindow_p.h
M  +15   -14   core/utilities/import/backend/cameracontroller.cpp
M  +26   -3    core/utilities/import/backend/gpcamera.cpp
M  +15   -14   core/utilities/import/backend/umscamera.cpp
M  +4    -3    core/utilities/queuemanager/manager/batchtool.cpp

https://invent.kde.org/graphics/digikam/commit/86b911ae10d0f24ba5f2c9c637348605d8b53029
Comment 141 caulier.gilles 2020-09-20 11:33:11 UTC
Git commit feb2f2aeb8b45dd6d4d8628ad588efe7f5183050 by Gilles Caulier.
Committed on 20/09/2020 at 11:32.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +11   -2    core/libs/database/item/scanner/itemscanner_video.cpp
M  +4    -4    core/libs/fileactionmanager/metadatahub.cpp
M  +1    -1    core/libs/metadataengine/dmetadata/dmetadata.h
M  +1    -1    core/libs/metadataengine/dmetadata/dmetadatasettings.cpp
M  +1    -1    core/libs/metadataengine/dmetadata/dmetadatasettings.h
M  +1    -1    core/libs/metadataengine/dmetadata/dmetadatasettingscontainer.cpp
M  +1    -1    core/libs/metadataengine/dmetadata/dmetadatasettingscontainer.h
M  +8    -2    core/libs/properties/geolocation/itempropertiesgpstab.cpp
M  +17   -3    core/libs/properties/import/importitempropertiessidebar.cpp
M  +6    -5    core/libs/properties/itempropertiesmetadatatab.cpp
M  +8    -8    core/libs/properties/itempropertiessidebar.cpp
M  +4    -3    core/libs/properties/itempropertiessidebardb.cpp
M  +4    -3    core/libs/timeadjust/clockphotodialog.cpp
M  +8    -7    core/libs/widgets/metadata/exifwidget.cpp
M  +8    -7    core/libs/widgets/metadata/iptcwidget.cpp
M  +8    -7    core/libs/widgets/metadata/makernotewidget.cpp
M  +7    -6    core/libs/widgets/metadata/metadatapanel.cpp
M  +1    -1    core/libs/widgets/metadata/metadatawidget.cpp
M  +8    -7    core/libs/widgets/metadata/xmpwidget.cpp
M  +12   -12   core/showfoto/main/showfoto.cpp
M  +1    -0    core/showfoto/main/showfoto_p.h
M  +4    -3    core/showfoto/thumbbar/showfotothumbnailmodel.cpp

https://invent.kde.org/graphics/digikam/commit/feb2f2aeb8b45dd6d4d8628ad588efe7f5183050
Comment 142 caulier.gilles 2020-09-20 12:05:38 UTC
Git commit c3c15b7934c0faece918ff9ef16543a26909d33d by Gilles Caulier.
Committed on 20/09/2020 at 12:05.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +3    -2    core/tests/filters/testlensfuniface.cpp
M  +4    -3    core/tests/geolocation/geoiface/demo/mainwindow.cpp
M  +9    -9    core/tests/metadataengine/applytagstest.cpp
M  +15   -15   core/tests/metadataengine/commentreadwritetest.cpp
M  +1    -1    core/tests/metadataengine/commentreadwritetest.h
M  +6    -6    core/tests/metadataengine/createxmpsidecartest.cpp
M  +16   -16   core/tests/metadataengine/erasemetadatatagtest.cpp
M  +2    -2    core/tests/metadataengine/loadfrombatest.cpp
M  +13   -13   core/tests/metadataengine/patchpreviewtest.cpp
M  +12   -12   core/tests/metadataengine/printiteminfotest.cpp
M  +5    -5    core/tests/metadataengine/printmetadatatest.cpp
M  +9    -9    core/tests/metadataengine/printtagslisttest.cpp
M  +16   -16   core/tests/metadataengine/ratingreadwritetest.cpp
M  +1    -1    core/tests/metadataengine/ratingreadwritetest.h
M  +7    -7    core/tests/metadataengine/setiptcpreviewtest.cpp
M  +15   -15   core/tests/metadataengine/setxmpfacetest.cpp
M  +23   -23   core/tests/metadataengine/tagsreadwritetest.cpp
M  +1    -1    core/tests/metadataengine/tagsreadwritetest.h
M  +15   -15   core/tests/metadataengine/usexmpsidecartest.cpp
M  +24   -23   core/tests/timestampupdate/timestampupdatetest.cpp

https://invent.kde.org/graphics/digikam/commit/c3c15b7934c0faece918ff9ef16543a26909d33d
Comment 143 caulier.gilles 2020-09-20 12:33:27 UTC
Git commit 115956282471cb03a45ea61a821da26a0df2d698 by Gilles Caulier.
Committed on 20/09/2020 at 12:32.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +6    -6    core/dplugins/editor/colors/profileconversion/profileconversiontool.cpp
M  +4    -2    core/dplugins/editor/enhance/lensautofix/lensautofixtool.cpp
M  +6    -6    core/dplugins/generic/webservices/box/boxtalker.cpp
M  +6    -6    core/dplugins/generic/webservices/dropbox/dbtalker.cpp
M  +6    -6    core/dplugins/generic/webservices/facebook/fbwindow.cpp
M  +8    -8    core/dplugins/generic/webservices/filecopy/fctask.cpp
M  +8    -8    core/dplugins/generic/webservices/flickr/flickrtalker.cpp
M  +6    -6    core/dplugins/generic/webservices/google/gdrive/gdtalker.cpp
M  +7    -7    core/dplugins/generic/webservices/google/gphoto/gptalker.cpp
M  +10   -10   core/dplugins/generic/webservices/imgur/imgurimageslist.cpp
M  +8    -7    core/dplugins/generic/webservices/ipfs/ipfsimageslist.cpp
M  +8    -8    core/dplugins/generic/webservices/mediawiki/mediawikiwindow.cpp
M  +6    -6    core/dplugins/generic/webservices/onedrive/odtalker.cpp
M  +6    -6    core/dplugins/generic/webservices/piwigo/piwigotalker.cpp
M  +6    -6    core/dplugins/generic/webservices/rajce/rajcecommand.cpp
M  +6    -6    core/dplugins/generic/webservices/smugmug/smugwindow.cpp

https://invent.kde.org/graphics/digikam/commit/115956282471cb03a45ea61a821da26a0df2d698
Comment 144 caulier.gilles 2020-09-20 12:36:43 UTC
Git commit fc1026412d461c3922f430ab3b800323d2df98c7 by Gilles Caulier.
Committed on 20/09/2020 at 12:36.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +24   -24   core/dplugins/generic/metadata/metadataedit/exif/exifadjust.cpp
M  +29   -29   core/dplugins/generic/metadata/metadataedit/exif/exifcaption.cpp
M  +36   -36   core/dplugins/generic/metadata/metadataedit/exif/exifdatetime.cpp
M  +50   -50   core/dplugins/generic/metadata/metadataedit/exif/exifdevice.cpp
M  +14   -14   core/dplugins/generic/metadata/metadataedit/exif/exifeditwidget.cpp
M  +28   -28   core/dplugins/generic/metadata/metadataedit/exif/exiflens.cpp
M  +18   -18   core/dplugins/generic/metadata/metadataedit/exif/exiflight.cpp
M  +11   -11   core/dplugins/generic/metadata/metadataedit/iptc/iptccategories.cpp
M  +18   -18   core/dplugins/generic/metadata/metadataedit/iptc/iptccontent.cpp
M  +23   -23   core/dplugins/generic/metadata/metadataedit/iptc/iptccredits.cpp
M  +11   -11   core/dplugins/generic/metadata/metadataedit/iptc/iptceditwidget.cpp
M  +35   -35   core/dplugins/generic/metadata/metadataedit/iptc/iptcenvelope.cpp
M  +8    -8    core/dplugins/generic/metadata/metadataedit/iptc/iptckeywords.cpp
M  +39   -39   core/dplugins/generic/metadata/metadataedit/iptc/iptcorigin.cpp
M  +37   -37   core/dplugins/generic/metadata/metadataedit/iptc/iptcproperties.cpp
M  +17   -17   core/dplugins/generic/metadata/metadataedit/iptc/iptcstatus.cpp
M  +8    -8    core/dplugins/generic/metadata/metadataedit/iptc/iptcsubjects.cpp
M  +11   -11   core/dplugins/generic/metadata/metadataedit/xmp/xmpcategories.cpp
M  +23   -23   core/dplugins/generic/metadata/metadataedit/xmp/xmpcontent.cpp
M  +66   -66   core/dplugins/generic/metadata/metadataedit/xmp/xmpcredits.cpp
M  +11   -11   core/dplugins/generic/metadata/metadataedit/xmp/xmpeditwidget.cpp
M  +8    -8    core/dplugins/generic/metadata/metadataedit/xmp/xmpkeywords.cpp
M  +56   -56   core/dplugins/generic/metadata/metadataedit/xmp/xmporigin.cpp
M  +23   -23   core/dplugins/generic/metadata/metadataedit/xmp/xmpproperties.cpp
M  +17   -17   core/dplugins/generic/metadata/metadataedit/xmp/xmpstatus.cpp
M  +8    -8    core/dplugins/generic/metadata/metadataedit/xmp/xmpsubjects.cpp

https://invent.kde.org/graphics/digikam/commit/fc1026412d461c3922f430ab3b800323d2df98c7
Comment 145 caulier.gilles 2020-09-20 12:37:43 UTC
Marcel,

I patched all DMetadata/MetaEngine instance to use heap instead stack to prevent stack overflow.

This will improve the stability on your computer ?

Gilles
Comment 146 caulier.gilles 2020-09-20 14:31:20 UTC
Git commit e79a1a3bd23e886cbb230395b588470dfd04c236 by Gilles Caulier.
Committed on 20/09/2020 at 14:30.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +28   -26   core/dplugins/generic/tools/panorama/tasks/preprocesstask.cpp
M  +6    -6    core/dplugins/generic/webservices/google/gphoto/gptalker.cpp
M  +17   -13   core/dplugins/generic/webservices/google/gswindow.cpp
M  +8    -7    core/dplugins/generic/webservices/pinterest/ptalker.cpp
M  +8    -7    core/dplugins/generic/webservices/twitter/twittertalker.cpp
M  +22   -18   core/dplugins/generic/webservices/yandexfotki/yfwindow.cpp
M  +5    -5    core/libs/dialogs/imagedialog.cpp

https://invent.kde.org/graphics/digikam/commit/e79a1a3bd23e886cbb230395b588470dfd04c236
Comment 147 caulier.gilles 2020-09-20 15:49:54 UTC
Git commit b4589b0d2a0e31cb709da3554c0190c8d71f7b89 by Gilles Caulier.
Committed on 20/09/2020 at 15:49.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +10   -10   core/libs/properties/itempropertiesmetadatatab.cpp
M  +1    -1    core/libs/widgets/metadata/metadatawidget.cpp

https://invent.kde.org/graphics/digikam/commit/b4589b0d2a0e31cb709da3554c0190c8d71f7b89
Comment 148 caulier.gilles 2020-09-20 15:58:33 UTC
Git commit 10e2ecab81ecc4f45989f82987021a34dbb3558d by Gilles Caulier.
Committed on 20/09/2020 at 15:58.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +3    -3    core/libs/widgets/metadata/exifwidget.cpp
M  +3    -3    core/libs/widgets/metadata/iptcwidget.cpp
M  +3    -3    core/libs/widgets/metadata/makernotewidget.cpp
M  +3    -3    core/libs/widgets/metadata/xmpwidget.cpp

https://invent.kde.org/graphics/digikam/commit/10e2ecab81ecc4f45989f82987021a34dbb3558d
Comment 149 caulier.gilles 2020-09-20 16:10:37 UTC
Git commit 3f17d2d4ee8f2877ae6036387ad1360beac1ca84 by Gilles Caulier.
Committed on 20/09/2020 at 16:10.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +2    -2    core/libs/widgets/metadata/exifwidget.cpp
M  +2    -2    core/libs/widgets/metadata/iptcwidget.cpp
M  +2    -2    core/libs/widgets/metadata/makernotewidget.cpp
M  +12   -5    core/libs/widgets/metadata/metadatawidget.cpp
M  +5    -5    core/libs/widgets/metadata/metadatawidget.h
M  +3    -3    core/libs/widgets/metadata/xmpwidget.cpp

https://invent.kde.org/graphics/digikam/commit/3f17d2d4ee8f2877ae6036387ad1360beac1ca84
Comment 150 Marcel 2020-09-20 17:59:24 UTC
(In reply to caulier.gilles from comment #145)
> Marcel,
> 
> I patched all DMetadata/MetaEngine instance to use heap instead stack to
> prevent stack overflow.
> 
> This will improve the stability on your computer ?
> 
> Gilles

I tried the version directly after this comment. Unfortunately it still leaks memory, but not much. This time my computer run out of memory after around 1.5h. I now start the test again with your most recent commits. I could upload the system monitor picture during detection. 

What I don't understand is that during face detection 9 threads are running with full load on my dual core cpu with 4 tubes. From compiling a program it sait that the best would be the tube number or the tube number +1. Where is my misunderstanding?
Comment 151 caulier.gilles 2020-09-20 18:07:50 UTC
Marcel,

You lost memory a little bit, but the infamous crash in Preview thread disappear. Right ?

If yes, we will trying to discover the source of the problem for this memory. Next stage for me is to add a new compilation option to use ASAN tool:

https://en.wikipedia.org/wiki/AddressSanitizer

About multithread while face detection, i'm not sure. If i remmeber, Nghia report to me that multicore support in face detection have been disabled for performance reasons, as low level API is not re-entrant.

Perhaps Nghia can provide more details for this point.

Gilles
Comment 152 caulier.gilles 2020-09-20 18:16:40 UTC
Git commit a8f47f320928428f102df5ff40a86c3d41184c89 by Gilles Caulier.
Committed on 20/09/2020 at 18:16.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +6    -6    core/libs/properties/geolocation/itempropertiesgpstab.cpp
M  +1    -1    core/libs/properties/geolocation/itempropertiesgpstab.h
M  +17   -3    core/libs/properties/import/importitempropertiessidebar.cpp
M  +5    -5    core/libs/properties/import/importitempropertiestab.cpp
M  +1    -1    core/libs/properties/import/importitempropertiestab.h
M  +2    -2    core/libs/properties/itempropertiesmetadatatab.cpp
M  +3    -3    core/libs/properties/itempropertiesmetadatatab.h
M  +1    -1    core/libs/properties/itempropertiessidebardb.cpp

https://invent.kde.org/graphics/digikam/commit/a8f47f320928428f102df5ff40a86c3d41184c89
Comment 153 Minh Nghia Duong 2020-09-20 18:19:02 UTC
(In reply to caulier.gilles from comment #151)
> Marcel,
> 
> You lost memory a little bit, but the infamous crash in Preview thread
> disappear. Right ?
> 
> If yes, we will trying to discover the source of the problem for this
> memory. Next stage for me is to add a new compilation option to use ASAN
> tool:
> 
> https://en.wikipedia.org/wiki/AddressSanitizer
> 
> About multithread while face detection, i'm not sure. If i remmeber, Nghia
> report to me that multicore support in face detection have been disabled for
> performance reasons, as low level API is not re-entrant.
> 
> Perhaps Nghia can provide more details for this point.
> 
> Gilles

Hi Gilles,

It's not quite what you said. I disabled multiple face extractors in faces engine, which means for now, in faccial recognition, there is only one FaceExtractor being used. This avoid multiple memory allocation for faical recognition. However, with only one FaceExtractor, the facial recognition process still runs multi-threading by using cv::parallel_for_ function.

It's implemented in file : https://invent.kde.org/graphics/digikam/-/blob/master/core/libs/facesengine/recognition/opencv-dnn/opencvdnnfacerecognizer.cpp , at lines 465 and 505. 

If you want to desactivate it for a test run, just replace these line with a normal loop.

Nghia
Comment 154 caulier.gilles 2020-09-20 18:26:33 UTC
Nghia,

i think Marcel only play with face detection, not face recognition for the moment. FaceExtractor in only relevant with face recognition workflow, right ?

Gilles
Comment 155 Minh Nghia Duong 2020-09-20 18:32:17 UTC
(In reply to caulier.gilles from comment #154)

Yes, only one FaceExtractor is allocated at the init sequence of digikam. After that, the multi-threading only envoked in the recognition process.
Comment 156 Maik Qualmann 2020-09-20 20:01:59 UTC
Git commit cd24540e6da7170b56e6a2b9a2de3138d0769969 by Maik Qualmann.
Committed on 20/09/2020 at 20:01.
Pushed by mqualmann into branch 'master'.

check if DImg data equal

M  +4    -1    core/libs/dimg/dimg_data.cpp

https://invent.kde.org/graphics/digikam/commit/cd24540e6da7170b56e6a2b9a2de3138d0769969
Comment 157 caulier.gilles 2020-09-20 20:54:11 UTC
Git commit 2b8a165d227d8cb9f90c21db745bc699d254ce44 by Gilles Caulier.
Committed on 20/09/2020 at 20:53.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +1    -1    core/dplugins/bqm/enhance/lensautofix/lensautofix.cpp
M  +1    -2    core/dplugins/editor/enhance/lensautofix/lensautofixtool.cpp
M  +1    -1    core/libs/dimg/dimg.h
M  +13   -6    core/libs/dimg/filters/lens/lensfuncameraselector.cpp
M  +3    -2    core/libs/dimg/filters/lens/lensfuncameraselector.h
M  +9    -9    core/libs/dimg/filters/lens/lensfuniface.cpp
M  +1    -1    core/libs/dimg/filters/lens/lensfuniface.h
M  +5    -5    core/libs/fileactionmanager/metadatahub.h
M  +1    -1    core/tests/filters/testlensfuniface.cpp

https://invent.kde.org/graphics/digikam/commit/2b8a165d227d8cb9f90c21db745bc699d254ce44
Comment 158 caulier.gilles 2020-09-20 21:34:47 UTC
Git commit ef51f605528649e9f509bd37d2710409f3bf83f0 by Gilles Caulier.
Committed on 20/09/2020 at 21:32.
Pushed by cgilles into branch 'master'.

Add new compilation option to enable compiler sanitizers ASAN and UBSAN
Related: bug 426597

M  +1    -0    Mainpage.dox
M  +6    -1    core/CMakeLists.txt
M  +3    -1    core/cmake/modules/MacroCompiler.cmake

https://invent.kde.org/graphics/digikam/commit/ef51f605528649e9f509bd37d2710409f3bf83f0
Comment 159 Marcel 2020-09-21 05:22:23 UTC
I run the face detection over night and run out of memory again, but it's definitely not the seg fault from the Preview process that crashes to program. This part is solved. Thank you Gilles. 

It's true, I'm just playing with face detection and not recognition.

I'm asking about the worker threads as on my system this threads just use more memory and don't increase the speed of the process. Probably there is some space for lower memory demand.

I started now compiling with clang to see if the compiler has any effect on that memory leak.
Comment 160 caulier.gilles 2020-09-21 05:39:19 UTC
Marcel,

This morning i append ASAN support. There is a new cmake compilation option:

https://invent.kde.org/graphics/digikam/-/blob/master/Mainpage.dox#L461

You need to install libasan-devel for linking stage first (i need to add cmake check dependencies, it's in my TODO list).

Warning: ASAN require more memory to check... memory allocations (:-)))...

https://en.wikipedia.org/wiki/AddressSanitizer

There are plenty of checker to enable, but for the moment i just turn on the most common: address sanitizer. I can see a linking failure with other ones. This need investigations.

Gilles
Comment 161 Maik Qualmann 2020-09-21 06:06:04 UTC
Git commit edcab64cdca0ae1800bab455724b9753dffd000c by Maik Qualmann.
Committed on 21/09/2020 at 06:05.
Pushed by mqualmann into branch 'master'.

for a test, remove myself from the loading listener beforehand

M  +5    -5    core/libs/threadimageio/fileio/loadsavetask.cpp
M  +38   -36   core/libs/threadimageio/preview/previewtask.cpp
M  +4    -4    core/libs/threadimageio/thumb/thumbnailtask.cpp

https://invent.kde.org/graphics/digikam/commit/edcab64cdca0ae1800bab455724b9753dffd000c
Comment 162 Marcel 2020-09-21 06:59:16 UTC
Bug news, on my test with clang the bad old error occurred again after a few minutes of scanning:
Thread 47 "Thread (pooled)" received signal SIGSEGV, Segmentation fault.
#1  0x00007ffff6a852b5 in Digikam::PreviewLoadingTask::execute (this=0xaec5a80)
    at /home/marcel/Repos/digikam/core/libs/threadimageio/preview/previewtask.cpp:321

I will try running it with asan. If my memory is too small I will increase the swap for this test. I didn't increased swap so far because I think the scanning must work with 8GB of ram.
Comment 163 Marcel 2020-09-21 08:40:26 UTC
I now included the changes from Maik as well. My test runs now for over 40 minutes. Hey, it didn't crash and I can't see any significant increase in memory. Probably the last changes from Maik finally solved this bug. Thank you all for working on this bug. 

@Maik: What was the problem exactly?
 
I will confirm the result after the scan finishes.
Comment 164 Marcel 2020-09-21 16:19:38 UTC
Created attachment 131836 [details]
Face detection process in htop

I tested it on my system and it worked for more than 5 hours. After that Digikam was killed by another process. The log said only "killed". 

Despite that crash I consider this bug as solved. 

In this 5 hours of scanning I experiences some strange behavior. Memory increased sometimes in very small steps until it reached a limit, then in a few seconds 3GB of RAM was released again and all started from the beginning. 

See in the picture htop.png my memory consumption cut down to threads. Why are there so many threads using so much memory?
Comment 165 caulier.gilles 2020-09-21 16:26:50 UTC
Marcel,

In htop, you can setup to turn on the thread names. This can help to identify the thread functions (i hope that all QThread instances are explicitly identified with Linux Kernel. Typically, the object name is used for that. I use this powerful features in my office which run under Linux only.

I suspect that all these threads are used to register faces in database, but i'm not 100% sure. And the Question is why this happen only few times after a long running time...

Gilles
Comment 166 caulier.gilles 2020-09-21 16:29:19 UTC
QThread object name visible as Linux process : the explaination are there :

https://stackoverflow.com/questions/19854932/is-there-a-portable-way-to-give-thread-name-with-qt

Gilles
Comment 167 Marcel 2020-09-21 19:17:26 UTC
Gilles, thank you for this tip with htop. Great to know about such debug possibilities. Unfortunately the name of about half of the QThread's have the name "Thread (pooled)".

About the asan. It now works on my computer, but it needed a workaround in your script. 
find_library(ASAN_LIBRARIES NAMES asan) did not find anything on my system. On Ubuntu the asan is installed by default with the gcc. See the comment from yugr on https://stackoverflow.com/questions/37970758/how-to-use-addresssanitizer-with-gcc. Should I provide you a patch with a fix?

I now run the scanning again over night with asan enabled.
Comment 168 Maik Qualmann 2020-09-21 19:49:06 UTC
Git commit 090bfe8ffd2ec534a0f2a100e4e2e4c3083e300b by Maik Qualmann.
Committed on 21/09/2020 at 19:47.
Pushed by mqualmann into branch 'master'.

reduce sent face packets

M  +1    -1    core/utilities/facemanagement/threads/facepreviewloader.cpp

https://invent.kde.org/graphics/digikam/commit/090bfe8ffd2ec534a0f2a100e4e2e4c3083e300b
Comment 169 caulier.gilles 2020-09-21 20:50:16 UTC
Marcel,

yes sure a patch is always welcome.

Gilles
Comment 170 Maik Qualmann 2020-09-21 20:56:24 UTC
Git commit c12a37ca883329d746c74fa54b94cd39eb8324c9 by Maik Qualmann.
Committed on 21/09/2020 at 20:55.
Pushed by mqualmann into branch 'master'.

remove the created thumbnails threads because they are unused

M  +0    -5    core/utilities/facemanagement/threads/facepipeline.cpp

https://invent.kde.org/graphics/digikam/commit/c12a37ca883329d746c74fa54b94cd39eb8324c9
Comment 171 caulier.gilles 2020-09-22 03:22:23 UTC
Git commit 464c7cf5d10f84bacfbbe98fe4fa6193e1d64773 by Gilles Caulier.
Committed on 22/09/2020 at 03:09.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +15   -9    core/libs/jpegutils/jpegutils.cpp
M  +6    -1    core/libs/jpegutils/jpegutils.h

https://invent.kde.org/graphics/digikam/commit/464c7cf5d10f84bacfbbe98fe4fa6193e1d64773
Comment 172 caulier.gilles 2020-09-22 03:57:39 UTC
Git commit 198dc96fe39c17c038b4b0782fca81cc199bbdc7 by Gilles Caulier.
Committed on 22/09/2020 at 03:52.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +3    -3    core/libs/database/item/scanner/itemscanner.cpp
M  +2    -2    core/libs/database/item/scanner/itemscanner_file.cpp
M  +3    -3    core/libs/database/item/scanner/itemscanner_history.cpp
M  +6    -0    core/libs/database/item/scanner/itemscanner_p.cpp
M  +2    -1    core/libs/database/item/scanner/itemscanner_p.h
M  +10   -10   core/libs/database/item/scanner/itemscanner_photo.cpp
M  +6    -6    core/libs/database/item/scanner/itemscanner_video.cpp

https://invent.kde.org/graphics/digikam/commit/198dc96fe39c17c038b4b0782fca81cc199bbdc7
Comment 173 caulier.gilles 2020-09-22 03:57:47 UTC
Git commit aa1de72a075ef92c753b90021488e555b246ae42 by Gilles Caulier.
Committed on 22/09/2020 at 03:38.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +25   -23   core/dplugins/generic/tools/panorama/tasks/copyfilestask.cpp
M  +0    -2    core/dplugins/generic/tools/panorama/tasks/copyfilestask.h

https://invent.kde.org/graphics/digikam/commit/aa1de72a075ef92c753b90021488e555b246ae42
Comment 174 caulier.gilles 2020-09-22 03:57:55 UTC
Git commit a2f03025488e56fc2c1ae2f3f2e48388b09c0e0f by Gilles Caulier.
Committed on 22/09/2020 at 03:39.
Pushed by cgilles into branch 'master'.

Use heap to create MetaEngine instance to prevent stack overflow

M  +20   -13   core/dplugins/generic/metadata/geolocationedit/kmlexport/kmlexport.cpp
M  +1    -1    core/dplugins/generic/metadata/geolocationedit/kmlexport/kmlexport.h

https://invent.kde.org/graphics/digikam/commit/a2f03025488e56fc2c1ae2f3f2e48388b09c0e0f
Comment 175 Marcel 2020-09-22 05:08:53 UTC
Created attachment 131853 [details]
heap-use-after-free error

I run Digikam with the address sanitizer (fbf127ecf30867613d3fe247be85b4f09533e046). Indeed there was an error in the previewtask. See the output from asan.
Comment 176 caulier.gilles 2020-09-22 05:34:58 UTC
Git commit 51e1ccaa60e71f253bb7c760a3f5c678f3cbc902 by Gilles Caulier.
Committed on 22/09/2020 at 05:33.
Pushed by cgilles into branch 'master'.

move all implmentations to cpp files. Header must provide declarations only
always provide a default destructor in implementations.

M  +4    -0    core/libs/threadimageio/preview/previewloadthread.cpp
M  +23   -7    core/libs/threadimageio/preview/previewloadthread.h
M  +14   -1    core/libs/threadimageio/preview/previewsettings.cpp
M  +4    -3    core/libs/threadimageio/preview/previewsettings.h
M  +10   -0    core/libs/threadimageio/preview/previewtask.cpp
M  +2    -5    core/libs/threadimageio/preview/previewtask.h

https://invent.kde.org/graphics/digikam/commit/51e1ccaa60e71f253bb7c760a3f5c678f3cbc902
Comment 177 caulier.gilles 2020-09-22 13:12:26 UTC
Git commit 5ce6276093c8a636d6844b060cb9020f01c20f18 by Gilles Caulier.
Committed on 22/09/2020 at 13:12.
Pushed by cgilles into branch 'master'.

move all implmentations to cpp files. Header must provide declarations only
always provide a default destructor in implementations.

M  +70   -3    core/libs/threadimageio/fileio/loadsavetask.cpp
M  +35   -62   core/libs/threadimageio/fileio/loadsavetask.h

https://invent.kde.org/graphics/digikam/commit/5ce6276093c8a636d6844b060cb9020f01c20f18
Comment 178 Marcel 2020-09-22 13:42:37 UTC
Created attachment 131868 [details]
Address Sanitizer error on PreviewLoadingTask

Gilles, I just checked your changes on the PreviewLoadingTask. I got an error on the PreviewLoadingTask with the address sanitizer.
Comment 179 caulier.gilles 2020-09-22 14:34:05 UTC
Hi Marcel,

This is interresting, because my change are pure cosmetic for the moment.

Typically i moved all class method implementation from headers to cpp files, and append missing default constructors or destructors.

For this last point, C++ norm is clear :

"Defining a class without a constructor but with a destructor is perfectly legal C++, but is not recommended. Your compiler will try to generate a default constructor for you, which will automatically call the default constructor of all members in your class. You can explicitly request this by marking your constructor as default, but you lose finer control over the instantiation of your class, and cannot do anything but default construct all members of your class."

Gilles
Comment 180 caulier.gilles 2020-09-22 14:50:26 UTC
Git commit d4b496877424462cc01c1860bf2bbc544cdb9585 by Gilles Caulier.
Committed on 22/09/2020 at 14:49.
Pushed by cgilles into branch 'master'.

add default contructor and destructor
move implementations to .cpp

M  +24   -2    core/libs/threadimageio/fileio/loadingcache.cpp
M  +7    -4    core/libs/threadimageio/fileio/loadingcache.h

https://invent.kde.org/graphics/digikam/commit/d4b496877424462cc01c1860bf2bbc544cdb9585
Comment 181 Maik Qualmann 2020-09-22 19:22:24 UTC
Git commit 9f89a5264d45d9cffdd331b7a5d98a4dbde45b51 by Maik Qualmann.
Committed on 22/09/2020 at 19:21.
Pushed by mqualmann into branch 'master'.

preview loading tasks with new stop logic

M  +20   -51   core/libs/threadimageio/fileio/loadsavetask.cpp
M  +20   -20   core/libs/threadimageio/preview/previewtask.cpp
M  +14   -14   core/libs/threadimageio/thumb/thumbnailtask.cpp

https://invent.kde.org/graphics/digikam/commit/9f89a5264d45d9cffdd331b7a5d98a4dbde45b51
Comment 182 caulier.gilles 2020-09-23 05:54:46 UTC
Git commit 8333a8f06f714950729d7a85d87c9eb9d0cc4a6c by Gilles Caulier.
Committed on 23/09/2020 at 05:53.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes

M  +6    -0    core/libs/threadimageio/preview/previewloadthread.h
M  +6    -0    core/libs/threadimageio/preview/previewtask.h

https://invent.kde.org/graphics/digikam/commit/8333a8f06f714950729d7a85d87c9eb9d0cc4a6c
Comment 183 caulier.gilles 2020-09-23 08:47:23 UTC
Git commit 1f11358b3918a16d7576f6f8a80b644544784975 by Gilles Caulier.
Committed on 23/09/2020 at 08:46.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes

M  +24   -0    core/libs/threadimageio/fileio/loadsavetask.h

https://invent.kde.org/graphics/digikam/commit/1f11358b3918a16d7576f6f8a80b644544784975
Comment 184 caulier.gilles 2020-09-23 08:55:32 UTC
Git commit 5ca5eb5260994a4b4200c1626a31b78aeef02858 by Gilles Caulier.
Committed on 23/09/2020 at 08:55.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes

M  +21   -3    core/libs/threadimageio/fileio/loadingcache.h

https://invent.kde.org/graphics/digikam/commit/5ca5eb5260994a4b4200c1626a31b78aeef02858
Comment 185 caulier.gilles 2020-09-23 09:01:39 UTC
Git commit d0cf9690e91b59a98e152985535912516eb1e392 by Gilles Caulier.
Committed on 23/09/2020 at 09:00.
Pushed by cgilles into branch 'master'.

This class is just a static methods container. No need construtor, destructor, copy constructor and equality operator

M  +9    -0    core/libs/threadimageio/fileio/loadingcacheinterface.h

https://invent.kde.org/graphics/digikam/commit/d0cf9690e91b59a98e152985535912516eb1e392
Comment 186 caulier.gilles 2020-09-23 09:12:52 UTC
Git commit 1ac2776d8ccee771a09dcb4ee4ec1699d7eaf65a by Gilles Caulier.
Committed on 23/09/2020 at 09:12.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes
Add missing destructor

M  +4    -0    core/libs/threadimageio/engine/managedloadsavethread.h
M  +4    -0    core/libs/threadimageio/engine/sharedloadsavethread.cpp
M  +7    -0    core/libs/threadimageio/engine/sharedloadsavethread.h

https://invent.kde.org/graphics/digikam/commit/1ac2776d8ccee771a09dcb4ee4ec1699d7eaf65a
Comment 187 caulier.gilles 2020-09-23 09:28:37 UTC
Git commit 8b8aae7b266b94181239ed6e559deef4a4512f6c by Gilles Caulier.
Committed on 23/09/2020 at 09:27.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes

M  +1    -1    core/libs/threadimageio/CMakeLists.txt
R  +0    -0    core/libs/threadimageio/thumb/thumbnailcreator_basic.cpp [from: core/libs/threadimageio/thumb/thumbnailbasic.cpp - 100% similarity]
M  +12   -0    core/libs/threadimageio/thumb/thumbnailloadthread.h
M  +6    -0    core/libs/threadimageio/thumb/thumbnailtask.h

https://invent.kde.org/graphics/digikam/commit/8b8aae7b266b94181239ed6e559deef4a4512f6c
Comment 188 caulier.gilles 2020-09-23 10:11:07 UTC
Git commit f08d194f50b41c6d68fbb0855db20f807e5e7bfd by Gilles Caulier.
Committed on 23/09/2020 at 10:10.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes

M  +8    -1    core/libs/threads/dynamicthread.h
M  +12   -0    core/libs/threads/parallelworkers.h
M  +6    -0    core/libs/threads/threadmanager.h
M  +4    -0    core/libs/threads/workerobject.h

https://invent.kde.org/graphics/digikam/commit/f08d194f50b41c6d68fbb0855db20f807e5e7bfd
Comment 189 caulier.gilles 2020-09-23 15:55:31 UTC
Git commit 7600efe92d5979545189649229f01b4568361485 by Gilles Caulier.
Committed on 23/09/2020 at 15:54.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes.
Add missing destructor

M  +4    -0    core/utilities/facemanagement/workers/databasewriter.cpp
M  +7    -0    core/utilities/facemanagement/workers/databasewriter.h
M  +6    -0    core/utilities/facemanagement/workers/detectionworker.h
M  +6    -0    core/utilities/facemanagement/workers/recognitionworker.h

https://invent.kde.org/graphics/digikam/commit/7600efe92d5979545189649229f01b4568361485
Comment 190 caulier.gilles 2020-09-24 01:56:26 UTC
Git commit cd1b6f82957813746a3f44a0251c1decab4790d7 by Gilles Caulier.
Committed on 24/09/2020 at 01:56.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes.
Add missing destructor

M  +6    -0    core/utilities/facemanagement/threads/facepipeline.h
M  +33   -0    core/utilities/facemanagement/threads/facepipelinepackage.cpp
M  +10   -9    core/utilities/facemanagement/threads/facepipelinepackage.h
M  +10   -3    core/utilities/facemanagement/threads/facepreviewloader.cpp
M  +7    -0    core/utilities/facemanagement/threads/facepreviewloader.h
M  +6    -0    core/utilities/facemanagement/threads/parallelpipes.h
M  +16   -12   core/utilities/facemanagement/threads/scanstatefilter.cpp
M  +7    -0    core/utilities/facemanagement/threads/scanstatefilter.h

https://invent.kde.org/graphics/digikam/commit/cd1b6f82957813746a3f44a0251c1decab4790d7
Comment 191 caulier.gilles 2020-09-24 02:04:33 UTC
Git commit 04ca7557c15c4c7ce2cca005d054301d6d192562 by Gilles Caulier.
Committed on 24/09/2020 at 02:04.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes.
Add missing destructor

M  +6    -0    core/utilities/facemanagement/items/facegroup.h
M  +4    -0    core/utilities/facemanagement/items/faceitem.cpp
M  +7    -0    core/utilities/facemanagement/items/faceitem.h

https://invent.kde.org/graphics/digikam/commit/04ca7557c15c4c7ce2cca005d054301d6d192562
Comment 192 caulier.gilles 2020-09-24 02:15:58 UTC
Git commit 7ed7c3ae44b23e1f5f5da0fe7bf195bf2b325fe1 by Gilles Caulier.
Committed on 24/09/2020 at 02:15.
Pushed by cgilles into branch 'master'.

We can disable copy constructor and equality operator with these classes.
Add missing destructor

M  +11   -7    core/utilities/facemanagement/database/faceitemretriever.cpp
M  +2    -0    core/utilities/facemanagement/database/faceitemretriever.h
M  +19   -18   core/utilities/facemanagement/database/faceutils.cpp
M  +6    -0    core/utilities/facemanagement/database/faceutils.h

https://invent.kde.org/graphics/digikam/commit/7ed7c3ae44b23e1f5f5da0fe7bf195bf2b325fe1
Comment 193 caulier.gilles 2020-09-24 08:47:48 UTC
Maik,

I experiment the clang-tidy "linter" like analyzer with digiKam code and i found this report :

clang-tidy -header-filter=^/home/gilles/Documents/GIT/7.x/build/.* -p=/home/gilles/Documents/GIT/7.x/build /home/gilles/Documents/GIT/7.x/core/libs/threads/workerobject.cpp
/home/gilles/Documents/GIT/7.x/core/libs/threads/workerobject.cpp:247:5: warning: Call to virtual function during destruction [clang-analyzer-optin.cplusplus.VirtualCall]
    this->aboutToDeactivate();
    ^
/home/gilles/Documents/GIT/7.x/core/libs/threads/workerobject.cpp:73:5: note: This destructor of an object of type '~WorkerObject' has not returned when the virtual method was called
    shutDown();
    ^
/home/gilles/Documents/GIT/7.x/core/libs/threads/workerobject.cpp:73:5: note: Calling 'WorkerObject::shutDown'
/home/gilles/Documents/GIT/7.x/core/libs/threads/workerobject.cpp:85:5: note: Calling 'WorkerObject::deactivate'
    deactivate(PhaseOut);
    ^
/home/gilles/Documents/GIT/7.x/core/libs/threads/workerobject.cpp:232:9: note: Control jumps to 'case Running:'  at line 235
        switch (d->state)
        ^
/home/gilles/Documents/GIT/7.x/core/libs/threads/workerobject.cpp:237:17: note:  Execution continues on line 213
                break;
                ^
/home/gilles/Documents/GIT/7.x/core/libs/threads/workerobject.cpp:247:5: note: Call to virtual function during destruction
    this->aboutToDeactivate();
    ^

More info :

https://clang.llvm.org/extra/clang-tidy/

Gilles
Comment 194 Maik Qualmann 2020-09-24 11:17:37 UTC
Yes, calling virtual functions in the constructor / destructor can cause problems. I also think that "this->aboutToDeactivate()" now never calls the derived class through "this". I'm testing it out tonight. I think we can mark the derived class with the "final" keyword to prevent problems.

Maik
Comment 195 caulier.gilles 2020-09-24 12:04:41 UTC
Right Maik,

I fixed some this-> entry in source code in the pass after a large cppcheck report about dynamic binding problem. This one is a fix by me.

You are right, instead to use this->, we must use final-> c++11 keyword.

Note : all dynamic binding fixed are annotated with a "NOTE: use dynamic binding" comment in source code. It's easy to double check all entries to see if this-> or final-> must be used at these places (32 entries).

grep -r "NOTE: use dynamic" core/*

../../core/app/date/ddateedit.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/app/date/ddateedit.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/app/date/ddateedit.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/app/items/views/digikamitemview.cpp:    // --- NOTE: use dynamic binding as slotSetupChanged() is a virtual method which can be re-implemented in derived classes.
../../core/app/items/thumbbar/itemthumbnailbar.cpp:    // --- NOTE: use dynamic binding as slotSetupChanged() is a virtual slot which can be re-implemented in derived classes.
../../core/dplugins/rawimport/native/rawpostprocessing.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/libs/models/abstractalbummodel.cpp:    // --- NOTE: use dynamic binding as all slots above are virtual methods which can be re-implemented in derived classes.
../../core/libs/database/models/itemmodel.cpp:    // --- NOTE: use dynamic binding as slotImageChange() is a virtual slot which can be re-implemented in derived classes.
../../core/libs/database/models/itemmodel.cpp:    // --- NOTE: use dynamic binding as slotImageTagChange() is a virtual slot which can be re-implemented in derived classes.
../../core/libs/dimg/filters/dimgthreadedfilter.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/libs/dimg/filters/raw/rawprocessingfilter.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/libs/dimg/filters/greycstoration/greycstorationfilter.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/libs/dimg/filters/greycstoration/greycstorationfilter.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/libs/widgets/itemview/itemviewcategorized.cpp:    // --- NOTE: use dynamic binding as slots below are virtual methods which can be re-implemented in derived classes.
../../core/libs/widgets/layout/sidebar.cpp:    // --- NOTE: use dynamic binding as slotClicked() is a virtual method which can be re-implemented in derived classes.
../../core/libs/widgets/metadata/subjectwidget.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/libs/widgets/metadata/subjectwidget.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/libs/properties/itempropertiessidebar.cpp:    // --- NOTE: use dynamic binding as slotChangedTab() is a virtual method which can be re-implemented in derived classes.
../../core/libs/properties/import/importitempropertiessidebar.cpp:    // --- NOTE: use dynamic binding as slotChangedTab() is a virtual method which can be re-implemented in derived classes.
../../core/libs/album/widgets/albumselectcombobox.cpp:    // --- NOTE: use dynamic binding as updateText() is a virtual slot which can be re-implemented in derived classes.
../../core/libs/dialogs/infodlg.cpp:    // --- NOTE: use dynamic binding as slotCopy2ClipBoard() is a virtual slot which can be re-implemented in derived classes.
../../core/libs/metadataengine/engine/metaengine.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/libs/threads/workerobject.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/showfoto/thumbbar/showfotothumbnailbar.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/utilities/maintenance/maintenancetool.cpp:    // --- NOTE: use dynamic binding as slotCancel() is a virtual method which can be re-implemented in derived classes.
../../core/utilities/imageeditor/editor/editortool.cpp:    // --- NOTE: use dynamic binding as slotPreview() is a virtual method which can be re-implemented in derived classes.
../../core/utilities/setup/metadata/namespaceeditdlg.cpp:    // --- NOTE: use dynamic binding as slots below are virtual method which can be re-implemented in derived classes.
../../core/utilities/import/views/importiconview.cpp:    // --- NOTE: use dynamic binding as slotSetupChanged() is a virtual method which can be re-implemented in derived classes.
../../core/utilities/import/views/importthumbnailbar.cpp:    // --- NOTE: use dynamic binding as slotSetupChanged() is a virtual method which can be re-implemented in derived classes.
../../core/utilities/geolocation/geoiface/tiles/abstractmarkertiler.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.
../../core/utilities/geolocation/geoiface/bookmark/bookmarksmenu.cpp:    // --- NOTE: use dynamic binding as slotAboutToShow() is a virtual method which can be re-implemented in derived classes.
../../core/utilities/geolocation/geoiface/bookmark/bookmarksmenu.cpp:    // NOTE: use dynamic binding as this virtual method can be re-implemented in derived classes.

Gilles
Comment 196 Maik Qualmann 2020-09-24 18:57:30 UTC
I specifically debugged this WorkerObject class. Even if the analysis tools report an error here, everything in the order of destruction is OK. Right, the derived class is destroyed first. But then the "aboutToDeactivate()" function falls back to the base class and is then called during the destruction. Since the function is not implemented as a pure virtual function, everything is OK.

The "this" pointer has no influence on the derived or basis class execution.

Maik
Comment 197 caulier.gilles 2020-09-25 05:20:15 UTC
Git commit c8721ba35c4b99cfed8696d2f7a01f2a8a66bac2 by Gilles Caulier.
Committed on 25/09/2020 at 03:42.
Pushed by cgilles into branch 'master'.

Face engine: remove dead codes aka Eigen face recognition and Fisher face recognition algorithms.

M  +3    -20   core/libs/facesengine/CMakeLists.txt
M  +1    -1    core/libs/facesengine/TODO
M  +0    -28   core/libs/facesengine/facedb/facedb.h
D  +0    -204  core/libs/facesengine/facedb/facedb_eigen.cpp
D  +0    -87   core/libs/facesengine/facedb/facedb_fisher.cpp
M  +0    -8    core/libs/facesengine/facedb/facedb_p.h
M  +0    -3    core/libs/facesengine/recognition/README
M  +0    -6    core/libs/facesengine/recognition/facialrecognition_wrapper_p.cpp
M  +0    -13   core/libs/facesengine/recognition/facialrecognition_wrapper_p.h
M  +0    -8    core/libs/facesengine/recognition/facialrecognition_wrapper_training.cpp
D  +0    -187  core/libs/facesengine/recognition/opencv-eigenfaces/eigenfacemodel.cpp
D  +0    -107  core/libs/facesengine/recognition/opencv-eigenfaces/eigenfacemodel.h
D  +0    -253  core/libs/facesengine/recognition/opencv-eigenfaces/facerec_eigenborrowed.cpp
D  +0    -143  core/libs/facesengine/recognition/opencv-eigenfaces/facerec_eigenborrowed.h
D  +0    -201  core/libs/facesengine/recognition/opencv-eigenfaces/opencveigenfacerecognizer.cpp
D  +0    -91   core/libs/facesengine/recognition/opencv-eigenfaces/opencveigenfacerecognizer.h
D  +0    -311  core/libs/facesengine/recognition/opencv-fisherfaces/facerec_fisherborrowed.cpp
D  +0    -143  core/libs/facesengine/recognition/opencv-fisherfaces/facerec_fisherborrowed.h
D  +0    -184  core/libs/facesengine/recognition/opencv-fisherfaces/fisherfacemodel.cpp
D  +0    -104  core/libs/facesengine/recognition/opencv-fisherfaces/fisherfacemodel.h
D  +0    -197  core/libs/facesengine/recognition/opencv-fisherfaces/opencvfisherfacerecognizer.cpp
D  +0    -91   core/libs/facesengine/recognition/opencv-fisherfaces/opencvfisherfacerecognizer.h
R  +0    -0    core/libs/facesengine/recognition/opencv-lbph/opencv_face.cpp [from: core/libs/facesengine/recognition/opencv-face/opencv_face.cpp - 100% similarity]
R  +0    -0    core/libs/facesengine/recognition/opencv-lbph/opencv_face.h [from: core/libs/facesengine/recognition/opencv-face/opencv_face.h - 100% similarity]

https://invent.kde.org/graphics/digikam/commit/c8721ba35c4b99cfed8696d2f7a01f2a8a66bac2
Comment 198 caulier.gilles 2020-09-25 05:20:39 UTC
Git commit 65c9aa3d4fa6eea92409f786a0ba786332777682 by Gilles Caulier.
Committed on 25/09/2020 at 04:05.
Pushed by cgilles into branch 'master'.

Face Engine: remove dead code aka flandmarkaligner algorithm

M  +0    -2    core/libs/facesengine/CMakeLists.txt
D  +0    -126  core/libs/facesengine/alignment/flandmark/flandmarkaligner.cpp
D  +0    -61   core/libs/facesengine/alignment/flandmark/flandmarkaligner.h

https://invent.kde.org/graphics/digikam/commit/65c9aa3d4fa6eea92409f786a0ba786332777682
Comment 199 caulier.gilles 2020-09-25 05:20:58 UTC
Git commit 880c377630be3393779185e1d1297e4957ff595b by Gilles Caulier.
Committed on 25/09/2020 at 04:15.
Pushed by cgilles into branch 'master'.

Face Engine: move dead code not exported and only used in an experimental preprocessing CLI test tool.

M  +0    -4    core/libs/facesengine/CMakeLists.txt
M  +6    -1    core/tests/facesengine/CMakeLists.txt
R  +2    -5    core/tests/facesengine/preprocess/preprocess.cpp [from: core/tests/facesengine/preprocess.cpp - 095% similarity]
R  +0    -0    core/tests/facesengine/preprocess/tantriggspreprocessor.cpp [from: core/libs/facesengine/preprocessing/tantriggs/tantriggspreprocessor.cpp - 100% similarity]
R  +0    -0    core/tests/facesengine/preprocess/tantriggspreprocessor.h [from: core/libs/facesengine/preprocessing/tantriggs/tantriggspreprocessor.h - 100% similarity]

https://invent.kde.org/graphics/digikam/commit/880c377630be3393779185e1d1297e4957ff595b
Comment 200 caulier.gilles 2020-09-25 05:39:53 UTC
Git commit b6a240f95754eaa188f3aa97c59fdbd9785daa91 by Gilles Caulier.
Committed on 25/09/2020 at 05:38.
Pushed by cgilles into branch 'master'.

Face Engine: remove dead code stage 2: Switch definitively to pure DNN detection and recognition.
Remove all OpenCV/LBPH based algorithms.

M  +19   -61   core/libs/facesengine/CMakeLists.txt
M  +1    -1    core/libs/facesengine/TODO
M  +0    -1    core/libs/facesengine/detection/README
M  +6    -152  core/libs/facesengine/detection/facedetector.cpp
M  +1    -0    core/libs/facesengine/detection/opencv-dnn/dnnfacedetectorbase.h
D  +0    -658  core/libs/facesengine/detection/opencv-face/opencvfacedetector.cpp
D  +0    -128  core/libs/facesengine/detection/opencv-face/opencvfacedetector.h
D  +0    -194  core/libs/facesengine/detection/opencv-face/opencvfacedetector_p.cpp
D  +0    -170  core/libs/facesengine/detection/opencv-face/opencvfacedetector_p.h
M  +0    -19   core/libs/facesengine/facedb/facedb.h
D  +0    -245  core/libs/facesengine/facedb/facedb_lbph.cpp
M  +1    -6    core/libs/facesengine/facedb/facedb_p.h
M  +0    -11   core/libs/facesengine/facedb/facedbschemaupdater.cpp
M  +0    -1    core/libs/facesengine/recognition/README
M  +0    -8    core/libs/facesengine/recognition/facialrecognition_wrapper_p.cpp
M  +1    -16   core/libs/facesengine/recognition/facialrecognition_wrapper_p.h
M  +0    -18   core/libs/facesengine/recognition/facialrecognition_wrapper_setup.cpp
M  +0    -9    core/libs/facesengine/recognition/facialrecognition_wrapper_training.cpp
D  +0    -553  core/libs/facesengine/recognition/opencv-lbph/facerec_borrowed.cpp
D  +0    -187  core/libs/facesengine/recognition/opencv-lbph/facerec_borrowed.h
D  +0    -230  core/libs/facesengine/recognition/opencv-lbph/lbphfacemodel.cpp
D  +0    -112  core/libs/facesengine/recognition/opencv-lbph/lbphfacemodel.h
D  +0    -204  core/libs/facesengine/recognition/opencv-lbph/opencv_face.cpp
D  +0    -439  core/libs/facesengine/recognition/opencv-lbph/opencv_face.h
D  +0    -218  core/libs/facesengine/recognition/opencv-lbph/opencvlbphfacerecognizer.cpp
D  +0    -98   core/libs/facesengine/recognition/opencv-lbph/opencvlbphfacerecognizer.h

https://invent.kde.org/graphics/digikam/commit/b6a240f95754eaa188f3aa97c59fdbd9785daa91
Comment 201 caulier.gilles 2020-09-25 05:55:56 UTC
Marcel,

I cleaned all dead code in face engine, typically all legacy algorithms not based on DNN.

I'm not sure if this will have an effect at run time, but at least, now the implementations are more simpler as now more pre-processors puzzle are included in codes. This will definitively simplify the code maintenance in the future.

This want mean that, after 10 years of development on face engine, we switch definitively to DNN for detection and recognition (as expected of course)...

Gilles
Comment 202 Marcel 2020-09-25 09:19:48 UTC
Gilles, 

thank you for your effort. I will test the code now again. Should I run Digikam with the address sanitizer enabled? 

In my opinion we could close this huge bug and open new bugs for new problems, is that all right?

I followed all of your code changes concerning this bug. In my opinion there is one thing that could increase the readability. You often completed the class definition to match the rule of 3. See https://en.cppreference.com/w/cpp/language/rule_of_three. Why didn't you defined the constructors in a c++11 way? Instead of making it private you could use delete or instead of implement an empty constructor/destructor, default could be used. Or did I miss a technical detail about that?
Comment 203 caulier.gilles 2020-09-26 06:50:10 UTC
Git commit 3bcd5d8a28bf10acd0710805b2fa5f8ed2ad42a2 by Gilles Caulier.
Committed on 26/09/2020 at 06:46.
Pushed by cgilles into branch 'master'.

FacesEngine: cleanup stage 3 : move usused FunnelReal class to tests suite as it's not used in core implementation
Move data model for FunnelReal algorithm to tests suite.
We will deal later if this code can be removed definitively.
More cmake scripts cleanup for faces engine.
move faces engine tests suite to dedicated subdirectories

M  +0    -21   core/data/facesengine/CMakeLists.txt
D  +0    -3    core/data/facesengine/alignment/README
D  +-    --    core/data/facesengine/alignment/flandmark_model.dat.gz
M  +4    -13   core/libs/facesengine/CMakeLists.txt
M  +3    -56   core/tests/facesengine/CMakeLists.txt
A  +33   -0    core/tests/facesengine/alignment/CMakeLists.txt
A  +1    -0    core/tests/facesengine/alignment/README
R  +0    -3    core/tests/facesengine/alignment/align.cpp [from: core/tests/facesengine/align.cpp - 099% similarity]
R  +-    --    core/tests/facesengine/alignment/face-funnel.data.tgz [from: core/data/facesengine/alignment/face-funnel.data.tgz - 100% similarity]
R  +4    -1    core/tests/facesengine/alignment/funnelreal.cpp [from: core/libs/facesengine/alignment/congealing/funnelreal.cpp - 099% similarity]
R  +0    -0    core/tests/facesengine/alignment/funnelreal.h [from: core/libs/facesengine/alignment/congealing/funnelreal.h - 100% similarity]
A  +32   -0    core/tests/facesengine/demo/CMakeLists.txt
A  +27   -0    core/tests/facesengine/preprocess/CMakeLists.txt

https://invent.kde.org/graphics/digikam/commit/3bcd5d8a28bf10acd0710805b2fa5f8ed2ad42a2
Comment 204 caulier.gilles 2020-09-26 07:07:44 UTC
Hi Marcel,

Sorry for my late response but i'm very busy in my office.

>Should I run Digikam with the address sanitizer enabled? 

Both. First without, and in second if it continue to crash, with ASAN.

>In my opinion we could close this huge bug and open new bugs for new problems, >is that all right?

Not yet. Memory leak is fixed but a crash still here typically with PreviewEngine. Maik work on it.

>Why didn't you defined the constructors in a c++11 way? Instead of making it private you could use delete or instead of implement an empty constructor/destructor, default could be used. Or did I miss a technical detail about that?

You don't miss anything. Both method are valid, but :

1/ Other classes use C++0x implementation to disable constructors/destructor/operators. We need to still homogeneous everywhere as amount of source code is really huge. If we differ, this will become the hel to maintain.

2/ Don't forget MSVC : this compiler is a shit. C++ support is a pain, sure better with MSVC 2019, but M$ is so far than G++ and Clang to support standard. If you look into MSVC 2019, there is a plugin to use Clang instead native M$ compiler. In fact M$ continue to introduce more and more and more Open Source tools/codes/modules in Windows stuff. What's the next stage ? replace Windows kernel by Linux kernel ???
In fact KDE CI still to use MSVC 2017. So we need to not support totally C++11 yet.

3/ I'm not against whole C++11 standard at all, but some keywords as auto make code opaque and do not help in readbility. Also, why the compiler will automatically cast the target class with auto in a better way that the developper ??? I dislike also the labda method which make the code as a puzzle, as Javascript (excepted for some use where Lambda method can help to improve bianry compatibility but this must not generalized at all).

4/ Clang-tidy is a nice CLI tool to patch automatically all C++0x to C++11. I see an higher activity in Exiv2 project to port all codes to C++11, and clang-tidy is used in background. We will do the same later, but a specific release must be done only for this kind of changes to check for regressions.

Gilles
Comment 205 caulier.gilles 2020-09-26 08:04:52 UTC
Git commit a2b9c9b54eacdc0b5fc66b4b740d4f5b629e9556 by Gilles Caulier.
Committed on 26/09/2020 at 08:03.
Pushed by cgilles into branch 'master'.

Faces Engine stage 4: remove another legacy unused class OpenCVMatData

M  +0    -1    core/libs/facesengine/CMakeLists.txt
D  +0    -82   core/libs/facesengine/common/opencvmatdata.cpp
D  +0    -65   core/libs/facesengine/common/opencvmatdata.h
M  +1    -1    core/libs/facesengine/facedb/facedb.h

https://invent.kde.org/graphics/digikam/commit/a2b9c9b54eacdc0b5fc66b4b740d4f5b629e9556
Comment 206 Marcel 2020-09-26 10:14:05 UTC
Gilles, thank you very much for this detailed explanation. I really appreciate that. So it definitely makes sense as it is :-)

@Maik, does it help to upload some resent backtraces and address sanitizer errors or do you have enough information for debuging? Can you reproduce the crashes?
Comment 207 caulier.gilles 2020-09-27 10:59:21 UTC
Marcel,

Don't forget to install at least asan and lsan devel packages. First one is to check address in uses sanitizer and second one is memory leak sanitizer.

Gilles
Comment 208 Marcel 2020-09-28 14:36:21 UTC
Created attachment 131985 [details]
Again address Sanitizer error on PreviewLoadingTask

I tested the face detection feature again. This time with YOLO and clang compiler on 6c5b6ba2f989591aa1913d08ba4a82576aec0721. I get the same error with gcc. This time the error occurs again after a few minutes of scanning. Isn't it in the part that Maik already fixed?

@Gilles. I sent you a private message because it does not has anything to do with this bug.
Comment 209 Maik Qualmann 2020-09-28 19:02:35 UTC
Git commit 85c6c8c4585bcf3e7e779daaf9b17bfa386fab1a by Maik Qualmann.
Committed on 28/09/2020 at 19:01.
Pushed by mqualmann into branch 'master'.

we switch to a QHash for testing

M  +2    -2    core/libs/threadimageio/fileio/loadingcache.cpp

https://invent.kde.org/graphics/digikam/commit/85c6c8c4585bcf3e7e779daaf9b17bfa386fab1a
Comment 210 caulier.gilles 2020-09-29 06:17:22 UTC
Created attachment 132002 [details]
clang tidy patch for override fixes in theadimageio

Generated following this note:

https://github.com/KratosMultiphysics/Kratos/wiki/How-to-use-Clang-Tidy-to-automatically-correct-code
Comment 211 caulier.gilles 2020-09-29 06:19:19 UTC
Created attachment 132003 [details]
clang tidy patch for override fixes in theads

Maik, please review the 2 patches generated by cland-tidy modernize C++ python script.

Gilles
Comment 212 Marcel 2020-09-29 06:29:41 UTC
Feedback about patch:

only one override
+    ~ActionJob() override override;
+    ~ActionThreadBase() override override;
+    ~DynamicThread() override override;
+    ~ThreadManager() override override;
+    ~WorkerObject() override override;

After my knowledge it does the same as before.
Comment 213 caulier.gilles 2020-09-29 08:11:47 UTC
Hi Marcel,

Yes i seen too the double "override" keyword. Why clang-tidy do that, i don't know. This is why it's very important to review patch created by non human.

Gilles
Comment 215 Maik Qualmann 2020-09-29 17:32:33 UTC
Git commit e73d3b37b565081d6768cc22f75464befa7e4ded by Maik Qualmann.
Committed on 29/09/2020 at 17:31.
Pushed by mqualmann into branch 'master'.

apply the clang tidy patches

M  +1    -1    core/libs/threadimageio/engine/filereadwritelock.h
M  +1    -1    core/libs/threadimageio/engine/managedloadsavethread.h
M  +1    -1    core/libs/threadimageio/engine/sharedloadsavethread.h
M  +2    -2    core/libs/threadimageio/fileio/loadingcache.h
M  +1    -1    core/libs/threadimageio/fileio/loadsavetask.h
M  +10   -10   core/libs/threadimageio/fileio/loadsavethread.h
M  +1    -1    core/libs/threadimageio/preview/previewloadthread.h
M  +1    -1    core/libs/threadimageio/preview/previewtask.h
M  +3    -3    core/libs/threadimageio/thumb/thumbnailloadthread.h
M  +3    -3    core/libs/threads/actionthreadbase.h
M  +1    -1    core/libs/threads/dynamicthread.cpp
M  +1    -1    core/libs/threads/dynamicthread.h
M  +6    -6    core/libs/threads/parallelworkers.h
M  +3    -3    core/libs/threads/threadmanager.cpp
M  +1    -1    core/libs/threads/threadmanager.h
M  +2    -2    core/libs/threads/workerobject.h

https://invent.kde.org/graphics/digikam/commit/e73d3b37b565081d6768cc22f75464befa7e4ded
Comment 216 caulier.gilles 2020-09-30 03:42:49 UTC
Git commit e331ff2d3241ce4fec690ddd67a7ffbff1ac587c by Gilles Caulier.
Committed on 30/09/2020 at 03:40.
Pushed by cgilles into branch 'master'.

apply clang tidy override modernize C++11 patch

M  +4    -4    core/libs/facesengine/detection/opencv-dnn/dnnfacedetectorssd.h
M  +4    -4    core/libs/facesengine/detection/opencv-dnn/dnnfacedetectoryolo.h
M  +1    -1    core/libs/facesengine/facedb/facedbbackend.h
M  +2    -2    core/libs/facesengine/preprocessing/recognition/recognitionpreprocessor.h
M  +11   -11   core/libs/facesengine/recognition/dataproviders.h
M  +0    -1    core/libs/facesengine/recognition/facialrecognition_wrapper_p.h
M  +2    -2    core/libs/facesengine/recognition/opencv-dnn/opencvdnnfacerecognizer_p.h
M  +3    -2    core/libs/facesengine/recognition/recognitiontrainingprovider.h

https://invent.kde.org/graphics/digikam/commit/e331ff2d3241ce4fec690ddd67a7ffbff1ac587c
Comment 217 caulier.gilles 2020-09-30 03:43:40 UTC
Git commit edbd8d5d2f401f6c4bbda134749fa092d76b565e by Gilles Caulier.
Committed on 30/09/2020 at 03:43.
Pushed by cgilles into branch 'master'.

apply clang tidy override modernize C++11 patch

M  +4    -4    core/utilities/facemanagement/database/faceutils.h
M  +1    -1    core/utilities/facemanagement/items/facegroup.h
M  +1    -1    core/utilities/facemanagement/items/faceitem.h
M  +1    -1    core/utilities/facemanagement/threads/facepipeline.h
M  +1    -1    core/utilities/facemanagement/threads/facepreviewloader.h
M  +1    -1    core/utilities/facemanagement/threads/parallelpipes.h
M  +2    -2    core/utilities/facemanagement/threads/scanstatefilter.h
M  +1    -1    core/utilities/facemanagement/widgets/assignnamewidget.h
M  +1    -1    core/utilities/facemanagement/widgets/assignnamewidgetstates.h
M  +1    -1    core/utilities/facemanagement/widgets/facemanagementhelpdlg.h
M  +3    -3    core/utilities/facemanagement/widgets/facescanwidget.h
M  +1    -1    core/utilities/facemanagement/workers/databasewriter.h
M  +1    -1    core/utilities/facemanagement/workers/detectionworker.h
M  +2    -2    core/utilities/facemanagement/workers/recognitionworker.h
M  +2    -2    core/utilities/facemanagement/workers/trainerworker.cpp
M  +2    -2    core/utilities/facemanagement/workers/trainerworker.h

https://invent.kde.org/graphics/digikam/commit/edbd8d5d2f401f6c4bbda134749fa092d76b565e
Comment 218 Maik Qualmann 2020-09-30 06:05:12 UTC
Git commit a296fd327a98a5d5ffbf6690cec4eb5b85de0330 by Maik Qualmann.
Committed on 30/09/2020 at 06:04.
Pushed by mqualmann into branch 'master'.

drastic change to the image cache lock

M  +0    -1    core/libs/database/utils/scan/scancontroller_start.cpp
M  +48   -6    core/libs/threadimageio/fileio/loadingcache.cpp
M  +2    -0    core/libs/threadimageio/fileio/loadingcache.h
M  +1    -6    core/libs/threadimageio/fileio/loadingcacheinterface.cpp
M  +71   -72   core/libs/threadimageio/fileio/loadsavetask.cpp
M  +108  -112  core/libs/threadimageio/preview/previewtask.cpp
M  +8    -16   core/libs/threadimageio/thumb/thumbnailloadthread.cpp
M  +4    -8    core/libs/threadimageio/thumb/thumbnailloadthread_p.cpp
M  +52   -53   core/libs/threadimageio/thumb/thumbnailtask.cpp

https://invent.kde.org/graphics/digikam/commit/a296fd327a98a5d5ffbf6690cec4eb5b85de0330
Comment 219 Maik Qualmann 2020-09-30 06:06:23 UTC
Hi Marcel,
it would be good if you could test the last change.

Maik
Comment 220 Maik Qualmann 2020-09-30 18:11:30 UTC
Git commit 84779e8dca5c96503a77a127874a0f13b37894d4 by Maik Qualmann.
Committed on 30/09/2020 at 18:10.
Pushed by mqualmann into branch 'master'.

do not return a pointer from QCache, it can be deleted

M  +18   -4    core/libs/threadimageio/fileio/loadingcache.cpp
M  +2    -2    core/libs/threadimageio/fileio/loadingcache.h
M  +8    -14   core/libs/threadimageio/fileio/loadsavetask.cpp
M  +6    -12   core/libs/threadimageio/preview/previewtask.cpp
M  +2    -6    core/libs/threadimageio/thumb/thumbnailtask.cpp

https://invent.kde.org/graphics/digikam/commit/84779e8dca5c96503a77a127874a0f13b37894d4
Comment 221 Marcel 2020-09-30 19:54:07 UTC
Maik, I tried it with your commit https://invent.kde.org/graphics/digikam/-/commit/a296fd327a98a5d5ffbf6690cec4eb5b85de0330 but it again crashed after a few minutes. 
Digikam::LoadingCache::notifyNewLoadingProcess(Digikam::LoadingProcess*, Digikam::LoadingDescription const&) /home/marcel/Repos/digikam/core/libs/threadimageio/fileio/loadingcache.cpp:293

Now I'm on https://invent.kde.org/graphics/digikam/-/commit/a0157f8d2bcc6dfb33e74518b546c1553a864ae2 and it works until now (more than 50 minutes). I will keep it running during the night.
Comment 222 Marcel 2020-09-30 20:23:14 UTC
No look, it still crashes, at least with asan enabled.
Comment 223 Maik Qualmann 2020-09-30 20:45:07 UTC
Git commit 92cfeba3a750ba95b4665a4c2e3e8ccfe0dee627 by Maik Qualmann.
Committed on 30/09/2020 at 20:44.
Pushed by mqualmann into branch 'master'.

remove only one listener

M  +1    -1    core/libs/threadimageio/fileio/loadsavetask.cpp

https://invent.kde.org/graphics/digikam/commit/92cfeba3a750ba95b4665a4c2e3e8ccfe0dee627
Comment 224 Maik Qualmann 2020-09-30 20:48:57 UTC
Git commit 0e2cc57c29f218cb0e9002d0b852aca4c0e25bf2 by Maik Qualmann.
Committed on 30/09/2020 at 20:48.
Pushed by mqualmann into branch 'master'.

use QMap again

M  +12   -12   core/libs/threadimageio/fileio/loadingcache.cpp

https://invent.kde.org/graphics/digikam/commit/0e2cc57c29f218cb0e9002d0b852aca4c0e25bf2
Comment 225 Maik Qualmann 2020-10-01 10:39:40 UTC
Git commit e2be75c6cf36a6a8252a353679b2a871103d6116 by Maik Qualmann.
Committed on 01/10/2020 at 10:39.
Pushed by mqualmann into branch 'master'.

after revert to the old code, we test this small change

M  +11   -11   core/libs/threadimageio/fileio/loadingcache.cpp
M  +1    -1    core/libs/threadimageio/fileio/loadsavetask.cpp

https://invent.kde.org/graphics/digikam/commit/e2be75c6cf36a6a8252a353679b2a871103d6116
Comment 226 Marcel 2020-10-01 16:01:36 UTC
Maik, the version now looks very promising. It already works for hours without a crash. I saw you reverted most changes (even the typo fix :-) ). In your commit message are you writing  "test this small change". What did you change, that we did not test so far?
Comment 227 caulier.gilles 2020-10-01 18:46:06 UTC
Created attachment 132054 [details]
threadimageio clang-tidy patch about pass by value

Maik, 

clang-tidy provide also fixes about pass by value arguments of methods.
This also include to use std::move constructor if necessary.

Look well the description here:

https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html

Gilles
Comment 228 Maik Qualmann 2020-10-01 20:45:19 UTC
I'll have to read it through tomorrow, the patch looks strange to me...

Maik
Comment 229 Marcel 2020-10-02 05:47:00 UTC
Created attachment 132064 [details]
asan error in CPGFMemoryStream

Maik, the test run for a long time. I original error did not occur. Instead this one was shown after hours. What is now the change? Is it now just the removeOne() function?

@Gilles. In my work place we also had discussions about this clang-tidy rule. Technically it can help in a few places to decrease the move effort. But we came up with the conclusion that the readability is lower and its error prune when copying from such a place. Therefore we decided to not to use this rule. 
Anyway, in my opinion there are other rules far more important for Digikam than this one. On my work place it turned out that a good way to go is:
- Activate new rule and auto fix it
- Most autofix problems turned out to be compiler errors es well. Fix them.
- After applying, look for a compiler flag doing the same (e.g override) and apply it and treat warnings as errors 

With this way you also find the false positive and ensure that nobody in the future does it wrong again.

If you like I could spend some time with that and send you a merge request...
Comment 230 Maik Qualmann 2020-10-02 18:54:51 UTC
The problem is clear, memory was created with new. It is necessary to increase the memory, now realloc() is used. We should report this to libpgf as a bug. I will patch our internal libpgf.

Maik
Comment 231 Maik Qualmann 2020-10-02 19:27:03 UTC
Git commit 28697b87b1ea439425711eec490df15ab8e089e9 by Maik Qualmann.
Committed on 02/10/2020 at 19:24.
Pushed by mqualmann into branch 'master'.

fix internal libpgf, do not use realloc() when memory was allocated with new

M  +3    -1    core/libs/pgfutils/libpgf/PGFstream.cpp

https://invent.kde.org/graphics/digikam/commit/28697b87b1ea439425711eec490df15ab8e089e9
Comment 232 Maik Qualmann 2020-10-03 05:21:38 UTC
Git commit 4f16275e2d2c19b59f991d087e98af707daf0ae8 by Maik Qualmann.
Committed on 03/10/2020 at 05:20.
Pushed by mqualmann into branch 'master'.

we can optimize this due to changes in setStatus()

M  +4    -22   core/libs/threadimageio/fileio/loadsavetask.cpp
M  +4    -22   core/libs/threadimageio/preview/previewtask.cpp
M  +4    -20   core/libs/threadimageio/thumb/thumbnailtask.cpp

https://invent.kde.org/graphics/digikam/commit/4f16275e2d2c19b59f991d087e98af707daf0ae8
Comment 233 Marcel 2020-10-03 07:30:55 UTC
The test runs now for 12 hours without a crash. Good job Maik and Gilles. Thank you for fixing that. I will continue the run to see if it finishes without a crash but I guess we can close this issue.
Comment 234 caulier.gilles 2020-10-03 08:00:17 UTC
Maik, 

About your fix in libpgf, well seen. This is a good candidate for an UPSTREAM report, but i think we can do more.

All static analyzers ignore libpgf errors and warnings. There is a rule in .krazy file to drop libpgf reports. As lipgf is a core components used for thumbnails engine, we need to check all major reports in this code.

Gilles
Comment 235 caulier.gilles 2020-10-03 08:03:39 UTC
Git commit 11bd6030db6497f4ecf3be05daa10c64743e8750 by Gilles Caulier.
Committed on 03/10/2020 at 08:02.
Pushed by cgilles into branch 'master'.

enable static analyzer reports for libpgf to check if major errors are present in code as memory leak

M  +1    -1    .krazy
M  +0    -1    project/reports/cppcheck.sh

https://invent.kde.org/graphics/digikam/commit/11bd6030db6497f4ecf3be05daa10c64743e8750
Comment 236 caulier.gilles 2020-10-03 18:17:09 UTC
Maik,

cppcheck and clang-scan reports now parse libpgf. Look the results:

https://www.digikam.org/reports/cppcheck/master/

https://www.digikam.org/reports/clang/master/

Gilles
Comment 237 caulier.gilles 2020-10-03 18:18:26 UTC
Maik,

about the clang-tidy patch for pass by value in constructor, you can forget it. It make the code less clear than before.

Gilles
Comment 238 Maik Qualmann 2020-10-03 18:38:12 UTC
The reported problems in libpgf are almost negligible. And yes, I've never seen anything like the clang-tidy patch for pass by value in a Qt project.

Maik
Comment 239 Maik Qualmann 2020-10-03 19:29:47 UTC
Ok, thank you for the long test. I will close this and other bugs. If you still have a problem, please open a new bug.

Maik
Comment 240 caulier.gilles 2020-10-03 21:19:37 UTC
I think we can plan to release the 7.2.0 beta1 soon.

It's fine for you?

Gilles
Comment 241 Maik Qualmann 2020-10-04 05:42:44 UTC
Yes, I agree.

Maik