From 5445fae9cf33177f87ad85e2b4f63ed5fc7ff436 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Mon, 20 May 2024 14:25:44 +0200 Subject: [PATCH] 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 --- .../blender/blenkernel/BKE_preview_image.hh | 17 +++ .../blenkernel/intern/preview_image.cc | 114 ++++++++---------- .../editors/interface/interface_icons.cc | 2 +- .../blender/editors/render/render_preview.cc | 15 +-- source/blender/imbuf/IMB_thumbs.hh | 2 +- source/blender/makesdna/DNA_ID.h | 22 ++-- 6 files changed, 87 insertions(+), 85 deletions(-) diff --git a/source/blender/blenkernel/BKE_preview_image.hh b/source/blender/blenkernel/BKE_preview_image.hh index 7cfc1ec009f..47ec50df9fd 100644 --- a/source/blender/blenkernel/BKE_preview_image.hh +++ b/source/blender/blenkernel/BKE_preview_image.hh @@ -4,6 +4,7 @@ #pragma once +#include #include #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 deferred_loading_data; + PreviewImageRuntime(); + PreviewImageRuntime(const PreviewImageRuntime &other); + ~PreviewImageRuntime(); +}; + +} // namespace blender::bke + void BKE_preview_images_init(); void BKE_preview_images_free(); diff --git a/source/blender/blenkernel/intern/preview_image.cc b/source/blender/blenkernel/intern/preview_image.cc index 6ba5b2b9b2c..446052c2704 100644 --- a/source/blender/blenkernel/intern/preview_image.cc +++ b/source/blender/blenkernel/intern/preview_image.cc @@ -6,6 +6,7 @@ * \ingroup bke */ +#include #include #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( + *other.deferred_loading_data); } } +PreviewImageRuntime::~PreviewImageRuntime() = default; -PreviewImageDeferred &PreviewImageDeferred::from_base(PreviewImage &prv) -{ - return static_cast(prv); -} -const PreviewImageDeferred &PreviewImageDeferred::from_base(const PreviewImage &prv) -{ - return static_cast(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(__func__, filepath, source); + PreviewImage *prv = BKE_previewimg_create(); + prv->runtime->deferred_loading_data = + std::make_unique(); + prv->runtime->deferred_loading_data->filepath = filepath; + prv->runtime->deferred_loading_data->source = source; + return prv; } PreviewImage *BKE_previewimg_create() { - return MEM_new(__func__); + PreviewImage *prv = static_cast(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(__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(__func__, blender::dna::shallow_copy(*prv)); + prv_img->runtime = MEM_new(__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(*prv_p); + prv = static_cast(*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 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(__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]); diff --git a/source/blender/editors/interface/interface_icons.cc b/source/blender/editors/interface/interface_icons.cc index c5929546500..f767e6f7854 100644 --- a/source/blender/editors/interface/interface_icons.cc +++ b/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); } } diff --git a/source/blender/editors/render/render_preview.cc b/source/blender/editors/render/render_preview.cc index ed64af8f506..5975ab7190c 100644 --- a/source/blender/editors/render/render_preview.cc +++ b/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(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; diff --git a/source/blender/imbuf/IMB_thumbs.hh b/source/blender/imbuf/IMB_thumbs.hh index 8d95adcb225..18712104d64 100644 --- a/source/blender/imbuf/IMB_thumbs.hh +++ b/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, diff --git a/source/blender/makesdna/DNA_ID.h b/source/blender/makesdna/DNA_ID.h index d5812d36cf7..841a2629d18 100644 --- a/source/blender/makesdna/DNA_ID.h +++ b/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)