Cycles: refactor to keep session thread alive for duration of session

Instead of creating and destroying threads when starting and stopping renders,
keep a single thread alive for the duration of the session. This makes it so all
display driver OpenGL resource allocation and destruction can happen in the same
thread.

This was implemented as part of trying to solve another bug, but it did not
help. Still I prefer this behavior, to eliminate potential future issues wit
graphics drivers or with future Cycles display driver implementations.

Differential Revision: https://developer.blender.org/D14086

2 files changed, 102 insertions, 26 deletions

diff --git a/intern/cycles/session/session.cpp b/intern/cycles/session/session.cpp
index d03063a7dda..f6e06f20aba 100644
--- a/intern/cycles/session/session.cpp
+++ b/intern/cycles/session/session.cpp
@@ -49,12 +49,9 @@ Session::Session(const SessionParams &params_, const SceneParams &scene_params)
 {
   TaskScheduler::init(params.threads);
 
-  session_thread_ = nullptr;
-
   delayed_reset_.do_reset = false;
 
   pause_ = false;
-  cancel_ = false;
   new_work_added_ = false;
 
   device = Device::create(params.device, stats, profiler);
@@ -73,48 +70,79 @@ Session::Session(const SessionParams &params_, const SceneParams &scene_params)
     }
     full_buffer_written_cb(filename);
   };
+
+  /* Create session thread. */
+  session_thread_ = new thread(function_bind(&Session::thread_run, this));
 }
 
 Session::~Session()
 {
+  /* Cancel any ongoing render operation. */
   cancel();
 
-  /* Make sure path tracer is destroyed before the device. This is needed because destruction might
-   * need to access device for device memory free. */
-  /* TODO(sergey): Convert device to be unique_ptr, and rely on C++ to destruct objects in the
+  /* Signal session thread to end. */
+  {
+    thread_scoped_lock session_thread_lock(session_thread_mutex_);
+    session_thread_state_ = SESSION_THREAD_END;
+  }
+  session_thread_cond_.notify_all();
+
+  /* Destroy session thread. */
+  session_thread_->join();
+  delete session_thread_;
+
+  /* Destroy path tracer, before the device. This is needed because destruction might need to
+   * access device for device memory free.
+   * TODO(sergey): Convert device to be unique_ptr, and rely on C++ to destruct objects in the
    * pre-defined order. */
   path_trace_.reset();
 
+  /* Destroy scene and device. */
   delete scene;
   delete device;
 
+  /* Stop task scheduler. */
   TaskScheduler::exit();
 }
 
 void Session::start()
 {
-  if (!session_thread_) {
-    session_thread_ = new thread(function_bind(&Session::run, this));
+  {
+    /* Signal session thread to start rendering. */
+    thread_scoped_lock session_thread_lock(session_thread_mutex_);
+    assert(session_thread_state_ == SESSION_THREAD_WAIT);
+    session_thread_state_ = SESSION_THREAD_RENDER;
   }
+
+  session_thread_cond_.notify_all();
 }
 
 void Session::cancel(bool quick)
 {
-  if (quick && path_trace_) {
-    path_trace_->cancel();
+  /* Check if session thread is rendering. */
+  bool rendering;
+  {
+    thread_scoped_lock session_thread_lock(session_thread_mutex_);
+    rendering = (session_thread_state_ == SESSION_THREAD_RENDER);
   }
 
-  if (session_thread_) {
-    /* wait for session thread to end */
+  if (rendering) {
+    /* Cancel path trace operations. */
+    if (quick && path_trace_) {
+      path_trace_->cancel();
+    }
+
+    /* Cancel other operations. */
     progress.set_cancel("Exiting");
 
+    /* Signal unpause in case the render was paused. */
     {
       thread_scoped_lock pause_lock(pause_mutex_);
       pause_ = false;
-      cancel_ = true;
     }
     pause_cond_.notify_all();
 
+    /* Wait for render thread to be cancelled or finished. */
     wait();
   }
 }
@@ -192,11 +220,46 @@ void Session::run_main_render_loop()
       break;
     }
   }
+}
+
+void Session::thread_run()
+{
+  while (true) {
+    {
+      thread_scoped_lock session_thread_lock(session_thread_mutex_);
+
+      if (session_thread_state_ == SESSION_THREAD_WAIT) {
+        /* Continue waiting for any signal from the main thread. */
+        session_thread_cond_.wait(session_thread_lock);
+        continue;
+      }
+      else if (session_thread_state_ == SESSION_THREAD_END) {
+        /* End thread immediately. */
+        break;
+      }
+    }
+
+    /* Execute a render. */
+    thread_render();
+
+    /* Go back from rendering to waiting. */
+    {
+      thread_scoped_lock session_thread_lock(session_thread_mutex_);
+      if (session_thread_state_ == SESSION_THREAD_RENDER) {
+        session_thread_state_ = SESSION_THREAD_WAIT;
+      }
+    }
+    session_thread_cond_.notify_all();
+  }
 
+  /* Flush any remaining operations and destroy display driver here. This ensure
+   * graphics API resources are created and destroyed all in the session thread,
+   * which can avoid problems contexts and multiple threads. */
   path_trace_->flush_display();
+  path_trace_->set_display_driver(nullptr);
 }
 
