UI: Refactor deferred preview image storage

Previously we'd just allocate extra space for additional data for the
deferred loading, and then do pointer arithmetic to access that data.
This is cryptic and not type safe.

Instead, use an internal type deriving from `PreviewImage`, and manage
that internally. This ensures type safety, while keeping this as
implementation detail (not visible outside of the preview image API) and
keeping internals easy to understand.

Some code did shallow copies of preview images, making ownership
unclear. That made things a bit tricky, but I've made support for
shallow copies explicit now and noted this in comments.

source/blender/blenkernel/BKE_icons.h

@@ -16,6 +16,9 @@
  */
 
 #ifdef __cplusplus
+
+#  include <optional>
+
 extern "C" {
 #endif
 
@@ -211,6 +214,11 @@ struct PreviewImage *BKE_previewimg_id_ensure(struct ID *id);
  */
 void BKE_previewimg_ensure(struct PreviewImage *prv, int size);
 
+const char *BKE_previewimg_deferred_filepath_get(const struct PreviewImage *prv);
+#ifdef __cplusplus
+std::optional<int> BKE_previewimg_deferred_thumb_source_get(const struct PreviewImage *prv);
+#endif
+
 /**
  * Create an #ImBuf holding a copy of the preview image buffer in \a prv.
  * \note The returned image buffer has to be free'd (#IMB_freeImBuf()).

source/blender/blenkernel/intern/icons.cc

@@ -15,6 +15,7 @@
 
 #include "MEM_guardedalloc.h"
 
+#include "DNA_ID.h"
 #include "DNA_brush_types.h"
 #include "DNA_collection_types.h"
 #include "DNA_gpencil_legacy_types.h"
@@ -85,6 +86,24 @@ struct DeferredIconDeleteNode {
 /* Protected by gIconMutex. */
 static LockfreeLinkList g_icon_delete_queue;
 
+class PreviewImageDeferred : public PreviewImage {
+ public:
+  const std::string filepath;
+  const ThumbSource source;
+
+  /* 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;
+};
+
 static void icon_free(void *val)
 {
   Icon *icon = (Icon *)val;
@@ -230,40 +249,40 @@ void BKE_icons_deferred_free()
   BLI_linklist_lockfree_clear(&g_icon_delete_queue, MEM_freeN);
 }
 
-static PreviewImage *previewimg_create_ex(size_t deferred_data_size)
+PreviewImage::PreviewImage()
 {
-  PreviewImage *prv_img = (PreviewImage *)MEM_mallocN(sizeof(PreviewImage) + deferred_data_size,
-                                                      "img_prv");
-  memset(prv_img, 0, sizeof(*prv_img)); /* leave deferred data dirty */
-
-  if (deferred_data_size) {
-    prv_img->tag |= PRV_TAG_DEFFERED;
-  }
+  /* Zero initialize */
+  memset(this, 0, sizeof(*this));
 
   for (int i = 0; i < NUM_ICON_SIZES; i++) {
-    prv_img->flag[i] |= PRV_CHANGED;
-    prv_img->changed_timestamp[i] = 0;
+    flag[i] |= PRV_CHANGED;
+    changed_timestamp[i] = 0;
   }
-  return prv_img;
 }
 
-static PreviewImage *previewimg_deferred_create(const char *filepath, int source)
+PreviewImageDeferred &PreviewImageDeferred::from_base(PreviewImage &prv)
 {
-  /* We pack needed data for lazy loading (source type, in a single char, and filepath). */
-  const size_t deferred_data_size = strlen(filepath) + 2;
-  char *deferred_data;
+  return static_cast<PreviewImageDeferred &>(prv);
+}
+const PreviewImageDeferred &PreviewImageDeferred::from_base(const PreviewImage &prv)
+{
+  return static_cast<const PreviewImageDeferred &>(prv);
+}
 
-  PreviewImage *prv = previewimg_create_ex(deferred_data_size);
-  deferred_data = (char *)PRV_DEFERRED_DATA(prv);
-  deferred_data[0] = source;
-  memcpy(&deferred_data[1], filepath, deferred_data_size - 1);
+PreviewImageDeferred::PreviewImageDeferred(blender::StringRef filepath, ThumbSource source)
+    : PreviewImage(), filepath(filepath), source(source)
+{
+  tag |= PRV_TAG_DEFFERED;
+}
 
-  return prv;
+static PreviewImageDeferred *previewimg_deferred_create(const char *filepath, ThumbSource source)
+{
+  return MEM_new<PreviewImageDeferred>(__func__, filepath, source);
 }
 
 PreviewImage *BKE_previewimg_create()
 {
-  return previewimg_create_ex(0);
+  return MEM_new<PreviewImage>(__func__);
 }
 
 void BKE_previewimg_freefunc(void *link)
@@ -272,27 +291,40 @@ void BKE_previewimg_freefunc(void *link)
   if (!prv) {
     return;
   }
-
-  for (int i = 0; i < NUM_ICON_SIZES; i++) {
-    if (prv->rect[i]) {
-      MEM_freeN(prv->rect[i]);
-    }
-    if (prv->gputexture[i]) {
-      GPU_texture_free(prv->gputexture[i]);
-    }
-  }
-
-  MEM_freeN(prv);
+  BKE_previewimg_freefunc(&prv);
 }
 
 void BKE_previewimg_free(PreviewImage **prv)
 {
   if (prv && (*prv)) {
-    BKE_previewimg_freefunc(*prv);
+    for (int i = 0; i < NUM_ICON_SIZES; i++) {
+      if ((*prv)->rect[i]) {
+        MEM_freeN((*prv)->rect[i]);
+      }
+      if ((*prv)->gputexture[i]) {
+        GPU_texture_free((*prv)->gputexture[i]);
+      }
+    }
+
+    if ((*prv)->tag & PRV_TAG_DEFFERED) {
+      PreviewImageDeferred &this_deferred = PreviewImageDeferred::from_base(**prv);
+      std::destroy_at(&this_deferred.filepath);
+    }
+    else {
+      MEM_delete(*prv);
+    }
     *prv = nullptr;
   }
 }
 
