Refactor: Use more standard storage for PreviewImage runtime data

Using a non-virtual derived struct for polymorphism is error prone,
especially combined with the requirements of DNA. Instead, use a
separately allocated runtime struct as done for many other DNA structs.

In a followup commit, the remaining runtime members of `PreviewImage`
could be moved to the new runtime struct.

Pull Request: https://projects.blender.org/blender/blender/pulls/121509

source/blender/blenkernel/BKE_preview_image.hh

@@ -4,6 +4,7 @@
 
 #pragma once
 
+#include <memory>
 #include <optional>
 
 #include "BLI_sys_types.h"
@@ -16,6 +17,22 @@ struct ID;
 struct ImBuf;
 struct PreviewImage;
 
+enum ThumbSource : int8_t;
+
+namespace blender::bke {
+
+struct PreviewDeferredLoadingData;
+
+struct PreviewImageRuntime {
+  /** Used to store data to defer the loading of the preview. If empty, loading is not deferred. */
+  std::unique_ptr<PreviewDeferredLoadingData> deferred_loading_data;
+  PreviewImageRuntime();
+  PreviewImageRuntime(const PreviewImageRuntime &other);
+  ~PreviewImageRuntime();
+};
+
+}  // namespace blender::bke
+
 void BKE_preview_images_init();
 void BKE_preview_images_free();
 

source/blender/blenkernel/intern/preview_image.cc

@@ -6,6 +6,7 @@
  * \ingroup bke
  */
 
+#include <iostream>
 #include <string>
 
 #include "DNA_ID.h"
@@ -43,58 +44,46 @@
 /* Not mutex-protected! */
 static GHash *gCachedPreviews = nullptr;
 
-class PreviewImageDeferred : public PreviewImage {
- public:
-  const std::string filepath;
-  const ThumbSource source;
+namespace blender::bke {
 
-  /* Behavior is undefined if \a prv is not a deferred preview (#PRV_TAG_DEFFERED not set). */
-  static PreviewImageDeferred &from_base(PreviewImage &prv);
-  static const PreviewImageDeferred &from_base(const PreviewImage &prv);
-
-  PreviewImageDeferred(blender::StringRef filepath, ThumbSource source);
-  PreviewImageDeferred(const PreviewImageDeferred &) = delete;
-  /* Delete through #BKE_previewimg_free()! */
-  ~PreviewImageDeferred() = delete;
-  /* Keep this type non-copyable since ownership of #PreviewImage can be ambiguous (#PreviewImage
-   * allows shallow copies). */
-  PreviewImageDeferred &operator=(const PreviewImageDeferred &) = delete;
+struct PreviewDeferredLoadingData {
+  std::string filepath;
+  ThumbSource source;
 };
 
-PreviewImage::PreviewImage()
+PreviewImageRuntime::PreviewImageRuntime() = default;
+PreviewImageRuntime::PreviewImageRuntime(const PreviewImageRuntime &other)
 {
-  /* Zero initialize */
-  memset(this, 0, sizeof(*this));
-
-  for (int i = 0; i < NUM_ICON_SIZES; i++) {
-    flag[i] |= PRV_CHANGED;
-    changed_timestamp[i] = 0;
+  if (other.deferred_loading_data) {
+    this->deferred_loading_data = std::make_unique<PreviewDeferredLoadingData>(
+        *other.deferred_loading_data);
   }
 }
+PreviewImageRuntime::~PreviewImageRuntime() = default;
 
-PreviewImageDeferred &PreviewImageDeferred::from_base(PreviewImage &prv)
-{
-  return static_cast<PreviewImageDeferred &>(prv);
-}
-const PreviewImageDeferred &PreviewImageDeferred::from_base(const PreviewImage &prv)
-{
-  return static_cast<const PreviewImageDeferred &>(prv);
-}
+}  // namespace blender::bke
 
-PreviewImageDeferred::PreviewImageDeferred(blender::StringRef filepath, ThumbSource source)
-    : PreviewImage(), filepath(filepath), source(source)
+static PreviewImage *previewimg_deferred_create(const char *filepath, ThumbSource source)
 {
-  tag |= PRV_TAG_DEFFERED;
-}
-
-static PreviewImageDeferred *previewimg_deferred_create(const char *filepath, ThumbSource source)
-{
-  return MEM_new<PreviewImageDeferred>(__func__, filepath, source);
+  PreviewImage *prv = BKE_previewimg_create();
+  prv->runtime->deferred_loading_data =
+      std::make_unique<blender::bke::PreviewDeferredLoadingData>();
+  prv->runtime->deferred_loading_data->filepath = filepath;
+  prv->runtime->deferred_loading_data->source = source;
+  return prv;
 }
 
 PreviewImage *BKE_previewimg_create()
 {
-  return MEM_new<PreviewImage>(__func__);
+  PreviewImage *prv = static_cast<PreviewImage *>(MEM_callocN(sizeof(PreviewImage), __func__));
+
+  for (int i = 0; i < NUM_ICON_SIZES; i++) {
+    prv->flag[i] |= PRV_CHANGED;
+    prv->changed_timestamp[i] = 0;
+  }
+
+  prv->runtime = MEM_new<blender::bke::PreviewImageRuntime>(__func__);
+  return prv;
 }
 
 void BKE_previewimg_free(PreviewImage **prv)
@@ -109,10 +98,7 @@ void BKE_previewimg_free(PreviewImage **prv)
       }
     }
 