-void Session::run()
+void Session::thread_render()
 {
   if (params.use_profiling && (params.device.type == DEVICE_CPU)) {
     profiler.start();
@@ -338,9 +401,9 @@ bool Session::run_wait_for_work(const RenderWork &render_work)
   const bool no_work = !render_work;
   update_status_time(pause_, no_work);
 
-  /* Only leave the loop when rendering is not paused. But even if the current render is un-paused
-   * but there is nothing to render keep waiting until new work is added. */
-  while (!cancel_) {
+  /* Only leave the loop when rendering is not paused. But even if the current render is
+   * un-paused but there is nothing to render keep waiting until new work is added. */
+  while (!progress.get_cancel()) {
     scoped_timer pause_timer;
 
     if (!pause_ && (render_work || new_work_added_ || delayed_reset_.do_reset)) {
@@ -427,7 +490,8 @@ void Session::do_delayed_reset()
   tile_manager_.update(buffer_params_, scene);
 
   /* Update temp directory on reset.
-   * This potentially allows to finish the existing rendering with a previously configure temporary
+   * This potentially allows to finish the existing rendering with a previously configure
+   * temporary
    * directory in the host software and switch to a new temp directory when new render starts. */
   tile_manager_.set_temp_dir(params.temp_dir);
 
@@ -544,12 +608,14 @@ double Session::get_estimated_remaining_time() const
 
 void Session::wait()
 {
-  if (session_thread_) {
-    session_thread_->join();
-    delete session_thread_;
+  /* Wait until session thread either is waiting or ending. */
+  while (true) {
+    thread_scoped_lock session_thread_lock(session_thread_mutex_);
+    if (session_thread_state_ != SESSION_THREAD_RENDER) {
+      break;
+    }
+    session_thread_cond_.wait(session_thread_lock);
   }
-
-  session_thread_ = nullptr;
 }
 
 bool Session::update_scene(int width, int height)
diff --git a/intern/cycles/session/session.h b/intern/cycles/session/session.h
index adfd1346600..4017964d4aa 100644
--- a/intern/cycles/session/session.h
+++ b/intern/cycles/session/session.h
@@ -172,7 +172,8 @@ class Session {
     BufferParams buffer_params;
   } delayed_reset_;
 
-  void run();
+  void thread_run();
+  void thread_render();
 
   /* Update for the new iteration of the main loop in run implementation (run_cpu and run_gpu).
    *
@@ -205,10 +206,19 @@ class Session {
 
   int2 get_effective_tile_size() const;
 
-  thread *session_thread_;
+  /* Session thread that performs rendering tasks decoupled from the thread
+   * controlling the sessions. The thread is created and destroyed along with
+   * the session. */
+  thread *session_thread_ = nullptr;
+  thread_condition_variable session_thread_cond_;
+  thread_mutex session_thread_mutex_;
+  enum {
+    SESSION_THREAD_WAIT,
+    SESSION_THREAD_RENDER,
+    SESSION_THREAD_END,
+  } session_thread_state_ = SESSION_THREAD_WAIT;
 
   bool pause_ = false;
-  bool cancel_ = false;
   bool new_work_added_ = false;
 
   thread_condition_variable pause_cond_;