Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically disable Auto-Fill when enabling LCP Disortion Correction, #1791 #3318

Merged
merged 2 commits into from Jun 3, 2016

Conversation

Beep6581
Copy link
Owner

fillConn.block (true);
fill->set_active(false);
if (listener) {
listener->panelChanged (EvTransAutoFill, M("GENERAL_DISABLED"));
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line causes the release build to hang when opening a file, and the debug build prints:
MyMutex already locked!
If I comment out this line, no problem. But we need feedback via history so the user knows what's happening.

@heckflosse
Copy link
Collaborator

@Floessie Do you have an idea to solve this issue?

@Floessie
Copy link
Collaborator

@heckflosse @Beep6581 Okay, I had a quick look and know what's happening, although I have no instant idea how to solve this.

First a patch to backtrace in the apllication itself (though gdb also revealed the path, but this here is more obvious). Feel free to integrate it somewhere else. Only tested with GCC on Linux:

diff --git a/rtengine/improccoordinator.cc b/rtengine/improccoordinator.cc
index 7434776..779a4da 100644
--- a/rtengine/improccoordinator.cc
+++ b/rtengine/improccoordinator.cc
@@ -1339,6 +1339,7 @@ void ImProcCoordinator::process ()

 ProcParams* ImProcCoordinator::beginUpdateParams ()
 {
+    backtrace();
     paramsUpdateMutex.lock ();

     return &nextParams;
diff --git a/rtengine/utils.cc b/rtengine/utils.cc
index 2366f3a..59bbdd2 100644
--- a/rtengine/utils.cc
+++ b/rtengine/utils.cc
@@ -16,15 +16,38 @@
  *  You should have received a copy of the GNU General Public License
  *  along with RawTherapee.  If not, see <http://www.gnu.org/licenses/>.
  */
+#include <string>
 #include <cmath>
 #include <cstring>
 #include <cstdio>
-#include "rt_math.h"
+
+#include <execinfo.h>
+#include <cxxabi.h>

 #include "utils.h"
 #include "rt_math.h"

-using namespace std;
+namespace
+{
+
+std::string demangleTypename(const char* mangled_typename)
+{
+    std::string res;
+
+    int status = 0;
+    char* const demangled_typename = abi::__cxa_demangle(mangled_typename, nullptr, nullptr, &status);
+    if (!status) {
+        res = demangled_typename;
+        free(demangled_typename);
+    } else {
+        res = mangled_typename;
+    }
+
+    return res;
+}
+
+}

 namespace rtengine
 {
@@ -228,6 +251,30 @@ void vflip (unsigned char* img, int w, int h)
     delete [] flipped;
 }

+void backtrace()
+{
+    void* items[100];
+    const size_t items_received = ::backtrace(items, 100);
+
+    char** strings = backtrace_symbols(items, items_received);
+    std::fprintf(stderr, "Obtained %zu stack frames:\n", items_received);
+
+    for (size_t i = 0; i < items_received; ++i) {
+        std::string str = strings[i];
+
+        std::string::size_type start_pos = str.find("(_Z");
+        const std::string::size_type end_pos = str.find("+0x", start_pos);
+        if (start_pos != std::string::npos && end_pos != std::string::npos) {
+            ++start_pos;
+            str = str.substr(0, start_pos) + demangleTypename(str.substr(start_pos, end_pos - start_pos).c_str()) + str.substr(end_pos);
+        }
+
+        std::fprintf(stderr, "  %02zu: %s\n", i, str.c_str());
+    }
+
+    std::free(strings);
+}
+
 }


diff --git a/rtengine/utils.h b/rtengine/utils.h
index 1e742ff..e7568f3 100644
--- a/rtengine/utils.h
+++ b/rtengine/utils.h
@@ -35,5 +35,7 @@ void rotate (unsigned char* img, int& w, int& h, int deg);
 void hflip (unsigned char* img, int w, int h);
 void vflip (unsigned char* img, int w, int h);

+void backtrace();
+
 }
 #endif

Now the discovery:

Obtained 21 stack frames:
  00: ./debug/rawtherapee(rtengine::backtrace()+0x20) [0xd321d6]
  01: ./debug/rawtherapee(rtengine::ImProcCoordinator::beginUpdateParams()+0x11) [0xd15ebb]
  02: ./debug/rawtherapee(ToolPanelCoordinator::profileChange(rtengine::procparams::PartialProfile const*, rtengine::ProcEvent, Glib::ustring const&, ParamsEdited const*)+0x72) [0xac86f0]
  03: ./debug/rawtherapee(ProfilePanel::initProfile(Glib::ustring const&, rtengine::procparams::ProcParams*)+0x485) [0xa8cb09]
  04: ./debug/rawtherapee(EditorPanel::open(Thumbnail*, rtengine::InitialImage*)+0x462) [0x8134b6]
  05: ./debug/rawtherapee(FilePanel::imageLoaded(Thumbnail*, ProgressConnector<rtengine::InitialImage*>*)+0x323) [0x8008ad]
  06: ./debug/rawtherapee(sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*>::operator()(Thumbnail* const&, ProgressConnector<rtengine::InitialImage*>* const&) const+0x7c) [0x807ab6]
  07: ./debug/rawtherapee(sigc::adaptor_functor<sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*> >::deduce_result_type<Thumbnail*&, ProgressConnector<rtengine::InitialImage*>*&, void, void, void, void, void>::type sigc::adaptor_functor<sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*> >::operator()<Thumbnail*&, ProgressConnector<rtengine::InitialImage*>*&>(Thumbnail*&, ProgressConnector<rtengine::InitialImage*>*&) const+0x2f) [0x806d7b]
  08: ./debug/rawtherapee(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()()+0x49) [0x805d11]
  09: ./debug/rawtherapee(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*)+0x24) [0x804b7f]
  10: ./debug/rawtherapee(sigc::internal::signal_emit0<bool, sigc::nil>::emit(sigc::internal::signal_impl*)+0x1d2) [0x806f72]
  11: ./debug/rawtherapee(sigc::signal0<bool, sigc::nil>::emit() const+0x1c) [0x806062]
  12: ./debug/rawtherapee(ProgressConnector<rtengine::InitialImage*>::emitEndSignalUI(void*)+0x21) [0x804c49]
  13: /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_context_dispatch+0x15a) [0x7fad0572705a]
  14: /lib/x86_64-linux-gnu/libglib-2.0.so.0(+0x4a400) [0x7fad05727400]
  15: /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_loop_run+0xc2) [0x7fad05727722]
  16: /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0(gtk_main+0xb7) [0x7fad04b395b7]
  17: /usr/lib/x86_64-linux-gnu/libgtkmm-2.4.so.1(Gtk::Main::run(Gtk::Window&)+0x11f) [0x7fad02d7192f]
  18: ./debug/rawtherapee(main+0xffc) [0x9ce26a]
  19: /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7facff9d55f0]
  20: ./debug/rawtherapee(_start+0x29) [0x7772b9]
