From 072baa139d85c15992cb0040e15337c7e4a2cd42 Mon Sep 17 00:00:00 2001 From: Hanno Schwalm Date: Sat, 14 Dec 2024 08:05:33 +0100 Subject: [PATCH 1/5] Cleanup image and mipmap caches after shutting down control system As we now have a lot of things in background jobs related to images and mipmaps we must keep those systems available until the control system has been fully closed. NOTE: for some reason we can't have dt_imageio_cleanup() after dt_control_shutdown() --- src/common/darktable.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/common/darktable.c b/src/common/darktable.c index 18915a048581..9d686974522c 100644 --- a/src/common/darktable.c +++ b/src/common/darktable.c @@ -2060,12 +2060,6 @@ void dt_cleanup() // we can no longer call dt_gui_process_events after this point, as that will cause a segfault // if some delayed event fires - dt_image_cache_cleanup(darktable.image_cache); - free(darktable.image_cache); - darktable.image_cache = NULL; - dt_mipmap_cache_cleanup(darktable.mipmap_cache); - free(darktable.mipmap_cache); - darktable.mipmap_cache = NULL; if(init_gui) { dt_imageio_cleanup(darktable.imageio); @@ -2081,6 +2075,13 @@ void dt_cleanup() darktable.gui = NULL; } + dt_image_cache_cleanup(darktable.image_cache); + free(darktable.image_cache); + darktable.image_cache = NULL; + dt_mipmap_cache_cleanup(darktable.mipmap_cache); + free(darktable.mipmap_cache); + darktable.mipmap_cache = NULL; + dt_colorspaces_cleanup(darktable.color_profiles); dt_conf_cleanup(darktable.conf); free(darktable.conf); From a042ba8c956160209e80441c23b52cd32e00fab7 Mon Sep 17 00:00:00 2001 From: Hanno Schwalm Date: Sun, 15 Dec 2024 06:34:52 +0100 Subject: [PATCH 2/5] Some job and control cleanup and fixing 1. For code readability use 'control' instead of 's' 2. Simplify dt_control_running() 3. Removed not needed checks on errors 4. Two functions are strictly internal so no exposing via .h files and be static - _control_get_threadid() - _control_job_set_synchronous() 5. Two function were not used at all (so were untested) so the were removed - dt_control_job_set_state_callback() - dt_control_job_wait() 6. gphoto joining after cleanup test 7. Never create a control job if control is not in running state --- src/control/control.c | 81 ++++++++++++++------------------ src/control/jobs.c | 107 +++++++++++++----------------------------- src/control/jobs.h | 7 --- 3 files changed, 69 insertions(+), 126 deletions(-) diff --git a/src/control/control.c b/src/control/control.c index 8fd9d5fa265c..bbe775f8a248 100644 --- a/src/control/control.c +++ b/src/control/control.c @@ -288,29 +288,27 @@ void dt_control_change_cursor(dt_cursor_t curs) gboolean dt_control_running() { - dt_control_t *dc = darktable.control; - const int status = dc ? dt_atomic_get_int(&dc->running) : DT_CONTROL_STATE_DISABLED; - return status == DT_CONTROL_STATE_RUNNING; + dt_control_t *control = darktable.control; + return control ? dt_atomic_get_int(&control->running) == DT_CONTROL_STATE_RUNNING : FALSE; } void dt_control_quit() { if(dt_control_running()) { - dt_control_t *dc = darktable.control; - + dt_control_t *control = darktable.control; #ifdef HAVE_PRINT dt_printers_abort_discovery(); // Cups timeout could be pretty long, at least 30seconds // but don't rely on cups returning correctly so a timeout - for(int i = 0; i < 40000 && !dc->cups_started; i++) + for(int i = 0; i < 40000 && !control->cups_started; i++) g_usleep(1000); #endif - dt_pthread_mutex_lock(&dc->cond_mutex); + dt_pthread_mutex_lock(&control->cond_mutex); // set the "pending cleanup work" flag to be handled in dt_control_shutdown() - dt_atomic_set_int(&dc->running, DT_CONTROL_STATE_CLEANUP); - dt_pthread_mutex_unlock(&dc->cond_mutex); + dt_atomic_set_int(&control->running, DT_CONTROL_STATE_CLEANUP); + dt_pthread_mutex_unlock(&control->cond_mutex); } if(g_atomic_int_get(&darktable.gui_running)) @@ -320,61 +318,54 @@ void dt_control_quit() } } -void dt_control_shutdown(dt_control_t *s) +void dt_control_shutdown(dt_control_t *control) { - if(!s) + if(!control) return; - dt_pthread_mutex_lock(&s->cond_mutex); - const gboolean cleanup = dt_atomic_exch_int(&s->running, DT_CONTROL_STATE_DISABLED) == DT_CONTROL_STATE_CLEANUP; - pthread_cond_broadcast(&s->cond); - dt_pthread_mutex_unlock(&s->cond_mutex); + dt_pthread_mutex_lock(&control->cond_mutex); + const gboolean cleanup = dt_atomic_exch_int(&control->running, DT_CONTROL_STATE_DISABLED) == DT_CONTROL_STATE_CLEANUP; + pthread_cond_broadcast(&control->cond); + dt_pthread_mutex_unlock(&control->cond_mutex); - int err = 0; // collect all joining errors - /* first wait for gphoto device updater */ -#ifdef HAVE_GPHOTO2 - err = pthread_join(s->update_gphoto_thread, NULL); -#endif + dt_print(DT_DEBUG_CONTROL, "[dt_control_shutdown] closing control threads%s", + cleanup ? " in cleanup mode" : ""); if(!cleanup) return; // if not running there are no threads to join - dt_print(DT_DEBUG_CONTROL, "[dt_control_shutdown] closing control threads"); +#ifdef HAVE_GPHOTO2 + /* first and always wait for gphoto device updater */ + pthread_join(control->update_gphoto_thread, NULL); +#endif - /* then wait for kick_on_workers_thread */ - err = pthread_join(s->kick_on_workers_thread, NULL); - dt_print(DT_DEBUG_CONTROL, "[dt_control_shutdown] joined kicker%s", err ? ", error" : ""); + /* wait for kick_on_workers_thread */ + pthread_join(control->kick_on_workers_thread, NULL); - for(int k = 0; k < s->num_threads-1; k++) - { - err = pthread_join(s->thread[k], NULL); - dt_print(DT_DEBUG_CONTROL, "[dt_control_shutdown] joined num_thread %i%s", k, err ? ", error" : ""); - } + for(int k = 0; k < control->num_threads-1; k++) + pthread_join(control->thread[k], NULL); for(int k = 0; k < DT_CTL_WORKER_RESERVED; k++) - { - err = pthread_join(s->thread_res[k], NULL); - dt_print(DT_DEBUG_CONTROL, "[dt_control_shutdown] joined worker %i%s", k, err ? ", error" : ""); - } + pthread_join(control->thread_res[k], NULL); } -void dt_control_cleanup(dt_control_t *s) +void dt_control_cleanup(dt_control_t *control) { - if(!s) + if(!control) return; // vacuum TODO: optional? // DT_DEBUG_SQLITE3_EXEC(dt_database_get(darktable.db), "PRAGMA incremental_vacuum(0)", NULL, NULL, NULL); // DT_DEBUG_SQLITE3_EXEC(dt_database_get(darktable.db), "vacuum", NULL, NULL, NULL); - dt_control_jobs_cleanup(s); - dt_pthread_mutex_destroy(&s->queue_mutex); - dt_pthread_mutex_destroy(&s->cond_mutex); - dt_pthread_mutex_destroy(&s->log_mutex); - dt_pthread_mutex_destroy(&s->toast_mutex); - dt_pthread_mutex_destroy(&s->res_mutex); - dt_pthread_mutex_destroy(&s->progress_system.mutex); - if(s->widgets) g_hash_table_destroy(s->widgets); - if(s->shortcuts) g_sequence_free(s->shortcuts); - if(s->input_drivers) g_slist_free_full(s->input_drivers, g_free); + dt_control_jobs_cleanup(control); + dt_pthread_mutex_destroy(&control->queue_mutex); + dt_pthread_mutex_destroy(&control->cond_mutex); + dt_pthread_mutex_destroy(&control->log_mutex); + dt_pthread_mutex_destroy(&control->toast_mutex); + dt_pthread_mutex_destroy(&control->res_mutex); + dt_pthread_mutex_destroy(&control->progress_system.mutex); + if(control->widgets) g_hash_table_destroy(control->widgets); + if(control->shortcuts) g_sequence_free(control->shortcuts); + if(control->input_drivers) g_slist_free_full(control->input_drivers, g_free); } diff --git a/src/control/jobs.c b/src/control/jobs.c index 90bec7a41c36..13819b7d5aa7 100644 --- a/src/control/jobs.c +++ b/src/control/jobs.c @@ -77,6 +77,20 @@ static inline gboolean _control_job_equal(_dt_job_t *j1, _dt_job_t *j2) && (g_strcmp0(j1->description, j2->description) == 0)); } +static __thread int threadid = -1; + +static inline int32_t _control_get_threadid() +{ + if(threadid > -1) return threadid; + return darktable.control->num_threads; +} + +static inline int32_t _control_get_threadid_res() +{ + if(threadid > -1) return threadid; + return DT_CTL_WORKER_RESERVED; +} + static void _control_job_set_state(_dt_job_t *job, dt_job_state_t state) { @@ -132,6 +146,7 @@ void *dt_control_job_get_params(const _dt_job_t *job) dt_job_t *dt_control_job_create(dt_job_execute_callback execute, const char *msg, ...) { + if(!dt_control_running()) return NULL; _dt_job_t *job = calloc(1, sizeof(_dt_job_t)); if(!job) return NULL; @@ -159,7 +174,7 @@ gboolean dt_control_job_is_synchronous(const dt_job_t *job) return job->is_synchronous; } -void dt_control_job_set_synchronous(dt_job_t *job, gboolean sync) +static void _control_job_set_synchronous(dt_job_t *job, gboolean sync) { job->is_synchronous = sync; } @@ -178,14 +193,6 @@ void dt_control_job_dispose(_dt_job_t *job) free(job); } -void dt_control_job_set_state_callback(_dt_job_t *job, dt_job_state_change_callback cb) -{ - // once the job got added to the queue it may not be changed from the outside - if(dt_control_job_get_state(job) != DT_JOB_STATE_INITIALIZED) - return; // get_state returns DISPOSED when job == NULL - job->state_changed_cb = cb; -} - // We don't want to log dt_get_wtime() as we already show the stamp static void _control_job_print(_dt_job_t *job, const char *info, const char *err, int32_t res) { @@ -199,34 +206,6 @@ void dt_control_job_cancel(_dt_job_t *job) _control_job_set_state(job, DT_JOB_STATE_CANCELLED); } -void dt_control_job_wait(_dt_job_t *job) -{ - if(!job) return; - dt_job_state_t state = dt_control_job_get_state(job); - - // NOTE: could also use signals. - - // if the job is merely queued and hasn't started yet, we - // need to wait until it is actually started before attempting - // to grab the mutex, or it will always succeed immediately - while(state == DT_JOB_STATE_QUEUED) - { - g_usleep(100000); // wait 0.1 seconds - state = dt_control_job_get_state(job); - } - - /* if job execution is not finished let's wait for it */ - if(state == DT_JOB_STATE_RUNNING || state == DT_JOB_STATE_CANCELLED) - { - // once the job finishes, it unlocks the mutex - // so by locking the mutex here, we will only get the lock once the job - // has finished and unlocked it. - dt_pthread_mutex_lock(&job->wait_mutex); - // yay, the job finished, we got the lock. nothing more to do. - dt_pthread_mutex_unlock(&job->wait_mutex); - } -} - static gboolean _control_run_job_res(dt_control_t *control, int32_t res) { if(((unsigned int)res) >= DT_CTL_WORKER_RESERVED) @@ -311,7 +290,7 @@ static _dt_job_t *_control_schedule_job(dt_control_t *control) if(winner_queue == DT_JOB_QUEUE_USER_EXPORT) control->export_scheduled = TRUE; // and place it in scheduled job array (for job deduping) - control->job[dt_control_get_threadid()] = job; + control->job[_control_get_threadid()] = job; // increment the priorities of the others for(int i = 0; i < DT_JOB_QUEUE_MAX; i++) @@ -327,7 +306,7 @@ static _dt_job_t *_control_schedule_job(dt_control_t *control) static void _control_job_execute(_dt_job_t *job) { - _control_job_print(job, "run_job+", "", DT_CTL_WORKER_RESERVED + dt_control_get_threadid()); + _control_job_print(job, "run_job+", "", DT_CTL_WORKER_RESERVED + _control_get_threadid()); _control_job_set_state(job, DT_JOB_STATE_RUNNING); @@ -335,7 +314,7 @@ static void _control_job_execute(_dt_job_t *job) job->result = job->execute(job); _control_job_set_state(job, DT_JOB_STATE_FINISHED); - _control_job_print(job, "run_job-", "", DT_CTL_WORKER_RESERVED + dt_control_get_threadid()); + _control_job_print(job, "run_job-", "", DT_CTL_WORKER_RESERVED + _control_get_threadid()); } static gboolean _control_run_job(dt_control_t *control) @@ -353,7 +332,7 @@ static gboolean _control_run_job(dt_control_t *control) // remove the job from scheduled job array (for job deduping) dt_pthread_mutex_lock(&control->queue_mutex); - control->job[dt_control_get_threadid()] = NULL; + control->job[_control_get_threadid()] = NULL; if(job->queue == DT_JOB_QUEUE_USER_EXPORT) control->export_scheduled = FALSE; dt_pthread_mutex_unlock(&control->queue_mutex); @@ -413,7 +392,7 @@ gboolean dt_control_add_job(dt_control_t *control, { // whatever we are adding here won't be scheduled as the system isn't running. execute it synchronous instead. dt_pthread_mutex_lock(&job->wait_mutex); // is that even needed? - dt_control_job_set_synchronous(job, TRUE); + _control_job_set_synchronous(job, TRUE); _control_job_execute(job); dt_pthread_mutex_unlock(&job->wait_mutex); @@ -457,7 +436,7 @@ gboolean dt_control_add_job(dt_control_t *control, // if the job is already in the queue -> move it to the top for(GList *iter = *queue; iter; iter = g_list_next(iter)) { - _dt_job_t *other_job = (_dt_job_t *)iter->data; + _dt_job_t *other_job = iter->data; if(_control_job_equal(job, other_job)) { _control_job_print(other_job, "add_job", "found job already in queue", -1); @@ -480,8 +459,8 @@ gboolean dt_control_add_job(dt_control_t *control, if(length > DT_CONTROL_MAX_JOBS) { GList *last = g_list_last(*queue); - _control_job_set_state((_dt_job_t *)last->data, DT_JOB_STATE_DISCARDED); - dt_control_job_dispose((_dt_job_t *)last->data); + _control_job_set_state(last->data, DT_JOB_STATE_DISCARDED); + dt_control_job_dispose(last->data); *queue = g_list_delete_link(*queue, last); length--; } @@ -515,27 +494,13 @@ gboolean dt_control_add_job(dt_control_t *control, return FALSE; } -static __thread int threadid = -1; - -int32_t dt_control_get_threadid() -{ - if(threadid > -1) return threadid; - return darktable.control->num_threads; -} - -static inline int32_t _control_get_threadid_res() -{ - if(threadid > -1) return threadid; - return DT_CTL_WORKER_RESERVED; -} - static void *_control_work_res(void *ptr) { #ifdef _OPENMP // need to do this in every thread omp_set_num_threads(dt_get_num_threads()); #endif worker_thread_parameters_t *params = (worker_thread_parameters_t *)ptr; - dt_control_t *s = params->self; + dt_control_t *control = params->self; threadid = params->threadid; char name[16] = {0}; snprintf(name, sizeof(name), "worker res %d", threadid); @@ -544,15 +509,14 @@ static void *_control_work_res(void *ptr) int32_t threadid_res = _control_get_threadid_res(); while(dt_control_running()) { - // dt_print(DT_DEBUG_CONTROL, "[control_work] %d", threadid_res); - if(_control_run_job_res(s, threadid_res)) + if(_control_run_job_res(control, threadid_res)) { // wait for a new job. int old; pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old); - dt_pthread_mutex_lock(&s->cond_mutex); - dt_pthread_cond_wait(&s->cond, &s->cond_mutex); - dt_pthread_mutex_unlock(&s->cond_mutex); + dt_pthread_mutex_lock(&control->cond_mutex); + dt_pthread_cond_wait(&control->cond, &control->cond_mutex); + dt_pthread_mutex_unlock(&control->cond_mutex); int tmp; pthread_setcancelstate(old, &tmp); } @@ -589,7 +553,6 @@ static void *_control_work(void *ptr) // int32_t threadid = dt_control_get_threadid(); while(dt_control_running()) { - // dt_print(DT_DEBUG_CONTROL, "[control_work] %d", threadid); if(_control_run_job(control)) { // wait for a new job. @@ -640,18 +603,16 @@ void dt_control_jobs_init(dt_control_t *control) g_atomic_int_set(&control->running, DT_CONTROL_STATE_RUNNING); - int err = 0; // collected errors while creating all threads - for(int k = 0; k < control->num_threads; k++) { worker_thread_parameters_t *params = calloc(1, sizeof(worker_thread_parameters_t)); params->self = control; params->threadid = k; - err |= dt_pthread_create(&control->thread[k], _control_work, params); + dt_pthread_create(&control->thread[k], _control_work, params); } /* create queue kicker thread */ - err |= dt_pthread_create(&control->kick_on_workers_thread, _control_worker_kicker, control); + dt_pthread_create(&control->kick_on_workers_thread, _control_worker_kicker, control); for(int k = 0; k < DT_CTL_WORKER_RESERVED; k++) { @@ -660,14 +621,12 @@ void dt_control_jobs_init(dt_control_t *control) worker_thread_parameters_t *params = calloc(1, sizeof(worker_thread_parameters_t)); params->self = control; params->threadid = k; - err |= dt_pthread_create(&control->thread_res[k], _control_work_res, params); + dt_pthread_create(&control->thread_res[k], _control_work_res, params); } /* create thread taking care of connecting gphoto2 devices */ #ifdef HAVE_GPHOTO2 - err |= dt_pthread_create(&control->update_gphoto_thread, dt_update_cameras_thread, control); + dt_pthread_create(&control->update_gphoto_thread, dt_update_cameras_thread, control); #endif - if(err != 0) - dt_print(DT_DEBUG_ALWAYS, "[dt_control_jobs_init] couldn't create all threads, problems ahead"); } void dt_control_jobs_cleanup(dt_control_t *control) diff --git a/src/control/jobs.h b/src/control/jobs.h index ffbd0a24373a..530afc002f12 100644 --- a/src/control/jobs.h +++ b/src/control/jobs.h @@ -62,13 +62,9 @@ typedef void (*dt_job_destroy_callback)(void *data); dt_job_t *dt_control_job_create(dt_job_execute_callback execute, const char *msg, ...) __attribute__((format(printf, 2, 3))); /** destroy a job object and free its memory. this does NOT remove it from any job queues! */ void dt_control_job_dispose(dt_job_t *job); -/** setup a state callback for job. */ -void dt_control_job_set_state_callback(dt_job_t *job, dt_job_state_change_callback cb); /** cancel a job, running or in queue. */ void dt_control_job_cancel(dt_job_t *job); dt_job_state_t dt_control_job_get_state(dt_job_t *job); -/** wait for a job to finish execution. */ -void dt_control_job_wait(dt_job_t *job); /** set job params and a callback to destroy those params */ void dt_control_job_set_params(dt_job_t *job, void *params, dt_job_destroy_callback callback); /** set job params (with size params_size) and a callback to destroy those params. @@ -92,9 +88,6 @@ gboolean dt_control_add_job_res(struct dt_control_t *s, dt_job_t *job, int32_t r dt_view_type_flags_t dt_control_job_get_view_creator(const dt_job_t *job); gboolean dt_control_job_is_synchronous(const dt_job_t *job); -void dt_control_job_set_synchronous(dt_job_t *job, gboolean sync); - -int32_t dt_control_get_threadid(); #ifdef HAVE_GPHOTO2 #include "control/jobs/camera_jobs.h" From 96d33fb5430e6b95ebbc9387c17efd45a14e55d7 Mon Sep 17 00:00:00 2001 From: Hanno Schwalm Date: Sat, 14 Dec 2024 17:12:24 +0100 Subject: [PATCH 3/5] Implement pending jobs counter While running the job dispatchers we keep track on jobs being inserted and executed so we at least know about the number of unprocessed jobs via dt_control_jobs_pending() Also show pending jobs in the log if closing dt. --- src/control/control.c | 6 ++++-- src/control/control.h | 1 + src/control/jobs.c | 10 ++++++++++ src/control/jobs.h | 1 + 4 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/control/control.c b/src/control/control.c index bbe775f8a248..a863a83d8298 100644 --- a/src/control/control.c +++ b/src/control/control.c @@ -201,6 +201,7 @@ void dt_control_init(dt_control_t *s) s->widget_definitions = g_ptr_array_new (); s->input_drivers = NULL; dt_atomic_set_int(&s->running, DT_CONTROL_STATE_DISABLED); + dt_atomic_set_int(&s->pending_jobs, 0); s->cups_started = FALSE; dt_action_define_fallback(DT_ACTION_TYPE_IOP, &dt_action_def_iop); @@ -328,8 +329,9 @@ void dt_control_shutdown(dt_control_t *control) pthread_cond_broadcast(&control->cond); dt_pthread_mutex_unlock(&control->cond_mutex); - dt_print(DT_DEBUG_CONTROL, "[dt_control_shutdown] closing control threads%s", - cleanup ? " in cleanup mode" : ""); + dt_print(DT_DEBUG_CONTROL, "[dt_control_shutdown] closing control threads%s, %d pending jobs", + cleanup ? " in cleanup mode" : "", + dt_control_jobs_pending(control)); if(!cleanup) return; // if not running there are no threads to join diff --git a/src/control/control.h b/src/control/control.h index 07e76c6d607a..64a83fab6fec 100644 --- a/src/control/control.h +++ b/src/control/control.h @@ -187,6 +187,7 @@ typedef struct dt_control_t // job management dt_atomic_int running; + dt_atomic_int pending_jobs; gboolean cups_started; gboolean export_scheduled; dt_pthread_mutex_t queue_mutex, cond_mutex; diff --git a/src/control/jobs.c b/src/control/jobs.c index 13819b7d5aa7..2e889543bef8 100644 --- a/src/control/jobs.c +++ b/src/control/jobs.c @@ -315,6 +315,7 @@ static void _control_job_execute(_dt_job_t *job) _control_job_set_state(job, DT_JOB_STATE_FINISHED); _control_job_print(job, "run_job-", "", DT_CTL_WORKER_RESERVED + _control_get_threadid()); + dt_atomic_add_int(&darktable.control->pending_jobs, -1); } static gboolean _control_run_job(dt_control_t *control) @@ -433,6 +434,7 @@ gboolean dt_control_add_job(dt_control_t *control, } } + dt_atomic_add_int(&darktable.control->pending_jobs, 1); // if the job is already in the queue -> move it to the top for(GList *iter = *queue; iter; iter = g_list_next(iter)) { @@ -447,6 +449,7 @@ gboolean dt_control_add_job(dt_control_t *control, job_for_disposal = job; job = other_job; + dt_atomic_add_int(&darktable.control->pending_jobs, -1); break; // there can't be any further copy in the list } } @@ -463,6 +466,7 @@ gboolean dt_control_add_job(dt_control_t *control, dt_control_job_dispose(last->data); *queue = g_list_delete_link(*queue, last); length--; + dt_atomic_add_int(&darktable.control->pending_jobs, -1); } control->queue_length[queue_id] = length; @@ -478,6 +482,7 @@ gboolean dt_control_add_job(dt_control_t *control, job->priority = DT_CONTROL_FG_PRIORITY; *queue = g_list_append(*queue, job); control->queue_length[queue_id]++; + dt_atomic_add_int(&darktable.control->pending_jobs, 1); } _control_job_set_state(job, DT_JOB_STATE_QUEUED); dt_pthread_mutex_unlock(&control->queue_mutex); @@ -637,6 +642,11 @@ void dt_control_jobs_cleanup(dt_control_t *control) control->thread = NULL; } +int dt_control_jobs_pending(dt_control_t *control) +{ + return control ? dt_atomic_get_int(&control->pending_jobs) : 0; +} + // clang-format off // modelines: These editor modelines have been set for all relevant files by tools/update_modelines.py // vim: shiftwidth=2 expandtab tabstop=2 cindent diff --git a/src/control/jobs.h b/src/control/jobs.h index 530afc002f12..7f36bc79ee7d 100644 --- a/src/control/jobs.h +++ b/src/control/jobs.h @@ -82,6 +82,7 @@ double dt_control_job_get_progress(dt_job_t *job); struct dt_control_t; void dt_control_jobs_init(struct dt_control_t *control); void dt_control_jobs_cleanup(struct dt_control_t *control); +int dt_control_jobs_pending(struct dt_control_t *control); gboolean dt_control_add_job(struct dt_control_t *control, dt_job_queue_t queue_id, dt_job_t *job); gboolean dt_control_add_job_res(struct dt_control_t *s, dt_job_t *job, int32_t res); From 12776507b849848e378505ef353073edf40552d5 Mon Sep 17 00:00:00 2001 From: Hanno Schwalm Date: Sat, 14 Dec 2024 17:33:31 +0100 Subject: [PATCH 4/5] Only accept the first dt_control_quit() Strictly prohibit reentrant use of dt_control_quit() and avoid a race condition via atomic instruction. --- src/control/control.c | 8 +++++++- src/control/control.h | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/control/control.c b/src/control/control.c index a863a83d8298..f7ddefc0bcd4 100644 --- a/src/control/control.c +++ b/src/control/control.c @@ -202,6 +202,7 @@ void dt_control_init(dt_control_t *s) s->input_drivers = NULL; dt_atomic_set_int(&s->running, DT_CONTROL_STATE_DISABLED); dt_atomic_set_int(&s->pending_jobs, 0); + dt_atomic_set_int(&s->quitting, 0); s->cups_started = FALSE; dt_action_define_fallback(DT_ACTION_TYPE_IOP, &dt_action_def_iop); @@ -295,9 +296,14 @@ gboolean dt_control_running() void dt_control_quit() { + dt_control_t *control = darktable.control; + if(!control) return; + + const gboolean quitting = dt_atomic_exch_int(&control->quitting, 1) == 1; + if(quitting) return; + if(dt_control_running()) { - dt_control_t *control = darktable.control; #ifdef HAVE_PRINT dt_printers_abort_discovery(); // Cups timeout could be pretty long, at least 30seconds diff --git a/src/control/control.h b/src/control/control.h index 64a83fab6fec..e295331ee45f 100644 --- a/src/control/control.h +++ b/src/control/control.h @@ -188,6 +188,7 @@ typedef struct dt_control_t // job management dt_atomic_int running; dt_atomic_int pending_jobs; + dt_atomic_int quitting; gboolean cups_started; gboolean export_scheduled; dt_pthread_mutex_t queue_mutex, cond_mutex; From ecaa37bc6b09871c3788eb6ae1cee27a8c41d5df Mon Sep 17 00:00:00 2001 From: Hanno Schwalm Date: Sun, 15 Dec 2024 14:37:14 +0100 Subject: [PATCH 5/5] File related control jobs safety As we have control jobs on "file actions" we have to make sure that we have 'dt_imageio` available for: export, import, hdr creation, flip and exif refresh inside the job. While working the list of images we test for dt_control_running() and only process the job specific action if so. Also we have some **very** time consuming jobs that can be canceled in the UI, it seems to be much better to understand a ctrl-q or click on close-darktable button as a request-to-cancel so there will be no lengthy background actions after the window has closed. While we aborted file import via ctrl-q we don't want to wait ... --- src/control/jobs/control_jobs.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/control/jobs/control_jobs.c b/src/control/jobs/control_jobs.c index 62908d100c31..f970746b6ff9 100644 --- a/src/control/jobs/control_jobs.c +++ b/src/control/jobs/control_jobs.c @@ -207,7 +207,7 @@ static int32_t _generic_dt_control_fileop_images_job_run gboolean completeSuccess = TRUE; double prev_time = 0; - while(t && !_job_cancelled(job)) + while(dt_control_running() && t && !_job_cancelled(job)) { completeSuccess &= (fileop_callback(GPOINTER_TO_INT(t->data), film_id) != -1); t = g_list_next(t); @@ -622,7 +622,7 @@ static int32_t dt_control_merge_hdr_job_run(dt_job_t *job) (dt_control_merge_hdr_format_t){.parent = { 0 }, .d = &d }; int num = 1; - while(t) + while(t && dt_control_running()) { if(d.abort) goto end; @@ -641,7 +641,7 @@ static int32_t dt_control_merge_hdr_job_run(dt_job_t *job) num++; } - if(d.abort) goto end; + if(d.abort || !dt_control_running()) goto end; // normalize by white level to make clipping at 1.0 work as expected @@ -761,7 +761,7 @@ static int32_t dt_control_flip_images_job_run(dt_job_t *job) "flipping %d images", total), total); dt_control_job_set_progress_message(job, message); double prev_time = 0; - for(; t && !_job_cancelled(job); t = g_list_next(t)) + for(; dt_control_running() && t && !_job_cancelled(job); t = g_list_next(t)) { const dt_imgid_t imgid = GPOINTER_TO_INT(t->data); dt_image_flip(imgid, cw); @@ -942,7 +942,7 @@ static int32_t dt_control_remove_images_job_run(dt_job_t *job) double fraction = 0.0; double prev_time = 0; - for(; t && !_job_cancelled(job); t = g_list_next(t)) + for(; dt_control_running() && t && !_job_cancelled(job); t = g_list_next(t)) { const dt_imgid_t imgid = GPOINTER_TO_INT(t->data); const int32_t exist_count = _count_images_using_overlay(imgid); @@ -1527,7 +1527,7 @@ static int32_t dt_control_refresh_exif_run(dt_job_t *job) dt_control_job_set_progress_message(job, message); double prev_time = 0; - while(t) + while(t && dt_control_running()) { const dt_imgid_t imgid = GPOINTER_TO_INT(t->data); if(dt_is_valid_imgid(imgid)) @@ -1871,7 +1871,7 @@ static int32_t dt_control_export_job_run(dt_job_t *job) double prev_time = 0; - while(t && !_job_cancelled(job)) + while(dt_control_running() && t && !_job_cancelled(job)) { const dt_imgid_t imgid = GPOINTER_TO_INT(t->data); t = g_list_next(t); @@ -2887,7 +2887,7 @@ static int32_t _control_import_job_run(dt_job_t *job) double update_interval = INIT_UPDATE_INTERVAL; char *prev_filename = NULL; char *prev_output = NULL; - for(GList *img = t; img && !_job_cancelled(job); img = g_list_next(img)) + for(GList *img = t; dt_control_running() && img && !_job_cancelled(job); img = g_list_next(img)) { if(data->session) { @@ -3016,7 +3016,7 @@ void dt_control_import(GList *imgs, _control_import_job_create(imgs, datetime_override, inplace, wait ? &wait : NULL)); // if import in place single image => synchronous import - while(wait) + while(wait && dt_control_running()) g_usleep(100); }