Skip to content

Commit

Permalink
Pipelines: use mutices more sparingly, don't try keep alive
Browse files Browse the repository at this point in the history 
- Keeping pipe threads alive is biting too much right now, for little benefit
- Do not protect flags read/write with thread locks. It's unnecessary (setting ints is supposed to be atomic) and triggers delays for nothing.
- Use the history mutex lock more sparingly
- Use local copies of int flags to account for external changes.
  • Loading branch information
@aurelienpierre
aurelienpierre committed Jan 17, 2025
1 parent d13902d commit 92f383c
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 95 deletions.
149 changes: 68 additions & 81 deletions src/develop/develop.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,92 +249,96 @@ void dt_dev_refresh_ui_images_real(dt_develop_t *dev)
// else : join current pipe
}

void _dev_pixelpipe_set_dirty(dt_dev_pixelpipe_t *pipe)
{
pipe->status = DT_DEV_PIXELPIPE_DIRTY;
}

void dt_dev_pixelpipe_rebuild(dt_develop_t *dev)
{
if(!dev || !dev->gui_attached) return;
if(!dev || !dev->gui_attached || !dev->pipe || !dev->preview_pipe) return;

dt_atomic_set_int(&dev->pipe->shutdown, TRUE);
dt_atomic_set_int(&dev->preview_pipe->shutdown, TRUE);
dt_times_t start;
dt_get_times(&start);

_dev_pixelpipe_set_dirty(dev->pipe);
_dev_pixelpipe_set_dirty(dev->preview_pipe);

dt_pthread_mutex_lock(&dev->history_mutex);
dev->pipe->changed |= DT_DEV_PIPE_REMOVE;
dev->preview_pipe->changed |= DT_DEV_PIPE_REMOVE;
dev->pipe->status = DT_DEV_PIXELPIPE_DIRTY;
dev->preview_pipe->status = DT_DEV_PIXELPIPE_DIRTY;
dt_pthread_mutex_unlock(&dev->history_mutex);

dt_atomic_set_int(&dev->pipe->shutdown, TRUE);
dt_atomic_set_int(&dev->preview_pipe->shutdown, TRUE);

dt_show_times(&start, "[dt_dev_invalidate] sending killswitch signal on all pipelines");
}

void dt_dev_pixelpipe_resync_main(dt_develop_t *dev)
{
if(!dev->gui_attached) return;
if(!dev || !dev->gui_attached || !dev->pipe) return;

dt_atomic_set_int(&dev->pipe->shutdown, TRUE);

dt_pthread_mutex_lock(&dev->history_mutex);
_dev_pixelpipe_set_dirty(dev->pipe);
dev->pipe->changed |= DT_DEV_PIPE_SYNCH;
dev->pipe->status = DT_DEV_PIXELPIPE_DIRTY;
dt_pthread_mutex_unlock(&dev->history_mutex);
dt_atomic_set_int(&dev->pipe->shutdown, TRUE);
}

void dt_dev_pixelpipe_resync_all(dt_develop_t *dev)
{
if(!dev || !dev->gui_attached) return;
if(!dev || !dev->gui_attached || !dev->pipe || !dev->preview_pipe) return;

dt_atomic_set_int(&dev->preview_pipe->shutdown, TRUE);

dt_pthread_mutex_lock(&dev->history_mutex);
_dev_pixelpipe_set_dirty(dev->preview_pipe);
dev->preview_pipe->changed |= DT_DEV_PIPE_SYNCH;
dev->preview_pipe->status = DT_DEV_PIXELPIPE_DIRTY;
dt_pthread_mutex_unlock(&dev->history_mutex);
dt_atomic_set_int(&dev->preview_pipe->shutdown, TRUE);

dt_dev_pixelpipe_resync_main(dev);
}

void dt_dev_invalidate_real(dt_develop_t *dev)
{
if(!dev || !dev->gui_attached || !dev->pipe) return;

dt_times_t start;
dt_get_times(&start);
dt_show_times(&start, "[dt_dev_invalidate] sending killswitch signal on running pipelines");

_dev_pixelpipe_set_dirty(dev->pipe);
dev->pipe->changed |= DT_DEV_PIPE_TOP_CHANGED;
dt_atomic_set_int(&dev->pipe->shutdown, TRUE);

dt_pthread_mutex_lock(&dev->history_mutex);
dev->pipe->status = DT_DEV_PIXELPIPE_DIRTY;
dev->pipe->changed |= DT_DEV_PIPE_TOP_CHANGED;
dt_pthread_mutex_unlock(&dev->history_mutex);
dt_show_times(&start, "[dt_dev_invalidate] sending killswitch signal on main image pipeline");
}

