From 7ee9467f8da4211b82e9f40dd194f8204271cf78 Mon Sep 17 00:00:00 2001
From: rdb <git@rdb.name>
Date: Sat, 30 Dec 2017 17:44:52 +0100
Subject: [PATCH] shadows: fix shadow buffer creation deadlock in multithreaded
 pipeline

Fixes #162
---
 panda/src/display/displayRegion.cxx           |  70 ++++---
 panda/src/display/graphicsEngine.cxx          | 198 ++++++++++--------
 panda/src/display/graphicsEngine.h            |  11 +-
 panda/src/display/graphicsStateGuardian.cxx   | 150 +++++--------
 panda/src/display/graphicsStateGuardian.h     |   2 +-
 panda/src/display/windowProperties.cxx        |   6 +
 panda/src/display/windowProperties.h          |   1 +
 .../glstuff/glGraphicsStateGuardian_src.cxx   |  30 +++
 .../src/glstuff/glGraphicsStateGuardian_src.h |   2 +
 panda/src/pgraphnodes/lightLensNode.I         |  17 +-
 panda/src/pgraphnodes/lightLensNode.cxx       |  40 +++-
 panda/src/pgraphnodes/lightLensNode.h         |   6 +-
 panda/src/pgraphnodes/pointLight.cxx          |  30 +++
 panda/src/pgraphnodes/pointLight.h            |   2 +
 14 files changed, 335 insertions(+), 230 deletions(-)

diff --git a/panda/src/display/displayRegion.cxx b/panda/src/display/displayRegion.cxx
index 4ffa29285c..99aea51477 100644
--- a/panda/src/display/displayRegion.cxx
+++ b/panda/src/display/displayRegion.cxx
@@ -84,22 +84,30 @@ DisplayRegion::
  */
 void DisplayRegion::
 cleanup() {
-  set_camera(NodePath());
+  CDStageWriter cdata(_cycler, 0);
+  if (cdata->_camera_node != nullptr) {
+    // We need to tell the old camera we're not using it anymore.
+    cdata->_camera_node->remove_display_region(this);
+  }
+  cdata->_camera_node = nullptr;
+  cdata->_camera = NodePath();
 
-  CDCullWriter cdata(_cycler_cull, true);
-  cdata->_cull_result = NULL;
+  CDCullWriter cdata_cull(_cycler_cull, true);
+  cdata_cull->_cull_result = nullptr;
 }
 
 /**
  * Sets the lens index, allows for multiple lenses to be attached to a camera.
  * This is useful for a variety of setups, such as fish eye rendering.  The
  * default is 0.
+ *
+ * Don't call this in a downstream thread unless you don't mind it blowing
+ * away other changes you might have recently made in an upstream thread.
  */
 void DisplayRegion::
 set_lens_index(int index) {
-  int pipeline_stage = Thread::get_current_pipeline_stage();
-  nassertv(pipeline_stage == 0);
-  CDWriter cdata(_cycler);
+  Thread *current_thread = Thread::get_current_thread();
+  CDWriter cdata(_cycler, true, current_thread);
   cdata->_lens_index = index;
 }
 
