[Bf-blender-cvs] [e06399c2236] master: Fix badly broken caches handling during undo/redo.
Bastien Montagne
noreply at git.blender.org
Wed Apr 6 11:37:31 CEST 2022
Commit: e06399c223686aee7c2b6f9fb2db66a5f2cffad6
Author: Bastien Montagne
Date: Wed Apr 6 10:48:33 2022 +0200
Branches: master
https://developer.blender.org/rBe06399c223686aee7c2b6f9fb2db66a5f2cffad6
Fix badly broken caches handling during undo/redo.
Original rework of caches during undo/redo (see D8183) had a very bad
flaw hidden in it: using the key of a ghash as source of data.
While this was effectively working then (cache pointer itself being part
of the key, and said cache pointers not being cleared on file write),
this is a general very bad way to do things.
Now that cache pointers are more and more cleared on file write (as part
of clearing runtime-data to reduce false-positives when checking if an
ID has changed or not), this has to be fixed properly by:
* Not storing the cache pointer itself in the IDCacheKey.
* In undo context, in readfile code trying to preserve caches, store the
cache pointers as values of the mapping, together with the usages counter
The first change potentially affects all usages of
`BKE_idtype_id_foreach_cache`, but in practice this code is only used by
memfile reading code (i.e. undo) currently.
Related to T97015.
Reviewed By: brecht
Maniphest Tasks: T97015
Differential Revision: https://developer.blender.org/D14559
===================================================================
M source/blender/blenkernel/BKE_idtype.h
M source/blender/blenkernel/intern/idtype.c
M source/blender/blenkernel/intern/image.cc
M source/blender/blenkernel/intern/movieclip.c
M source/blender/blenkernel/intern/node.cc
M source/blender/blenkernel/intern/scene.cc
M source/blender/blenkernel/intern/sound.c
M source/blender/blenkernel/intern/volume.cc
M source/blender/blenloader/intern/readfile.c
===================================================================
diff --git a/source/blender/blenkernel/BKE_idtype.h b/source/blender/blenkernel/BKE_idtype.h
index bd64b03cc7d..30f7ed45859 100644
--- a/source/blender/blenkernel/BKE_idtype.h
+++ b/source/blender/blenkernel/BKE_idtype.h
@@ -47,8 +47,6 @@ typedef struct IDCacheKey {
/* Value uniquely identifying the cache within its ID.
* Typically the offset of its member in the data-block struct, but can be anything. */
size_t offset_in_ID;
- /* Actual address of the cached data to save and restore. */
- void *cache_v;
} IDCacheKey;
uint BKE_idtype_cache_key_hash(const void *key_v);
diff --git a/source/blender/blenkernel/intern/idtype.c b/source/blender/blenkernel/intern/idtype.c
index 5b9dfa55c45..e55143d6852 100644
--- a/source/blender/blenkernel/intern/idtype.c
+++ b/source/blender/blenkernel/intern/idtype.c
@@ -33,7 +33,7 @@ uint BKE_idtype_cache_key_hash(const void *key_v)
const IDCacheKey *key = key_v;
size_t hash = BLI_ghashutil_uinthash(key->id_session_uuid);
hash = BLI_ghashutil_combine_hash(hash, BLI_ghashutil_uinthash((uint)key->offset_in_ID));
- return (uint)BLI_ghashutil_combine_hash(hash, BLI_ghashutil_ptrhash(key->cache_v));
+ return (uint)hash;
}
bool BKE_idtype_cache_key_cmp(const void *key_a_v, const void *key_b_v)
@@ -41,7 +41,7 @@ bool BKE_idtype_cache_key_cmp(const void *key_a_v, const void *key_b_v)
const IDCacheKey *key_a = key_a_v;
const IDCacheKey *key_b = key_b_v;
return (key_a->id_session_uuid != key_b->id_session_uuid) ||
- (key_a->offset_in_ID != key_b->offset_in_ID) || (key_a->cache_v != key_b->cache_v);
+ (key_a->offset_in_ID != key_b->offset_in_ID);
}
static IDTypeInfo *id_types[INDEX_ID_MAX] = {NULL};
diff --git a/source/blender/blenkernel/intern/image.cc b/source/blender/blenkernel/intern/image.cc
index 3eade265bf2..b1385eab9cf 100644
--- a/source/blender/blenkernel/intern/image.cc
+++ b/source/blender/blenkernel/intern/image.cc
@@ -234,7 +234,6 @@ static void image_foreach_cache(ID *id,
IDCacheKey key;
key.id_session_uuid = id->session_uuid;
key.offset_in_ID = offsetof(Image, cache);
- key.cache_v = image->cache;
function_callback(id, &key, (void **)&image->cache, 0, user_data);
auto gputexture_offset = [image](int target, int eye, int resolution) {
@@ -253,19 +252,16 @@ static void image_foreach_cache(ID *id,
continue;
}
key.offset_in_ID = gputexture_offset(a, eye, resolution);
- key.cache_v = texture;
function_callback(id, &key, (void **)&image->gputexture[a][eye][resolution], 0, user_data);
}
}
}
key.offset_in_ID = offsetof(Image, rr);
- key.cache_v = image->rr;
function_callback(id, &key, (void **)&image->rr, 0, user_data);
LISTBASE_FOREACH (RenderSlot *, slot, &image->renderslots) {
key.offset_in_ID = (size_t)BLI_ghashutil_strhash_p(slot->name);
- key.cache_v = slot->render;
function_callback(id, &key, (void **)&slot->render, 0, user_data);
}
}
diff --git a/source/blender/blenkernel/intern/movieclip.c b/source/blender/blenkernel/intern/movieclip.c
index 3a93b7cde84..accbca42da6 100644
--- a/source/blender/blenkernel/intern/movieclip.c
+++ b/source/blender/blenkernel/intern/movieclip.c
@@ -139,12 +139,10 @@ static void movie_clip_foreach_cache(ID *id,
IDCacheKey key = {
.id_session_uuid = id->session_uuid,
.offset_in_ID = offsetof(MovieClip, cache),
- .cache_v = movie_clip->cache,
};
function_callback(id, &key, (void **)&movie_clip->cache, 0, user_data);
key.offset_in_ID = offsetof(MovieClip, tracking.camera.intrinsics);
- key.cache_v = movie_clip->tracking.camera.intrinsics;
function_callback(id, &key, (void **)&movie_clip->tracking.camera.intrinsics, 0, user_data);
}
diff --git a/source/blender/blenkernel/intern/node.cc b/source/blender/blenkernel/intern/node.cc
index e3fe5d77d63..76b66beaf0d 100644
--- a/source/blender/blenkernel/intern/node.cc
+++ b/source/blender/blenkernel/intern/node.cc
@@ -359,7 +359,6 @@ static void node_foreach_cache(ID *id,
IDCacheKey key = {0};
key.id_session_uuid = id->session_uuid;
key.offset_in_ID = offsetof(bNodeTree, previews);
- key.cache_v = nodetree->previews;
/* TODO: see also `direct_link_nodetree()` in readfile.c. */
#if 0
@@ -370,7 +369,6 @@ static void node_foreach_cache(ID *id,
LISTBASE_FOREACH (bNode *, node, &nodetree->nodes) {
if (node->type == CMP_NODE_MOVIEDISTORTION) {
key.offset_in_ID = (size_t)BLI_ghashutil_strhash_p(node->name);
- key.cache_v = node->storage;
function_callback(id, &key, (void **)&node->storage, 0, user_data);
}
}
diff --git a/source/blender/blenkernel/intern/scene.cc b/source/blender/blenkernel/intern/scene.cc
index d6381bf3f18..4e6191cca6f 100644
--- a/source/blender/blenkernel/intern/scene.cc
+++ b/source/blender/blenkernel/intern/scene.cc
@@ -858,7 +858,6 @@ static void scene_foreach_cache(ID *id,
IDCacheKey key{};
key.id_session_uuid = id->session_uuid;
key.offset_in_ID = offsetof(Scene, eevee.light_cache_data);
- key.cache_v = scene->eevee.light_cache_data;
function_callback(id,
&key,
diff --git a/source/blender/blenkernel/intern/sound.c b/source/blender/blenkernel/intern/sound.c
index b991805fae8..864a4f3281b 100644
--- a/source/blender/blenkernel/intern/sound.c
+++ b/source/blender/blenkernel/intern/sound.c
@@ -112,7 +112,6 @@ static void sound_foreach_cache(ID *id,
IDCacheKey key = {
.id_session_uuid = id->session_uuid,
.offset_in_ID = offsetof(bSound, waveform),
- .cache_v = sound->waveform,
};
function_callback(id, &key, &sound->waveform, 0, user_data);
diff --git a/source/blender/blenkernel/intern/volume.cc b/source/blender/blenkernel/intern/volume.cc
index 07db0328f56..0c131863edd 100644
--- a/source/blender/blenkernel/intern/volume.cc
+++ b/source/blender/blenkernel/intern/volume.cc
@@ -569,8 +569,7 @@ static void volume_foreach_cache(ID *id,
Volume *volume = (Volume *)id;
IDCacheKey key = {
/* id_session_uuid */ id->session_uuid,
- /*offset_in_ID*/ offsetof(Volume, runtime.grids),
- /* cache_v */ volume->runtime.grids,
+ /* offset_in_ID */ offsetof(Volume, runtime.grids),
};
function_callback(id, &key, (void **)&volume->runtime.grids, 0, user_data);
diff --git a/source/blender/blenloader/intern/readfile.c b/source/blender/blenloader/intern/readfile.c
index 4ea6287399c..aba0bfe84d2 100644
--- a/source/blender/blenloader/intern/readfile.c
+++ b/source/blender/blenloader/intern/readfile.c
@@ -1689,12 +1689,14 @@ typedef struct BLOCacheStorage {
MemArena *memarena;
} BLOCacheStorage;
+typedef struct BLOCacheStorageValue {
+ void *cache_v;
+ uint new_usage_count;
+} BLOCacheStorageValue;
+
/** Register a cache data entry to be preserved when reading some undo memfile. */
-static void blo_cache_storage_entry_register(ID *id,
- const IDCacheKey *key,
- void **UNUSED(cache_p),
- uint UNUSED(flags),
- void *cache_storage_v)
+static void blo_cache_storage_entry_register(
+ ID *id, const IDCacheKey *key, void **cache_p, uint UNUSED(flags), void *cache_storage_v)
{
BLI_assert(key->id_session_uuid == id->session_uuid);
UNUSED_VARS_NDEBUG(id);
@@ -1704,7 +1706,11 @@ static void blo_cache_storage_entry_register(ID *id,
IDCacheKey *storage_key = BLI_memarena_alloc(cache_storage->memarena, sizeof(*storage_key));
*storage_key = *key;
- BLI_ghash_insert(cache_storage->cache_map, storage_key, POINTER_FROM_UINT(0));
+ BLOCacheStorageValue *storage_value = BLI_memarena_alloc(cache_storage->memarena,
+ sizeof(*storage_value));
+ storage_value->cache_v = *cache_p;
+ storage_value->new_usage_count = 0;
+ BLI_ghash_insert(cache_storage->cache_map, storage_key, storage_value);
}
/** Restore a cache data entry from old ID into new one, when reading some undo memfile. */
@@ -1723,13 +1729,13 @@ static void blo_cache_storage_entry_restore_in_new(
return;
}
- void **value = BLI_ghash_lookup_p(cache_storage->cache_map, key);
- if (value == NULL) {
+ BLOCacheStorageValue *storage_value = BLI_ghash_lookup(cache_storage->cache_map, key);
+ if (storage_value == NULL) {
*cache_p = NULL;
return;
}
- *value = POINTER_FROM_UINT(POINTER_AS_UINT(*value) + 1);
- *cache_p = key->cache_v;
+ storage_value->new_usage_count++;
+ *cache_p = storage_value->cache_v;
}
/** Clear as needed a cache data entry from old ID, when reading some undo memfile. */
@@ -1741,14 +1747,19 @@ static void blo_cache_storage_entry_clear_in_old(ID *UNUSED(id),
{
BLOCacheStorage *cache_storage = cache_storage_v;
- void **value = BLI_ghash_lookup_p(cache_storage->cache_map, key);
- if (value == NULL) {
+ BLOCacheStorageValue *storage_value = BLI_ghash_lookup(cache_storage->cache_map, key);
+ if (storage_value == NULL) {
*cache_p = NULL;
return;
}
/* If that cache has been restored into some new ID, we want to remove it from old one, otherwise
* keep it there so that it gets properly freed together with its ID. */
- *cache_p = POINTER_AS_UINT(*value) != 0 ? NULL : key->cache_v;
+ if (storage_value->new_usage_count != 0) {
+ *cache_p = NULL;
+ }
+ else {
+ BLI_assert(*cache_p == storage_value->cache_v);
+ }
}
void blo_cache_storage_init(FileData *fd, Main *bmain)
More information about the Bf-blender-cvs
mailing list