+/** 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_clear_single(PreviewImage *prv, enum eIconSizes size)
 {
   MEM_SAFE_FREE(prv->rect[size]);
@@ -442,7 +474,7 @@ void BKE_previewimg_deferred_release(PreviewImage *prv)
   if (prv->icon_id) {
     BKE_icon_delete(prv->icon_id);
   }
-  BKE_previewimg_freefunc(prv);
+  BKE_previewimg_free(&prv);
 }
 
 PreviewImage *BKE_previewimg_cached_get(const char *name)
@@ -475,19 +507,19 @@ PreviewImage *BKE_previewimg_cached_thumbnail_read(const char *name,
 {
   BLI_assert(BLI_thread_is_main());
 
-  PreviewImage *prv = nullptr;
+  PreviewImageDeferred *prv = nullptr;
   void **prv_p;
 
   prv_p = BLI_ghash_lookup_p(gCachedPreviews, name);
 
   if (prv_p) {
-    prv = *(PreviewImage **)prv_p;
+    prv = static_cast<PreviewImageDeferred *>(*prv_p);
     BLI_assert(prv);
+    BLI_assert(prv->tag & PRV_TAG_DEFFERED);
   }
 
   if (prv && force_update) {
-    const char *prv_deferred_data = (char *)PRV_DEFERRED_DATA(prv);
-    if ((int(prv_deferred_data[0]) == source) && STREQ(&prv_deferred_data[1], filepath)) {
+    if ((prv->source == source) && (prv->filepath == filepath)) {
       /* If same filepath, no need to re-allocate preview, just clear it up. */
       BKE_previewimg_clear(prv);
     }
@@ -497,7 +529,7 @@ PreviewImage *BKE_previewimg_cached_thumbnail_read(const char *name,
   }
 
   if (!prv) {
-    prv = previewimg_deferred_create(filepath, source);
+    prv = previewimg_deferred_create(filepath, ThumbSource(source));
     force_update = true;
   }
 
@@ -536,13 +568,10 @@ void BKE_previewimg_ensure(PreviewImage *prv, const int size)
     return;
   }
 
