Pipe threads locks: remove redundant locks, change the logic

- Remove dev->pipe_mutex (shared between pipes) that was redundant wich a much older pipe->busy_mutex lock.
- Remove pipe->busy_mutex locks from internal functions and protect top-most functions, aka pipeline jobs.

So, from there we have 3 locks:

- dt_mimap_cache_lock/release, which captures an app-global lock to protect access to image buffer (input), from disk or memory cache
- dev->history_mutex, which captures a dev-centric lock to protect history manipulations from/to the development stack
- pipe->busy_mutex, which captures a pipe-centric lock to protect the pipeline nodes topology.

Remains to implement an app-wise DB lock to ensure only  one part of the soft can access DB histories at the same time.

Partial fix for #262

src/common/imageio.c

@@ -1052,7 +1052,6 @@ int dt_imageio_export_with_flags(const int32_t imgid, const char *filename,
 
   dt_mipmap_cache_release(darktable.mipmap_cache, &buf);
   dt_dev_pixelpipe_cleanup(&pipe);
-  dt_dev_unload_image(&dev);
   dt_dev_cleanup(&dev);
 
   /* now write xmp into that container, if possible */

src/develop/develop.c

@@ -67,7 +67,6 @@ void dt_dev_init(dt_develop_t *dev, int32_t gui_attached)
   dev->image_status = dev->preview_status = DT_DEV_PIXELPIPE_DIRTY;
   dev->image_invalid_cnt = 0;
   dev->pipe = dev->preview_pipe = NULL;
-  dt_pthread_mutex_init(&dev->pipe_mutex, NULL);
   dev->histogram_pre_tonecurve = NULL;
   dev->histogram_pre_levels = NULL;
   dev->forms = NULL;
@@ -146,7 +145,6 @@ void dt_dev_cleanup(dt_develop_t *dev)
 {
   if(!dev) return;
   // image_cache does not have to be unref'd, this is done outside develop module.
-  dt_pthread_mutex_destroy(&dev->pipe_mutex);
 
   if(dev->raw_histogram.buffer) dt_free_align(dev->raw_histogram.buffer);
   if(dev->output_histogram.buffer) dt_free_align(dev->output_histogram.buffer);
@@ -302,7 +300,7 @@ void dt_dev_invalidate_all_real(dt_develop_t *dev)
 
 void dt_dev_process_preview_job(dt_develop_t *dev)
 {
-  dt_pthread_mutex_lock(&dev->pipe_mutex);
+  dt_pthread_mutex_lock(&dev->preview_pipe->busy_mutex);
   dt_control_log_busy_enter();
   dt_control_toast_busy_enter();
   dev->preview_status = DT_DEV_PIXELPIPE_RUNNING;
@@ -316,7 +314,7 @@ void dt_dev_process_preview_job(dt_develop_t *dev)
     dt_control_log_busy_leave();
     dt_control_toast_busy_leave();
     dt_mipmap_cache_release(darktable.mipmap_cache, &buf);
-    dt_pthread_mutex_unlock(&dev->pipe_mutex);
+    dt_pthread_mutex_unlock(&dev->preview_pipe->busy_mutex);
     return;
   }
 
@@ -347,7 +345,7 @@ void dt_dev_process_preview_job(dt_develop_t *dev)
       dt_control_toast_busy_leave();
       dev->preview_status = DT_DEV_PIXELPIPE_INVALID;
       dt_mipmap_cache_release(darktable.mipmap_cache, &buf);
-      dt_pthread_mutex_unlock(&dev->pipe_mutex);
+      dt_pthread_mutex_unlock(&dev->preview_pipe->busy_mutex);
       return;
     }
   }
@@ -361,15 +359,15 @@ void dt_dev_process_preview_job(dt_develop_t *dev)
   dt_control_log_busy_leave();
   dt_control_toast_busy_leave();
   dt_mipmap_cache_release(darktable.mipmap_cache, &buf);