void dt_dev_invalidate_zoom_real(dt_develop_t *dev)
{
if(!dev || !dev->gui_attached || !dev->pipe) return;

dt_times_t start;
dt_get_times(&start);
dt_show_times(&start, "[dt_dev_invalidate_zoom] sending killswitch signal on running pipelines");

_dev_pixelpipe_set_dirty(dev->pipe);
dev->pipe->changed |= DT_DEV_PIPE_ZOOMED;
dt_atomic_set_int(&dev->pipe->shutdown, TRUE);

dt_pthread_mutex_lock(&dev->history_mutex);
dev->pipe->status = DT_DEV_PIXELPIPE_DIRTY;
dev->pipe->changed |= DT_DEV_PIPE_ZOOMED;
dt_pthread_mutex_unlock(&dev->history_mutex);
dt_show_times(&start, "[dt_dev_invalidate_zoom] sending killswitch signal on main image pipeline");
}

void dt_dev_invalidate_preview_real(dt_develop_t *dev)
{
if(!dev || !dev->gui_attached || !dev->preview_pipe) return;

dt_times_t start;
dt_get_times(&start);
dt_show_times(&start, "[dt_dev_invalidate_preview] sending killswitch signal on running pipelines");

_dev_pixelpipe_set_dirty(dev->preview_pipe);
dev->preview_pipe->changed |= DT_DEV_PIPE_TOP_CHANGED;
dt_atomic_set_int(&dev->preview_pipe->shutdown, TRUE);

dt_pthread_mutex_lock(&dev->history_mutex);
dev->preview_pipe->changed |= DT_DEV_PIPE_TOP_CHANGED;
dev->preview_pipe->status = DT_DEV_PIXELPIPE_DIRTY;
dt_pthread_mutex_unlock(&dev->history_mutex);
dt_show_times(&start, "[dt_dev_invalidate_preview] sending killswitch signal on preview pipeline");
}

void dt_dev_invalidate_all_real(dt_develop_t *dev)
{
if(!dev || !dev->gui_attached) return;
if(!dev || !dev->gui_attached || !dev->pipe || !dev->preview_pipe) return;

// Send killswitch ASAP
dt_atomic_set_int(&dev->pipe->shutdown, TRUE);
dt_atomic_set_int(&dev->preview_pipe->shutdown, TRUE);
Expand Down Expand Up @@ -394,25 +398,8 @@ void dt_dev_process_preview_job(dt_develop_t *dev)
dt_print(DT_DEBUG_DEV, "[pixelpipe] Started thumbnail preview recompute at %i×%i px\n", width, height);
}

// Infinite loop until darkroom sends dev->exit signal
dt_times_t loop_start;
dt_get_times(&loop_start);

dt_times_t last_call;
dt_get_times(&last_call);

while(!dev->exit && !finish_on_error && (loop_start.clock - last_call.clock) < KEEP_ALIVE)
while(!dev->exit && !finish_on_error && (pipe->status != DT_DEV_PIXELPIPE_VALID))
{
dt_get_times(&loop_start);

if(pipe->status == DT_DEV_PIXELPIPE_VALID)
{
// Nothing to recompute. Wait 2 ms.
dt_iop_nap(2000);
continue;
}
// else: resync pipe with history and recompute :

dt_pthread_mutex_lock(&pipe->busy_mutex);
pipe->processing = 1;

Expand All @@ -425,11 +412,12 @@ void dt_dev_process_preview_job(dt_develop_t *dev)
// this locks dev->history_mutex.
dt_dev_pixelpipe_change(pipe, dev);

if(dt_atomic_get_int(&pipe->shutdown)) continue;

dt_control_log_busy_enter();
dt_control_toast_busy_enter();

// Signal that we are starting
pipe->status = DT_DEV_PIXELPIPE_UNDEF;

// OpenCL devices have their own lock, called internally
// Here we lock the whole stack, including CPU.
// Processing pipelines in parallel triggers memory contention.
Expand All @@ -450,15 +438,23 @@ void dt_dev_process_preview_job(dt_develop_t *dev)
dt_show_times(&thread_start, "[dev_process_preview] pixel pipeline thread");
dt_dev_average_delay_update(&thread_start, &dev->preview_average_delay);

if(ret && pipe->status == DT_DEV_PIXELPIPE_INVALID) finish_on_error = TRUE;
// The internal dt_dev_pixelpipe_process() sets the status to invalid
// upon memory errors. Else, it doesn't change it so it is supposed to
// be dirty so far.
if(ret && pipe->status == DT_DEV_PIXELPIPE_INVALID)
finish_on_error = TRUE;
else if(!ret && pipe->backbuf && pipe->status == DT_DEV_PIXELPIPE_UNDEF)
pipe->status = DT_DEV_PIXELPIPE_VALID;
// else if(ret || pipe->status == DT_DEV_PIXELPIPE_DIRTY)
// history was changed from mainthread since we started,
// pipe will be reprocessed in next loop

pipe->processing = 0;
dt_pthread_mutex_unlock(&pipe->busy_mutex);

if(!finish_on_error)
DT_DEBUG_CONTROL_SIGNAL_RAISE(darktable.signals, DT_SIGNAL_DEVELOP_PREVIEW_PIPE_FINISHED);

dt_get_times(&last_call);
dt_iop_nap(200);
}

