Noise reduction crash

[romandocent]

RawTherapee crashes without error messages during the Luminance or Luminance - Detail changing (Noise reduction switched on). Crash happens randomly, not every time when I use this function. Firstly noticed in 4.2.976. In 4.2.1005 this bug still exists.

System: Ubuntu 16.04 LTS 64bit. Asus Zenbook UX303L, Intel Core i5 5200U, Intel HD Graphics 5500 + Nvidia 940M, 12 Gb ram, SSD OCZ Trion 150 240GB.

Processing files: Nikon NEF # #

[Beep6581]

http://rawpedia.rawtherapee.com/How_to_write_useful_bug_reports We need a stack backtrace and PP3.

[ClaesGitHub]

Stack backtrace of noise reduction crash enclosed.

Host: Windows 7 64-bit 12 gig RAM Intel i7 Guest Xubuntu Xfce 16.04 in Oracle M VirtualBox, 8192 8 gig RAM RT 4.2.1013 (debug), own compilation Backtrace, pp3 and RAF here: http://filebin.net/q4hchooolw

[heckflosse]

With that massive amount of RAM at least we can exclude 'out of memory' as a reason for the crash ;-)

[ClaesGitHub]

@Beep6581 You got it :-) First I tried under Win 7, but did not manage to get it to crash. 20 minutes of crash attempts under Xubuntu, however, were more fruitful. For backtrace &c, see two posts above.

[romandocent]

The same random crash happens when move the White Balance sliders.

[romandocent]

Version 4.2.1152. Still crashes randomly at noise reduction and white balance. ((

[Beep6581]

@romandocent still no stack backtrace from you.

[romandocent]

log.txt

Back trace about noise reduction crush done

[Floessie]

Backtrace is fine. Thanks.

[Floessie]

Okay, I had a first look: Both backtraces crash near the end of rtengine::Crop::update(), which is a lengthy function doing a lot of things. Most members are properly protected by cropMutex. Here is a patch to protect them all:

diff --git a/rtengine/dcrop.cc b/rtengine/dcrop.cc
index 577a3f4..ed95960 100644
--- a/rtengine/dcrop.cc
+++ b/rtengine/dcrop.cc
@@ -111,6 +111,12 @@ void Crop::setEditSubscriber(EditSubscriber* newSubscriber)
     // If oldSubscriber == NULL && newSubscriber != NULL && newSubscriber->getEditingType() == ET_PIPETTE-> the image will be allocated when necessary
 }

+bool Crop::hasListener()
+{
+    MyMutex::MyLock cropLock(cropMutex);
+    return cropImageListener;
+}
+
 void Crop::update (int todo)
 {
     MyMutex::MyLock cropLock(cropMutex);
@@ -1322,5 +1328,22 @@ void Crop::fullUpdate ()
     parent->updaterThreadStart.unlock ();
 }

+int Crop::get_skip()
+{
+    MyMutex::MyLock lock(cropMutex);
+    return skip;
 }

+int Crop::getLeftBorder()
+{
+    MyMutex::MyLock lock(cropMutex);
+    return leftBorder;
+}
+
+int Crop::getUpperBorder()
+{
+    MyMutex::MyLock lock(cropMutex);
+    return upperBorder;
+}
+
+}
diff --git a/rtengine/dcrop.h b/rtengine/dcrop.h
index f1230bf..c4398ae 100644
--- a/rtengine/dcrop.h
+++ b/rtengine/dcrop.h
@@ -57,19 +57,18 @@ protected:
     bool updating;         /// Flag telling if an updater thread is currently processing
     bool newUpdatePending; /// Flag telling the updater thread that a new update is pending
     int skip;
-    int padding;           /// Minimum space allowed around image in the display area
     int cropx, cropy, cropw, croph;         /// size of the detail crop image ('skip' taken into account), with border
     int trafx, trafy, trafw, trafh;         /// the size and position to get from the imagesource that is transformed to the requested crop area
     int rqcropx, rqcropy, rqcropw, rqcroph; /// size of the requested detail crop image (the image might be smaller) (without border)
-    int borderRequested;                    /// requested extra border size for image processing
+    const int borderRequested;              /// requested extra border size for image processing
     int upperBorder, leftBorder;            /// extra border size really allocated for image processing

     bool cropAllocated;
     DetailedCropListener* cropImageListener;

     MyMutex cropMutex;
-    ImProcCoordinator* parent;
-    bool isDetailWindow;
+    ImProcCoordinator* const parent;
+    const bool isDetailWindow;
     EditUniqueID getCurrEditID();
     bool setCropSizes (int cropX, int cropY, int cropW, int cropH, int skip, bool internal);
     void freeAll ();
@@ -78,19 +77,8 @@ public:
     Crop             (ImProcCoordinator* parent, EditDataProvider *editDataProvider, bool isDetailWindow);
     virtual ~Crop    ();

-    void mLock       ()
-    {
-        cropMutex.lock();
-    }
-    void mUnlock     ()
-    {
-        cropMutex.lock();
-    }
     void setEditSubscriber(EditSubscriber* newSubscriber);
-    bool hasListener ()
-    {
-        return cropImageListener;
-    }
+    bool hasListener();
     void update      (int todo);
     void setWindow   (int cropX, int cropY, int cropW, int cropH, int skip)
     {
@@ -106,22 +94,9 @@ public:

     void setListener    (DetailedCropListener* il);
     void destroy        ();
-    int  get_skip       ()
-    {
-        return skip;
-    }
-    int getPadding      ()
-    {
-        return padding;
-    }
-    int  getLeftBorder  ()
-    {
-        return leftBorder;
-    }
-    int  getUpperBorder ()
-    {
-        return upperBorder;
-    }
+    int get_skip();
+    int getLeftBorder();
+    int getUpperBorder();
 };
 }
 #endif

The interaction between tryUpdate() and fullUpdate() with a pair of non-atomic bools looks somewhat waggly but has been there for a long time.

I still can't reproduce the crash within minutes of testing. In fact, I've not yet seen it at all. :(

@romandocent @ClaesGitHub Could you give the patch above a try? I don't think it'll fix your problem, but who knows... Do you have any other hints to reproduce the crash more quickly?

@heckflosse Any idea how to proceed? Could again be a rare buffer overrun triggered by a race somewhere.

[romandocent]

How to use it? @Floessie

[heckflosse]

@Floessie No idea. I also can't reproduce the crash here :(

[Beep6581]

@romandocent do you compile RawTherapee yourself? Guide: http://rawpedia.rawtherapee.com/Linux To apply the patch, before you do the "Compile RawTherapee" step as described in the link above, assuming you are in the cloned repo as you should be at that point of the guide, run this:

wget https://filebin.net/1b6q87ouafqw538k/rt3306.patch
git apply rt3306.patch

Then proceed with compilation.

[romandocent]

Ok, I'll try, then write here if it helps. Thanks.

[romandocent]

@Beep6581 can you reupload patch? just have timeframe to test it, but "ERROR 410: Gone" when try wget.

[romandocent]

@Beep6581 ..and if I try just copy code above, paste it to created rt3306.patch file and then run git apply rt3306.patch, I get

/home/roman/repo-rt/rtengine/dcrop.cc: In constructor ‘rtengine::Crop::Crop(rtengine::ImProcCoordinator*, EditDataProvider*, bool)’:
/home/roman/repo-rt/rtengine/dcrop.cc:36:59: error: class ‘rtengine::Crop’ does not have any field named ‘padding’
       updating(false), newUpdatePending(false), skip(10), padding(0),
                                                           ^
rtengine/CMakeFiles/rtengine.dir/build.make:494: recipe for target 'rtengine/CMakeFiles/rtengine.dir/dcrop.cc.o' failed
make[2]: *** [rtengine/CMakeFiles/rtengine.dir/dcrop.cc.o] Error 1
make[2]: *** Waiting for unfinished jobs....
CMakeFiles/Makefile2:178: recipe for target 'rtengine/CMakeFiles/rtengine.dir/all' failed

[romandocent]

@Beep6581 ??

[Floessie]

@romandocent Had not expected to see you back so soon. :wink:

Here is an updated patch. Don't wait for RT6 to test it on.

diff --git a/rtengine/dcrop.cc b/rtengine/dcrop.cc
index e308ab1..8acf1ec 100644
--- a/rtengine/dcrop.cc
+++ b/rtengine/dcrop.cc
@@ -33,7 +33,7 @@ extern const Settings* settings;
 Crop::Crop (ImProcCoordinator* parent, EditDataProvider *editDataProvider, bool isDetailWindow)
     : PipetteBuffer(editDataProvider), origCrop(nullptr), laboCrop(nullptr), labnCrop(nullptr),
       cropImg(nullptr), cbuf_real(nullptr), cshmap(nullptr), transCrop(nullptr), cieCrop(nullptr), cbuffer(nullptr),
-      updating(false), newUpdatePending(false), skip(10), padding(0),
+      updating(false), newUpdatePending(false), skip(10),
       cropx(0), cropy(0), cropw(-1), croph(-1),
       trafx(0), trafy(0), trafw(-1), trafh(-1),
       rqcropx(0), rqcropy(0), rqcropw(-1), rqcroph(-1),
@@ -111,6 +111,12 @@ void Crop::setEditSubscriber(EditSubscriber* newSubscriber)
     // If oldSubscriber == NULL && newSubscriber != NULL && newSubscriber->getEditingType() == ET_PIPETTE-> the image will be allocated when necessary
 }

+bool Crop::hasListener()
+{
+    MyMutex::MyLock cropLock(cropMutex);
+    return cropImageListener;
+}
+
 void Crop::update (int todo)
 {
     MyMutex::MyLock cropLock(cropMutex);
@@ -1285,5 +1291,22 @@ void Crop::fullUpdate ()
     parent->updaterThreadStart.unlock ();
 }

+int Crop::get_skip()
+{
+    MyMutex::MyLock lock(cropMutex);
+    return skip;
 }

+int Crop::getLeftBorder()
+{
+    MyMutex::MyLock lock(cropMutex);
+    return leftBorder;
+}
+
+int Crop::getUpperBorder()
+{
+    MyMutex::MyLock lock(cropMutex);
+    return upperBorder;
+}
+
+}
diff --git a/rtengine/dcrop.h b/rtengine/dcrop.h
index 450c659..7472a4d 100644
--- a/rtengine/dcrop.h
+++ b/rtengine/dcrop.h
@@ -57,19 +57,18 @@ protected:
     bool updating;         /// Flag telling if an updater thread is currently processing
     bool newUpdatePending; /// Flag telling the updater thread that a new update is pending
     int skip;
-    int padding;           /// Minimum space allowed around image in the display area
     int cropx, cropy, cropw, croph;         /// size of the detail crop image ('skip' taken into account), with border
     int trafx, trafy, trafw, trafh;         /// the size and position to get from the imagesource that is transformed to the requested crop area
     int rqcropx, rqcropy, rqcropw, rqcroph; /// size of the requested detail crop image (the image might be smaller) (without border)
-    int borderRequested;                    /// requested extra border size for image processing
+    const int borderRequested;              /// requested extra border size for image processing
     int upperBorder, leftBorder;            /// extra border size really allocated for image processing

     bool cropAllocated;
     DetailedCropListener* cropImageListener;

     MyMutex cropMutex;
-    ImProcCoordinator* parent;
-    bool isDetailWindow;
+    ImProcCoordinator* const parent;
+    const bool isDetailWindow;
     EditUniqueID getCurrEditID();
     bool setCropSizes (int cropX, int cropY, int cropW, int cropH, int skip, bool internal);
     void freeAll ();
@@ -78,19 +77,8 @@ public:
     Crop             (ImProcCoordinator* parent, EditDataProvider *editDataProvider, bool isDetailWindow);
     virtual ~Crop    ();

-    void mLock       ()
-    {
-        cropMutex.lock();
-    }
-    void mUnlock     ()
-    {
-        cropMutex.lock();
-    }
     void setEditSubscriber(EditSubscriber* newSubscriber);
-    bool hasListener ()
-    {
-        return cropImageListener;
-    }
+    bool hasListener();
     void update      (int todo);
     void setWindow   (int cropX, int cropY, int cropW, int cropH, int skip)
     {
@@ -106,22 +94,9 @@ public:

     void setListener    (DetailedCropListener* il);
     void destroy        ();
-    int  get_skip       ()
-    {
-        return skip;
-    }
-    int getPadding      ()
-    {
-        return padding;
-    }
-    int  getLeftBorder  ()
-    {
-        return leftBorder;
-    }
-    int  getUpperBorder ()
-    {
-        return upperBorder;
-    }
+    int get_skip();
+    int getLeftBorder();
+    int getUpperBorder();
 };
 }
 #endif

[romandocent]

@Floessie @Beep6581 Looks like this patch solves the noise reduction crush bug. But WB Equalizers are still crushing randomly.

[Hombre57]

@Floessie Have you committed this patch ?

[Floessie]

@Hombre57 No, not yet. I assume, this should be done now. I'll do so directly on dev tonight.