-    if ((*prv)->tag & PRV_TAG_DEFFERED) {
-      PreviewImageDeferred &this_deferred = PreviewImageDeferred::from_base(**prv);
-      std::destroy_at(&this_deferred.filepath);
-    }
+    MEM_delete((*prv)->runtime);
     MEM_delete(*prv);
     *prv = nullptr;
   }
@@ -142,14 +128,6 @@ void BKE_previewimg_freefunc(void *link)
   BKE_previewimg_free(&prv);
 }
 
-/** Handy override for the deferred type (derives from #PreviewImage). */
-static void BKE_previewimg_free(PreviewImageDeferred **prv)
-{
-  PreviewImage *prv_base = *prv;
-  BKE_previewimg_free(&prv_base);
-  *prv = nullptr;
-}
-
 void BKE_previewimg_runtime_data_clear(PreviewImage *prv)
 {
   prv->tag = 0;
@@ -184,7 +162,8 @@ PreviewImage *BKE_previewimg_copy(const PreviewImage *prv)
     return nullptr;
   }
 
-  PreviewImage *prv_img = (PreviewImage *)MEM_dupallocN(prv);
+  PreviewImage *prv_img = MEM_new<PreviewImage>(__func__, blender::dna::shallow_copy(*prv));
+  prv_img->runtime = MEM_new<blender::bke::PreviewImageRuntime>(__func__, *prv->runtime);
 
   for (int i = 0; i < NUM_ICON_SIZES; i++) {
     if (prv->rect[i]) {
@@ -341,19 +320,22 @@ PreviewImage *BKE_previewimg_cached_thumbnail_read(const char *name,
 {
   BLI_assert(BLI_thread_is_main());
 
-  PreviewImageDeferred *prv = nullptr;
+  PreviewImage *prv = nullptr;
   void **prv_p;
 
   prv_p = BLI_ghash_lookup_p(gCachedPreviews, name);
 
   if (prv_p) {
-    prv = static_cast<PreviewImageDeferred *>(*prv_p);
+    prv = static_cast<PreviewImage *>(*prv_p);
     BLI_assert(prv);
-    BLI_assert(prv->tag & PRV_TAG_DEFFERED);
+    BLI_assert(prv->runtime->deferred_loading_data);
   }
 
   if (prv && force_update) {
-    if ((prv->source == source) && (prv->filepath == filepath)) {
+    if (prv->runtime->deferred_loading_data &&
+        (prv->runtime->deferred_loading_data->source == source) &&
+        (prv->runtime->deferred_loading_data->filepath == filepath))
+    {
       /* If same filepath, no need to re-allocate preview, just clear it up. */
       BKE_previewimg_clear(prv);
     }
@@ -390,7 +372,7 @@ void BKE_previewimg_cached_release(const char *name)
 
 void BKE_previewimg_ensure(PreviewImage *prv, const int size)
 {
-  if ((prv->tag & PRV_TAG_DEFFERED) == 0) {
+  if (!prv->runtime->deferred_loading_data) {
     return;
   }
 
@@ -402,7 +384,8 @@ void BKE_previewimg_ensure(PreviewImage *prv, const int size)
     return;
   }
 
-  PreviewImageDeferred &prv_deferred = PreviewImageDeferred::from_base(*prv);
+  const blender::bke::PreviewDeferredLoadingData &prv_deferred =
+      *prv->runtime->deferred_loading_data;
   int icon_w, icon_h;
 
   ImBuf *thumb = IMB_thumb_manage(prv_deferred.filepath.c_str(), THB_LARGE, prv_deferred.source);
@@ -443,22 +426,20 @@ void BKE_previewimg_ensure(PreviewImage *prv, const int size)
 
 const char *BKE_previewimg_deferred_filepath_get(const PreviewImage *prv)
 {
-  if ((prv->tag & PRV_TAG_DEFFERED) == 0) {
+  if (!prv->runtime->deferred_loading_data) {
     return nullptr;
   }
 
-  const PreviewImageDeferred &prv_deferred = PreviewImageDeferred::from_base(*prv);
-  return prv_deferred.filepath.c_str();
+  return prv->runtime->deferred_loading_data->filepath.c_str();
 }
 
 std::optional<int> BKE_previewimg_deferred_thumb_source_get(const PreviewImage *prv)
 {
-  if ((prv->tag & PRV_TAG_DEFFERED) == 0) {
+  if (!prv->runtime->deferred_loading_data) {
     return std::nullopt;
   }
 
-  const PreviewImageDeferred &prv_deferred = PreviewImageDeferred::from_base(*prv);
-  return prv_deferred.source;
+  return prv->runtime->deferred_loading_data->source;
 }
 
 ImBuf *BKE_previewimg_to_imbuf(PreviewImage *prv, const int size)
@@ -499,7 +480,8 @@ void BKE_previewimg_blend_write(BlendWriter *writer, const PreviewImage *prv)
     return;
   }
 
-  PreviewImage prv_copy = *prv;
+  PreviewImage prv_copy = blender::dna::shallow_copy(*prv);
+  prv_copy.runtime = nullptr;
   BLO_write_struct_at_address(writer, PreviewImage, prv, &prv_copy);
   if (prv_copy.rect[0]) {
     BLO_write_uint32_array(writer, prv_copy.w[0] * prv_copy.h[0], prv_copy.rect[0]);
@@ -515,6 +497,8 @@ void BKE_previewimg_blend_read(BlendDataReader *reader, PreviewImage *prv)
     return;
   }
 
+  prv->runtime = MEM_new<blender::bke::PreviewImageRuntime>(__func__);
+
   for (int i = 0; i < NUM_ICON_SIZES; i++) {
     if (prv->rect[i]) {
       BLO_read_uint32_array(reader, prv->w[i] * prv->h[i], &prv->rect[i]);

source/blender/editors/interface/interface_icons.cc

@@ -1517,7 +1517,7 @@ void ui_icon_ensure_deferred(const bContext *C, const int icon_id, const bool bi
       if (prv) {
         const int size = big ? ICON_SIZE_PREVIEW : ICON_SIZE_ICON;
 
-        if (id || (prv->tag & PRV_TAG_DEFFERED) != 0) {
+        if (id || prv->runtime->deferred_loading_data) {
           ui_id_preview_image_render_size(C, nullptr, id, prv, size, use_jobs);
         }
       }

source/blender/editors/render/render_preview.cc

@@ -1579,8 +1579,9 @@ static void icon_preview_startjob_all_sizes(void *customdata, wmJobWorkerStatus
   LISTBASE_FOREACH (IconPreviewSize *, cur_size, &ip->sizes) {
     PreviewImage *prv = static_cast<PreviewImage *>(ip->owner);
     /* Is this a render job or a deferred loading job? */
-    const ePreviewRenderMethod pr_method = (prv->tag & PRV_TAG_DEFFERED) ? PR_ICON_DEFERRED :
-                                                                           PR_ICON_RENDER;
+    const ePreviewRenderMethod pr_method = (prv->runtime->deferred_loading_data) ?
+                                               PR_ICON_DEFERRED :
+                                               PR_ICON_RENDER;
 
     if (worker_status->stop) {
       break;
@@ -1704,7 +1705,7 @@ static void icon_preview_endjob(void *customdata)
     }
 
     if (prv_img->tag & PRV_TAG_DEFFERED_DELETE) {
-      BLI_assert(prv_img->tag & PRV_TAG_DEFFERED);
+      BLI_assert(prv_img->runtime->deferred_loading_data);
       BKE_previewimg_deferred_release(prv_img);
     }
   }
@@ -1790,7 +1791,7 @@ void PreviewLoadJob::load_jobless(PreviewImage *preview, const eIconSizes icon_s
 
 void PreviewLoadJob::push_load_request(PreviewImage *preview, const eIconSizes icon_size)
 {
-  BLI_assert(preview->tag & PRV_TAG_DEFFERED);
+  BLI_assert(preview->runtime->deferred_loading_data);
   RequestedPreview requested_preview{};
   requested_preview.preview = preview;
   requested_preview.icon_size = icon_size;
@@ -1859,7 +1860,7 @@ void PreviewLoadJob::finish_request(RequestedPreview &request)
   BLI_assert_msg(BLI_thread_is_main(),
                  "Deferred releasing of preview images should only run on the main thread");
   if (preview->tag & PRV_TAG_DEFFERED_DELETE) {
-    BLI_assert(preview->tag & PRV_TAG_DEFFERED);
+    BLI_assert(preview->runtime->deferred_loading_data);
     BKE_previewimg_deferred_release(preview);
   }
 }
@@ -1935,7 +1936,7 @@ void ED_preview_icon_render(
     const bContext *C, Scene *scene, PreviewImage *prv_img, ID *id, eIconSizes icon_size)
 {
   /* Deferred loading of previews from the file system. */
-  if (prv_img->tag & PRV_TAG_DEFFERED) {
+  if (prv_img->runtime->deferred_loading_data) {
     if (prv_img->flag[icon_size] & PRV_RENDERING) {
       /* Already in the queue, don't add it again. */
       return;
@@ -1979,7 +1980,7 @@ void ED_preview_icon_job(
     const bContext *C, PreviewImage *prv_img, ID *id, eIconSizes icon_size, const bool delay)
 {
   /* Deferred loading of previews from the file system. */
-  if (prv_img->tag & PRV_TAG_DEFFERED) {
+  if (prv_img->runtime->deferred_loading_data) {
     if (prv_img->flag[icon_size] & PRV_RENDERING) {
       /* Already in the queue, don't add it again. */
       return;

source/blender/imbuf/IMB_thumbs.hh

@@ -22,7 +22,7 @@ enum ThumbSize {
   THB_FAIL,
 };
 
-enum ThumbSource {
+enum ThumbSource : int8_t {
   THB_SOURCE_IMAGE,
   THB_SOURCE_MOVIE,
   THB_SOURCE_BLEND,

source/blender/makesdna/DNA_ID.h

@@ -13,6 +13,15 @@
 #include "DNA_defs.h"
 #include "DNA_listBase.h"
 
+#ifdef __cplusplus
+namespace blender::bke {
+struct PreviewImageRuntime;
+}
+using PreviewImageRuntimeHandle = blender::bke::PreviewImageRuntime;
+#else
+typedef struct PreviewImageRuntimeHandle PreviewImageRuntimeHandle;
+#endif
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -614,8 +623,6 @@ enum ePreviewImage_Flag {
 
 /* PreviewImage.tag */
 enum {
-  /** Actual loading of preview is deferred. */
-  PRV_TAG_DEFFERED = (1 << 0),
   /** Deferred preview is being loaded. */
   PRV_TAG_DEFFERED_RENDERING = (1 << 1),
   /** Deferred preview should be deleted asap. */
@@ -627,6 +634,7 @@ enum {
  * Don't call this for shallow copies (or the original instance will have dangling pointers).
  */
 typedef struct PreviewImage {
+  DNA_DEFINE_CXX_METHODS(PreviewImage)
   /* All values of 2 are really NUM_ICON_SIZES */
   unsigned int w[2];
   unsigned int h[2];
@@ -643,15 +651,7 @@ typedef struct PreviewImage {
   short tag;
   char _pad[2];
 
-#ifdef __cplusplus
-  PreviewImage();
-  /* Shallow copy! Contained data is not copied. */
-  PreviewImage(const PreviewImage &) = default;
-  /* Don't free contained data to allow shallow copies. */
-  ~PreviewImage() = default;
-  /* Shallow copy! Contained data is not copied. */
-  PreviewImage &operator=(const PreviewImage &) = default;
-#endif
+  PreviewImageRuntimeHandle *runtime;
 } PreviewImage;
 
 #define ID_FAKE_USERS(id) ((((const ID *)id)->flag & LIB_FAKEUSER) ? 1 : 0)