Obtained 37 stack frames:
  00: ./debug/rawtherapee(rtengine::backtrace()+0x20) [0xd321d6]
  01: ./debug/rawtherapee(rtengine::ImProcCoordinator::beginUpdateParams()+0x11) [0xd15ebb]
  02: ./debug/rawtherapee(ToolPanelCoordinator::panelChanged(rtengine::ProcEvent, Glib::ustring const&)+0x74) [0xac8074]
  03: ./debug/rawtherapee(LensGeometry::disableAutoFillIfActive()+0xd7) [0x88b2f3]
  04: ./debug/rawtherapee(LensProfilePanel::onUseDistChanged()+0x4b) [0x90a8dd]
  05: ./debug/rawtherapee(sigc::bound_mem_functor0<void, LensProfilePanel>::operator()() const+0x66) [0x90b64e]
  06: ./debug/rawtherapee(sigc::adaptor_functor<sigc::bound_mem_functor0<void, LensProfilePanel> >::operator()() const+0x1c) [0x90b596]
  07: ./debug/rawtherapee(sigc::internal::slot_call0<sigc::bound_mem_functor0<void, LensProfilePanel>, void>::call_it(sigc::internal::slot_rep*)+0x24) [0x90b46d]
  08: /usr/lib/x86_64-linux-gnu/libglibmm-2.4.so.1(Glib::SignalProxyNormal::slot0_void_callback(_GObject*, void*)+0x28) [0x7fad052b84d8]
  09: /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0(g_closure_invoke+0x145) [0x7fad059fdfa5]
  10: /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0(+0x22264) [0x7fad05a10264]
  11: /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0(g_signal_emit_valist+0xfbc) [0x7fad05a18d5c]
  12: /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0(g_signal_emit+0x8f) [0x7fad05a1908f]
  13: /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0(+0x1f50e3) [0x7fad04bfe0e3]
  14: /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0(+0x101d4) [0x7fad059fe1d4]
  15: /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0(g_signal_emit_valist+0xc06) [0x7fad05a189a6]
  16: /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0(g_signal_emit+0x8f) [0x7fad05a1908f]
  17: ./debug/rawtherapee(LensProfilePanel::read(rtengine::procparams::ProcParams const*, ParamsEdited const*)+0x23d) [0x90a12d]
  18: ./debug/rawtherapee(ToolPanelCoordinator::profileChange(rtengine::procparams::PartialProfile const*, rtengine::ProcEvent, Glib::ustring const&, ParamsEdited const*)+0x381) [0xac89ff]
  19: ./debug/rawtherapee(ProfilePanel::initProfile(Glib::ustring const&, rtengine::procparams::ProcParams*)+0x485) [0xa8cb09]
  20: ./debug/rawtherapee(EditorPanel::open(Thumbnail*, rtengine::InitialImage*)+0x462) [0x8134b6]
  21: ./debug/rawtherapee(FilePanel::imageLoaded(Thumbnail*, ProgressConnector<rtengine::InitialImage*>*)+0x323) [0x8008ad]
  22: ./debug/rawtherapee(sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*>::operator()(Thumbnail* const&, ProgressConnector<rtengine::InitialImage*>* const&) const+0x7c) [0x807ab6]
  23: ./debug/rawtherapee(sigc::adaptor_functor<sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*> >::deduce_result_type<Thumbnail*&, ProgressConnector<rtengine::InitialImage*>*&, void, void, void, void, void>::type sigc::adaptor_functor<sigc::bound_mem_functor2<bool, FilePanel, Thumbnail*, ProgressConnector<rtengine::InitialImage*>*> >::operator()<Thumbnail*&, ProgressConnector<rtengine::InitialImage*>*&>(Thumbnail*&, ProgressConnector<rtengine::InitialImage*>*&) const+0x2f) [0x806d7b]
  24: ./debug/rawtherapee(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()()+0x49) [0x805d11]
  25: ./debug/rawtherapee(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*)+0x24) [0x804b7f]
  26: ./debug/rawtherapee(sigc::internal::signal_emit0<bool, sigc::nil>::emit(sigc::internal::signal_impl*)+0x1d2) [0x806f72]
  27: ./debug/rawtherapee(sigc::signal0<bool, sigc::nil>::emit() const+0x1c) [0x806062]
  28: ./debug/rawtherapee(ProgressConnector<rtengine::InitialImage*>::emitEndSignalUI(void*)+0x21) [0x804c49]
  29: /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_context_dispatch+0x15a) [0x7fad0572705a]
  30: /lib/x86_64-linux-gnu/libglib-2.0.so.0(+0x4a400) [0x7fad05727400]
  31: /lib/x86_64-linux-gnu/libglib-2.0.so.0(g_main_loop_run+0xc2) [0x7fad05727722]
  32: /usr/lib/x86_64-linux-gnu/libgtk-x11-2.0.so.0(gtk_main+0xb7) [0x7fad04b395b7]
  33: /usr/lib/x86_64-linux-gnu/libgtkmm-2.4.so.1(Gtk::Main::run(Gtk::Window&)+0x11f) [0x7fad02d7192f]
  34: ./debug/rawtherapee(main+0xffc) [0x9ce26a]
  35: /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7facff9d55f0]
  36: ./debug/rawtherapee(_start+0x29) [0x7772b9]

Analysis: rtengine::ImProcCoordinator::paramsUpdateMutex is locked twice, because listener->panelChanged() introduces an unwanted recursion. Having it deadlock is prehaps the nicest outcome for this. ;) Otherwise the user had to wait until all memory is consumed.

Solution: Make LensProfilePanel::onUseDistChanged() or ImProcCoordinator::beginUpdateParams() recursion aware by passing a bool parameter to it which defaults to false for callers from outside and propagate it back to it as true. This is, how I normally break those thing up. Don't know how practically this is here, and how well this works with sigc++ on the path. I guess, both of you have more insight and solve the problem more elegantly. Maybe the listener->panelChanged() is too much in this place. Or maybe you could move it to another thread. As a last resort, one can create a temporary (self-destroying) thread just to perform this action from another context.

HTH,
Flössie

@Beep6581
Copy link
Owner Author

Beep6581 commented May 31, 2016

I sometimes try to make patches, for the fun of it, but I am not a C++ developer, and you have infinitely more insight into this than I do. I hope to get to that level at some time in the future, but in the last few months I had to learn VB6 and I generally still have to learn Swedish, so learning C++ must be put on hold.

The patch works fine if I remove the listener->panelChanged part, but it is important that the user gets notified that Auto-Fill is being disabled, and using History for that is the intuitive choice. Can you help make the patch do that, preferably without using up all memory or hanging? :)

@Floessie
Copy link
Collaborator

Floessie commented Jun 1, 2016

@Beep6581 I'll have a look.

@Floessie
Copy link
Collaborator

Floessie commented Jun 1, 2016

@Beep6581 I think, I found a solution, though I'm no GUI expert. :) The idea is to shift the work to the main loop, as GTK is not thread-safe (which I grievously realized after employing std::thread to delay the work). Therefore I'm unsure, if the GThreadLock lock; line is really needed. It's working without, but perhaps @Hombre57 can shed some light on it.

Here is the patch:

diff --git a/rtgui/lensgeom.cc b/rtgui/lensgeom.cc
index d50a16f..1d39ced 100644
--- a/rtgui/lensgeom.cc
+++ b/rtgui/lensgeom.cc
@@ -43,6 +43,11 @@ LensGeometry::LensGeometry () : FoldableToolPanel(this, "lensgeom", M("TP_LENSGE
     show_all ();
 }

+LensGeometry::~LensGeometry ()
+{
+    g_idle_remove_by_data(this);
+}
+
 void LensGeometry::read (const ProcParams* pp, const ParamsEdited* pedited)
 {

@@ -116,16 +121,23 @@ void LensGeometry::setBatchMode (bool batchMode)

 void LensGeometry::disableAutoFillIfActive ()
 {
+    g_idle_add(doDisableAutoFillIfActive, this);
+}

-    if (!batchMode) {
-        if (fill->get_active()) {
-            fillConn.block (true);
-            fill->set_active(false);
-            if (listener) {
-                listener->panelChanged (EvTransAutoFill, M("GENERAL_DISABLED"));
+int LensGeometry::doDisableAutoFillIfActive (void* data)
+{
+    GThreadLock lock; // Is this really needed?
+
+    LensGeometry* const instance = static_cast<LensGeometry*>(data);
+
+    if (!instance->batchMode) {
+        if (instance->fill->get_active()) {
+            instance->fillConn.block (true);
+            instance->fill->set_active(false);
+            if (instance->listener) {
+                instance->listener->panelChanged (EvTransAutoFill, M("GENERAL_DISABLED"));
             }
-            fillConn.block (false);
+            instance->fillConn.block (false);
         }
     }
-
 }
diff --git a/rtgui/lensgeom.h b/rtgui/lensgeom.h
index fe21feb..51a6e10 100644
--- a/rtgui/lensgeom.h
+++ b/rtgui/lensgeom.h
@@ -32,11 +32,12 @@ protected:
     Gtk::CheckButton*   fill;
     bool                lastFill;
     sigc::connection    fillConn;
-    ToolParamBlock*           packBox;
+    ToolParamBlock*     packBox;

 public:

     LensGeometry ();
+    ~LensGeometry ();

     Gtk::Box* getPackBox ()
     {
@@ -54,6 +55,10 @@ public:
         rlistener = l;
     }
     void disableAutoFillIfActive ();
+
+private:
+    static int doDisableAutoFillIfActive (void* data);
+
 };

 #endif

HTH,
Flössie

@Floessie
Copy link
Collaborator

Floessie commented Jun 3, 2016

@Beep6581 Is it working for you? If @Hombre57 is busy, I'd say we keep the GThreadLock lock; line, as there are other places in RT following this scheme.

@Beep6581
Copy link
Owner Author

Beep6581 commented Jun 3, 2016

@Floessie I tested it and it works great! Thank you. 👍 for merge.

@Floessie
Copy link
Collaborator

Floessie commented Jun 3, 2016

Utmärkt!

@Beep6581
Copy link
Owner Author

Beep6581 commented Jun 3, 2016

@Floessie you can merge, you now have access. Welcome on board :)

@Floessie
Copy link
Collaborator

Floessie commented Jun 3, 2016

That's an honor. Thanks!

@Floessie Floessie merged commit 8b5cf38 into master Jun 3, 2016
@Floessie Floessie deleted the lcpautofill branch June 3, 2016 17:30
@heckflosse
Copy link
Collaborator

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Something could be better than it currently is
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants