From 1aef86f650e39586ebecdfb347f6ac0b702e23d5 Mon Sep 17 00:00:00 2001 From: Vojtech Bubnik Date: Tue, 4 May 2021 16:07:25 +0200 Subject: [PATCH] Implemented generic mechanism for executing tasks on UI thread synchronously from the background slicing thread, that supports cancellation. The generic mechanism is used for generating thumbnails into G-code and Fixes Fix deadlock when canceling the slicing while gcode is creating thumbnails #6476 Thanks @supermerill for pointing out the issue. --- src/libslic3r/GCode.cpp | 3 +- src/libslic3r/GCode/ThumbnailData.hpp | 14 ++- src/slic3r/GUI/BackgroundSlicingProcess.cpp | 110 +++++++++++++++----- src/slic3r/GUI/BackgroundSlicingProcess.hpp | 23 ++++ src/slic3r/GUI/Plater.cpp | 22 ++-- 5 files changed, 126 insertions(+), 46 deletions(-) diff --git a/src/libslic3r/GCode.cpp b/src/libslic3r/GCode.cpp index c5b28b3a0..3b1575f8f 100644 --- a/src/libslic3r/GCode.cpp +++ b/src/libslic3r/GCode.cpp @@ -887,8 +887,7 @@ namespace DoExport { if (thumbnail_cb != nullptr) { const size_t max_row_length = 78; - ThumbnailsList thumbnails; - thumbnail_cb(thumbnails, sizes, true, true, true, true); + ThumbnailsList thumbnails = thumbnail_cb(ThumbnailsParams{ sizes, true, true, true, true }); for (const ThumbnailData& data : thumbnails) { if (data.is_valid()) diff --git a/src/libslic3r/GCode/ThumbnailData.hpp b/src/libslic3r/GCode/ThumbnailData.hpp index 2a302ed85..195dfe633 100644 --- a/src/libslic3r/GCode/ThumbnailData.hpp +++ b/src/libslic3r/GCode/ThumbnailData.hpp @@ -19,8 +19,18 @@ struct ThumbnailData bool is_valid() const; }; -typedef std::vector ThumbnailsList; -typedef std::function ThumbnailsGeneratorCallback; +using ThumbnailsList = std::vector; + +struct ThumbnailsParams +{ + const Vec2ds sizes; + bool printable_only; + bool parts_only; + bool show_bed; + bool transparent_background; +}; + +typedef std::function ThumbnailsGeneratorCallback; } // namespace Slic3r diff --git a/src/slic3r/GUI/BackgroundSlicingProcess.cpp b/src/slic3r/GUI/BackgroundSlicingProcess.cpp index 089fba656..498391beb 100644 --- a/src/slic3r/GUI/BackgroundSlicingProcess.cpp +++ b/src/slic3r/GUI/BackgroundSlicingProcess.cpp @@ -145,7 +145,7 @@ void BackgroundSlicingProcess::process_fff() // Passing the timestamp evt.SetInt((int)(m_fff_print->step_state_with_timestamp(PrintStep::psSlicingFinished).timestamp)); wxQueueEvent(GUI::wxGetApp().mainframe->m_plater, evt.Clone()); - m_fff_print->export_gcode(m_temp_output_path, m_gcode_result, m_thumbnail_cb); + m_fff_print->export_gcode(m_temp_output_path, m_gcode_result, [this](const ThumbnailsParams& params) { return this->render_thumbnails(params); }); if (this->set_step_started(bspsGCodeFinalize)) { if (! m_export_path.empty()) { wxQueueEvent(GUI::wxGetApp().mainframe->m_plater, new wxCommandEvent(m_event_export_began_id)); @@ -221,21 +221,14 @@ void BackgroundSlicingProcess::process_sla() const std::string export_path = m_sla_print->print_statistics().finalize_output_path(m_export_path); + ThumbnailsList thumbnails = this->render_thumbnails( + ThumbnailsParams{current_print()->full_print_config().option("thumbnails")->values, true, true, true, true}); + Zipper zipper(export_path); - m_sla_archive.export_print(zipper, *m_sla_print); - - if (m_thumbnail_cb != nullptr) - { - ThumbnailsList thumbnails; - m_thumbnail_cb(thumbnails, current_print()->full_print_config().option("thumbnails")->values, true, true, true, true); -// m_thumbnail_cb(thumbnails, current_print()->full_print_config().option("thumbnails")->values, true, false, true, true); // renders also supports and pad - for (const ThumbnailData& data : thumbnails) - { - if (data.is_valid()) - write_thumbnail(zipper, data); - } - } - + m_sla_archive.export_print(zipper, *m_sla_print); // true, false, true, true); // renders also supports and pad + for (const ThumbnailData& data : thumbnails) + if (data.is_valid()) + write_thumbnail(zipper, data); zipper.finalize(); m_print->set_status(100, (boost::format(_utf8(L("Masked SLA file exported to %1%"))) % export_path).str()); @@ -362,6 +355,7 @@ bool BackgroundSlicingProcess::start() return true; } +// To be called on the UI thread. bool BackgroundSlicingProcess::stop() { // m_print->state_mutex() shall NOT be held. Unfortunately there is no interface to test for it. @@ -372,6 +366,8 @@ bool BackgroundSlicingProcess::stop() } // assert(this->running()); if (m_state == STATE_STARTED || m_state == STATE_RUNNING) { + // Cancel any task planned by the background thread on UI thread. + cancel_ui_task(m_ui_task); m_print->cancel(); // Wait until the background processing stops by being canceled. m_condition.wait(lck, [this](){ return m_state == STATE_CANCELED; }); @@ -396,7 +392,7 @@ bool BackgroundSlicingProcess::reset() return stopped; } -// To be called by Print::apply() through the Print::m_cancel_callback to stop the background +// To be called by Print::apply() on the UI thread through the Print::m_cancel_callback to stop the background // processing before changing any data of running or finalized milestones. // This function shall not trigger any UI update through the wxWidgets event. void BackgroundSlicingProcess::stop_internal() @@ -408,6 +404,8 @@ void BackgroundSlicingProcess::stop_internal() std::unique_lock lck(m_mutex); assert(m_state == STATE_STARTED || m_state == STATE_RUNNING || m_state == STATE_FINISHED || m_state == STATE_CANCELED); if (m_state == STATE_STARTED || m_state == STATE_RUNNING) { + // Cancel any task planned by the background thread on UI thread. + cancel_ui_task(m_ui_task); // At this point of time the worker thread may be blocking on m_print->state_mutex(). // Set the print state to canceled before unlocking the state_mutex(), so when the worker thread wakes up, // it throws the CanceledException(). @@ -424,6 +422,60 @@ void BackgroundSlicingProcess::stop_internal() m_print->set_cancel_callback([](){}); } +// Execute task from background thread on the UI thread. Returns true if processed, false if cancelled. +bool BackgroundSlicingProcess::execute_ui_task(std::function task) +{ + bool running = false; + if (m_mutex.try_lock()) { + // Cancellation is either not in process, or already canceled and waiting for us to finish. + // There must be no UI task planned. + assert(! m_ui_task); + if (! m_print->canceled()) { + running = true; + m_ui_task = std::make_shared(); + } + m_mutex.unlock(); + } else { + // Cancellation is in process. + } + + bool result = false; + if (running) { + std::shared_ptr ctx = m_ui_task; + GUI::wxGetApp().mainframe->m_plater->CallAfter([task, ctx]() { + // Running on the UI thread, thus ctx->state does not need to be guarded with mutex against ::cancel_ui_task(). + assert(ctx->state == UITask::Planned || ctx->state == UITask::Canceled); + if (ctx->state == UITask::Planned) { + task(); + std::unique_lock lck(ctx->mutex); + ctx->state = UITask::Finished; + } + // Wake up the worker thread from the UI thread. + ctx->condition.notify_all(); + }); + + { + std::unique_lock lock(ctx->mutex); + ctx->condition.wait(lock, [&ctx]{ return ctx->state == UITask::Finished || ctx->state == UITask::Canceled; }); + } + result = ctx->state == UITask::Finished; + m_ui_task.reset(); + } + + return result; +} + +// To be called on the UI thread from ::stop() and ::stop_internal(). +void BackgroundSlicingProcess::cancel_ui_task(std::shared_ptr task) +{ + if (task) { + std::unique_lock lck(task->mutex); + task->state = UITask::Canceled; + lck.unlock(); + task->condition.notify_all(); + } +} + bool BackgroundSlicingProcess::empty() const { assert(m_print != nullptr); @@ -546,19 +598,14 @@ void BackgroundSlicingProcess::prepare_upload() } else { m_upload_job.upload_data.upload_path = m_sla_print->print_statistics().finalize_output_path(m_upload_job.upload_data.upload_path.string()); + ThumbnailsList thumbnails = this->render_thumbnails( + ThumbnailsParams{current_print()->full_print_config().option("thumbnails")->values, true, true, true, true}); + // true, false, true, true); // renders also supports and pad Zipper zipper{source_path.string()}; m_sla_archive.export_print(zipper, *m_sla_print, m_upload_job.upload_data.upload_path.string()); - if (m_thumbnail_cb != nullptr) - { - ThumbnailsList thumbnails; - m_thumbnail_cb(thumbnails, current_print()->full_print_config().option("thumbnails")->values, true, true, true, true); -// m_thumbnail_cb(thumbnails, current_print()->full_print_config().option("thumbnails")->values, true, false, true, true); // renders also supports and pad - for (const ThumbnailData& data : thumbnails) - { - if (data.is_valid()) - write_thumbnail(zipper, data); - } - } + for (const ThumbnailData& data : thumbnails) + if (data.is_valid()) + write_thumbnail(zipper, data); zipper.finalize(); } @@ -569,4 +616,13 @@ void BackgroundSlicingProcess::prepare_upload() GUI::wxGetApp().printhost_job_queue().enqueue(std::move(m_upload_job)); } +// Executed by the background thread, to start a task on the UI thread. +ThumbnailsList BackgroundSlicingProcess::render_thumbnails(const ThumbnailsParams ¶ms) +{ + ThumbnailsList thumbnails; + if (m_thumbnail_cb) + this->execute_ui_task([this, ¶ms, &thumbnails](){ thumbnails = this->m_thumbnail_cb(params); }); + return thumbnails; +} + }; // namespace Slic3r diff --git a/src/slic3r/GUI/BackgroundSlicingProcess.hpp b/src/slic3r/GUI/BackgroundSlicingProcess.hpp index d3819f15c..dc24ec0ad 100644 --- a/src/slic3r/GUI/BackgroundSlicingProcess.hpp +++ b/src/slic3r/GUI/BackgroundSlicingProcess.hpp @@ -212,6 +212,23 @@ private: std::condition_variable m_condition; State m_state = STATE_INITIAL; + // For executing tasks from the background thread on UI thread synchronously (waiting for result) using wxWidgets CallAfter(). + // When the background proces is canceled, the UITask has to be invalidated as well, so that it will not be + // executed on the UI thread referencing invalid data. + struct UITask { + enum State { + Planned, + Finished, + Canceled, + }; + State state = Planned; + std::mutex mutex; + std::condition_variable condition; + }; + // Only one UI task may be planned by the background thread to be executed on the UI thread, as the background + // thread is blocking until the UI thread calculation finishes. + std::shared_ptr m_ui_task; + PrintState m_step_state; mutable tbb::mutex m_step_state_mutex; bool set_step_started(BackgroundSlicingProcessStep step); @@ -222,6 +239,12 @@ private: // If the background processing stop was requested, throw CanceledException. void throw_if_canceled() const { if (m_print->canceled()) throw CanceledException(); } void prepare_upload(); + // To be executed at the background thread. + ThumbnailsList render_thumbnails(const ThumbnailsParams ¶ms); + // Execute task from background thread on the UI thread synchronously. Returns true if processed, false if cancelled before executing the task. + bool execute_ui_task(std::function task); + // To be called from inside m_mutex to cancel a planned UI task. + static void cancel_ui_task(std::shared_ptr task); // wxWidgets command ID to be sent to the plater to inform that the slicing is finished, and the G-code export will continue. int m_event_slicing_completed_id = 0; diff --git a/src/slic3r/GUI/Plater.cpp b/src/slic3r/GUI/Plater.cpp index c0cda0042..841a07513 100644 --- a/src/slic3r/GUI/Plater.cpp +++ b/src/slic3r/GUI/Plater.cpp @@ -1684,7 +1684,7 @@ struct Plater::priv bool can_split(bool to_objects) const; void generate_thumbnail(ThumbnailData& data, unsigned int w, unsigned int h, bool printable_only, bool parts_only, bool show_bed, bool transparent_background); - void generate_thumbnails(ThumbnailsList& thumbnails, const Vec2ds& sizes, bool printable_only, bool parts_only, bool show_bed, bool transparent_background); + ThumbnailsList generate_thumbnails(const ThumbnailsParams& params); void bring_instance_forward() const; @@ -1760,15 +1760,7 @@ Plater::priv::priv(Plater *q, MainFrame *main_frame) background_process.set_fff_print(&fff_print); background_process.set_sla_print(&sla_print); background_process.set_gcode_result(&gcode_result); - background_process.set_thumbnail_cb([this](ThumbnailsList& thumbnails, const Vec2ds& sizes, bool printable_only, bool parts_only, bool show_bed, bool transparent_background) - { - std::packaged_task task([this](ThumbnailsList& thumbnails, const Vec2ds& sizes, bool printable_only, bool parts_only, bool show_bed, bool transparent_background) { - generate_thumbnails(thumbnails, sizes, printable_only, parts_only, show_bed, transparent_background); - }); - std::future result = task.get_future(); - wxTheApp->CallAfter([&]() { task(thumbnails, sizes, printable_only, parts_only, show_bed, transparent_background); }); - result.wait(); - }); + background_process.set_thumbnail_cb([this](const ThumbnailsParams& params) { return this->generate_thumbnails(params); }); background_process.set_slicing_completed_event(EVT_SLICING_COMPLETED); background_process.set_finished_event(EVT_PROCESS_COMPLETED); background_process.set_export_began_event(EVT_EXPORT_BEGAN); @@ -3812,17 +3804,17 @@ void Plater::priv::generate_thumbnail(ThumbnailData& data, unsigned int w, unsig view3D->get_canvas3d()->render_thumbnail(data, w, h, printable_only, parts_only, show_bed, transparent_background); } -void Plater::priv::generate_thumbnails(ThumbnailsList& thumbnails, const Vec2ds& sizes, bool printable_only, bool parts_only, bool show_bed, bool transparent_background) +ThumbnailsList Plater::priv::generate_thumbnails(const ThumbnailsParams& params) { - thumbnails.clear(); - for (const Vec2d& size : sizes) - { + ThumbnailsList thumbnails; + for (const Vec2d& size : params.sizes) { thumbnails.push_back(ThumbnailData()); Point isize(size); // round to ints - generate_thumbnail(thumbnails.back(), isize.x(), isize.y(), printable_only, parts_only, show_bed, transparent_background); + generate_thumbnail(thumbnails.back(), isize.x(), isize.y(), params.printable_only, params.parts_only, params.show_bed, params.transparent_background); if (!thumbnails.back().is_valid()) thumbnails.pop_back(); } + return thumbnails; } wxString Plater::priv::get_project_filename(const wxString& extension) const