[Bf-blender-cvs] [e018da98b9d] new-object-types: Volumes: fix memory leak in grids depsgraph backup

[Brecht Van Lommel]

Commit: e018da98b9d8ff85ec798f9c1a2d5e7704f4c731
Author: Brecht Van Lommel
Date:   Sun Feb 9 23:03:54 2020 +0100
Branches: new-object-types
https://developer.blender.org/rBe018da98b9d8ff85ec798f9c1a2d5e7704f4c731

Volumes: fix memory leak in grids depsgraph backup

===================================================================

M	source/blender/blenkernel/BKE_volume.h
M	source/blender/blenkernel/intern/volume.cc
M	source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.cc
M	source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.h

===================================================================

diff --git a/source/blender/blenkernel/BKE_volume.h b/source/blender/blenkernel/BKE_volume.h
index ad4db53b31a..a76996afe20 100644
--- a/source/blender/blenkernel/BKE_volume.h
+++ b/source/blender/blenkernel/BKE_volume.h
@@ -37,6 +37,7 @@ struct Main;
 struct Object;
 struct Scene;
 struct Volume;
+struct VolumeGridVector;
 
 /* Module */
 
@@ -67,6 +68,10 @@ void BKE_volume_data_update(struct Depsgraph *depsgraph,
                             struct Scene *scene,
                             struct Object *object);
 