@@ -107,12 +115,14 @@ set_lens_index(int index) {
  * Changes the portion of the framebuffer this DisplayRegion corresponds to.
  * The parameters range from 0 to 1, where 0,0 is the lower left corner and
  * 1,1 is the upper right; (0, 1, 0, 1) represents the whole screen.
+ *
+ * Don't call this in a downstream thread unless you don't mind it blowing
+ * away other changes you might have recently made in an upstream thread.
  */
 void DisplayRegion::
 set_dimensions(int i, const LVecBase4 &dimensions) {
-  int pipeline_stage = Thread::get_current_pipeline_stage();
-  nassertv(pipeline_stage == 0);
-  CDWriter cdata(_cycler);
+  Thread *current_thread = Thread::get_current_thread();
+  CDWriter cdata(_cycler, true, current_thread);
 
   cdata->_regions[i]._dimensions = dimensions;
 
@@ -145,15 +155,13 @@ is_stereo() const {
  *
  * The camera is actually set via a NodePath, which clarifies which instance
  * of the camera (if there happen to be multiple instances) we should use.
+ *
+ * Don't call this in a downstream thread unless you don't mind it blowing
+ * away other changes you might have recently made in an upstream thread.
  */
 void DisplayRegion::
 set_camera(const NodePath &camera) {
-  int pipeline_stage = Thread::get_current_pipeline_stage();
-
-  // We allow set_camera(NodePath()) to happen in cleanup(), which can be
-  // called from any pipeline stage.
-  nassertv(pipeline_stage == 0 || camera.is_empty());
-  CDStageWriter cdata(_cycler, 0);
+  CDWriter cdata(_cycler, true);
 
   Camera *camera_node = (Camera *)NULL;
   if (!camera.is_empty()) {
@@ -181,16 +189,17 @@ set_camera(const NodePath &camera) {
 /**
  * Sets the active flag associated with the DisplayRegion.  If the
  * DisplayRegion is marked inactive, nothing is rendered.
+ *
+ * Don't call this in a downstream thread unless you don't mind it blowing
+ * away other changes you might have recently made in an upstream thread.
  */
 void DisplayRegion::
 set_active(bool active) {
-  int pipeline_stage = Thread::get_current_pipeline_stage();
-  nassertv(pipeline_stage == 0);
-  CDLockedReader cdata(_cycler);
+  Thread *current_thread = Thread::get_current_thread();
+  CDWriter cdata(_cycler, true, current_thread);
 
   if (active != cdata->_active) {
-    CDWriter cdataw(_cycler, cdata);
-    cdataw->_active = active;
+    cdata->_active = active;
     win_display_regions_changed();
   }
 }
@@ -199,15 +208,17 @@ set_active(bool active) {
  * Sets the sort value associated with the DisplayRegion.  Within a window,
  * DisplayRegions will be rendered in order from the lowest sort value to the
  * highest.
+ *
+ * Don't call this in a downstream thread unless you don't mind it blowing
+ * away other changes you might have recently made in an upstream thread.
  */
 void DisplayRegion::
 set_sort(int sort) {
-  nassertv(Thread::get_current_pipeline_stage() == 0);
-  CDLockedReader cdata(_cycler);
+  Thread *current_thread = Thread::get_current_thread();
+  CDWriter cdata(_cycler, true, current_thread);
 
   if (sort != cdata->_sort) {
-    CDWriter cdataw(_cycler, cdata);
-    cdataw->_sort = sort;
+    cdata->_sort = sort;
     win_display_regions_changed();
   }
 }
@@ -332,12 +343,14 @@ get_cull_traverser() {
  *
  * This is particularly useful when rendering cube maps and/or stereo
  * textures.
+ *
+ * Don't call this in a downstream thread unless you don't mind it blowing
+ * away other changes you might have recently made in an upstream thread.
  */
 void DisplayRegion::
 set_target_tex_page(int page) {
-  int pipeline_stage = Thread::get_current_pipeline_stage();
-  nassertv(pipeline_stage == 0);
-  CDWriter cdata(_cycler);
+  Thread *current_thread = Thread::get_current_thread();
+  CDWriter cdata(_cycler, true, current_thread);
   cdata->_target_tex_page = page;
 }
 
@@ -555,9 +568,6 @@ compute_pixels() {
  */
 void DisplayRegion::
 compute_pixels_all_stages() {
-  int pipeline_stage = Thread::get_current_pipeline_stage();
-  nassertv(pipeline_stage == 0);
-
   if (_window != (GraphicsOutput *)NULL) {
     OPEN_ITERATE_ALL_STAGES(_cycler) {
       CDStageWriter cdata(_cycler, pipeline_stage);
diff --git a/panda/src/display/graphicsEngine.cxx b/panda/src/display/graphicsEngine.cxx
index 9c11fc9bdc..1a38089e01 100644
--- a/panda/src/display/graphicsEngine.cxx
+++ b/panda/src/display/graphicsEngine.cxx
@@ -153,7 +153,6 @@ GraphicsEngine(Pipeline *pipeline) :
 
   _windows_sorted = true;
   _window_sort_index = 0;
-  _needs_open_windows = false;
 
   set_threading_model(GraphicsThreadingModel(threading_model));
   if (!_threading_model.is_default()) {
@@ -326,13 +325,10 @@ make_output(GraphicsPipe *pipe,
 
   // Sanity check everything.
 
-  GraphicsThreadingModel threading_model = get_threading_model();
   nassertr(pipe != (GraphicsPipe *)NULL, NULL);
   if (gsg != (GraphicsStateGuardian *)NULL) {
     nassertr(pipe == gsg->get_pipe(), NULL);
     nassertr(this == gsg->get_engine(), NULL);
-    nassertr(threading_model.get_draw_name() ==
-             gsg->get_threading_model().get_draw_name(), NULL);
   }
 
   // Are we really asking for a callback window?
@@ -346,8 +342,8 @@ make_output(GraphicsPipe *pipe,
     if (this_gsg != (GraphicsStateGuardian *)NULL) {
       CallbackGraphicsWindow *window = new CallbackGraphicsWindow(this, pipe, name, fb_prop, win_prop, flags, this_gsg);
       window->_sort = sort;
-      do_add_window(window, threading_model);
-      do_add_gsg(window->get_gsg(), pipe, threading_model);
+      do_add_window(window);
+      do_add_gsg(window->get_gsg(), pipe);
       display_cat.info() << "Created output of type CallbackGraphicsWindow\n";
       return window;
     }
@@ -386,8 +382,8 @@ make_output(GraphicsPipe *pipe,
       (host->get_fb_properties().subsumes(fb_prop))) {
     ParasiteBuffer *buffer = new ParasiteBuffer(host, name, x_size, y_size, flags);
     buffer->_sort = sort;
-    do_add_window(buffer, threading_model);
-    do_add_gsg(host->get_gsg(), pipe, threading_model);
+    do_add_window(buffer);
+    do_add_gsg(host->get_gsg(), pipe);
     display_cat.info() << "Created output of type ParasiteBuffer\n";
     return buffer;
   }
@@ -398,8 +394,8 @@ make_output(GraphicsPipe *pipe,
   if (force_parasite_buffer && can_use_parasite) {
     ParasiteBuffer *buffer = new ParasiteBuffer(host, name, x_size, y_size, flags);
     buffer->_sort = sort;
-    do_add_window(buffer, threading_model);
-    do_add_gsg(host->get_gsg(), pipe, threading_model);
+    do_add_window(buffer);
+    do_add_gsg(host->get_gsg(), pipe);
     display_cat.info() << "Created output of type ParasiteBuffer\n";
     return buffer;
   }
@@ -412,17 +408,15 @@ make_output(GraphicsPipe *pipe,
       pipe->make_output(name, fb_prop, win_prop, flags, this, gsg, host, retry, precertify);
     if (window != (GraphicsOutput *)NULL) {
       window->_sort = sort;
-      if ((precertify) && (gsg != 0) && (window->get_gsg()==gsg)) {
-        do_add_window(window, threading_model);
-        do_add_gsg(window->get_gsg(), pipe, threading_model);
+      if (precertify && gsg != nullptr && window->get_gsg() == gsg) {
+        do_add_window(window);
         display_cat.info()
           << "Created output of type " << window->get_type() << "\n";
         return window;
       }
-      do_add_window(window, threading_model);
+      do_add_window(window);
       open_windows();
       if (window->is_valid()) {
-        do_add_gsg(window->get_gsg(), pipe, threading_model);
         display_cat.info()
           << "Created output of type " << window->get_type() << "\n";
 
@@ -462,8 +456,8 @@ make_output(GraphicsPipe *pipe,
   if (can_use_parasite) {
     ParasiteBuffer *buffer = new ParasiteBuffer(host, name, x_size, y_size, flags);
     buffer->_sort = sort;
-    do_add_window(buffer, threading_model);
-    do_add_gsg(host->get_gsg(), pipe, threading_model);
+    do_add_window(buffer);
+    do_add_gsg(host->get_gsg(), pipe);
     display_cat.info() << "Created output of type ParasiteBuffer\n";
     return buffer;
   }
@@ -479,30 +473,24 @@ make_output(GraphicsPipe *pipe,
  * shouldn't be called by user code as make_output normally does this under
  * the hood; it may be useful in esoteric cases in which a custom window
  * object is used.
+ *
+ * This can be called during the rendering loop, unlike make_output(); the
+ * window will be opened before the next frame begins rendering.  Because it
+ * doesn't call open_windows(), however, it's not guaranteed that the window
+ * will succeed opening even if it returns true.
  */
 bool GraphicsEngine::
 add_window(GraphicsOutput *window, int sort) {
-  nassertr(window != NULL, false);
-
-  GraphicsThreadingModel threading_model = get_threading_model();
+  nassertr(window != nullptr, false);
   nassertr(this == window->get_engine(), false);
 
   window->_sort = sort;
-  do_add_window(window, threading_model);
+  do_add_window(window);
 
-  open_windows();
-  if (window->is_valid()) {
-    do_add_gsg(window->get_gsg(), window->get_pipe(), threading_model);
+  display_cat.info()
+    << "Added output of type " << window->get_type() << "\n";
 
-    display_cat.info()
-      << "Added output of type " << window->get_type() << "\n";
-
-    return true;
-
-  } else {
-    remove_window(window);
-    return false;
-  }
+  return true;
 }
 
 /**
@@ -537,6 +525,13 @@ remove_window(GraphicsOutput *window) {
     }
     count = _windows.erase(ptwin);
   }
+
+  // Also check whether it is in _new_windows.
+  {
+    MutexHolder new_windows_holder(_new_windows_lock, current_thread);
+    _new_windows.erase(std::remove(_new_windows.begin(), _new_windows.end(), ptwin));
+  }
+
   if (count == 0) {
     // Never heard of this window.  Do nothing.
     return false;
@@ -601,6 +596,11 @@ remove_all_windows() {
     }
   }
 
+  {
+    MutexHolder new_windows_holder(_new_windows_lock, current_thread);
+    _new_windows.clear();
+  }
+
   _app.do_close(this, current_thread);
   _app.do_pending(this, current_thread);
   terminate_threads(current_thread);
@@ -694,14 +694,12 @@ render_frame() {
   }
 #endif
 
-  if (_needs_open_windows) {
-    // Make sure our buffers and windows are fully realized before we render a
-    // frame.  We do this particularly to realize our offscreen buffers, so
-    // that we don't render a frame before the offscreen buffers are ready
-    // (which might result in a frame going by without some textures having
-    // been rendered).
-    open_windows();
-  }
+  // Make sure our buffers and windows are fully realized before we render a
+  // frame.  We do this particularly to realize our offscreen buffers, so
+  // that we don't render a frame before the offscreen buffers are ready
+  // (which might result in a frame going by without some textures having
+  // been rendered).
+  open_windows();
 
   ClockObject *global_clock = ClockObject::get_global_clock();
 
@@ -945,10 +943,58 @@ open_windows() {
 
   ReMutexHolder holder(_lock, current_thread);
 
-  if (!_windows_sorted) {
-    do_resort_windows();
+  pvector<PT(GraphicsOutput)> new_windows;
+  {
+    MutexHolder new_windows_holder(_new_windows_lock, current_thread);
+    if (_new_windows.empty()) {
+      return;
+    }
+
+    for (auto it = _new_windows.begin(); it != _new_windows.end(); ++it) {
+      GraphicsOutput *window = *it;
+
+      WindowRenderer *cull =
+        get_window_renderer(_threading_model.get_cull_name(),
+                            _threading_model.get_cull_stage());
+      WindowRenderer *draw =
+        get_window_renderer(_threading_model.get_draw_name(),
+                            _threading_model.get_draw_stage());
+
+      if (_threading_model.get_cull_sorting()) {
+        cull->add_window(cull->_cull, window);
+        draw->add_window(draw->_draw, window);
+      } else {
+        cull->add_window(cull->_cdraw, window);
+      }
+
+      // Ask the pipe which thread it prefers to run its windowing commands in
+      // (the "window thread").  This is the thread that handles the commands
+      // to open, resize, etc.  the window.  X requires this to be done in the
+      // app thread (along with all the other windows, since X is strictly
+      // single-threaded), but Windows requires this to be done in draw
+      // (because once an OpenGL context has been bound in a given thread, it
+      // cannot subsequently be bound in any other thread, and we have to bind
+      // a context in open_window()).
+
+      switch (window->get_pipe()->get_preferred_window_thread()) {
+      case GraphicsPipe::PWT_app:
+        _app.add_window(_app._window, window);
+        break;
+
+      case GraphicsPipe::PWT_draw:
+        draw->add_window(draw->_window, window);
+        break;
+      }
+
+      _windows.push_back(window);
+    }
+
+    // Steal the list, since remove_window() may remove from _new_windows.
+    new_windows.swap(_new_windows);
   }
 
+  do_resort_windows();
+
   // We do it twice, to allow both cull and draw to process the window.
   for (int i = 0; i < 2; ++i) {
     _app.do_windows(this, current_thread);
@@ -970,7 +1016,15 @@ open_windows() {
     }
   }
 
-  _needs_open_windows = false;
+  // Now go through the list again to check whether they opened successfully.
+  for (auto it = new_windows.begin(); it != new_windows.end(); ++it) {
+    GraphicsOutput *window = *it;
+    if (window->is_valid()) {
+      do_add_gsg(window->get_gsg(), window->get_pipe());
+    } else {
+      remove_window(window);
+    }
+  }
 }
 
 /**
@@ -1927,10 +1981,10 @@ do_draw(GraphicsOutput *win, GraphicsStateGuardian *gsg, DisplayRegion *dr, Thre
  * list of windows, and to request that the window be opened.
  */
 void GraphicsEngine::
-do_add_window(GraphicsOutput *window,
-              const GraphicsThreadingModel &threading_model) {
-  nassertv(window != NULL);
-  ReMutexHolder holder(_lock);
+do_add_window(GraphicsOutput *window) {
+  nassertv(window != nullptr);
+
+  MutexHolder holder(_new_windows_lock);
   nassertv(window->get_engine() == this);
 
   // We have a special counter that is unique per window that allows us to
@@ -1938,50 +1992,13 @@ do_add_window(GraphicsOutput *window,
   window->_internal_sort_index = _window_sort_index;
   ++_window_sort_index;
 
-  _windows_sorted = false;
-  _windows.push_back(window);
-
-  WindowRenderer *cull =
-    get_window_renderer(threading_model.get_cull_name(),
-                        threading_model.get_cull_stage());
-  WindowRenderer *draw =
-    get_window_renderer(threading_model.get_draw_name(),
-                        threading_model.get_draw_stage());
-
-  if (threading_model.get_cull_sorting()) {
-    cull->add_window(cull->_cull, window);
-    draw->add_window(draw->_draw, window);
-  } else {
-    cull->add_window(cull->_cdraw, window);
-  }
-
-/*
- * Ask the pipe which thread it prefers to run its windowing commands in (the
- * "window thread").  This is the thread that handles the commands to open,
- * resize, etc.  the window.  X requires this to be done in the app thread
- * (along with all the other windows, since X is strictly single-threaded),
- * but Windows requires this to be done in draw (because once an OpenGL
- * context has been bound in a given thread, it cannot subsequently be bound
- * in any other thread, and we have to bind a context in open_window()).
- */
-
-  switch (window->get_pipe()->get_preferred_window_thread()) {
-  case GraphicsPipe::PWT_app:
-    _app.add_window(_app._window, window);
-    break;
-
-  case GraphicsPipe::PWT_draw:
-    draw->add_window(draw->_window, window);
-    break;
-  }
-
   if (display_cat.is_debug()) {
     display_cat.debug()
       << "Created " << window->get_type() << " " << (void *)window << "\n";
   }
 
   window->request_open();
-  _needs_open_windows = true;
+  _new_windows.push_back(window);
 }
 
 /**
@@ -1990,13 +2007,12 @@ do_add_window(GraphicsOutput *window,
  * variables based on the gsg's capabilities.
  */
 void GraphicsEngine::
-do_add_gsg(GraphicsStateGuardian *gsg, GraphicsPipe *pipe,
-           const GraphicsThreadingModel &threading_model) {
+do_add_gsg(GraphicsStateGuardian *gsg, GraphicsPipe *pipe) {
   nassertv(gsg != NULL);
 
   ReMutexHolder holder(_lock);
   nassertv(gsg->get_pipe() == pipe && gsg->get_engine() == this);
-  gsg->_threading_model = threading_model;
+  gsg->_threading_model = _threading_model;
   if (!_default_loader.is_null()) {
     gsg->set_loader(_default_loader);
   }
@@ -2004,8 +2020,8 @@ do_add_gsg(GraphicsStateGuardian *gsg, GraphicsPipe *pipe,
   auto_adjust_capabilities(gsg);
 
   WindowRenderer *draw =
-    get_window_renderer(threading_model.get_draw_name(),
-                        threading_model.get_draw_stage());
+    get_window_renderer(_threading_model.get_draw_name(),
+                        _threading_model.get_draw_stage());
 
   draw->add_gsg(gsg);
 }
diff --git a/panda/src/display/graphicsEngine.h b/panda/src/display/graphicsEngine.h
index d18d86f690..853487d05a 100644
--- a/panda/src/display/graphicsEngine.h
+++ b/panda/src/display/graphicsEngine.h
@@ -166,10 +166,8 @@ private:
   void do_draw(GraphicsOutput *win, GraphicsStateGuardian *gsg,
                DisplayRegion *dr, Thread *current_thread);
 
-  void do_add_window(GraphicsOutput *window,
-                     const GraphicsThreadingModel &threading_model);
-  void do_add_gsg(GraphicsStateGuardian *gsg, GraphicsPipe *pipe,
-                  const GraphicsThreadingModel &threading_model);
+  void do_add_window(GraphicsOutput *window);
+  void do_add_gsg(GraphicsStateGuardian *gsg, GraphicsPipe *pipe);
   void do_remove_window(GraphicsOutput *window, Thread *current_thread);
   void do_resort_windows();
   void terminate_threads(Thread *current_thread);
@@ -308,8 +306,11 @@ private:
   Pipeline *_pipeline;
   Windows _windows;
   bool _windows_sorted;
+
+  // This lock protects the next two fields.
+  Mutex _new_windows_lock;
   unsigned int _window_sort_index;
-  bool _needs_open_windows;
+  pvector<PT(GraphicsOutput)> _new_windows;
 
   WindowRenderer _app;
   typedef pmap<string, PT(RenderThread) > Threads;
diff --git a/panda/src/display/graphicsStateGuardian.cxx b/panda/src/display/graphicsStateGuardian.cxx
index 2d07f2c971..d590aa8d37 100644
--- a/panda/src/display/graphicsStateGuardian.cxx
+++ b/panda/src/display/graphicsStateGuardian.cxx
@@ -3231,9 +3231,10 @@ async_reload_texture(TextureContext *tc) {
 PT(Texture) GraphicsStateGuardian::
 get_shadow_map(const NodePath &light_np, GraphicsOutputBase *host) {
   PandaNode *node = light_np.node();
+  bool is_point = node->is_of_type(PointLight::get_class_type());
   nassertr(node->is_of_type(DirectionalLight::get_class_type()) ||
-           node->is_of_type(PointLight::get_class_type()) ||
-           node->is_of_type(Spotlight::get_class_type()), NULL);
+           node->is_of_type(Spotlight::get_class_type()) ||
+           is_point, nullptr);
 
   LightLensNode *light = (LightLensNode *)node;
   if (light == nullptr || !light->_shadow_caster) {
@@ -3246,20 +3247,49 @@ get_shadow_map(const NodePath &light_np, GraphicsOutputBase *host) {
     }
   }
 
+  // The light's shadow map should have been created by set_shadow_caster().
+  nassertr(light->_shadow_map != nullptr, nullptr);
+
   // See if we already have a buffer.  If not, create one.
-  if (light->_sbuffers.count(this) == 0) {
-    if (host == (GraphicsOutputBase *)NULL) {
-      host = _current_display_region->get_window();
-    }
-    nassertr(host != NULL, NULL);
-
-    // Nope, the light doesn't have a buffer for our GSG. Make one.
-    return make_shadow_buffer(light_np, host);
-
-  } else {
+  if (light->_sbuffers.count(this) != 0) {
     // There's already a buffer - use that.
-    return light->_sbuffers[this]->get_texture();
+    return light->_shadow_map;
   }
+
+  if (display_cat.is_debug()) {
+    display_cat.debug()
+      << "Constructing shadow buffer for light '" << light->get_name()
+      << "', size=" << light->_sb_size[0] << "x" << light->_sb_size[1]
+      << ", sort=" << light->_sb_sort << "\n";
+  }
+
+  if (host == nullptr) {
+    nassertr(_current_display_region != nullptr, nullptr);
+    host = _current_display_region->get_window();
+  }
+  nassertr(host != nullptr, nullptr);
+
+  // Nope, the light doesn't have a buffer for our GSG. Make one.
+  GraphicsOutput *sbuffer = make_shadow_buffer(light, light->_shadow_map,
+                                               DCAST(GraphicsOutput, host));
+
+  // Assign display region(s) to the buffer and camera
+  if (is_point) {
+    for (int i = 0; i < 6; ++i) {
+      PT(DisplayRegion) dr = sbuffer->make_mono_display_region(0, 1, 0, 1);
+      dr->set_lens_index(i);
+      dr->set_target_tex_page(i);
+      dr->set_camera(light_np);
+      dr->set_clear_depth_active(true);
+    }
+  } else {
+    PT(DisplayRegion) dr = sbuffer->make_mono_display_region(0, 1, 0, 1);
+    dr->set_camera(light_np);
+    dr->set_clear_depth_active(true);
+  }
+
+  light->_sbuffers[this] = sbuffer;
+  return light->_shadow_map;
 }
 
 /**
@@ -3299,101 +3329,33 @@ get_dummy_shadow_map(Texture::TextureType texture_type) const {
 }
 
 /**
- * Creates a depth buffer for shadow mapping.  This is a convenience function
- * for the ShaderGenerator; putting this directly in the ShaderGenerator would
- * cause circular dependency issues.  Returns the depth texture.
+ * Creates a depth buffer for shadow mapping.  A derived GSG can override this
+ * if it knows that a particular buffer type works best for shadow rendering.
  */
-PT(Texture) GraphicsStateGuardian::
-make_shadow_buffer(const NodePath &light_np, GraphicsOutputBase *host) {
-  // Make sure everything is valid.
-  PandaNode *node = light_np.node();
-  nassertr(node->is_of_type(DirectionalLight::get_class_type()) ||
-           node->is_of_type(PointLight::get_class_type()) ||
-           node->is_of_type(Spotlight::get_class_type()), NULL);
-
-  LightLensNode *light = (LightLensNode *)node;
-  if (light == NULL || !light->_shadow_caster) {
-    return NULL;
-  }
-
+GraphicsOutput *GraphicsStateGuardian::
+make_shadow_buffer(LightLensNode *light, Texture *tex, GraphicsOutput *host) {
   bool is_point = light->is_of_type(PointLight::get_class_type());
 
-  nassertr(light->_sbuffers.count(this) == 0, NULL);
-
-  if (display_cat.is_debug()) {
-    display_cat.debug()
-      << "Constructing shadow buffer for light '" << light->get_name()
-      << "', size=" << light->_sb_size[0] << "x" << light->_sb_size[1]
-      << ", sort=" << light->_sb_sort << "\n";
-  }
-
   // Determine the properties for creating the depth buffer.
   FrameBufferProperties fbp;
   fbp.set_depth_bits(shadow_depth_bits);
 
-  WindowProperties props = WindowProperties::size(light->_sb_size[0], light->_sb_size[1]);
+  WindowProperties props = WindowProperties::size(light->_sb_size);
   int flags = GraphicsPipe::BF_refuse_window;
   if (is_point) {
     flags |= GraphicsPipe::BF_size_square;
   }
 
-  // Create the buffer
-  PT(GraphicsOutput) sbuffer = get_engine()->make_output(get_pipe(), light->get_name(),
-      light->_sb_sort, fbp, props, flags, this, DCAST(GraphicsOutput, host));
-  nassertr(sbuffer != NULL, NULL);
+  // Create the buffer.  This is a bit tricky because make_output() can only
+  // be called from the app thread, but it won't cause issues as long as the
+  // pipe can precertify the buffer, which it can in most cases.
+  GraphicsOutput *sbuffer = get_engine()->make_output(get_pipe(),
+    light->get_name(), light->_sb_sort, fbp, props, flags, this, host);
 
-  // Create a texture and fill it in with some data to workaround an OpenGL
-  // error
-  PT(Texture) tex = new Texture(light->get_name());
-  if (is_point) {
-    if (light->_sb_size[0] != light->_sb_size[1]) {
-      display_cat.error()
-        << "PointLight shadow buffers must have an equal width and height!\n";
-    }
-    tex->setup_cube_map(light->_sb_size[0], Texture::T_unsigned_byte, Texture::F_depth_component);
-  } else {
-    tex->setup_2d_texture(light->_sb_size[0], light->_sb_size[1], Texture::T_unsigned_byte, Texture::F_depth_component);
+  if (sbuffer != nullptr) {
+    sbuffer->add_render_texture(tex, GraphicsOutput::RTM_bind_or_copy, GraphicsOutput::RTP_depth);
   }
-  tex->make_ram_image();
-  sbuffer->add_render_texture(tex, GraphicsOutput::RTM_bind_or_copy, GraphicsOutput::RTP_depth);
-
-  // Set the wrap mode
-  if (is_point) {
-    tex->set_wrap_u(SamplerState::WM_clamp);
-    tex->set_wrap_v(SamplerState::WM_clamp);
-  } else {
-    tex->set_wrap_u(SamplerState::WM_border_color);
-    tex->set_wrap_v(SamplerState::WM_border_color);
-    tex->set_border_color(LVecBase4(1, 1, 1, 1));
-  }
-
-  // Note: cube map shadow filtering doesn't seem to work in Cg.
-  if (get_supports_shadow_filter() && !is_point) {
-    // If we have the ARB_shadow extension, enable shadow filtering.
-    tex->set_minfilter(SamplerState::FT_shadow);
-    tex->set_magfilter(SamplerState::FT_shadow);
-  } else {
-    tex->set_minfilter(SamplerState::FT_linear);
-    tex->set_magfilter(SamplerState::FT_linear);
-  }
-
-  // Assign display region(s) to the buffer and camera
-  if (is_point) {
-    for (int i = 0; i < 6; ++i) {
-      PT(DisplayRegion) dr = sbuffer->make_mono_display_region(0, 1, 0, 1);
-      dr->set_lens_index(i);
-      dr->set_target_tex_page(i);
-      dr->set_camera(light_np);
-      dr->set_clear_depth_active(true);
-    }
-  } else {
-    PT(DisplayRegion) dr = sbuffer->make_mono_display_region(0, 1, 0, 1);
-    dr->set_camera(light_np);
-    dr->set_clear_depth_active(true);
-  }
-  light->_sbuffers[this] = sbuffer;
-
-  return tex;
+  return sbuffer;
 }
 
 /**
diff --git a/panda/src/display/graphicsStateGuardian.h b/panda/src/display/graphicsStateGuardian.h
index 8a0ca541d0..9b7855dc31 100644
--- a/panda/src/display/graphicsStateGuardian.h
+++ b/panda/src/display/graphicsStateGuardian.h
@@ -424,7 +424,7 @@ public:
 
   PT(Texture) get_shadow_map(const NodePath &light_np, GraphicsOutputBase *host=NULL);
   PT(Texture) get_dummy_shadow_map(Texture::TextureType texture_type) const;
-  PT(Texture) make_shadow_buffer(const NodePath &light_np, GraphicsOutputBase *host);
+  virtual GraphicsOutput *make_shadow_buffer(LightLensNode *light, Texture *tex, GraphicsOutput *host);
 
   virtual void ensure_generated_shader(const RenderState *state);
 
diff --git a/panda/src/display/windowProperties.cxx b/panda/src/display/windowProperties.cxx
index c0af101b41..298bed4b45 100644
--- a/panda/src/display/windowProperties.cxx
+++ b/panda/src/display/windowProperties.cxx
@@ -133,6 +133,12 @@ clear_default() {
  * size is the only property that matters to buffers.
  */
 WindowProperties WindowProperties::
+size(const LVecBase2i &size) {
+  WindowProperties props;
+  props.set_size(size);
+  return props;
+}
+WindowProperties WindowProperties::
 size(int x_size, int y_size) {
   WindowProperties props;
   props.set_size(x_size, y_size);
diff --git a/panda/src/display/windowProperties.h b/panda/src/display/windowProperties.h
index bd5e964f52..16a65c0096 100644
--- a/panda/src/display/windowProperties.h
+++ b/panda/src/display/windowProperties.h
@@ -52,6 +52,7 @@ PUBLISHED:
   MAKE_PROPERTY(config_properties, get_config_properties);
   MAKE_PROPERTY(default, get_default, set_default);
 
+  static WindowProperties size(const LVecBase2i &size);
   static WindowProperties size(int x_size, int y_size);
 
   bool operator == (const WindowProperties &other) const;
diff --git a/panda/src/glstuff/glGraphicsStateGuardian_src.cxx b/panda/src/glstuff/glGraphicsStateGuardian_src.cxx
index fab18bcb62..ec9d1d603f 100644
--- a/panda/src/glstuff/glGraphicsStateGuardian_src.cxx
+++ b/panda/src/glstuff/glGraphicsStateGuardian_src.cxx
@@ -7561,6 +7561,36 @@ bind_light(Spotlight *light_obj, const NodePath &light, int light_id) {
 }
 #endif  // SUPPORT_FIXED_FUNCTION
 
+/**
+ * Creates a depth buffer for shadow mapping.  A derived GSG can override this
+ * if it knows that a particular buffer type works best for shadow rendering.
+ */
+GraphicsOutput *CLP(GraphicsStateGuardian)::
+make_shadow_buffer(LightLensNode *light, Texture *tex, GraphicsOutput *host) {
+  // We override this to circumvent the fact that GraphicsEngine::make_output
+  // can only be called from the app thread.
+  if (!_supports_framebuffer_object) {
+    return GraphicsStateGuardian::make_shadow_buffer(light, tex, host);
+  }
+
+  bool is_point = light->is_of_type(PointLight::get_class_type());
+
+  // Determine the properties for creating the depth buffer.
+  FrameBufferProperties fbp;
+  fbp.set_depth_bits(shadow_depth_bits);
+
+  WindowProperties props = WindowProperties::size(light->get_shadow_buffer_size());
+  int flags = GraphicsPipe::BF_refuse_window;
+  if (is_point) {
+    flags |= GraphicsPipe::BF_size_square;
+  }
+
+  CLP(GraphicsBuffer) *sbuffer = new GLGraphicsBuffer(get_engine(), get_pipe(), light->get_name(), fbp, props, flags, this, host);
+  sbuffer->add_render_texture(tex, GraphicsOutput::RTM_bind_or_copy, GraphicsOutput::RTP_depth);
+  get_engine()->add_window(sbuffer, light->get_shadow_buffer_sort());
+  return sbuffer;
+}
+
 #ifdef SUPPORT_IMMEDIATE_MODE
 /**
  * Uses the ImmediateModeSender to draw a series of primitives of the
diff --git a/panda/src/glstuff/glGraphicsStateGuardian_src.h b/panda/src/glstuff/glGraphicsStateGuardian_src.h
index e4da7cade1..2b357ece5b 100644
--- a/panda/src/glstuff/glGraphicsStateGuardian_src.h
+++ b/panda/src/glstuff/glGraphicsStateGuardian_src.h
@@ -381,6 +381,8 @@ public:
                           int light_id);
 #endif
 
+  virtual GraphicsOutput *make_shadow_buffer(LightLensNode *light, Texture *tex, GraphicsOutput *host);
+
   LVecBase4 get_light_color(Light *light) const;
 
 #ifdef SUPPORT_IMMEDIATE_MODE
diff --git a/panda/src/pgraphnodes/lightLensNode.I b/panda/src/pgraphnodes/lightLensNode.I
index eb2b192f97..1df14eb971 100644
--- a/panda/src/pgraphnodes/lightLensNode.I
+++ b/panda/src/pgraphnodes/lightLensNode.I
@@ -41,6 +41,9 @@ set_shadow_caster(bool caster) {
   }
   _shadow_caster = caster;
   set_active(caster);
+  if (caster) {
+    setup_shadow_map();
+  }
 }
 
 /**
@@ -65,6 +68,17 @@ set_shadow_caster(bool caster, int buffer_xsize, int buffer_ysize, int buffer_so
     _sb_sort = buffer_sort;
   }
   set_active(caster);
+  if (caster) {
+    setup_shadow_map();
+  }
+}
+
+/**
+ * Returns the sort of the shadow buffer to be created for this light source.
+ */
+INLINE int LightLensNode::
+get_shadow_buffer_sort() const {
+  return _sb_sort;
 }
 
 /**
@@ -82,8 +96,9 @@ INLINE void LightLensNode::
 set_shadow_buffer_size(const LVecBase2i &size) {
   if (size != _sb_size) {
     clear_shadow_buffers();
+    _sb_size = size;
+    setup_shadow_map();
   }
-  _sb_size = size;
 }
 
 /**
diff --git a/panda/src/pgraphnodes/lightLensNode.cxx b/panda/src/pgraphnodes/lightLensNode.cxx
index f68b7084ec..b79b5e8dc0 100644
--- a/panda/src/pgraphnodes/lightLensNode.cxx
+++ b/panda/src/pgraphnodes/lightLensNode.cxx
@@ -67,6 +67,9 @@ LightLensNode(const LightLensNode &copy) :
   _has_specular_color(copy._has_specular_color),
   _attrib_count(0)
 {
+  if (_shadow_caster) {
+    setup_shadow_map();
+  }
 }
 
 /**
@@ -75,20 +78,43 @@ LightLensNode(const LightLensNode &copy) :
  */
 void LightLensNode::
 clear_shadow_buffers() {
+  if (_shadow_map) {
+    // Clear it to all ones, so that any shaders that might still be using
+    // it will see the shadows being disabled.
+    _shadow_map->clear_image();
+  }
+
   ShadowBuffers::iterator it;
   for(it = _sbuffers.begin(); it != _sbuffers.end(); ++it) {
-    PT(Texture) tex = (*it).second->get_texture();
-    if (tex) {
-      // Clear it to all ones, so that any shaders that might still be using
-      // it will see the shadows being disabled.
-      tex->set_clear_color(LColor(1));
-      tex->clear_image();
-    }
     (*it).first->remove_window((*it).second);
   }
   _sbuffers.clear();
 }
 
+/**
+ * Creates the shadow map texture.  Can be overridden.
+ */
+void LightLensNode::
+setup_shadow_map() {
+  if (_shadow_map != nullptr &&
+      _shadow_map->get_x_size() == _sb_size[0] &&
+      _shadow_map->get_y_size() == _sb_size[1]) {
+    // Nothing to do.
+    return;
+  }
+
+  if (_shadow_map == nullptr) {
+    _shadow_map = new Texture(get_name());
+  }
+
+  _shadow_map->setup_2d_texture(_sb_size[0], _sb_size[1], Texture::T_unsigned_byte, Texture::F_depth_component);
+  _shadow_map->set_clear_color(LColor(1));
+  _shadow_map->set_wrap_u(SamplerState::WM_border_color);
+  _shadow_map->set_wrap_v(SamplerState::WM_border_color);
+  _shadow_map->set_border_color(LColor(1));
+  _shadow_map->set_minfilter(SamplerState::FT_shadow);
+  _shadow_map->set_magfilter(SamplerState::FT_shadow);
+}
 
 /**
  * This is called when the light is added to a LightAttrib.
diff --git a/panda/src/pgraphnodes/lightLensNode.h b/panda/src/pgraphnodes/lightLensNode.h
index 2102acc541..03c1342927 100644
--- a/panda/src/pgraphnodes/lightLensNode.h
+++ b/panda/src/pgraphnodes/lightLensNode.h
@@ -41,6 +41,8 @@ PUBLISHED:
   INLINE void set_shadow_caster(bool caster);
   INLINE void set_shadow_caster(bool caster, int buffer_xsize, int buffer_ysize, int sort = -10);
 
+  INLINE int get_shadow_buffer_sort() const;
+
   INLINE LVecBase2i get_shadow_buffer_size() const;
   INLINE void set_shadow_buffer_size(const LVecBase2i &size);
 
@@ -53,12 +55,15 @@ PUBLISHED:
 protected:
   LightLensNode(const LightLensNode &copy);
   void clear_shadow_buffers();
+  virtual void setup_shadow_map();
 
   LVecBase2i _sb_size;
   bool _shadow_caster;
   bool _has_specular_color;
   int _sb_sort;
 
+  PT(Texture) _shadow_map;
+
   // This is really a map of GSG -> GraphicsOutput.
   typedef pmap<PT(GraphicsStateGuardianBase), PT(GraphicsOutputBase) > ShadowBuffers;
   ShadowBuffers _sbuffers;
@@ -106,7 +111,6 @@ private:
   static TypeHandle _type_handle;
 
   friend class GraphicsStateGuardian;
-  friend class ShaderGenerator;
 };
 
 INLINE ostream &operator << (ostream &out, const LightLensNode &light) {
diff --git a/panda/src/pgraphnodes/pointLight.cxx b/panda/src/pgraphnodes/pointLight.cxx
index caa8d86261..02cc33fda9 100644
--- a/panda/src/pgraphnodes/pointLight.cxx
+++ b/panda/src/pgraphnodes/pointLight.cxx
@@ -17,6 +17,7 @@
 #include "bamReader.h"
 #include "datagram.h"
 #include "datagramIterator.h"
+#include "config_pgraphnodes.h"
 
 TypeHandle PointLight::_type_handle;
 
@@ -184,6 +185,35 @@ bind(GraphicsStateGuardianBase *gsg, const NodePath &light, int light_id) {
   gsg->bind_light(this, light, light_id);
 }
 
+/**
+ * Creates the shadow map texture.  Can be overridden.
+ */
+void PointLight::
+setup_shadow_map() {
+  if (_shadow_map != nullptr && _shadow_map->get_x_size() == _sb_size[0]) {
+    // Nothing to do.
+    return;
+  }
+
+  if (_sb_size[0] != _sb_size[1]) {
+    pgraphnodes_cat.error()
+      << "PointLight shadow buffers must have an equal width and height!\n";
+  }
+
+  if (_shadow_map == nullptr) {
+    _shadow_map = new Texture(get_name());
+  }
+
+  _shadow_map->setup_cube_map(_sb_size[0], Texture::T_unsigned_byte, Texture::F_depth_component);
+  _shadow_map->set_clear_color(LColor(1));
+  _shadow_map->set_wrap_u(SamplerState::WM_clamp);
+  _shadow_map->set_wrap_v(SamplerState::WM_clamp);
+
+  // Note: cube map shadow filtering doesn't seem to work in Cg.
+  _shadow_map->set_minfilter(SamplerState::FT_linear);
+  _shadow_map->set_magfilter(SamplerState::FT_linear);
+}
+
 /**
  * Tells the BamReader how to create objects of type PointLight.
  */
diff --git a/panda/src/pgraphnodes/pointLight.h b/panda/src/pgraphnodes/pointLight.h
index cbfcf9b50d..ba3a334d47 100644
--- a/panda/src/pgraphnodes/pointLight.h
+++ b/panda/src/pgraphnodes/pointLight.h
@@ -63,6 +63,8 @@ public:
                     int light_id);
 
 private:
+  virtual void setup_shadow_map();
+
   // This is the data that must be cycled between pipeline stages.
   class EXPCL_PANDA_PGRAPHNODES CData : public CycleData {
   public: