Detail windows seem broken

[Beep6581]

The detail windows show a greatly pixelized image when I zoom in past about 300-400%. In the screenshot you can see that some detail windows already break at 400% zoom while others can still show the preview at 400% correctly.

Tested using a SONY SLT-A77V raw file from: https://filebin.net/vko4wznvvaulyz40/DSC00989.ARW Mirror: https://discuss.pixls.us/t/playraw-presolana-was-salvaging-a-raw/5769/4

Branch: colortab-tools-onoff 15.3-283-g11ca61e3

[heckflosse]

Confirmed

[Hombre57]

I think that what we see is the coarse image instead of the real preview. Might be connected to the bug of the detail window frozen to the coarse image when it opens (I don't see any opened issue for this).

This might be a race condition somewhere.

[Floessie]

@Hombre57

Might be connected to the bug of the detail window frozen to the coarse image when it opens (I don't see any opened issue for this).

It was mentioned on pixls lately.

[heckflosse]

This fixes the issue when zooming in:

diff --git a/rtgui/crophandler.cc b/rtgui/crophandler.cc
index f6ff83a7..0abff627 100644
--- a/rtgui/crophandler.cc
+++ b/rtgui/crophandler.cc
@@ -426,11 +426,11 @@ bool CropHandler::getWindow (int& cwx, int& cwy, int& cww, int& cwh, int& cskip)
     cwh = cropH;

     // hack: if called before first size allocation the size will be 0
-    if (cww < 10) {
+    if (cww == 0) {
         cww = 10;
     }

-    if (cwh < 32) {
+    if (cwh == 0) {
         cwh = 32;
     }

[heckflosse]

@Hombre57

Might be connected to the bug of the detail window frozen to the coarse image when it opens

The first detail window opens always blocky here while the following ones open always fine. Maybe a hint to find the culprit: With the following patch all detail windows open blocky.

diff --git a/rtgui/crophandler.cc b/rtgui/crophandler.cc
index f6ff83a7..479b1c66 100644
--- a/rtgui/crophandler.cc
+++ b/rtgui/crophandler.cc
@@ -448,13 +448,13 @@ void CropHandler::update ()
         cropPixbuf.clear ();

         // To save threads, try to mark "needUpdate" without a thread first
-        if (crop->tryUpdate()) {
+//        if (crop->tryUpdate()) {
             if (isLowUpdatePriority) {
                 Glib::Thread::create(sigc::mem_fun(*crop, &DetailedCrop::fullUpdate), 0, false, true, Glib::THREAD_PRIORITY_LOW);
             } else {
                 Glib::Thread::create(sigc::mem_fun(*crop, &DetailedCrop::fullUpdate), false );
             }
-        }
+//        }
     }
 }

[Beep6581]

The patch fixes the zoom levels, but the first detail window starts out blocky and stays that way unless the user triggers a redraw event.

[Floessie]

I've been trying to nail this down, but haven't found the culprit yet. There seems to be a race somewhere in CropHandler::setDetailedCrop() where in the inner async func ch->cropimg is NULL, so that ch->cropPixbuf isn't created, leading to a rough image in CropWindow::expose(). I don't really grok what's depending on the cimg mutex, but the problem lies somewhere there...

HTH, Flössie

[Floessie]

@Hombre57 I'm stuck, but CropHandler::setDetailedCrop() is the problem:

OKAY:
>fullUpdate
>updatePreviewImage
delete cropimg A
create cropimg
<updatePreviewImage
>func
delete cropimg C
<func
delete cropimg A
create cropimg
<fullUpdate
>func
delete cropimg C
<func

NOT OKAY:
>fullUpdate
>updatePreviewImage
delete cropimg A
create cropimg
<updatePreviewImage
delete cropimg A
create cropimg
<fullUpdate
>func
delete cropimg C
<func
>func
<func

Here's the corresponding patch:

diff --git a/rtengine/dcrop.cc b/rtengine/dcrop.cc
index 88397d6ee..352c3c059 100644
--- a/rtengine/dcrop.cc
+++ b/rtengine/dcrop.cc
@@ -1397,6 +1397,7 @@ bool Crop::tryUpdate()
  */
 void Crop::fullUpdate ()
 {
+    printf(">fullUpdate\n");

     parent->updaterThreadStart.lock ();

@@ -1426,6 +1427,7 @@ void Crop::fullUpdate ()
     }

     parent->updaterThreadStart.unlock ();
+    printf("<fullUpdate\n");
 }

 int Crop::get_skip()
diff --git a/rtengine/improccoordinator.cc b/rtengine/improccoordinator.cc
index 62891a03f..5c7f91a6c 100644
--- a/rtengine/improccoordinator.cc
+++ b/rtengine/improccoordinator.cc
@@ -828,12 +828,16 @@ void ImProcCoordinator::updatePreviewImage (int todo, Crop* cropCall)
         ipf.updateColorProfiles (monitorProfile, monitorIntent, softProof, gamutCheck);
     }

+    printf(">updatePreviewImage\n");
+
     // process crop, if needed
     for (size_t i = 0; i < crops.size(); i++)
         if (crops[i]->hasListener () && cropCall != crops[i] ) {
             crops[i]->update (todo);    // may call ourselves
         }

+    printf("<updatePreviewImage\n");
+
     progress ("Conversion to RGB...", 100 * readyphase / numofphases);

     if ((todo != CROP && todo != MINUPDATE) || (todo & M_MONITOR)) {
diff --git a/rtgui/crophandler.cc b/rtgui/crophandler.cc
index 0abff627b..c357be2bd 100644
--- a/rtgui/crophandler.cc
+++ b/rtgui/crophandler.cc
@@ -313,6 +313,7 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp
     }

     cropimg = nullptr;
+    printf("delete cropimg A\n");

     if (cropimgtrue) {
         delete [] cropimgtrue;
@@ -324,6 +325,7 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp
         cropimg_width = im->getWidth ();
         cropimg_height = im->getHeight ();
         cropimg = new unsigned char [3 * cropimg_width * cropimg_height];
+        printf("create cropimg\n");
         cropimgtrue = new unsigned char [3 * cropimg_width * cropimg_height];
         memcpy (cropimg, im->getData(), 3 * cropimg_width * cropimg_height);
         memcpy (cropimgtrue, imtrue->getData(), 3 * cropimg_width * cropimg_height);
@@ -335,6 +337,8 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp
         idle_helper->pending++;

         const auto func = [](gpointer data) -> gboolean {
+            printf(">func\n");
+
             IdleHelper* const idle_helper = static_cast<IdleHelper*>(data);

             if (idle_helper->destroyed) {
@@ -344,6 +348,7 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp
                     idle_helper->pending--;
                 }

+                printf("<func\n");
                 return FALSE;
             }

@@ -355,9 +360,11 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp
             if (!ch->enabled) {
                 delete [] ch->cropimg;
                 ch->cropimg = nullptr;
+                printf("delete cropimg B\n");
                 delete [] ch->cropimgtrue;
                 ch->cropimgtrue = nullptr;
                 ch->cimg.unlock ();
+                printf("<func\n");
                 return FALSE;
             }

@@ -391,6 +398,7 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp

                 delete [] ch->cropimg;
                 ch->cropimg = nullptr;
+                printf("delete cropimg C\n");
                 delete [] ch->cropimgtrue;
                 ch->cropimgtrue = nullptr;
             }
@@ -408,6 +416,8 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp

             idle_helper->pending--;

+            printf("<func\n");
+
             return FALSE;
         };

@@ -471,6 +481,7 @@ void CropHandler::setEnabled (bool e)
         cimg.lock ();
         delete [] cropimg;
         cropimg = nullptr;
+        printf("delete cropimg D\n");
         delete [] cropimgtrue;
         cropimgtrue = nullptr;
         cropPixbuf.clear ();

It can be easily provoked by switching between a RAW and a JPG via F3/F4: The JPG is faster and results in a coarse image in the main window.

HTH, Flössie

[agriggio]

@Floessie, thanks for the analysis. I've never been able to trigger this bug here (I'm still on gtk+ 3.18), but your log above made me consider this:

diff --git a/rtgui/crophandler.cc b/rtgui/crophandler.cc
--- a/rtgui/crophandler.cc
+++ b/rtgui/crophandler.cc
@@ -411,7 +411,7 @@
             return FALSE;
         };

-        idle_register.add(func, idle_helper);
+        idle_register.add(func, idle_helper, G_PRIORITY_HIGH_IDLE);
     }

     cimg.unlock ();

which seems to fix the problem (at least for @heckflosse, as discussed on IRC). Can someone else please confirm?

[Beep6581]

Fix confirmed!

[Floessie]

