Merge pull request #18523 from jenshannoschwalm/shutdown_fixes_3

Fix some pixelpipe shutdown issues

Author: TurboGit

Diff for: src/develop/develop.c

@@ -338,7 +338,7 @@ void dt_dev_process_image_job(dt_develop_t *dev,
                              port ? 1.0 : buf.iscale);
 
   // We require calculation of pixelpipe dimensions via dt_dev_pixelpipe_change() in these cases
-  const gboolean initial = pipe->loading || dev->image_force_reload || pipe->input_changed;
+  gboolean initial = pipe->loading || dev->image_force_reload || pipe->input_changed;
 
   if(pipe->loading)
   {
@@ -391,11 +391,21 @@ void dt_dev_process_image_job(dt_develop_t *dev,
   if(port == &dev->full)
     pipe->input_timestamp = dev->timestamp;
 
-  const gboolean pipe_changed = pipe->changed != DT_DEV_PIPE_UNCHANGED;
-  // dt_dev_pixelpipe_change() locks history mutex while syncing nodes and finally calculates dimensions
-  if(pipe_changed || initial || (port && port->pipe->loading))
+  const gboolean changing = (pipe->changed != DT_DEV_PIPE_UNCHANGED) || initial;
+
+  // to be checked: can we possibly restrict later dt_dev_zoom_move() calls
+  const gboolean require_zoom_test = ((pipe->changed & ~DT_DEV_PIPE_ZOOMED) != DT_DEV_PIPE_UNCHANGED) || initial;
+
+  /* dt_dev_pixelpipe_change()
+      locks history mutex while syncing nodes
+      finally calculates dimensions
+      leaves clean pipe->changed
+  */
+  if(changing || (port && port->pipe->loading))
     dt_dev_pixelpipe_change(pipe, dev);
 
+  initial = FALSE; // don't enforce dt_dev_pixelpipe_change() for restarts
+
   float scale = 1.0f;
   int window_width = G_MAXINT;
   int window_height = G_MAXINT;
@@ -407,8 +417,11 @@ void dt_dev_process_image_job(dt_develop_t *dev,
     // if just changed to an image with a different aspect ratio or
     // altered image orientation, the prior zoom xy could now be beyond
     // the image boundary
-    if(port->pipe->loading || pipe_changed)
+    if(port->pipe->loading || require_zoom_test)
+    {
+      dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_VERBOSE, "[dt_dev_zoom_move]", pipe, NULL, DT_DEVICE_NONE, NULL, NULL);
       dt_dev_zoom_move(port, DT_ZOOM_MOVE, 0.0f, 0, 0.0f, 0.0f, TRUE);
+    }
 
     // determine scale according to new dimensions
     dt_dev_zoom_t zoom;
@@ -418,13 +431,6 @@ void dt_dev_process_image_job(dt_develop_t *dev,
     window_width = port->width * port->ppd / (1<<closeup);
     window_height = port->height * port->ppd / (1<<closeup);
   }
-  // else
-  // {
-  //   // FIXME full pipe may be busy, so update processed sizes here
-  //   // make sure preview pipe is newer than full/preview2 pipes
-  //   dev->full.pipe->processed_width = pipe->processed_width * pipe->iscale;
-  //   dev->full.pipe->processed_height = pipe->processed_height * pipe->iscale;
-  // }
 
   const int wd = MIN(window_width, scale * pipe->processed_width);
   const int ht = MIN(window_height, scale * pipe->processed_height);
@@ -435,8 +441,25 @@ void dt_dev_process_image_job(dt_develop_t *dev,
 
   if(dt_dev_pixelpipe_process(pipe, dev, x, y, wd, ht, scale, devid))
   {
+    const gboolean img_changed = dev->image_force_reload || pipe->loading || pipe->input_changed;
+    // As image_force_reload could be set while we are restarting we clear it and possibly flush the cache too.
+    if(dev->image_force_reload) dt_dev_pixelpipe_cache_flush(pipe);
+    dev->image_force_reload = FALSE;
+    const dt_dev_pixelpipe_stopper_t shutdown = dt_atomic_exch_int(&pipe->shutdown, DT_DEV_PIXELPIPE_STOP_NO);
+    if(shutdown != DT_DEV_PIXELPIPE_STOP_NO || img_changed)
+      dt_print_pipe(DT_DEBUG_PIPE,
+                    shutdown == DT_DEV_PIXELPIPE_STOP_NODES ? "DT_DEV_PIXELPIPE_STOP_NODES shutdown"
+                  : shutdown == DT_DEV_PIXELPIPE_STOP_HQ    ? "DT_DEV_PIXELPIPE_STOP_HQ shutdown"
+                  : shutdown == DT_DEV_PIXELPIPE_STOP_NO    ? "pixelpipe_process ERR"
+                  : "PROCESS shutdown",
+                  pipe, NULL, DT_DEVICE_NONE, NULL, NULL,
+                  "%s%s%sshutdown=%d",
+                  dev->image_force_reload ? "image_force_reload, " : "",
+                  pipe->loading ? "pipe loading, " : "",
+                  pipe->input_changed ? "input_changed, " : "",
+                  shutdown);
     // interrupted because image changed?
-    if(dev->image_force_reload || pipe->loading || pipe->input_changed)
+    if(img_changed)
     {
       dt_mipmap_cache_release(&buf);
       dt_control_busy_leave();
@@ -447,12 +470,6 @@ void dt_dev_process_image_job(dt_develop_t *dev,
     // or because the pipeline changed or shutdown?
     else
     {
-      const dt_dev_pixelpipe_stopper_t downer = dt_atomic_exch_int(&pipe->shutdown, DT_DEV_PIXELPIPE_STOP_NO);
-      if(downer)
-        dt_print_pipe(DT_DEBUG_PIPE,  downer == DT_DEV_PIXELPIPE_STOP_NODES ? "DT_DEV_PIXELPIPE_STOP_NODES shutdown"
-                                    : downer == DT_DEV_PIXELPIPE_STOP_HQ    ? "DT_DEV_PIXELPIPE_STOP_HQ shutdown"
-                                    : "PROCESS shutdown",
-          pipe, NULL, DT_DEVICE_NONE, NULL, NULL, "downer=%d", downer);
       if(port && port->widget) dt_control_queue_redraw_widget(port->widget);
       goto restart;
     }
@@ -463,7 +480,11 @@ void dt_dev_process_image_job(dt_develop_t *dev,
   _dev_average_delay_update(&start, &pipe->average_delay);
 
   // maybe we got zoomed/panned in the meantime?
-  if(port && pipe->changed != DT_DEV_PIPE_UNCHANGED) goto restart;
+  if(port && pipe->changed != DT_DEV_PIPE_UNCHANGED)
+  {
+    dt_atomic_set_int(&pipe->shutdown, DT_DEV_PIXELPIPE_STOP_NO);
+    goto restart;
+  }
 
   pipe->status = DT_DEV_PIXELPIPE_VALID;
   pipe->loading = FALSE;

Diff for: src/develop/pixelpipe_hb.c

@@ -1199,6 +1199,24 @@ static inline float *_get_fast_blendcache(const size_t nfloats,
   return pipe->bcache_data;
 }
 
+/* use _module_pipe_stop to test for the just processed module leaving a flag to shutdown the pipeline
+   immediately.
+   This requires extra care as we want pipecache data after the module and it's input data to be invalided.
+*/
+static inline gboolean _module_pipe_stop(dt_dev_pixelpipe_t *pipe, float *input)
+{
+  const dt_dev_pixelpipe_stopper_t stopper = dt_atomic_get_int(&pipe->shutdown);
+  if(stopper != DT_DEV_PIXELPIPE_STOP_NO)
+  {
+    if(stopper > DT_DEV_PIXELPIPE_STOP_LAST)
+    {
+      dt_dev_pixelpipe_invalidate_cacheline(pipe, input);
+      dt_dev_pixelpipe_cache_invalidate_later(pipe, stopper);
+    }
+  }
+  return stopper != DT_DEV_PIXELPIPE_STOP_NO;
+}
+
 static gboolean _pixelpipe_process_on_CPU(dt_dev_pixelpipe_t *pipe,
                                           dt_develop_t *dev,
                                           float *input,
@@ -1229,7 +1247,7 @@ static gboolean _pixelpipe_process_on_CPU(dt_dev_pixelpipe_t *pipe,
                      "in module '%s'\nplease report on GitHub"),
                      module->op);
     // this is a fundamental problem with severe problems ahead so good to finish
-    // the pipe as if good to avoid reprocessing and endless loop.
+    // the pipe as if good to avoid reprocessing in an endless loop.
     return FALSE;
   }
 
@@ -1362,7 +1380,7 @@ static gboolean _pixelpipe_process_on_CPU(dt_dev_pixelpipe_t *pipe,
         if(module->process_plain)
         {
           dt_get_times(&start);
-          for(int i = 0; i < counter; i++)
+          for(int i = 0; i < counter && !dt_pipe_shutdown(pipe); i++)
             module->process_plain(module, piece, input, *output, roi_in, roi_out);
           dt_get_times(&end);
           const float clock = (end.clock - start.clock) / (float) counter;
@@ -1399,15 +1417,10 @@ static gboolean _pixelpipe_process_on_CPU(dt_dev_pixelpipe_t *pipe,
                          | PIXELPIPE_FLOW_PROCESSED_WITH_TILING);
   }
 
-  const dt_dev_pixelpipe_stopper_t stopper = dt_atomic_get_int(&pipe->shutdown);
-  if(stopper != DT_DEV_PIXELPIPE_STOP_NO)
-  {
-    if(stopper > DT_DEV_PIXELPIPE_STOP_LAST)
-      dt_dev_pixelpipe_cache_invalidate_later(pipe, stopper);
+  if(_module_pipe_stop(pipe, input))
     return TRUE;
-  }
 
-  if(pfm_dump && !dt_pipe_shutdown(pipe))
+  if(pfm_dump)
   {
     dt_dump_pipe_pfm(module->op, *output,
                      roi_out->width, roi_out->height, bpp,
@@ -1474,7 +1487,7 @@ static gboolean _pixelpipe_process_on_CPU(dt_dev_pixelpipe_t *pipe,
     }
   }
 
-  if(dt_atomic_get_int(&pipe->shutdown))
+  if(dt_pipe_shutdown(pipe))
     return TRUE;
 
   /* process blending on CPU */
@@ -2070,7 +2083,7 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe,
               const int counter = (pipe->type & DT_DEV_PIXELPIPE_FULL) ? 100 : 50;
               gboolean success = TRUE;
               dt_get_times(&bench);
-              for(int i = 0; i < counter; i++)
+              for(int i = 0; i < counter && !dt_pipe_shutdown(pipe); i++)
               {
                 if(success)
                   success = module->process_cl(module, piece, cl_mem_input, *cl_mem_output,
@@ -2193,14 +2206,8 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe,
           pipe->dsc.cst = module->output_colorspace(module, pipe, piece);
         }
 
-        const dt_dev_pixelpipe_stopper_t stopper = dt_atomic_get_int(&pipe->shutdown);
-        if(stopper != DT_DEV_PIXELPIPE_STOP_NO)
+        if(_module_pipe_stop(pipe, input))
         {
-          if(stopper > DT_DEV_PIXELPIPE_STOP_LAST)
-          {
-            dt_dev_pixelpipe_invalidate_cacheline(pipe, input);
-            dt_dev_pixelpipe_cache_invalidate_later(pipe, stopper);
-          }
           dt_opencl_release_mem_object(cl_mem_input);
           return TRUE;
         }
@@ -2414,13 +2421,8 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe,
           pipe->dsc.cst = module->output_colorspace(module, pipe, piece);
         }
 
-        const dt_dev_pixelpipe_stopper_t stopper = dt_atomic_get_int(&pipe->shutdown);
-        if(stopper != DT_DEV_PIXELPIPE_STOP_NO)
-        {
-          if(stopper > DT_DEV_PIXELPIPE_STOP_LAST)
-            dt_dev_pixelpipe_cache_invalidate_later(pipe, stopper);
+        if(_module_pipe_stop(pipe, input))
           return TRUE;
-        }
 
         dt_iop_colorspace_type_t blend_cst =
           dt_develop_blend_colorspace(piece, pipe->dsc.cst);