Expand Down Expand Up @@ -492,25 +488,8 @@ void dt_dev_process_image_job(dt_develop_t *dev)
dt_dev_pixelpipe_set_input(pipe, dev, buffer, width, height, 1.0);

float scale = 1.f, zoom_x = 1.f, zoom_y = 1.f;

dt_times_t loop_start;
dt_get_times(&loop_start);

dt_times_t last_call;
dt_get_times(&last_call);

while(!dev->exit && !finish_on_error && (loop_start.clock - last_call.clock) < KEEP_ALIVE)
while(!dev->exit && !finish_on_error && (pipe->status != DT_DEV_PIXELPIPE_VALID))
{
dt_get_times(&loop_start);

if(pipe->status == DT_DEV_PIXELPIPE_VALID)
{
// Nothing to recompute. Wait 2 ms.
dt_iop_nap(2000);
continue;
}
// else: resync pipe with history and recompute :

dt_pthread_mutex_lock(&pipe->busy_mutex);
pipe->processing = 1;

Expand All @@ -521,11 +500,10 @@ void dt_dev_process_image_job(dt_develop_t *dev)
dt_atomic_set_int(&pipe->shutdown, FALSE);

dt_dev_pixelpipe_change_t pipe_changed = pipe->changed;

// this locks dev->history_mutex
dt_dev_pixelpipe_change(pipe, dev);

if(dt_atomic_get_int(&pipe->shutdown)) continue;

// determine scale according to new dimensions
dt_dev_zoom_t zoom = dt_control_get_dev_zoom();
int closeup = dt_control_get_dev_closeup();
Expand Down Expand Up @@ -554,11 +532,12 @@ void dt_dev_process_image_job(dt_develop_t *dev)
int x = MAX(0, scale * pipe->processed_width * (.5 + zoom_x) - wd / 2);
int y = MAX(0, scale * pipe->processed_height * (.5 + zoom_y) - ht / 2);

if(dt_atomic_get_int(&pipe->shutdown)) continue;

dt_control_log_busy_enter();
dt_control_toast_busy_enter();

// Signal that we are starting
pipe->status = DT_DEV_PIXELPIPE_UNDEF;

// OpenCL devices have their own lock, called internally
// Here we lock the whole stack, including CPU.
// Processing pipelines in parallel triggers memory contention.
Expand All @@ -579,7 +558,16 @@ void dt_dev_process_image_job(dt_develop_t *dev)
dt_show_times(&thread_start, "[dev_process_image] pixel pipeline thread");
dt_dev_average_delay_update(&thread_start, &dev->average_delay);

if(ret && pipe->status == DT_DEV_PIXELPIPE_INVALID) finish_on_error = TRUE;
// The internal dt_dev_pixelpipe_process() sets the status to invalid
// upon memory errors. Else, it doesn't change it so it is supposed to
// be dirty so far.
if(ret && pipe->status == DT_DEV_PIXELPIPE_INVALID)
finish_on_error = TRUE;
else if(!ret && pipe->backbuf && pipe->status == DT_DEV_PIXELPIPE_UNDEF)
pipe->status = DT_DEV_PIXELPIPE_VALID;
// else if(ret || pipe->status == DT_DEV_PIXELPIPE_DIRTY)
// history was changed from mainthread since we started,
// pipe will be reprocessed in next loop

// cool, we got a new image!
if(!finish_on_error && !ret)
Expand All @@ -596,7 +584,6 @@ void dt_dev_process_image_job(dt_develop_t *dev)
if(!finish_on_error)
DT_DEBUG_CONTROL_SIGNAL_RAISE(darktable.signals, DT_SIGNAL_DEVELOP_UI_PIPE_FINISHED);

dt_get_times(&last_call);
dt_iop_nap(200);
}

Expand Down
23 changes: 11 additions & 12 deletions src/develop/pixelpipe_hb.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,12 @@ void dt_dev_pixelpipe_change(dt_dev_pixelpipe_t *pipe, struct dt_develop_t *dev)
dt_times_t start;
dt_get_times(&start);