-  ImBuf *thumb;
-  char *prv_deferred_data = (char *)PRV_DEFERRED_DATA(prv);
-  int source = prv_deferred_data[0];
-  char *filepath = &prv_deferred_data[1];
+  PreviewImageDeferred &prv_deferred = PreviewImageDeferred::from_base(*prv);
   int icon_w, icon_h;
 
-  thumb = IMB_thumb_manage(filepath, THB_LARGE, (ThumbSource)source);
+  ImBuf *thumb = IMB_thumb_manage(prv_deferred.filepath.c_str(), THB_LARGE, prv_deferred.source);
   if (!thumb) {
     return;
   }
@@ -578,6 +607,26 @@ void BKE_previewimg_ensure(PreviewImage *prv, const int size)
   IMB_freeImBuf(thumb);
 }
 
+const char *BKE_previewimg_deferred_filepath_get(const PreviewImage *prv)
+{
+  if ((prv->tag & PRV_TAG_DEFFERED) == 0) {
+    return nullptr;
+  }
+
+  const PreviewImageDeferred &prv_deferred = PreviewImageDeferred::from_base(*prv);
+  return prv_deferred.filepath.c_str();
+}
+
+std::optional<int> BKE_previewimg_deferred_thumb_source_get(const PreviewImage *prv)
+{
+  if ((prv->tag & PRV_TAG_DEFFERED) == 0) {
+    return std::nullopt;
+  }
+
+  const PreviewImageDeferred &prv_deferred = PreviewImageDeferred::from_base(*prv);
+  return prv_deferred.source;
+}
+
 ImBuf *BKE_previewimg_to_imbuf(PreviewImage *prv, const int size)
 {
   const uint w = prv->w[size];

source/blender/editors/render/render_preview.cc

@@ -1809,14 +1809,17 @@ void PreviewLoadJob::run_fn(void *customdata, bool *stop, bool *do_update, float
 
     PreviewImage *preview = request->preview;
 
-    const char *deferred_data = static_cast<char *>(PRV_DEFERRED_DATA(preview));
-    const ThumbSource source = static_cast<ThumbSource>(deferred_data[0]);
-    const char *filepath = &deferred_data[1];
+    const std::optional<int> source = BKE_previewimg_deferred_thumb_source_get(preview);
+    const char *filepath = BKE_previewimg_deferred_filepath_get(preview);
+
+    if (!source || !filepath) {
+      continue;
+    }
 
     // printf("loading deferred %d×%d preview for %s\n", request->sizex, request->sizey, filepath);
 
     IMB_thumb_path_lock(filepath);
-    ImBuf *thumb = IMB_thumb_manage(filepath, THB_LARGE, source);
+    ImBuf *thumb = IMB_thumb_manage(filepath, THB_LARGE, ThumbSource(*source));
     IMB_thumb_path_unlock(filepath);
 
     if (thumb) {

source/blender/makesdna/DNA_ID.h

@@ -604,6 +604,10 @@ enum {
   PRV_TAG_DEFFERED_DELETE = (1 << 2),
 };
 
+/**
+ * This type allows shallow copies. Use #BKE_previewimg_free() to release contained resources.
+ * Don't call this for shallow copies (or the original instance will have dangling pointers).
+ */
 typedef struct PreviewImage {
   /* All values of 2 are really NUM_ICON_SIZES */
   unsigned int w[2];
@@ -620,12 +624,17 @@ typedef struct PreviewImage {
   /** Runtime data. */
   short tag;
   char _pad[2];
-} PreviewImage;
 
-#define PRV_DEFERRED_DATA(prv) \
-  (CHECK_TYPE_INLINE(prv, PreviewImage *), \
-   BLI_assert((prv)->tag & PRV_TAG_DEFFERED), \
-   (void *)((prv) + 1))
+#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
+} PreviewImage;
 
 #define ID_FAKE_USERS(id) ((((const ID *)id)->flag & LIB_FAKEUSER) ? 1 : 0)
 #define ID_REAL_USERS(id) (((const ID *)id)->us - ID_FAKE_USERS(id))