From eefee47a8acbcdb71add432fc789f41676627da4 Mon Sep 17 00:00:00 2001 From: Julian Eisel Date: Wed, 30 Aug 2023 17:39:51 +0200 Subject: [PATCH] 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 | 8 + source/blender/blenkernel/intern/icons.cc | 137 ++++++++++++------ .../blender/editors/render/render_preview.cc | 11 +- source/blender/makesdna/DNA_ID.h | 19 ++- 4 files changed, 122 insertions(+), 53 deletions(-) diff --git a/source/blender/blenkernel/BKE_icons.h b/source/blender/blenkernel/BKE_icons.h index c64538873be..146f530e675 100644 --- a/source/blender/blenkernel/BKE_icons.h +++ b/source/blender/blenkernel/BKE_icons.h @@ -16,6 +16,9 @@ */ #ifdef __cplusplus + +# include + 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 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()). diff --git a/source/blender/blenkernel/intern/icons.cc b/source/blender/blenkernel/intern/icons.cc index 19efb4831d2..d9d6dbfb51b 100644 --- a/source/blender/blenkernel/intern/icons.cc +++ b/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(prv); +} +const PreviewImageDeferred &PreviewImageDeferred::from_base(const PreviewImage &prv) +{ + return static_cast(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(__func__, filepath, source); } PreviewImage *BKE_previewimg_create() { - return previewimg_create_ex(0); + return MEM_new(__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(*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 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]; diff --git a/source/blender/editors/render/render_preview.cc b/source/blender/editors/render/render_preview.cc index b1458c15560..ea21b5ad297 100644 --- a/source/blender/editors/render/render_preview.cc +++ b/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(PRV_DEFERRED_DATA(preview)); - const ThumbSource source = static_cast(deferred_data[0]); - const char *filepath = &deferred_data[1]; + const std::optional 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) { diff --git a/source/blender/makesdna/DNA_ID.h b/source/blender/makesdna/DNA_ID.h index 2296090b688..1cf87d8f808 100644 --- a/source/blender/makesdna/DNA_ID.h +++ b/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))