dt_pthread_mutex_lock(&dev->history_mutex);
// Read and write immediately to ensure cross-thread consistency of the value
// in case the GUI overwrites that while we are syncing history and nodes
const dt_dev_pixelpipe_change_t status = pipe->changed;
pipe->changed = DT_DEV_PIPE_UNCHANGED;

dt_print(DT_DEBUG_DEV, "[dt_dev_pixelpipe_change] pipeline state changing for pipe %i, flag %i\n", pipe->type, status);

// mask display off as a starting point
pipe->mask_display = DT_DEV_PIXELPIPE_DISPLAY_NONE;
Expand All @@ -613,36 +618,33 @@ void dt_dev_pixelpipe_change(dt_dev_pixelpipe_t *pipe, struct dt_develop_t *dev)
else if(dt_image_is_rawprepare_supported(img))
pipe->want_detail_mask |= DT_DEV_DETAIL_MASK_RAWPREPARE;

dt_print(DT_DEBUG_DEV, "[dt_dev_pixelpipe_change] pipeline state changing for pipe %i, flag %i\n", pipe->type, pipe->changed);
dt_pthread_mutex_lock(&dev->history_mutex);

// case DT_DEV_PIPE_UNCHANGED: case DT_DEV_PIPE_ZOOMED:
if(pipe->changed & DT_DEV_PIPE_REMOVE)
if(status & DT_DEV_PIPE_REMOVE)
{
// modules have been added in between or removed. need to rebuild the whole pipeline.
dt_dev_pixelpipe_cleanup_nodes(pipe);
dt_dev_pixelpipe_create_nodes(pipe, dev);
dt_dev_pixelpipe_synch_all(pipe, dev);
}
else if(pipe->changed & DT_DEV_PIPE_SYNCH)
else if(status & DT_DEV_PIPE_SYNCH)
{
// pipeline topology remains intact, only change all params.
dt_dev_pixelpipe_synch_all(pipe, dev);
}
else if(pipe->changed & DT_DEV_PIPE_TOP_CHANGED)
else if(status & DT_DEV_PIPE_TOP_CHANGED)
{
// only top history item changed.
// FIXME: this seems to never be called.
dt_dev_pixelpipe_synch_top(pipe, dev);
}

pipe->changed = DT_DEV_PIPE_UNCHANGED;
dt_pthread_mutex_unlock(&dev->history_mutex);

// Get the final output size of the pipe, for GUI coordinates mapping between image buffer and window
dt_dev_pixelpipe_get_roi_out(pipe, dev, pipe->iwidth, pipe->iheight, &pipe->processed_width,
&pipe->processed_height);

dt_pthread_mutex_unlock(&dev->history_mutex);

dt_show_times_f(&start, "[dt_dev_pixelpipe_change] pipeline resync on the current modules stack", "for pipe %i", pipe->type);
}

Expand Down Expand Up @@ -2463,9 +2465,6 @@ restart:;
pipe->output_imgid = pipe->image.id;
}
dt_pthread_mutex_unlock(&pipe->backbuf_mutex);

// printf("pixelpipe homebrew process end\n");
pipe->status = DT_DEV_PIXELPIPE_VALID;
return 0;
}

Expand Down
5 changes: 3 additions & 2 deletions src/develop/pixelpipe_hb.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,9 @@ typedef enum dt_dev_pixelpipe_change_t
typedef enum dt_dev_pixelpipe_status_t
{
DT_DEV_PIXELPIPE_DIRTY = 0, // history stack changed or image new
DT_DEV_PIXELPIPE_VALID = 1, // pixelpipe has finished; valid result
DT_DEV_PIXELPIPE_INVALID = 2 // pixelpipe has finished; invalid result
DT_DEV_PIXELPIPE_UNDEF = 1, // pixelpipe computation started and we don't know yet
DT_DEV_PIXELPIPE_VALID = 2, // pixelpipe has finished; valid result
DT_DEV_PIXELPIPE_INVALID = 3 // pixelpipe has finished; invalid result
} dt_dev_pixelpipe_status_t;

/**
Expand Down
1 change: 1 addition & 0 deletions src/views/darkroom.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2399,6 +2399,7 @@ void leave(dt_view_t *self)
// update aspect ratio
if(dev->preview_pipe->backbuf && dev->preview_pipe->status == DT_DEV_PIXELPIPE_VALID)
{
// FIXME: map that to preview pipe recomputed event
double aspect_ratio = (double)dev->preview_pipe->backbuf_width / (double)dev->preview_pipe->backbuf_height;
dt_image_set_aspect_ratio_to(dev->preview_pipe->image.id, aspect_ratio, FALSE);
}
Expand Down

0 comments on commit 92f383c

Please sign in to comment.