+void BKE_volume_grids_backup_restore(struct Volume *volume,
+                                     struct VolumeGridVector *grids,
+                                     const char *filepath);
+
 /* Draw Cache */
 
 enum {
diff --git a/source/blender/blenkernel/intern/volume.cc b/source/blender/blenkernel/intern/volume.cc
index c820a0b7940..8d9efb01f50 100644
--- a/source/blender/blenkernel/intern/volume.cc
+++ b/source/blender/blenkernel/intern/volume.cc
@@ -79,7 +79,7 @@ static CLG_LogRef LOG = {"bke.volume"};
  * When the number of users drops to zero, the grid data is immediately deleted.
  *
  * TODO: also add a cache for OpenVDB files rather than individual grids,
- * so getting the list of grids is also cached?
+ * so getting the list of grids is also cached.
  * TODO: Further, we could cache openvdb::io::File so that loading a grid
  * does not re-open it every time. But then we have to take care not to run
  * out of file descriptors or prevent other applications from writing to it.
@@ -373,6 +373,11 @@ struct VolumeGridVector : public std::vector<VolumeGrid> {
     memcpy(filepath, other.filepath, sizeof(filepath));
   }
 
+  bool is_loaded() const
+  {
+    return filepath[0] != '\0';
+  }
+
   /* Absolute file path that grids have been loaded from. */
   char filepath[FILE_MAX];
   /* File loading error message. */
@@ -552,7 +557,7 @@ bool BKE_volume_is_loaded(const Volume *volume)
 {
 #ifdef WITH_OPENVDB
   /* Test if there is a file to load, or if already loaded. */
-  return (volume->filepath[0] == '\0' || volume->runtime.grids->filepath[0] != '\0');
+  return (volume->filepath[0] == '\0' || volume->runtime.grids->is_loaded());
 #else
   UNUSED_VARS(volume);
   return true;
@@ -778,15 +783,13 @@ static Volume *volume_evaluate_modifiers(struct Depsgraph *depsgraph,
 
 void BKE_volume_eval_geometry(struct Depsgraph *depsgraph, Volume *volume)
 {
+  /* TODO: can we avoid modifier re-evaluation when frame did not change? */
   int frame = volume_sequence_frame(depsgraph, volume);
-
   if (frame != volume->runtime.frame) {
     BKE_volume_unload(volume);
     volume->runtime.frame = frame;
   }
 
-  /* TODO: can we avoid modifier re-evaluation when frame did not change? */
-
   /* Flush back to original. */
   if (DEG_is_active(depsgraph)) {
     Volume *volume_orig = (Volume *)DEG_get_original_id(&volume->id);
@@ -808,6 +811,33 @@ void BKE_volume_data_update(struct Depsgraph *depsgraph, struct Scene *scene, Ob
   BKE_object_eval_assign_data(object, &volume_eval->id, is_owned);
 }
 
+void BKE_volume_grids_backup_restore(Volume *volume, VolumeGridVector *grids, const char *filepath)
+{
+#ifdef WITH_OPENVDB
+  /* Restore grids after datablock was re-copied from original by depsgraph,
+   * we don't want to load them again if possible. */
+  BLI_assert(volume->id.tag & LIB_TAG_COPIED_ON_WRITE);
+  BLI_assert(volume->runtime.grids != NULL && grids != NULL);
+
+  if (!grids->is_loaded()) {
+    /* No grids loaded in CoW datablock, nothing lost by discarding. */
+    OBJECT_GUARDED_DELETE(grids, VolumeGridVector);
+  }
+  else if (!STREQ(volume->filepath, filepath)) {
+    /* Filepath changed, discard grids from CoW datablock. */
+    OBJECT_GUARDED_DELETE(grids, VolumeGridVector);
+  }
+  else {
+    /* Keep grids from CoW datablock. We might still unload them a little
+     * later in BKE_volume_eval_geometry if the frame changes. */
+    OBJECT_GUARDED_DELETE(volume->runtime.grids, VolumeGridVector);
+    volume->runtime.grids = grids;
+  }
+#else
+  UNUSED_VARS(volume, grids);
+#endif
+}
+
 /* Draw Cache */
 
 void (*BKE_volume_batch_cache_dirty_tag_cb)(Volume *volume, int mode) = NULL;
diff --git a/source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.cc b/source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.cc
index fe1bf9ee644..09e13ec131d 100644
--- a/source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.cc
+++ b/source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.cc
@@ -23,37 +23,38 @@
 
 #include "intern/eval/deg_eval_runtime_backup_volume.h"
 
+#include "BLI_assert.h"
+#include "BLI_string.h"
 #include "BLI_utildefines.h"
 
 #include "DNA_volume_types.h"
+
+#include "BKE_volume.h"
+
 #include <stdio.h>
 
 namespace DEG {
 
-VolumeBackup::VolumeBackup(const Depsgraph * /*depsgraph*/)
-{
-  reset();
-}
-
-void VolumeBackup::reset()
+VolumeBackup::VolumeBackup(const Depsgraph * /*depsgraph*/) : grids(nullptr)
 {
-  grids = nullptr;
 }
 
 void VolumeBackup::init_from_volume(Volume *volume)
 {
-  /* TODO: this is leaking! */
+  STRNCPY(filepath, volume->filepath);
+  BLI_STATIC_ASSERT(sizeof(filepath) == sizeof(volume->filepath),
+                    "VolumeBackup filepath length wrong");
+
   grids = volume->runtime.grids;
   volume->runtime.grids = nullptr;
 }
 
 void VolumeBackup::restore_to_volume(Volume *volume)
 {
-  /* TODO: why does it call restore without init? */
   if (grids) {
-    volume->runtime.grids = grids;
+    BKE_volume_grids_backup_restore(volume, grids, filepath);
+    grids = nullptr;
   }
-  reset();
 }
 
 }  // namespace DEG
diff --git a/source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.h b/source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.h
index c9fd0f33734..cf57c702c8f 100644
--- a/source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.h
+++ b/source/blender/depsgraph/intern/eval/deg_eval_runtime_backup_volume.h
@@ -35,12 +35,11 @@ class VolumeBackup {
  public:
   VolumeBackup(const Depsgraph *depsgraph);
 
-  void reset();
-
   void init_from_volume(Volume *volume);
   void restore_to_volume(Volume *volume);
 
   VolumeGridVector *grids;
+  char filepath[1024]; /* FILE_MAX */
 };
 
 }  // namespace DEG