-  dt_pthread_mutex_unlock(&dev->pipe_mutex);
+  dt_pthread_mutex_unlock(&dev->preview_pipe->busy_mutex);
 
   DT_DEBUG_CONTROL_SIGNAL_RAISE(darktable.signals, DT_SIGNAL_DEVELOP_PREVIEW_PIPE_FINISHED);
 }
 
 
 void dt_dev_process_image_job(dt_develop_t *dev)
 {
-  dt_pthread_mutex_lock(&dev->pipe_mutex);
+  dt_pthread_mutex_lock(&dev->pipe->busy_mutex);
   dt_control_log_busy_enter();
   dt_control_toast_busy_enter();
   // let gui know to draw preview instead of us, if it's there:
@@ -383,7 +381,7 @@ void dt_dev_process_image_job(dt_develop_t *dev)
     dt_control_log_busy_leave();
     dt_control_toast_busy_leave();
     dt_mipmap_cache_release(darktable.mipmap_cache, &buf);
-    dt_pthread_mutex_unlock(&dev->pipe_mutex);
+    dt_pthread_mutex_unlock(&dev->pipe->busy_mutex);
     return;
   }
 
@@ -439,7 +437,7 @@ restart:;
       dt_control_toast_busy_leave();
       dev->image_status = DT_DEV_PIXELPIPE_INVALID;
       dt_mipmap_cache_release(darktable.mipmap_cache, &buf);
-      dt_pthread_mutex_unlock(&dev->pipe_mutex);
+      dt_pthread_mutex_unlock(&dev->pipe->busy_mutex);
       return;
     }
   }
@@ -458,7 +456,7 @@ restart:;
   dt_control_log_busy_leave();
   dt_control_toast_busy_leave();
   dt_mipmap_cache_release(darktable.mipmap_cache, &buf);
-  dt_pthread_mutex_unlock(&dev->pipe_mutex);
+  dt_pthread_mutex_unlock(&dev->pipe->busy_mutex);
 
   if(dev->gui_attached)
     DT_DEBUG_CONTROL_SIGNAL_RAISE(darktable.signals, DT_SIGNAL_DEVELOP_UI_PIPE_FINISHED);
@@ -500,13 +498,8 @@ static inline int _dt_dev_load_raw(dt_develop_t *dev, const uint32_t imgid)
   return (no_valid_image || no_valid_thumb);
 }
 
-void dt_dev_unload_image(dt_develop_t *dev)
-{
-}
-
 void dt_dev_reload_image(dt_develop_t *dev, const uint32_t imgid)
 {
-  dt_dev_unload_image(dev);
   dt_dev_load_image(dev, imgid);
 }
 
@@ -697,7 +690,7 @@ void dt_dev_add_history_item_ext(dt_develop_t *dev, struct dt_iop_module_t *modu
   // but keep the always-on modules
 
   for(GList *history = g_list_nth(dev->history, dt_dev_get_history_end(dev));
-      history;
+      history && history->data;
       history = g_list_next(history))
   {
     dt_dev_history_item_t *hist = (dt_dev_history_item_t *)(history->data);
@@ -1601,7 +1594,7 @@ void dt_dev_read_history_ext(dt_develop_t *dev, const int imgid, gboolean no_ima
     // cleanup
     DT_DEBUG_SQLITE3_EXEC(dt_database_get(darktable.db), "DELETE FROM memory.history", NULL, NULL, NULL);
 
-    dt_print(DT_DEBUG_PARAMS, "[history] temporary history deleted\n");
+    dt_print(DT_DEBUG_HISTORY, "[history] temporary history deleted\n");
 
     // make sure all modules default params are loaded to init history
     _dt_dev_load_pipeline_defaults(dev);
@@ -1614,12 +1607,12 @@ void dt_dev_read_history_ext(dt_develop_t *dev, const int imgid, gboolean no_ima
     first_run = _dev_auto_apply_presets(dev);
     auto_apply_modules = _dev_get_module_nb_records() - default_modules;
 
-    dt_print(DT_DEBUG_PARAMS, "[history] temporary history initialised with default params and presets\n");
+    dt_print(DT_DEBUG_HISTORY, "[history] temporary history initialised with default params and presets\n");
 
     // now merge memory.history into main.history
     _dev_merge_history(dev, imgid);
 
-    dt_print(DT_DEBUG_PARAMS, "[history] temporary history merged with image history\n");
+    dt_print(DT_DEBUG_HISTORY, "[history] temporary history merged with image history\n");
   }
 
   sqlite3_stmt *stmt;
@@ -1876,7 +1869,7 @@ void dt_dev_read_history_ext(dt_develop_t *dev, const int imgid, gboolean no_ima
   if(dev->gui_attached && !no_image)
   {
     /* signal history changed */
-    dt_dev_undo_end_record(dev);
+    //dt_dev_undo_end_record(dev);
   }
   dt_dev_masks_list_change(dev);
 

src/develop/develop.h

@@ -174,7 +174,6 @@ typedef struct dt_develop_t
 
   // image processing pipeline with caching
   struct dt_dev_pixelpipe_t *pipe, *preview_pipe;
-  dt_pthread_mutex_t pipe_mutex; // these are locked while the pipes are still in use
 
   // image under consideration, which
   // is copied each time an image is changed. this means we have some information
@@ -348,7 +347,6 @@ void dt_dev_refresh_ui_images_real(dt_develop_t *dev);
 
 int dt_dev_load_image(dt_develop_t *dev, const uint32_t imgid);
 void dt_dev_reload_image(dt_develop_t *dev, const uint32_t imgid);
-void dt_dev_unload_image(dt_develop_t *dev);
 /** checks if provided imgid is the image currently in develop */
 int dt_dev_is_current_image(dt_develop_t *dev, uint32_t imgid);
 

src/develop/pixelpipe_hb.c

@@ -282,10 +282,6 @@ void dt_dev_pixelpipe_cleanup(dt_dev_pixelpipe_t *pipe)
 
 void dt_dev_pixelpipe_cleanup_nodes(dt_dev_pixelpipe_t *pipe)
 {
-  // FIXME: either this or all process() -> gdk mutices have to be changed!
-  //        (this is a circular dependency on busy_mutex and the gdk mutex)
-  // [[does the above still apply?]]
-  dt_pthread_mutex_lock(&pipe->busy_mutex); // block until the pipe has shut down
   // destroy all nodes
   for(GList *nodes = pipe->nodes; nodes; nodes = g_list_next(nodes))
   {
@@ -311,12 +307,10 @@ void dt_dev_pixelpipe_cleanup_nodes(dt_dev_pixelpipe_t *pipe)
   // and iop order
   g_list_free_full(pipe->iop_order_list, free);
   pipe->iop_order_list = NULL;
-  dt_pthread_mutex_unlock(&pipe->busy_mutex);	// safe for others to mess with the pipe now
 }
 
 void dt_dev_pixelpipe_create_nodes(dt_dev_pixelpipe_t *pipe, dt_develop_t *dev)
 {
-  dt_pthread_mutex_lock(&pipe->busy_mutex); // block until pipe is idle
   // check that the pipe was actually properly cleaned up after the last run
   g_assert(pipe->nodes == NULL);
   g_assert(pipe->iop == NULL);
@@ -359,7 +353,6 @@ void dt_dev_pixelpipe_create_nodes(dt_dev_pixelpipe_t *pipe, dt_develop_t *dev)
     dt_iop_init_pipe(piece->module, pipe, piece);
     pipe->nodes = g_list_append(pipe->nodes, piece);
   }
-  dt_pthread_mutex_unlock(&pipe->busy_mutex); // safe for others to use/mess with the pipe now
 }
 
 static uint64_t _default_pipe_hash(dt_dev_pixelpipe_t *pipe)
@@ -473,8 +466,6 @@ void dt_dev_pixelpipe_synch(dt_dev_pixelpipe_t *pipe, dt_develop_t *dev, GList *
 
 void dt_dev_pixelpipe_synch_all_real(dt_dev_pixelpipe_t *pipe, dt_develop_t *dev, const char *caller_func)
 {
-  dt_pthread_mutex_lock(&pipe->busy_mutex);
-
   dt_print(DT_DEBUG_DEV, "[pixelpipe] synch all modules with defaults_params for pipe %i called from %s\n", pipe->type, caller_func);
 
   // call reset_params on all pieces first. This is mandatory to init utility modules that don't have an history stack
@@ -503,12 +494,10 @@ void dt_dev_pixelpipe_synch_all_real(dt_dev_pixelpipe_t *pipe, dt_develop_t *dev
     dt_dev_pixelpipe_synch(pipe, dev, history);
     ++k;
   }
-  dt_pthread_mutex_unlock(&pipe->busy_mutex);
 }
 
 void dt_dev_pixelpipe_synch_top(dt_dev_pixelpipe_t *pipe, dt_develop_t *dev)
 {
-  dt_pthread_mutex_lock(&pipe->busy_mutex);
   GList *history = g_list_nth(dev->history, dt_dev_get_history_end(dev) - 1);
   if(history)
   {
@@ -520,7 +509,6 @@ void dt_dev_pixelpipe_synch_top(dt_dev_pixelpipe_t *pipe, dt_develop_t *dev)
   {
     dt_print(DT_DEBUG_DEV, "[pixelpipe] synch top history module missing error for pipe %i\n", pipe->type);
   }
-  dt_pthread_mutex_unlock(&pipe->busy_mutex);
 }
 
 void dt_dev_pixelpipe_change(dt_dev_pixelpipe_t *pipe, struct dt_develop_t *dev)
@@ -2177,7 +2165,6 @@ static int dt_dev_pixelpipe_process_rec_and_backcopy(dt_dev_pixelpipe_t *pipe, d
                                                      const dt_iop_roi_t *roi_out, GList *modules, GList *pieces,
                                                      int pos)
 {
-  dt_pthread_mutex_lock(&pipe->busy_mutex);
   darktable.dtresources.group = 4 * darktable.dtresources.level;
 #ifdef HAVE_OPENCL
   dt_opencl_check_tuning(pipe->devid);
@@ -2210,7 +2197,6 @@ static int dt_dev_pixelpipe_process_rec_and_backcopy(dt_dev_pixelpipe_t *pipe, d
     }
   }
 #endif
-  dt_pthread_mutex_unlock(&pipe->busy_mutex);
   return ret;
 }
 
@@ -2276,11 +2262,9 @@ restart:;
     // Well, there were errors -> we might need to free an invalid opencl memory object
     dt_opencl_release_mem_object(cl_mem_out);
     dt_opencl_unlock_device(pipe->devid); // release opencl resource
-    dt_pthread_mutex_lock(&pipe->busy_mutex);
     pipe->opencl_enabled = 0; // disable opencl for this pipe
     pipe->opencl_error = 0;   // reset error status
     pipe->devid = -1;
-    dt_pthread_mutex_unlock(&pipe->busy_mutex);
 
     darktable.opencl->error_count++; // increase error count
     if(darktable.opencl->error_count >= DT_OPENCL_MAX_ERRORS)
@@ -2363,7 +2347,6 @@ void dt_dev_pixelpipe_flush_caches(dt_dev_pixelpipe_t *pipe)
 void dt_dev_pixelpipe_get_roi_out(dt_dev_pixelpipe_t *pipe, struct dt_develop_t *dev, int width_in,
                                      int height_in, int *width, int *height)
 {
-  dt_pthread_mutex_lock(&pipe->busy_mutex);
   dt_iop_roi_t roi_in = (dt_iop_roi_t){ 0, 0, width_in, height_in, 1.0 };
   dt_iop_roi_t roi_out;
   GList *modules = g_list_first(pipe->iop);
@@ -2396,7 +2379,6 @@ void dt_dev_pixelpipe_get_roi_out(dt_dev_pixelpipe_t *pipe, struct dt_develop_t
   }
   *width = roi_out.width;
   *height = roi_out.height;
-  dt_pthread_mutex_unlock(&pipe->busy_mutex);
 }
 
 void dt_dev_pixelpipe_get_roi_in(dt_dev_pixelpipe_t *pipe, struct dt_develop_t *dev, const struct dt_iop_roi_t roi_out)
@@ -2409,7 +2391,6 @@ void dt_dev_pixelpipe_get_roi_in(dt_dev_pixelpipe_t *pipe, struct dt_develop_t *
   // upstream in the pipeline for proper pipeline cache invalidation, so we need to browse the pipeline
   // backwards.
 
-  dt_pthread_mutex_lock(&pipe->busy_mutex);
   dt_iop_roi_t roi_out_temp = roi_out;
   dt_iop_roi_t roi_in;
   GList *modules = g_list_last(pipe->iop);
@@ -2444,7 +2425,6 @@ void dt_dev_pixelpipe_get_roi_in(dt_dev_pixelpipe_t *pipe, struct dt_develop_t *
     modules = g_list_previous(modules);
     pieces = g_list_previous(pieces);
   }
-  dt_pthread_mutex_unlock(&pipe->busy_mutex);
 }
 
 float *dt_dev_get_raster_mask(const dt_dev_pixelpipe_t *pipe, const dt_iop_module_t *raster_mask_source,

src/iop/liquify.c

@@ -2788,10 +2788,10 @@ void gui_post_expose(struct dt_iop_module_t *module,
   dt_iop_gui_leave_critical_section(module);
 
   // distort all points
-  dt_pthread_mutex_lock(&develop->pipe_mutex);
+  dt_pthread_mutex_lock(&develop->preview_pipe->busy_mutex);
   const distort_params_t d_params = { develop, develop->preview_pipe, iscale, 1.0 / scale, DT_DEV_TRANSFORM_DIR_ALL, FALSE };
   _distort_paths(module, &d_params, &copy_params);
-  dt_pthread_mutex_unlock(&develop->pipe_mutex);
+  dt_pthread_mutex_unlock(&develop->preview_pipe->busy_mutex);
 
   // You're not supposed to understand this
   const float zoom_x = dt_control_get_dev_zoom_x();

src/views/darkroom.c

@@ -2289,7 +2289,7 @@ void enter(dt_view_t *self)
   dt_develop_t *dev = (dt_develop_t *)self->data;
 
   // Make sure we don't start computing pipes until we have a proper history
-  dt_pthread_mutex_lock(&dev->pipe_mutex);
+  dt_pthread_mutex_lock(&dev->history_mutex);
 
   if(!dev->form_gui)
   {
@@ -2338,6 +2338,8 @@ void enter(dt_view_t *self)
     }
   }
 
+  dt_pthread_mutex_unlock(&dev->history_mutex);
+
 #ifdef USE_LUA
 
   dt_lua_async_call_alien(dt_lua_event_trigger_wrapper,
@@ -2353,6 +2355,7 @@ void enter(dt_view_t *self)
 
   // synch gui and flag pipe as dirty
   // this is done here and not in dt_read_history, as it would else be triggered before module->gui_init.
+  // locks history mutex internally
   dt_dev_pop_history_items(dev, dt_dev_get_history_end(dev));
 
   /* ensure that filmstrip shows current image */
@@ -2407,8 +2410,6 @@ void enter(dt_view_t *self)
 
   dt_image_check_camera_missing_sample(&dev->image_storage);
 
-  dt_pthread_mutex_unlock(&dev->pipe_mutex);
-
   // Init the starting point of undo/redo
   dt_dev_undo_start_record(dev);
   dt_dev_undo_end_record(dev);
@@ -2432,8 +2433,6 @@ void leave(dt_view_t *self)
 
   _unregister_modules_drag_n_drop(self);
 
-  dt_dev_unload_image(dev);
-
   /* disconnect from filmstrip image activate */
   DT_DEBUG_CONTROL_SIGNAL_DISCONNECT(darktable.signals, G_CALLBACK(_view_darkroom_filmstrip_activate_callback),
                                (gpointer)self);
@@ -2501,11 +2500,14 @@ void leave(dt_view_t *self)
   }
 
   // clear gui.
-
-  dt_pthread_mutex_lock(&dev->pipe_mutex);
-
+  dt_pthread_mutex_lock(&dev->pipe->busy_mutex);
   dt_dev_pixelpipe_cleanup_nodes(dev->pipe);
+  dt_pthread_mutex_unlock(&dev->pipe->busy_mutex);
+
+  dt_pthread_mutex_lock(&dev->preview_pipe->busy_mutex);
   dt_dev_pixelpipe_cleanup_nodes(dev->preview_pipe);
+  dt_pthread_mutex_unlock(&dev->preview_pipe->busy_mutex);
+
 
   dt_pthread_mutex_lock(&dev->history_mutex);
   while(dev->history)
@@ -2535,8 +2537,6 @@ void leave(dt_view_t *self)
 
   dt_pthread_mutex_unlock(&dev->history_mutex);
 
-  dt_pthread_mutex_unlock(&dev->pipe_mutex);
-
   // cleanup visible masks
   if(dev->form_gui)
   {