Not fixed here, I'm sorry.

From my experience, races can be hardly fixed by shifting thread priorities. We should cogitate on the meaning of IdleHelper, IMHO...

Best, Flössie

[agriggio]

@Floessie how about this? (remember I'm still sort of shooting in the dark because I can't reproduce):

diff --git a/rtgui/cropwindow.cc b/rtgui/cropwindow.cc
--- a/rtgui/cropwindow.cc
+++ b/rtgui/cropwindow.cc
@@ -2575,6 +2575,7 @@

 void CropWindow::initialImageArrived ()
 {
+    MyMutex::MyLock lock(cropHandler.cimg);

     for (auto listener : listeners) {
         listener->initialImageArrived (this);

[Floessie]

Thanks for your time, Alberto!

Unfortunately, it still occurs with your latest patch (though less often).

There's this dependency between CropHandler::cropPixbuf and CropHandler::cropimg, and they are both protected via CropHandler::cimg (bad name for a mutex BTW). My guess is, that one of them is either updated unlocked or changed without considering the other, so that there's an unlock in between. I checked the paths for the former but might have missed it. The latter would involve understanding the whole IdleHelper machinery (and replacing it with something sane)...

HTH, Flössie

[agriggio]

Ok, at least we are on the right track. Let's keep digging...

[agriggio]

@Floessie one more blind attempt:

diff --git a/rtgui/editorpanel.cc b/rtgui/editorpanel.cc
--- a/rtgui/editorpanel.cc
+++ b/rtgui/editorpanel.cc
@@ -1074,7 +1074,7 @@
         // normal redraw don't work, so this is the hard way
         // Disabled this with Issue 2435 because it seems to work fine now
 //        if (!options.tabbedUI && iareapanel->imageArea->mainCropWindow->getZoomFitVal() == 1.0) {
-//          iareapanel->imageArea->mainCropWindow->cropHandler.update();
+        iareapanel->imageArea->mainCropWindow->cropHandler.update();
 //        }
     } else {
         Gtk::Allocation alloc;

[Hombre57]

@Floessie @agriggio I'm not sure that I'll have time to look at it tonight, but here is a proposed patch. Like @agriggio , I can't reproduce the bug so it's a bit shooting in the dark too.

diff --git a/rtgui/crophandler.cc b/rtgui/crophandler.cc
index 6228cb1..a4f70a1 100644
--- a/rtgui/crophandler.cc
+++ b/rtgui/crophandler.cc
@@ -395,6 +395,8 @@
                 ch->cropimgtrue = nullptr;
             }

+            idle_helper->pending--;
+
             ch->cimg.unlock ();

             if (ch->displayHandler) {
@@ -405,8 +407,6 @@
                     ch->initial = false;
                 }
             }
-
-            idle_helper->pending--;

             return FALSE;
         };

If you look at idle_helper, it is protected by the cimg mutex, excepted in the lambda function, where the counter is decreased outside the cimg protection. This patch correct that, but I don't know if there will be side effects.

[Hombre57]

My patch above doesn't solve idle_helper protection completely. Maybe @Floessie can get ride of this helper class ? I'll try to find a solution too in the mean time.

[Floessie]

As for reproduction: I don't look at the detailed windows but at the main window (same problem). Either take a fast machine and open a RAW from the file browser. Sometimes it comes up coarse. Or take a slow machine or VM (both holds true for my current tests) and switch between a RAW and the JPG generated from it via F3/F4. The JPG is almost always displayed coarsely.

HTH, Flössie

[Floessie]

@agriggio @Hombre57 Thanks for your patches. Unfortunately, neither of them solves the problem. I'll try to ditch IdleHelper to get rid of at least one part of the equation.

[Floessie]

Okay now. Here's a patch that fixes the problem for me. It kills IdleHelper in favor of a single std::atomic<bool> redraw_needed. Maybe IdleHelper::pending wasn't the sole problem: I also changed initial to a std::atomic<bool>, didn't check it before, but this could have been a problem, too.

Please give it a test for regressions.

Best, Flössie

diff --git a/rtgui/crophandler.cc b/rtgui/crophandler.cc
index 6228cb1c7..8f9885b50 100644
--- a/rtgui/crophandler.cc
+++ b/rtgui/crophandler.cc
@@ -29,19 +29,35 @@

 using namespace rtengine;

-CropHandler::CropHandler ()
-    : zoom(100), ww(0), wh(0), cax(-1), cay(-1),
-      cx(0), cy(0), cw(0), ch(0), cropX(0), cropY(0), cropW(0), cropH(0), enabled(false),
-      cropimg(nullptr), cropimgtrue(nullptr), cropimg_width(0), cropimg_height(0),
-      cix(0), ciy(0), ciw(0), cih(0), cis(1),
-      initial(false), isLowUpdatePriority(false), ipc(nullptr), crop(nullptr),
-      displayHandler(nullptr)
+CropHandler::CropHandler() :
+    zoom(100),
+    ww(0),
+    wh(0),
+    cax(-1),
+    cay(-1),
+    cx(0),
+    cy(0),
+    cw(0),
+    ch(0),
+    cropX(0),
+    cropY(0),
+    cropW(0),
+    cropH(0),
+    enabled(false),
+    cropimg_width(0),
+    cropimg_height(0),
+    cix(0),
+    ciy(0),
+    ciw(0),
+    cih(0),
+    cis(1),
+    isLowUpdatePriority(false),
+    ipc(nullptr),
+    crop(nullptr),
+    displayHandler(nullptr),
+    redraw_needed(false),
+    initial(false)
 {
-
-    idle_helper = new IdleHelper;
-    idle_helper->destroyed = false;
-    idle_helper->pending = 0;
-    idle_helper->cropHandler = this;
 }

 CropHandler::~CropHandler ()
@@ -59,16 +75,6 @@ CropHandler::~CropHandler ()
         delete crop; // will do the same than destroy, plus delete the object
         crop = nullptr;
     }
-
-    cimg.lock ();
-
-    if (idle_helper->pending) {
-        idle_helper->destroyed = true;
-    } else {
-        delete idle_helper;
-    }
-
-    cimg.unlock ();
 }

 void CropHandler::setEditSubscriber (EditSubscriber* newSubscriber)
@@ -308,110 +314,94 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp

     cropPixbuf.clear ();

-    if (cropimg) {
-        delete [] cropimg;
+    if (!cropimg.empty()) {
+        cropimg.clear();
     }

-    cropimg = nullptr;
-
-    if (cropimgtrue) {
-        delete [] cropimgtrue;
+    if (!cropimgtrue.empty()) {
+        cropimgtrue.clear();
     }

-    cropimgtrue = nullptr;
-
     if (ax == cropX && ay == cropY && aw == cropW && ah == cropH && askip == (zoom >= 1000 ? 1 : zoom / 10)) {
         cropimg_width = im->getWidth ();
         cropimg_height = im->getHeight ();
-        cropimg = new unsigned char [3 * cropimg_width * cropimg_height];
-        cropimgtrue = new unsigned char [3 * cropimg_width * cropimg_height];
-        memcpy (cropimg, im->getData(), 3 * cropimg_width * cropimg_height);
-        memcpy (cropimgtrue, imtrue->getData(), 3 * cropimg_width * cropimg_height);
+        const std::size_t cropimg_size = 3 * cropimg_width * cropimg_height;
+        cropimg.assign(im->getData(), im->getData() + cropimg_size);
+        cropimgtrue.assign(imtrue->getData(), imtrue->getData() + cropimg_size);
         cix = ax;
         ciy = ay;
         ciw = aw;
         cih = ah;
         cis = askip;
-        idle_helper->pending++;

-        const auto func = [](gpointer data) -> gboolean {
-            IdleHelper* const idle_helper = static_cast<IdleHelper*>(data);
-
-            if (idle_helper->destroyed) {
-                if (idle_helper->pending == 1) {
-                    delete idle_helper;
-                } else {
-                    idle_helper->pending--;
-                }
-
-                return FALSE;
-            }
+        bool expected = false;

-            CropHandler* ch = idle_helper->cropHandler;
+        if (redraw_needed.compare_exchange_strong(expected, true)) {
+            const auto func = [](gpointer data) -> gboolean {
+                CropHandler* const self = static_cast<CropHandler*>(data);

-            ch->cimg.lock ();
-            ch->cropPixbuf.clear ();
+                self->cimg.lock ();

-            if (!ch->enabled) {
-                delete [] ch->cropimg;
-                ch->cropimg = nullptr;
-                delete [] ch->cropimgtrue;
-                ch->cropimgtrue = nullptr;
-                ch->cimg.unlock ();
-                return FALSE;
-            }
+                if (self->redraw_needed.exchange(false)) {
+                    self->cropPixbuf.clear ();

-            if (ch->cropimg) {
-                if (ch->cix == ch->cropX && ch->ciy == ch->cropY && ch->ciw == ch->cropW && ch->cih == ch->cropH && ch->cis == (ch->zoom >= 1000 ? 1 : ch->zoom / 10)) {
-                    // calculate final image size
-                    float czoom = ch->zoom >= 1000 ?
-                        ch->zoom / 1000.f :
-                        float((ch->zoom/10) * 10) / float(ch->zoom);
-                    int imw = ch->cropimg_width * czoom;
-                    int imh = ch->cropimg_height * czoom;
-
-                    if (imw > ch->ww) {
-                        imw = ch->ww;
+                    if (!self->enabled) {
+                        self->cropimg.clear();
+                        self->cropimgtrue.clear();
+                        self->cimg.unlock ();
+                        return FALSE;
                     }

-                    if (imh > ch->wh) {
-                        imh = ch->wh;
+                    if (!self->cropimg.empty()) {
+                        if (self->cix == self->cropX && self->ciy == self->cropY && self->ciw == self->cropW && self->cih == self->cropH && self->cis == (self->zoom >= 1000 ? 1 : self->zoom / 10)) {
+                            // calculate final image size
+                            float czoom = self->zoom >= 1000 ?
+                                self->zoom / 1000.f :
+                                float((self->zoom/10) * 10) / float(self->zoom);
+                            int imw = self->cropimg_width * czoom;
+                            int imh = self->cropimg_height * czoom;
+
+                            if (imw > self->ww) {
+                                imw = self->ww;
+                            }
+
+                            if (imh > self->wh) {
+                                imh = self->wh;
+                            }
+
+                            Glib::RefPtr<Gdk::Pixbuf> tmpPixbuf = Gdk::Pixbuf::create_from_data (self->cropimg.data(), Gdk::COLORSPACE_RGB, false, 8, self->cropimg_width, self->cropimg_height, 3 * self->cropimg_width);
+                            self->cropPixbuf = Gdk::Pixbuf::create (Gdk::COLORSPACE_RGB, false, 8, imw, imh);
+                            tmpPixbuf->scale (self->cropPixbuf, 0, 0, imw, imh, 0, 0, czoom, czoom, Gdk::INTERP_TILES);
+                            tmpPixbuf.clear ();
+
+                            Glib::RefPtr<Gdk::Pixbuf> tmpPixbuftrue = Gdk::Pixbuf::create_from_data (self->cropimgtrue.data(), Gdk::COLORSPACE_RGB, false, 8, self->cropimg_width, self->cropimg_height, 3 * self->cropimg_width);
+                            self->cropPixbuftrue = Gdk::Pixbuf::create (Gdk::COLORSPACE_RGB, false, 8, imw, imh);
+                            tmpPixbuftrue->scale (self->cropPixbuftrue, 0, 0, imw, imh, 0, 0, czoom, czoom, Gdk::INTERP_TILES);
+                            tmpPixbuftrue.clear ();
+                        }
+
+                        self->cropimg.clear();
+                        self->cropimgtrue.clear();
                     }

-                    Glib::RefPtr<Gdk::Pixbuf> tmpPixbuf = Gdk::Pixbuf::create_from_data (ch->cropimg, Gdk::COLORSPACE_RGB, false, 8, ch->cropimg_width, ch->cropimg_height, 3 * ch->cropimg_width);
-                    ch->cropPixbuf = Gdk::Pixbuf::create (Gdk::COLORSPACE_RGB, false, 8, imw, imh);
-                    tmpPixbuf->scale (ch->cropPixbuf, 0, 0, imw, imh, 0, 0, czoom, czoom, Gdk::INTERP_TILES);
-                    tmpPixbuf.clear ();
-
-                    Glib::RefPtr<Gdk::Pixbuf> tmpPixbuftrue = Gdk::Pixbuf::create_from_data (ch->cropimgtrue, Gdk::COLORSPACE_RGB, false, 8, ch->cropimg_width, ch->cropimg_height, 3 * ch->cropimg_width);
-                    ch->cropPixbuftrue = Gdk::Pixbuf::create (Gdk::COLORSPACE_RGB, false, 8, imw, imh);
-                    tmpPixbuftrue->scale (ch->cropPixbuftrue, 0, 0, imw, imh, 0, 0, czoom, czoom, Gdk::INTERP_TILES);
-                    tmpPixbuftrue.clear ();
-                }
-
-                delete [] ch->cropimg;
-                ch->cropimg = nullptr;
-                delete [] ch->cropimgtrue;
-                ch->cropimgtrue = nullptr;
-            }
-
-            ch->cimg.unlock ();
+                    self->cimg.unlock ();

-            if (ch->displayHandler) {
-                ch->displayHandler->cropImageUpdated ();
+                    if (self->displayHandler) {
+                        self->displayHandler->cropImageUpdated ();

-                if (ch->initial) {
-                    ch->displayHandler->initialImageArrived ();
-                    ch->initial = false;
+                        if (self->initial.exchange(false)) {
+                            self->displayHandler->initialImageArrived ();
+                        }
+                    }
+                } else {
+                    self->cimg.unlock();
                 }
-            }
-
-            idle_helper->pending--;

-            return FALSE;
-        };
+                return FALSE;
+            };

-        idle_register.add(func, idle_helper, G_PRIORITY_HIGH_IDLE);
+            idle_register.add(func, this/*, G_PRIORITY_HIGH_IDLE*/);
+        }
     }

     cimg.unlock ();
@@ -468,13 +458,11 @@ void CropHandler::setEnabled (bool e)
             crop->setListener (nullptr);
         }

-        cimg.lock ();
-        delete [] cropimg;
-        cropimg = nullptr;
-        delete [] cropimgtrue;
-        cropimgtrue = nullptr;
-        cropPixbuf.clear ();
-        cimg.unlock ();
+        cimg.lock();
+        cropimg.clear();
+        cropimgtrue.clear();
+        cropPixbuf.clear();
+        cimg.unlock();
     } else {
         update ();
     }
diff --git a/rtgui/crophandler.h b/rtgui/crophandler.h
index 1e7a9202b..17f2ca47d 100644
--- a/rtgui/crophandler.h
+++ b/rtgui/crophandler.h
@@ -19,11 +19,16 @@
 #ifndef __CROPHANDLER__
 #define __CROPHANDLER__

+#include <atomic>
+#include <vector>
+
+#include <gtkmm.h>
+
 #include "../rtengine/rtengine.h"
-#include "threadutils.h"
+
 #include "edit.h"
 #include "lockablecolorpicker.h"
-#include <gtkmm.h>
+#include "threadutils.h"

 class CropDisplayHandler
 {
@@ -98,12 +103,6 @@ public:
     MyMutex cimg;

 private:
-    struct IdleHelper {
-        CropHandler* cropHandler;
-        bool destroyed;
-        int pending;
-    };
-
     void    compDim ();

     int zoom;               // scale factor (e.g. 5 if 1:5 scale) ; if 1:1 scale and bigger, factor is multiplied by 1000  (i.e. 1000 for 1:1 scale, 2000 for 2:1, etc...)
@@ -112,17 +111,18 @@ private:
     int cx, cy, cw, ch;     // position and size of the requested crop ; position expressed in image coordinates, so cx and cy might be negative and cw and ch higher than the image's 1:1 size
     int cropX, cropY, cropW, cropH; // cropPixbuf's displayed area (position and size), i.e. coordinates in 1:1 scale, i.e. cx, cy, cw & ch trimmed to the image's bounds
     bool enabled;
-    unsigned char* cropimg;
-    unsigned char* cropimgtrue;
+    std::vector<unsigned char> cropimg;
+    std::vector<unsigned char> cropimgtrue;
     int cropimg_width, cropimg_height, cix, ciy, ciw, cih, cis;
-    bool initial;
     bool isLowUpdatePriority;

     rtengine::StagedImageProcessor* ipc;
     rtengine::DetailedCrop* crop;

     CropDisplayHandler* displayHandler;
-    IdleHelper* idle_helper;
+
+    std::atomic<bool> redraw_needed;
+    std::atomic<bool> initial;

     IdleRegister idle_register;
 };

PS: I also converted cropimg and cropimgtrue from raw pointers to std::vector<>. A potential benefit is the call to clear() which keeps the allocated heap memory for reuse. If that's too much memory, we can add shrink_to_fit() after the clear() calls.

[agriggio]

@Floessie thanks for the patch. I will test asap. But can you please consider separating the cosmetic parts (indentation, line breaks) from the actual contents? It would make it much easier to review/understand what is going on. I wouldn't stress you normally, but this is a delicate part it seems, so I think the more eyes are on this the better... thanks!

[Floessie]

@agriggio

But can you please consider separating the cosmetic parts (indentation, line breaks) from the actual contents?

Well, most of the indentation problems are part of the content (I had to if (...) { the core part). I'll try to do better next time. 👍

[agriggio]

ok, apologies then. I read your description and then I saw the formatting changes in the constructor, and I jumped to conclusions... :-(

[Floessie]

Here's a better readable patch. I renamed self back to ch, "fixed" indentation, and applied git diff -w.

diff --git a/rtgui/crophandler.cc b/rtgui/crophandler.cc
index 6228cb1c7..c49f05cda 100644
--- a/rtgui/crophandler.cc
+++ b/rtgui/crophandler.cc
@@ -32,16 +32,11 @@ using namespace rtengine;
 CropHandler::CropHandler ()
     : zoom(100), ww(0), wh(0), cax(-1), cay(-1),
       cx(0), cy(0), cw(0), ch(0), cropX(0), cropY(0), cropW(0), cropH(0), enabled(false),
-      cropimg(nullptr), cropimgtrue(nullptr), cropimg_width(0), cropimg_height(0),
+      cropimg_width(0), cropimg_height(0),
       cix(0), ciy(0), ciw(0), cih(0), cis(1),
-      initial(false), isLowUpdatePriority(false), ipc(nullptr), crop(nullptr),
-      displayHandler(nullptr)
+      isLowUpdatePriority(false), ipc(nullptr), crop(nullptr),
+      displayHandler(nullptr), redraw_needed(false), initial(false)
 {
-
-    idle_helper = new IdleHelper;
-    idle_helper->destroyed = false;
-    idle_helper->pending = 0;
-    idle_helper->cropHandler = this;
 }

 CropHandler::~CropHandler ()
@@ -59,16 +54,6 @@ CropHandler::~CropHandler ()
         delete crop; // will do the same than destroy, plus delete the object
         crop = nullptr;
     }
-
-    cimg.lock ();
-
-    if (idle_helper->pending) {
-        idle_helper->destroyed = true;
-    } else {
-        delete idle_helper;
-    }
-
-    cimg.unlock ();
 }

 void CropHandler::setEditSubscriber (EditSubscriber* newSubscriber)
@@ -308,60 +293,46 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp

     cropPixbuf.clear ();

-    if (cropimg) {
-        delete [] cropimg;
+    if (!cropimg.empty()) {
+        cropimg.clear();
     }

-    cropimg = nullptr;
-
-    if (cropimgtrue) {
-        delete [] cropimgtrue;
+    if (!cropimgtrue.empty()) {
+        cropimgtrue.clear();
     }

-    cropimgtrue = nullptr;
-
     if (ax == cropX && ay == cropY && aw == cropW && ah == cropH && askip == (zoom >= 1000 ? 1 : zoom / 10)) {
         cropimg_width = im->getWidth ();
         cropimg_height = im->getHeight ();
-        cropimg = new unsigned char [3 * cropimg_width * cropimg_height];
-        cropimgtrue = new unsigned char [3 * cropimg_width * cropimg_height];
-        memcpy (cropimg, im->getData(), 3 * cropimg_width * cropimg_height);
-        memcpy (cropimgtrue, imtrue->getData(), 3 * cropimg_width * cropimg_height);
+        const std::size_t cropimg_size = 3 * cropimg_width * cropimg_height;
+        cropimg.assign(im->getData(), im->getData() + cropimg_size);
+        cropimgtrue.assign(imtrue->getData(), imtrue->getData() + cropimg_size);
         cix = ax;
         ciy = ay;
         ciw = aw;
         cih = ah;
         cis = askip;
-        idle_helper->pending++;
-
-        const auto func = [](gpointer data) -> gboolean {
-            IdleHelper* const idle_helper = static_cast<IdleHelper*>(data);

-            if (idle_helper->destroyed) {
-                if (idle_helper->pending == 1) {
-                    delete idle_helper;
-                } else {
-                    idle_helper->pending--;
-                }
-
-                return FALSE;
-            }
+        bool expected = false;

-            CropHandler* ch = idle_helper->cropHandler;
+        if (redraw_needed.compare_exchange_strong(expected, true)) {
+        // As requested by Alberto: Wrong indent for better diff
+        const auto func = [](gpointer data) -> gboolean {
+            CropHandler* const ch = static_cast<CropHandler*>(data);

             ch->cimg.lock ();
+
+            if (ch->redraw_needed.exchange(false)) {
                 ch->cropPixbuf.clear ();

                 if (!ch->enabled) {
-                delete [] ch->cropimg;
-                ch->cropimg = nullptr;
-                delete [] ch->cropimgtrue;
-                ch->cropimgtrue = nullptr;
+                    ch->cropimg.clear();
+                    ch->cropimgtrue.clear();
                     ch->cimg.unlock ();
                     return FALSE;
                 }

-            if (ch->cropimg) {
+                if (!ch->cropimg.empty()) {
                     if (ch->cix == ch->cropX && ch->ciy == ch->cropY && ch->ciw == ch->cropW && ch->cih == ch->cropH && ch->cis == (ch->zoom >= 1000 ? 1 : ch->zoom / 10)) {
                         // calculate final image size
                         float czoom = ch->zoom >= 1000 ?
@@ -378,21 +349,19 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp
                             imh = ch->wh;
                         }

-                    Glib::RefPtr<Gdk::Pixbuf> tmpPixbuf = Gdk::Pixbuf::create_from_data (ch->cropimg, Gdk::COLORSPACE_RGB, false, 8, ch->cropimg_width, ch->cropimg_height, 3 * ch->cropimg_width);
+                        Glib::RefPtr<Gdk::Pixbuf> tmpPixbuf = Gdk::Pixbuf::create_from_data (ch->cropimg.data(), Gdk::COLORSPACE_RGB, false, 8, ch->cropimg_width, ch->cropimg_height, 3 * ch->cropimg_width);
                         ch->cropPixbuf = Gdk::Pixbuf::create (Gdk::COLORSPACE_RGB, false, 8, imw, imh);
                         tmpPixbuf->scale (ch->cropPixbuf, 0, 0, imw, imh, 0, 0, czoom, czoom, Gdk::INTERP_TILES);
                         tmpPixbuf.clear ();

-                    Glib::RefPtr<Gdk::Pixbuf> tmpPixbuftrue = Gdk::Pixbuf::create_from_data (ch->cropimgtrue, Gdk::COLORSPACE_RGB, false, 8, ch->cropimg_width, ch->cropimg_height, 3 * ch->cropimg_width);
+                        Glib::RefPtr<Gdk::Pixbuf> tmpPixbuftrue = Gdk::Pixbuf::create_from_data (ch->cropimgtrue.data(), Gdk::COLORSPACE_RGB, false, 8, ch->cropimg_width, ch->cropimg_height, 3 * ch->cropimg_width);
                         ch->cropPixbuftrue = Gdk::Pixbuf::create (Gdk::COLORSPACE_RGB, false, 8, imw, imh);
                         tmpPixbuftrue->scale (ch->cropPixbuftrue, 0, 0, imw, imh, 0, 0, czoom, czoom, Gdk::INTERP_TILES);
                         tmpPixbuftrue.clear ();
                     }

-                delete [] ch->cropimg;
-                ch->cropimg = nullptr;
-                delete [] ch->cropimgtrue;
-                ch->cropimgtrue = nullptr;
+                    ch->cropimg.clear();
+                    ch->cropimgtrue.clear();
                 }

                 ch->cimg.unlock ();
@@ -400,18 +369,19 @@ void CropHandler::setDetailedCrop (IImage8* im, IImage8* imtrue, rtengine::procp
                 if (ch->displayHandler) {
                     ch->displayHandler->cropImageUpdated ();

-                if (ch->initial) {
+                    if (ch->initial.exchange(false)) {
                         ch->displayHandler->initialImageArrived ();
-                    ch->initial = false;
                     }
                 }
-
-            idle_helper->pending--;
+            } else {
+                ch->cimg.unlock();
+            }

             return FALSE;
         };

-        idle_register.add(func, idle_helper, G_PRIORITY_HIGH_IDLE);
+        idle_register.add(func, this/*, G_PRIORITY_HIGH_IDLE*/);
+        }
     }

     cimg.unlock ();
@@ -469,10 +439,8 @@ void CropHandler::setEnabled (bool e)
         }

         cimg.lock();
-        delete [] cropimg;
-        cropimg = nullptr;
-        delete [] cropimgtrue;
-        cropimgtrue = nullptr;
+        cropimg.clear();
+        cropimgtrue.clear();
         cropPixbuf.clear();
         cimg.unlock();
     } else {
diff --git a/rtgui/crophandler.h b/rtgui/crophandler.h
index 1e7a9202b..17f2ca47d 100644
--- a/rtgui/crophandler.h
+++ b/rtgui/crophandler.h
@@ -19,11 +19,16 @@
 #ifndef __CROPHANDLER__
 #define __CROPHANDLER__

+#include <atomic>
+#include <vector>
+
+#include <gtkmm.h>
+
 #include "../rtengine/rtengine.h"
-#include "threadutils.h"
+
 #include "edit.h"
 #include "lockablecolorpicker.h"
-#include <gtkmm.h>
+#include "threadutils.h"

 class CropDisplayHandler
 {
@@ -98,12 +103,6 @@ public:
     MyMutex cimg;

 private:
-    struct IdleHelper {
-        CropHandler* cropHandler;
-        bool destroyed;
-        int pending;
-    };
-
     void    compDim ();

     int zoom;               // scale factor (e.g. 5 if 1:5 scale) ; if 1:1 scale and bigger, factor is multiplied by 1000  (i.e. 1000 for 1:1 scale, 2000 for 2:1, etc...)
@@ -112,17 +111,18 @@ private:
     int cx, cy, cw, ch;     // position and size of the requested crop ; position expressed in image coordinates, so cx and cy might be negative and cw and ch higher than the image's 1:1 size
     int cropX, cropY, cropW, cropH; // cropPixbuf's displayed area (position and size), i.e. coordinates in 1:1 scale, i.e. cx, cy, cw & ch trimmed to the image's bounds
     bool enabled;
-    unsigned char* cropimg;
-    unsigned char* cropimgtrue;
+    std::vector<unsigned char> cropimg;
+    std::vector<unsigned char> cropimgtrue;
     int cropimg_width, cropimg_height, cix, ciy, ciw, cih, cis;
-    bool initial;
     bool isLowUpdatePriority;

     rtengine::StagedImageProcessor* ipc;
     rtengine::DetailedCrop* crop;

     CropDisplayHandler* displayHandler;
-    IdleHelper* idle_helper;
+
+    std::atomic<bool> redraw_needed;
+    std::atomic<bool> initial;

     IdleRegister idle_register;
 };

HTH, Flössie

[agriggio]

Thanks @Floessie! One question (haven't tested yet):

-        idle_register.add(func, idle_helper, G_PRIORITY_HIGH_IDLE);
+        idle_register.add(func, this/*, G_PRIORITY_HIGH_IDLE*/);

any specific reason for this? According to the docs:

Use this for high priority idle functions.

GTK+ uses G_PRIORITY_HIGH_IDLE + 10 for resizing operations, and G_PRIORITY_HIGH_IDLE + 20 for redrawing operations. (This is done to ensure that any pending resizes are processed before any pending redraws, so that widgets are not redrawn twice unnecessarily.)

So I'd say we should keep it. What do you think?

[Floessie]

any specific reason for this?

To make it easier for you all to test. Sure we can keep it, it only hurts your tests, as @Beep6581 marked it fixed by this, although it just hid the problem.

I just tested to be sure: Fix works with and without G_PRIORITY_HIGH_IDLE here.

[agriggio]

I just tested to be sure: Fix works with and without G_PRIORITY_HIGH_IDLE here.

I think I was unclear, sorry. I didn't mean it was needed for correctness, but just to avoid unnecessary redraws. So I'd propose we keep the patch as is now, and once we are positive that it work we can bump the priority once again. What do you think?

[agriggio]

@Floessie the patch doesn't apply cleanly on dev. Do you confirm, or is this a problem on my side?

[Beep6581]

Good morning

separating the cosmetic parts (indentation, line breaks) from the actual contents

Though this was a misunderstanding, I wanted to point out that you can use diff --ignore-all-space, and kdiff3 uses different coloring for whitespace changes vs real changes making it easy to skip to the beef.

patch doesn't apply

Speaking of which, I often see long patches posted here in GitHub, and I don't know about your experience but here half the time I have issues when applying them, usually related to white-space present or missing from the last line, something to do with how they're pasted into a comment or copied from one. That means I need more than one attempt to apply a patch. In the case of the above patch, as @agriggio just pointed out, it doesn't apply to current dev. So on to my question: why not use branches more? They're cheap and disposable, and you can always squash if you don't want to pollute the commit history using git rebase -i.

[Floessie]

@agriggio

I think I was unclear, sorry.

No, that was perfectly clear. I just wanted to make sure, that my fix is independent of G_PRIORITY_HIGH_IDLE.

the patch doesn't apply cleanly on dev. Do you confirm, or is this a problem on my side?

I guess, thats due to git diff -w. Have you tried --whitespace? The second patch is only for better reading, the first one is the proper one to test. Only their formattings differ.

HTH, Flössie

[Beep6581]

One more benefit of using a branch: you can commit your proper patch including all necessary whitespace changes, and it's up to the person viewing the patch whether they want to --ignore-all-space or not.

[Floessie]

@Beep6581 Can't push from here, I'm firewalled. Uploading files to GitHub doesn't work either. But I can prepare a branch tonight.

[Beep6581]

why not use branches more?

Can't push from here, I'm firewalled

@Floessie well that answers the "why" ;)

[Beep6581]

@Floessie if you use HTTPS instead of SSH, you should be able to push from behind the firewall.

[agriggio]

@Floessie I tested, and it seems it introduces no regressions for me. However, now that I tried stressing the behaviour more, I can easily cause deadlocks by quickly switching images (e.g. by pressing F3 and F4 repeatedly and fast). This I suspect is a symptom that the problem is deeper and not solved yet, unfortunately. Note that this happens already with dev, even without your patch...

[Floessie]

Thanks, Alberto. Yes, we already had issues with F3/F4 in the past and I fixed at least one of them. Now, a deadlock can be nicely debugged with GDB. If I encounter it, too, I'll have a look.

[Floessie]

Obviously, my VM is very prone to subtle races: I could provoke the deadlock instantly. 😄

Here are the relevant threads:

Thread 363 (Thread 0x7fff68b84700 (LWP 14495)):
#0  0x00007ffff143cf5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffff1436c06 in __GI___pthread_mutex_lock (mutex=0x55555632daf0) at ../nptl/pthread_mutex_lock.c:115
#2  0x000055555595c120 in FileBrowserEntry::updateImage(rtengine::IImage8*, double, rtengine::procparams::CropParams) ()
#3  0x0000555555b169e4 in ThumbImageUpdater::Impl::processNextJob() ()
#4  0x00007ffff459ceb2 in  () at /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#5  0x00007ffff542adce in g_thread_pool_thread_proxy (data=<optimized out>) at ././glib/gthreadpool.c:307
#6  0x00007ffff542a3d5 in g_thread_proxy (data=0x7fffc4006c50) at ././glib/gthread.c:784
#7  0x00007ffff1434494 in start_thread (arg=0x7fff68b84700) at pthread_create.c:333
#8  0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97

Thread 362 (Thread 0x7fff69385700 (LWP 14494)):
#0  0x00007ffff14356cd in pthread_join (threadid=140735065900800, thread_return=thread_return@entry=0x0) at pthread_join.c:90
#1  0x00007ffff5448147 in g_system_thread_wait (thread=0x7fffc8003320) at ././glib/gthread-posix.c:1212
#2  0x00007ffff542a83a in g_thread_join (thread=0x7fffc8003320) at ././glib/gthread.c:952
#3  0x0000555555bc7258 in rtengine::Crop::fullUpdate() ()
#4  0x00007ffff458cdfd in  () at /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#5  0x00007ffff542a3d5 in g_thread_proxy (data=0x55555c34a590) at ././glib/gthread.c:784
#6  0x00007ffff1434494 in start_thread (arg=0x7fff69385700) at pthread_create.c:333
#7  0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97

Thread 358 (Thread 0x7fff6f9c4700 (LWP 14490)):
#0  0x00007ffff143cf5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffff1436c06 in __GI___pthread_mutex_lock (mutex=0x55555632daf0) at ../nptl/pthread_mutex_lock.c:115
#2  0x0000555555b2d79a in ToolPanelCoordinator::imageTypeChanged(bool, bool, bool) ()
#3  0x0000555555c10718 in rtengine::ImProcCoordinator::updatePreviewImage(int, rtengine::Crop*) ()
#4  0x0000555555c13d0c in rtengine::ImProcCoordinator::process() ()
#5  0x00007ffff458cdfd in  () at /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#6  0x00007ffff542a3d5 in g_thread_proxy (data=0x7fffc8003320) at ././glib/gthread.c:784
#7  0x00007ffff1434494 in start_thread (arg=0x7fff6f9c4700) at pthread_create.c:333
#8  0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97

Thread 350 (Thread 0x7fff817fa700 (LWP 14482)):
#0  0x00007ffff143cf5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffff1436c06 in __GI___pthread_mutex_lock (mutex=0x55555632daf0) at ../nptl/pthread_mutex_lock.c:115
#2  0x000055555595c120 in FileBrowserEntry::updateImage(rtengine::IImage8*, double, rtengine::procparams::CropParams) ()
#3  0x0000555555b169e4 in ThumbImageUpdater::Impl::processNextJob() ()
#4  0x00007ffff459ceb2 in  () at /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#5  0x00007ffff542adce in g_thread_pool_thread_proxy (data=<optimized out>) at ././glib/gthreadpool.c:307
#6  0x00007ffff542a3d5 in g_thread_proxy (data=0x55555c0a5800) at ././glib/gthread.c:784
#7  0x00007ffff1434494 in start_thread (arg=0x7fff817fa700) at pthread_create.c:333
#8  0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97

Thread 1 (Thread 0x7ffff7f2fb00 (LWP 14126)):
#0  0x00007ffff1172259 in syscall () at ../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
#1  0x00007ffff544787c in g_mutex_lock_slowpath (mutex=0x55555d362c18) at ././glib/gthread-posix.c:1313
#2  0x0000555555c09ff8 in rtengine::ImProcCoordinator::stopProcessing() ()
#3  0x0000555555b2eb96 in ToolPanelCoordinator::closeImage() ()
#4  0x000055555591256c in EditorPanel::close() ()
#5  0x0000555555913b8f in EditorPanel::open(Thumbnail*, rtengine::InitialImage*) ()
#6  0x0000555555981b95 in FilePanel::imageLoaded(Thumbnail*, ProgressConnector<rtengine::InitialImage*>*) ()
#7  0x0000555555982cff in ProgressConnector<rtengine::InitialImage*>::emitEndSignalUI(void*) ()
#8  0x00007ffff6aa06e8 in gdk_threads_dispatch (data=0x55555bb11ac0) at ././gdk/gdk.c:743
#9  0x00007ffff54026aa in g_main_dispatch (context=0x5555563e1800) at ././glib/gmain.c:3203
#10 0x00007ffff54026aa in g_main_context_dispatch (context=context@entry=0x5555563e1800) at ././glib/gmain.c:3856
#11 0x00007ffff5402a60 in g_main_context_iterate (context=0x5555563e1800, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ././glib/gmain.c:3929
#12 0x00007ffff5402d82 in g_main_loop_run (loop=0x5555592cade0) at ././glib/gmain.c:4125
#13 0x00007ffff6f99d55 in gtk_main () at ././gtk/gtkmain.c:1312
#14 0x00007ffff3d1ffb1 in Gtk::Main::run_impl() (this=<optimized out>) at main.cc:439
#15 0x00007ffff3d1ffb1 in Gtk::Main::run(Gtk::Window&) (window=warning: RTTI symbol not found for class 'RTWindow'
...) at main.cc:398
#16 0x000055555581b20a in main ()

Now we need some guru meditation. 💭

[Floessie]

Okay, only two mutexes involved: ImProcCoordinator::updaterThreadStart and the global lock via GThreadLock...

[Floessie]

Okay, after digging into the gtk+ sources, I think I understand the deadlock:

Thread 1 (Thread 0x7ffff7f2fb00 (LWP 16292)):
#0  0x00007ffff143cf5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffff1436c06 in __GI___pthread_mutex_lock (mutex=0x55555c362850) at ../nptl/pthread_mutex_lock.c:115
#2  0x0000555555b4a8be in MyMutex::lock() (this=0x55555dcab408) at /home/user/src/rawtherapee/rtgui/../rtengine/../rtgui/threadutils.h:162
#3  0x00005555562d2a80 in rtengine::ImProcCoordinator::stopProcessing() (this=0x55555dca7800) at /home/user/src/rawtherapee/rtengine/improccoordinator.cc:1301
updaterThreadStart
#4  0x0000555555ed7405 in ToolPanelCoordinator::closeImage() (this=0x555559876fb0) at /home/user/src/rawtherapee/rtgui/toolpanelcoord.cc:498
#5  0x0000555555c1fec2 in EditorPanel::close() (this=0x555559876a80) at /home/user/src/rawtherapee/rtgui/editorpanel.cc:1092
#6  0x0000555555c1f7f8 in EditorPanel::open(Thumbnail*, rtengine::InitialImage*) (this=0x555559876a80, tmb=0x7fffc0004390, isrc=0x7fffc0014600) at /home/user/src/rawtherapee/rtgui/editorpanel.cc:1020
#7  0x0000555555caa2ab in FilePanel::imageLoaded(Thumbnail*, ProgressConnector<rtengine::InitialImage*>*) (this=0x5555570a1490, thm=0x7fffc8004990, pc=0x55555d9d9710) at /home/user/src/rawtherapee/rtgui/filepanel.cc:310
pendingLoadMutex
#8  0x0000555555cb0172 in sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*>::operator()(Thumbnail* const&, ProgressConnector<rtengine::InitialImage*>* const&) const (this=0x55555b9d1f90, _A_a1=@0x55555b9d1fb0: 0x7fffc8004990, _A_a2=@0x55555b9d1fb8: 0x55555d9d9710) at /usr/include/sigc++-2.0/sigc++/functors/mem_fun.h:2143
#9  0x0000555555caf85d in sigc::adaptor_functor<sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*> >::operator()<Thumbnail*&, ProgressConnector<rtengine::InitialImage*>*&>(Thumbnail*&, ProgressConnector<rtengine::InitialImage*>*&) const (this=0x55555b9d1f88, _A_arg1=@0x55555b9d1fb0: 0x7fffc8004990, _A_arg2=@0x55555b9d1fb8: 0x55555d9d9710) at /usr/include/sigc++-2.0/sigc++/adaptors/adaptor_trait.h:108
#10 0x0000555555caeab1 in sigc::bind_functor<-1, sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*>, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*, sigc::nil, sigc::nil, sigc::nil, sigc::nil, sigc::nil>::operator()() (this=0x55555b9d1f80) at /usr/include/sigc++-2.0/sigc++/adaptors/bind.h:1340
#11 0x0000555555cadb59 in sigc::internal::slot_call0<sigc::bind_functor<-1, sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*>, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*, sigc::nil, sigc::nil, sigc::nil, sigc::nil, sigc::nil>, bool>::call_it(sigc::internal::slot_rep*) (rep=0x55555b9d1f50) at /usr/include/sigc++-2.0/sigc++/functors/slot.h:114
#12 0x0000555555c339b5 in sigc::slot0<bool>::operator()() const (this=0x55555c6ea7d8) at /usr/include/sigc++-2.0/sigc++/functors/slot.h:513
#13 0x0000555555c32592 in sigc::adaptor_functor<sigc::slot0<bool> >::operator()() const (this=0x55555c6ea7d0) at /usr/include/sigc++-2.0/sigc++/adaptors/adaptor_trait.h:256
#14 0x0000555555c3086b in sigc::internal::slot_call<sigc::slot0<bool>, bool>::call_it(sigc::internal::slot_rep*) (rep=0x55555c6ea7a0) at /usr/include/sigc++-2.0/sigc++/functors/slot.h:461
#15 0x0000555555c3270e in sigc::internal::signal_emit0<bool, sigc::nil>::emit(sigc::internal::signal_impl*) (impl=0x55555b97dfd0) at /usr/include/sigc++-2.0/sigc++/signal.h:709
#16 0x0000555555c30afe in sigc::signal0<bool, sigc::nil>::emit() const (this=0x7fff7c05c210) at /usr/include/sigc++-2.0/sigc++/signal.h:2804
#17 0x0000555555cadc8f in ProgressConnector<rtengine::InitialImage*>::emitEndSignalUI(void*) (data=0x7fff7c05c210) at /home/user/src/rawtherapee/rtgui/progressconnector.h:78
#18 0x00007ffff6aa06e8 in gdk_threads_dispatch (data=0x55555dc99d40) at ././gdk/gdk.c:743
GThreadLock
#19 0x00007ffff54026aa in g_main_dispatch (context=0x55555697a670) at ././glib/gmain.c:3203
#20 0x00007ffff54026aa in g_main_context_dispatch (context=context@entry=0x55555697a670) at ././glib/gmain.c:3856
#21 0x00007ffff5402a60 in g_main_context_iterate (context=0x55555697a670, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ././glib/gmain.c:3929
#22 0x00007ffff5402d82 in g_main_loop_run (loop=0x555559841300) at ././glib/gmain.c:4125
#23 0x00007ffff6f99d55 in gtk_main () at ././gtk/gtkmain.c:1312
#24 0x00007ffff3d1ffb1 in Gtk::Main::run_impl() (this=<optimized out>) at main.cc:439
#25 0x00007ffff3d1ffb1 in Gtk::Main::run(Gtk::Window&) (window=...) at main.cc:398
#26 0x0000555555d5e0f7 in main(int, char**) (argc=1, argv=0x7fffffffe228) at /home/user/src/rawtherapee/rtgui/main.cc:671

gdk_threads_dispatch() does what it is supposed to do under global lock (that's the same that we lock by GThreadLock). Way up in the hierarchy it tries to lock ImProcCoordinator::updaterThreadStart.

Waits for Thread 66
Thread 70 (Thread 0x7fffb6ffd700 (LWP 16364)):
#0  0x00007ffff14356cd in pthread_join (threadid=140736272000768, thread_return=thread_return@entry=0x0) at pthread_join.c:90
#1  0x00007ffff5448147 in g_system_thread_wait (thread=0x55555c6385e0) at ././glib/gthread-posix.c:1212
#2  0x00007ffff542a83a in g_thread_join (thread=0x55555c6385e0) at ././glib/gthread.c:952
#3  0x0000555555f8c54e in rtengine::Crop::fullUpdate() (this=0x55555c5f44f0) at /home/user/src/rawtherapee/rtengine/dcrop.cc:1407
updaterThreadStart
#4  0x0000555555bac0e2 in sigc::bound_mem_functor0<void, rtengine::DetailedCrop>::operator()() const (this=0x55555b98e4d8) at /usr/include/sigc++-2.0/sigc++/functors/mem_fun.h:1991
#5  0x0000555555bac02a in sigc::adaptor_functor<sigc::bound_mem_functor0<void, rtengine::DetailedCrop> >::operator()() const (this=0x55555b98e4d0) at /usr/include/sigc++-2.0/sigc++/adaptors/adaptor_trait.h:256
#6  0x0000555555babf05 in sigc::internal::slot_call0<sigc::bound_mem_functor0<void, rtengine::DetailedCrop>, void>::call_it(sigc::internal::slot_rep*) (rep=0x55555b98e4a0) at /usr/include/sigc++-2.0/sigc++/functors/slot.h:114
#7  0x00007ffff458cdfd in  () at /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#8  0x00007ffff542a3d5 in g_thread_proxy (data=0x5555599335e0) at ././glib/gthread.c:784
#9  0x00007ffff1434494 in start_thread (arg=0x7fffb6ffd700) at pthread_create.c:333
#10 0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97

This lock is held by thread 70. Thread 70 waits for thread 66 to finish its work.

Locked by Thread 1
Thread 66 (Thread 0x7fffb77fe700 (LWP 16360)):
#0  0x00007ffff143cf5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffff1436c06 in __GI___pthread_mutex_lock (mutex=0x5555568c7af0) at ../nptl/pthread_mutex_lock.c:115
#2  0x0000555555d5b00d in (anonymous namespace)::myGdkLockEnter() () at /home/user/src/rawtherapee/rtgui/main.cc:99
#3  0x0000555555aff9a1 in GThreadLock::GThreadLock() (this=0x7fffb77fca4f) at /home/user/src/rawtherapee/rtgui/guiutils.h:90
#4  0x0000555555ed5cf9 in ToolPanelCoordinator::imageTypeChanged(bool, bool, bool) (this=0x555559876fb0, isRaw=true, isBayer=true, isXtrans=false) at /home/user/src/rawtherapee/rtgui/toolpanelcoord.cc:265
GThreadLock
#5  0x00005555562cca4b in rtengine::ImProcCoordinator::updatePreviewImage(int, rtengine::Crop*) (this=0x55555dca7800, todo=4095, cropCall=0x0) at /home/user/src/rawtherapee/rtengine/improccoordinator.cc:218
mProcessing
#6  0x00005555562d2d1f in rtengine::ImProcCoordinator::process() (this=0x55555dca7800) at /home/user/src/rawtherapee/rtengine/improccoordinator.cc:1356
#7  0x00005555562d5840 in sigc::bound_mem_functor0<void, rtengine::ImProcCoordinator>::operator()() const (this=0x55555c61be98) at /usr/include/sigc++-2.0/sigc++/functors/mem_fun.h:1991
#8  0x00005555562d567c in sigc::adaptor_functor<sigc::bound_mem_functor0<void, rtengine::ImProcCoordinator> >::operator()() const (this=0x55555c61be90) at /usr/include/sigc++-2.0/sigc++/adaptors/adaptor_trait.h:256
#9  0x00005555562d53b9 in sigc::internal::slot_call0<sigc::bound_mem_functor0<void, rtengine::ImProcCoordinator>, void>::call_it(sigc::internal::slot_rep*) (rep=0x55555c61be60) at /usr/include/sigc++-2.0/sigc++/functors/slot.h:114
#10 0x00007ffff458cdfd in  () at /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#11 0x00007ffff542a3d5 in g_thread_proxy (data=0x55555c6385e0) at ././glib/gthread.c:784
#12 0x00007ffff1434494 in start_thread (arg=0x7fffb77fe700) at pthread_create.c:333
#13 0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97

Thread 66 on the other hand hangs on GThreadLock held by thread 1: Deadlock. 💥

Ideas how to solve that are welcome.

[Floessie]

How about killing ImProcCoordinator::updaterThreadStart and making ImProcCoordinator::updaterRunning atomic? Then, instead of joining the thread, we spin (with sleep) until process() has finished. After all, it's just coordinating whether a thread must be started or not.

Take a look at ImProcCoordinator::thread, destroying, or updateRunning: They are not handled savely. Especially thread is set unlocked.

When starting the thread in detached mode, there would be no need to store thread at all.

I have no idea, what to do with Crop::fullUpdate(). Maybe it will solve itself during refactoring, or it's just another problem lurking there.

Any ideas? @Hombre57?

[Hombre57]

@Floessie

Any ideas? @Hombre57?

No. I never did the effort to understand what is still sorcery to me, so I won't be able to help here, sorry.

[Floessie]

@Hombre57 But maybe you understand the intended goal of this code better than I/we do...

[Hombre57]

@Floessie I have to investigate again to refresh my NAND cells. I spent the whole evening to do merges, so I'll look tomorrow after work.

[Floessie]

As a proof of concept I implemented spinlocking (which is sh*t in userspace, I know), and guess what: It doesn't work, as suspected.

[Floessie]

This fixes the deadlock for me:

diff --git a/rtgui/toolpanelcoord.cc b/rtgui/toolpanelcoord.cc
index e9cb3ea43..00582942c 100644
--- a/rtgui/toolpanelcoord.cc
+++ b/rtgui/toolpanelcoord.cc
@@ -253,6 +253,7 @@ void ToolPanelCoordinator::addPanel (Gtk::Box* where, FoldableToolPanel* panel,

 ToolPanelCoordinator::~ToolPanelCoordinator ()
 {
+    idle_register.destroy();

     closeImage ();

@@ -262,29 +263,58 @@ ToolPanelCoordinator::~ToolPanelCoordinator ()

 void ToolPanelCoordinator::imageTypeChanged (bool isRaw, bool isBayer, bool isXtrans)
 {
-    GThreadLock lock;
-
     if (isRaw) {
-        rawPanelSW->set_sensitive (true);
-
         if (isBayer) {
-            sensorxtrans->FoldableToolPanel::hide();
-            sensorbayer->FoldableToolPanel::show();
-            preprocess->FoldableToolPanel::show();
-            flatfield->FoldableToolPanel::show();
-        } else if (isXtrans) {
-            sensorxtrans->FoldableToolPanel::show();
-            sensorbayer->FoldableToolPanel::hide();
-            preprocess->FoldableToolPanel::show();
-            flatfield->FoldableToolPanel::show();
-        } else {
-            sensorbayer->FoldableToolPanel::hide();
-            sensorxtrans->FoldableToolPanel::hide();
-            preprocess->FoldableToolPanel::hide();
-            flatfield->FoldableToolPanel::hide();
+            const auto func = [](gpointer data) -> gboolean {
+                ToolPanelCoordinator* const self = static_cast<ToolPanelCoordinator*>(data);
+
+                self->rawPanelSW->set_sensitive (true);
+                self->sensorxtrans->FoldableToolPanel::hide();
+                self->sensorbayer->FoldableToolPanel::show();
+                self->preprocess->FoldableToolPanel::show();
+                self->flatfield->FoldableToolPanel::show();
+
+                return FALSE;
+            };
+            idle_register.add(func, this);
+        }
+        else if (isXtrans) {
+            const auto func = [](gpointer data) -> gboolean {
+                ToolPanelCoordinator* const self = static_cast<ToolPanelCoordinator*>(data);
+
+                self->rawPanelSW->set_sensitive (true);
+                self->sensorxtrans->FoldableToolPanel::show();
+                self->sensorbayer->FoldableToolPanel::hide();
+                self->preprocess->FoldableToolPanel::show();
+                self->flatfield->FoldableToolPanel::show();
+
+                return FALSE;
+            };
+            idle_register.add(func, this);
+        }
+        else {
+            const auto func = [](gpointer data) -> gboolean {
+                ToolPanelCoordinator* const self = static_cast<ToolPanelCoordinator*>(data);
+
+                self->rawPanelSW->set_sensitive (true);
+                self->sensorbayer->FoldableToolPanel::hide();
+                self->sensorxtrans->FoldableToolPanel::hide();
+                self->preprocess->FoldableToolPanel::hide();
+                self->flatfield->FoldableToolPanel::hide();
+
+                return FALSE;
+            };
+            idle_register.add(func, this);
         }
     } else {
-        rawPanelSW->set_sensitive (false);
+        const auto func = [](gpointer data) -> gboolean {
+            ToolPanelCoordinator* const self = static_cast<ToolPanelCoordinator*>(data);
+
+            self->rawPanelSW->set_sensitive (false);
+
+            return FALSE;
+        };
+        idle_register.add(func, this);
     }

 }
diff --git a/rtgui/toolpanelcoord.h b/rtgui/toolpanelcoord.h
index c6d2e6380..b387ac988 100644
--- a/rtgui/toolpanelcoord.h
+++ b/rtgui/toolpanelcoord.h
@@ -301,6 +301,9 @@ public:
     void editModeSwitchedOff ();

     void setEditProvider (EditDataProvider *provider);
+
+private:
+    IdleRegister idle_register;
 };

 #endif

[agriggio]

@Floessie seems to work fine here! Excellent! :+1:

[agriggio]

:+1: from me to push both patches

[Floessie]

With the above patch it proceeds much further than before but either segfaults or deadlocks later:

#0  0x0000000000000000 in  ()
#1  0x0000555555bbf749 in rtengine::Crop::setCropSizes(int, int, int, int, int, bool) ()
#2  0x0000555555bc51e1 in rtengine::Crop::update(int) ()
#3  0x0000555555bc7c4c in rtengine::Crop::fullUpdate() ()
#4  0x00007ffff458cdfd in  () at /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#5  0x00007ffff542a3d5 in g_thread_proxy (data=0x55555c0589e0) at ././glib/gthread.c:784
#6  0x00007ffff1434494 in start_thread (arg=0x7fff89ffb700) at pthread_create.c:333
#7  0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97

Thread 326 (Thread 0x7fff56e96700 (LWP 9695)):
#0  0x00007ffff143cf5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffff1436c06 in __GI___pthread_mutex_lock (mutex=0x55555c418870) at ../nptl/pthread_mutex_lock.c:115
#2  0x0000555555b4a8e6 in MyMutex::lock() (this=0x55555da4d088) at /home/user/src/rawtherapee/rtgui/../rtengine/../rtgui/threadutils.h:162
#3  0x0000555555f8d25b in rtengine::Crop::fullUpdate() (this=0x55555d5bd250) at /home/user/src/rawtherapee/rtengine/dcrop.cc:1401
#4  0x0000555555bac10a in sigc::bound_mem_functor0<void, rtengine::DetailedCrop>::operator()() const (this=0x555558183c58) at /usr/include/sigc++-2.0/sigc++/functors/mem_fun.h:1991
#5  0x0000555555bac052 in sigc::adaptor_functor<sigc::bound_mem_functor0<void, rtengine::DetailedCrop> >::operator()() const (this=0x555558183c50) at /usr/include/sigc++-2.0/sigc++/adaptors/adaptor_trait.h:256
#6  0x0000555555babf2d in sigc::internal::slot_call0<sigc::bound_mem_functor0<void, rtengine::DetailedCrop>, void>::call_it(sigc::internal::slot_rep*) (rep=0x555558183c20) at /usr/include/sigc++-2.0/sigc++/functors/slot.h:114
#7  0x00007ffff458cdfd in  () at /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#8  0x00007ffff542a3d5 in g_thread_proxy (data=0x55555c4c0c50) at ././glib/gthread.c:784
#9  0x00007ffff1434494 in start_thread (arg=0x7fff56e96700) at pthread_create.c:333
#10 0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97

Thread 171 (Thread 0x7fff8dffb700 (LWP 9537)):
#0  0x00007ffff143cf5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffff1436c06 in __GI___pthread_mutex_lock (mutex=0x55555ac46540) at ../nptl/pthread_mutex_lock.c:115
#2  0x0000555555b4a8e6 in MyMutex::lock() (this=0x55555da4d088) at /home/user/src/rawtherapee/rtgui/../rtengine/../rtgui/threadutils.h:162
#3  0x0000555555f8d25b in rtengine::Crop::fullUpdate() (this=0x55555d630280) at /home/user/src/rawtherapee/rtengine/dcrop.cc:1401
#4  0x0000555555bac10a in sigc::bound_mem_functor0<void, rtengine::DetailedCrop>::operator()() const (this=0x55555c6c1898) at /usr/include/sigc++-2.0/sigc++/functors/mem_fun.h:1991
#5  0x0000555555bac052 in sigc::adaptor_functor<sigc::bound_mem_functor0<void, rtengine::DetailedCrop> >::operator()() const (this=0x55555c6c1890) at /usr/include/sigc++-2.0/sigc++/adaptors/adaptor_trait.h:256
#6  0x0000555555babf2d in sigc::internal::slot_call0<sigc::bound_mem_functor0<void, rtengine::DetailedCrop>, void>::call_it(sigc::internal::slot_rep*) (rep=0x55555c6c1860) at /usr/include/sigc++-2.0/sigc++/functors/slot.h:114
#7  0x00007ffff458cdfd in  () at /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1
#8  0x00007ffff542a3d5 in g_thread_proxy (data=0x55555c69ed90) at ././glib/gthread.c:784
#9  0x00007ffff1434494 in start_thread (arg=0x7fff8dffb700) at pthread_create.c:333
#10 0x00007ffff1176aff in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97

Thread 1 (Thread 0x7ffff7f2fb00 (LWP 9360)):
#0  0x00007ffff143cf5c in __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x00007ffff1436c06 in __GI___pthread_mutex_lock (mutex=0x5555568c8af0) at ../nptl/pthread_mutex_lock.c:115
#2  0x0000555555d5b05f in (anonymous namespace)::myGdkLockEnter() () at /home/user/src/rawtherapee/rtgui/main.cc:99
#3  0x00007ffff6adcece in gdk_event_source_check (source=0x55555697d000) at ././gdk/x11/gdkeventsource.c:300
#4  0x00007ffff54023f9 in g_main_context_check (context=context@entry=0x55555697d0f0, max_priority=2147483647, fds=fds@entry=0x5555597c0fe0, n_fds=n_fds@entry=3) at ././glib/gmain.c:3762
#5  0x00007ffff5402994 in g_main_context_iterate (context=0x55555697d0f0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ././glib/gmain.c:3926
#6  0x00007ffff5402d82 in g_main_loop_run (loop=0x555559765a70) at ././glib/gmain.c:4125
#7  0x00007ffff6f99d55 in gtk_main () at ././gtk/gtkmain.c:1312
#8  0x00007ffff3d1ffb1 in Gtk::Main::run_impl() (this=<optimized out>) at main.cc:439
#9  0x00007ffff3d1ffb1 in Gtk::Main::run(Gtk::Window&) (window=...) at main.cc:398
#10 0x0000555555d5e149 in main(int, char**) (argc=1, argv=0x7fffffffe228) at /home/user/src/rawtherapee/rtgui/main.cc:671

[agriggio]

hmmm... :angry: :sob: and yet I did my best to make it deadlock, but failed... I'll try on my old laptop tonight

[Floessie]

@agriggio Thanks for your tests! It could as well be that I ran out of memory. At least it's better than before now, so I'll push that patch, too. The crop update is completely screwed, IMHO (I guess you agree), so I hope on group intelligence for a comprehensive solution.

@Beep6581 I'll push the patch to fix-coarse-display tonight. IMO, this is relevant for 5.4. The bug itself is older than our git history. And it's broken by design, I'm afraid.