validate MaterialInstance references when destroyed (#8366)

* validate MaterialInstance references when destroyed With this change we now enforce two things: - All MaterialInstance of a Material must be destroyed when destroying said Material. This has always been a documented requirement of the public API, but wasn't enforced (only a warning was printed). This new assertion is unconditional. - A MaterialInstance, when destroyed is no longer in use by any Renderable. So before destroying a MaterialInstance, the user of API needs to ensure that either all Renderable using that MaterialInstance in one of their Render Primitives are destroyed, or, that these Renderable using that MaterialInstance are reset to another one or to null. There is a new RenderableManager::clearMaterialInstanceAt() that can be used to clear a MaterialInstance on a Render Primitive. Additionally, a Render Primitive with a null MaterialInstance is now silently skipped during rendering, instead of a null-dereference. Finally, that second assert is protected by a new feature flag: "features.engine.debug.assert_material_instance_in_use". This flag is enabled on DEBUG builds and disabled on RELEASE builds by default. The flag can be changed at any time using `Engine::setFeatureFlag()`. BUGS=[333907416] * Update filament/src/components/RenderableManager.cpp Co-authored-by: Powei Feng <[email protected]> --------- Co-authored-by: Powei Feng <[email protected]>
google · Jan 17, 2025 · 1747ae8 · 1747ae8
1 parent 4c3a324
commit 1747ae8
Show file tree

Hide file tree

Showing 15 changed files with 153 additions and 44 deletions.
diff --git a/android/filament-android/src/main/cpp/RenderableManager.cpp b/android/filament-android/src/main/cpp/RenderableManager.cpp
@@ -459,6 +459,13 @@ Java_com_google_android_filament_RenderableManager_nSetMaterialInstanceAt(JNIEnv
             materialInstance);
 }
 
+extern "C" JNIEXPORT void JNICALL
+Java_com_google_android_filament_RenderableManager_nClearMaterialInstanceAt(JNIEnv*, jclass,
+        jlong nativeRenderableManager, jint i, jint primitiveIndex) {
+    RenderableManager *rm = (RenderableManager *) nativeRenderableManager;
+    rm->clearMaterialInstanceAt((RenderableManager::Instance) i, (size_t) primitiveIndex);
+}
+
 extern "C" JNIEXPORT jlong JNICALL
 Java_com_google_android_filament_RenderableManager_nGetMaterialInstanceAt(JNIEnv*, jclass,
         jlong nativeRenderableManager, jint i, jint primitiveIndex) {

diff --git a/android/filament-android/src/main/java/com/google/android/filament/RenderableManager.java b/android/filament-android/src/main/java/com/google/android/filament/RenderableManager.java
@@ -869,6 +869,13 @@ public void setMaterialInstanceAt(@EntityInstance int i, @IntRange(from = 0) int
         nSetMaterialInstanceAt(mNativeObject, i, primitiveIndex, materialInstance.getNativeObject());
     }
 
+    /**
+     * Clears the material instance for the given primitive.
+     */
+    public void clearMaterialInstanceAt(@EntityInstance int i, @IntRange(from = 0) int primitiveIndex) {
+        nClearMaterialInstanceAt(mNativeObject, i, primitiveIndex);
+    }
+
     /**
      * Creates a MaterialInstance Java wrapper object for a particular material instance.
      */
@@ -1012,6 +1019,7 @@ public long getNativeObject() {
     private static native void nGetAxisAlignedBoundingBox(long nativeRenderableManager, int i, float[] center, float[] halfExtent);
     private static native int nGetPrimitiveCount(long nativeRenderableManager, int i);
     private static native void nSetMaterialInstanceAt(long nativeRenderableManager, int i, int primitiveIndex, long nativeMaterialInstance);
+    private static native void nClearMaterialInstanceAt(long nativeRenderableManager, int i, int primitiveIndex);
     private static native long nGetMaterialInstanceAt(long nativeRenderableManager, int i, int primitiveIndex);
     private static native void nSetGeometryAt(long nativeRenderableManager, int i, int primitiveIndex, int primitiveType, long nativeVertexBuffer, long nativeIndexBuffer, int offset, int count);
     private static native void nSetBlendOrderAt(long nativeRenderableManager, int i, int primitiveIndex, int blendOrder);

diff --git a/filament/include/filament/RenderableManager.h b/filament/include/filament/RenderableManager.h
@@ -798,6 +798,13 @@ class UTILS_PUBLIC RenderableManager : public FilamentAPI {
     void setMaterialInstanceAt(Instance instance,
             size_t primitiveIndex, MaterialInstance const* UTILS_NONNULL materialInstance);
 
+    /**
+     * Clear the MaterialInstance for the given primitive.
+     * @param instance Renderable's instance
+     * @param primitiveIndex Primitive index
+     */
+    void clearMaterialInstanceAt(Instance instance, size_t primitiveIndex);
+
     /**
      * Retrieves the material instance that is bound to the given primitive.
      */

diff --git a/filament/src/RenderPass.cpp b/filament/src/RenderPass.cpp
@@ -704,11 +704,22 @@ RenderPass::Command* RenderPass::generateCommandsImpl(RenderPass::CommandTypeFla
          */
         for (auto const& primitive: primitives) {
             FMaterialInstance const* const mi = primitive.getMaterialInstance();
-            FMaterial const* const ma = mi->getMaterial();
+            if (UTILS_UNLIKELY(!mi)) {
+                // This can happen because RenderPrimitives can be initialized with
+                // a null MaterialInstance. In this case, skip the primitive.
+                if constexpr (isColorPass) {
+                    curr->key = uint64_t(Pass::SENTINEL);
+                    ++curr;
+                }
+                curr->key = uint64_t(Pass::SENTINEL);
+                ++curr;
+                continue;
+            }
 
             // TODO: we should disable the SKN variant if this primitive doesn't have either
             //       skinning or morphing.
 
+            FMaterial const* const ma = mi->getMaterial();
             cmd.info.mi = mi;
             cmd.info.rph = primitive.getHwHandle();
             cmd.info.vbih = primitive.getVertexBufferInfoHandle();

diff --git a/filament/src/RenderableManager.cpp b/filament/src/RenderableManager.cpp
@@ -113,6 +113,10 @@ void RenderableManager::setMaterialInstanceAt(Instance instance,
     downcast(this)->setMaterialInstanceAt(instance, 0, primitiveIndex, downcast(materialInstance));
 }
 
+void RenderableManager::clearMaterialInstanceAt(Instance instance, size_t primitiveIndex) {
+    downcast(this)->clearMaterialInstanceAt(instance, 0, primitiveIndex);
+}
+
 MaterialInstance* RenderableManager::getMaterialInstanceAt(
         Instance instance, size_t primitiveIndex) const noexcept {
     return downcast(this)->getMaterialInstanceAt(instance, 0, primitiveIndex);

diff --git a/filament/src/components/RenderableManager.cpp b/filament/src/components/RenderableManager.cpp
@@ -792,10 +792,10 @@ void FRenderableManager::destroyComponentPrimitives(
 
 void FRenderableManager::setMaterialInstanceAt(Instance instance, uint8_t level,
         size_t primitiveIndex, FMaterialInstance const* mi) {
+    assert_invariant(mi);
     if (instance) {
         Slice<FRenderPrimitive>& primitives = getRenderPrimitives(instance, level);
-        if (primitiveIndex < primitives.size()) {
-            assert_invariant(mi);
+        if (primitiveIndex < primitives.size() && mi) {
             FMaterial const* material = mi->getMaterial();
 
             // we want a feature level violation to be a hard error (exception if enabled, or crash)
@@ -819,6 +819,16 @@ void FRenderableManager::setMaterialInstanceAt(Instance instance, uint8_t level,
     }
 }
 
+void FRenderableManager::clearMaterialInstanceAt(Instance instance, uint8_t level,
+        size_t primitiveIndex) {
+    if (instance) {
+        Slice<FRenderPrimitive>& primitives = getRenderPrimitives(instance, level);
+        if (primitiveIndex < primitives.size()) {
+            primitives[primitiveIndex].setMaterialInstance(nullptr);
+        }
+    }
+}
+
 MaterialInstance* FRenderableManager::getMaterialInstanceAt(
         Instance instance, uint8_t level, size_t primitiveIndex) const noexcept {
     if (instance) {

diff --git a/filament/src/components/RenderableManager.h b/filament/src/components/RenderableManager.h
@@ -204,6 +204,7 @@ class FRenderableManager : public RenderableManager {
     size_t getPrimitiveCount(Instance instance, uint8_t level) const noexcept;
     void setMaterialInstanceAt(Instance instance, uint8_t level,
             size_t primitiveIndex, FMaterialInstance const* materialInstance);
+    void clearMaterialInstanceAt(Instance instance, uint8_t level, size_t primitiveIndex);
     MaterialInstance* getMaterialInstanceAt(Instance instance, uint8_t level, size_t primitiveIndex) const noexcept;
     void setGeometryAt(Instance instance, uint8_t level, size_t primitiveIndex,
             PrimitiveType type, FVertexBuffer* vertices, FIndexBuffer* indices,

diff --git a/filament/src/details/Engine.cpp b/filament/src/details/Engine.cpp
@@ -1154,19 +1154,9 @@ UTILS_NOINLINE
 bool FEngine::destroy(const FMaterial* ptr) {
     if (ptr == nullptr) return true;
     bool const success = terminateAndDestroy(ptr, mMaterials);
-    if (success) {
+    if (UTILS_LIKELY(success)) {
         auto pos = mMaterialInstances.find(ptr);
-        if (pos != mMaterialInstances.cend()) {
-            // ensure we've destroyed all instances before destroying the material
-            if (!ASSERT_PRECONDITION_NON_FATAL(pos->second.empty(),
-                    "destroying material \"%s\" but %u instances still alive",
-                    ptr->getName().c_str(), (*pos).second.size())) {
-                // We return true here, because the material has been destroyed. However, this
-                // leaks this material's ResourceList<FMaterialInstance> until the engine
-                // is shut down. Note that it's still possible for the MaterialInstances to be
-                // destroyed manually.
-                return true;
-            }
+        if (UTILS_LIKELY(pos != mMaterialInstances.cend())) {
             mMaterialInstances.erase(pos);
         }
     }
@@ -1176,6 +1166,39 @@ bool FEngine::destroy(const FMaterial* ptr) {
 UTILS_NOINLINE
 bool FEngine::destroy(const FMaterialInstance* ptr) {
     if (ptr == nullptr) return true;
+
+    // Check that the material instance we're destroying is not in use in the RenderableManager
+    // To do this, we currently need to inspect all render primitives in the RenderableManager
+    EntityManager const& em = mEntityManager;
+    FRenderableManager const& rcm = mRenderableManager;
+    Entity const* entities = rcm.getEntities();
+    size_t const count = rcm.getComponentCount();
+    for (size_t i = 0; i < count; i++) {
+        Entity const entity = entities[i];
+        if (em.isAlive(entity)) {
+            RenderableManager::Instance const ri = rcm.getInstance(entity);
+            size_t const primitiveCount = rcm.getPrimitiveCount(ri, 0);
+            for (size_t j = 0; j < primitiveCount; j++) {
+                auto const* const mi = rcm.getMaterialInstanceAt(ri, 0, j);
+                if (features.engine.debug.assert_material_instance_in_use) {
+                    FILAMENT_CHECK_PRECONDITION(mi != ptr)
+                            << "destroying MaterialInstance \""
+                            << mi->getName() << "\" which is still in use by Renderable (entity="
+                            << entity.getId() << ", instance="
+                            << ri.asValue() << ", index=" << j << ")";
+                } else {
+                    if (UTILS_UNLIKELY(mi == ptr)) {
+                        slog.e  << "destroying MaterialInstance \""
+                                << mi->getName() << "\" which is still in use by Renderable (entity="
+                                << entity.getId() << ", instance="
+                                << ri.asValue() << ", index=" << j << ")"
+                                << io::endl;
+                    }
+                }
+            }
+        }
+    }
+
     if (ptr->isDefaultInstance()) return false;
     auto pos = mMaterialInstances.find(ptr->getMaterial());
     assert_invariant(pos != mMaterialInstances.cend());

diff --git a/filament/src/details/Engine.h b/filament/src/details/Engine.h
@@ -501,6 +501,10 @@ class FEngine : public Engine {
         return FEngine::getActiveFeatureLevel() >= neededFeatureLevel;
     }
 
+    auto const& getMaterialInstanceResourceList() const noexcept {
+        return mMaterialInstances;
+    }
+
 #if defined(__EMSCRIPTEN__)
     void resetBackendState() noexcept;
 #endif
@@ -683,6 +687,13 @@ class FEngine : public Engine {
             struct {
                 bool use_shadow_atlas = false;
             } shadows;
+            struct {
+#ifndef NDEBUG
+                bool assert_material_instance_in_use = true;
+#else
+                bool assert_material_instance_in_use = false;
+#endif
+            } debug;
         } engine;
         struct {
             struct {
@@ -705,7 +716,10 @@ class FEngine : public Engine {
               &features.backend.opengl.assert_native_window_is_valid, true },
             { "engine.shadows.use_shadow_atlas",
               "Uses an array of atlases to store shadow maps.",
-              &features.engine.shadows.use_shadow_atlas, true }
+              &features.engine.shadows.use_shadow_atlas, true },
+            { "features.engine.debug.assert_material_instance_in_use",
+              "Assert when a MaterialInstance is destroyed while it is in use by RenderableManager.",
+              &features.engine.debug.assert_material_instance_in_use, false }
     }};
 
     utils::Slice<const FeatureFlag> getFeatureFlags() const noexcept {

diff --git a/filament/src/details/Material.cpp b/filament/src/details/Material.cpp
@@ -373,6 +373,22 @@ void FMaterial::invalidate(Variant::type_t variantMask, Variant::type_t variantV
 
 void FMaterial::terminate(FEngine& engine) {
 
+    if (mDefaultMaterialInstance) {
+        mDefaultMaterialInstance->setDefaultInstance(false);
+        engine.destroy(mDefaultMaterialInstance);
+        mDefaultMaterialInstance = nullptr;
+    }
+
+    // ensure we've destroyed all instances before destroying the material
+    auto const& materialInstanceResourceList = engine.getMaterialInstanceResourceList();
+    auto pos = materialInstanceResourceList.find(this);
+    if (UTILS_LIKELY(pos != materialInstanceResourceList.cend())) {
+        FILAMENT_CHECK_PRECONDITION(pos->second.empty())
+                << "destroying material \"" << this->getName().c_str_safe() << "\" but "
+                << pos->second.size() << " instances still alive.";
+    }
+
+
 #if FILAMENT_ENABLE_MATDBG
     // Unregister the material with matdbg.
     matdbg::DebugServer* server = engine.debug.server;
@@ -383,16 +399,9 @@ void FMaterial::terminate(FEngine& engine) {
 
     destroyPrograms(engine);
 
-    if (mDefaultMaterialInstance) {
-        mDefaultMaterialInstance->setDefaultInstance(false);
-        engine.destroy(mDefaultMaterialInstance);
-        mDefaultMaterialInstance = nullptr;
-    }
-
-    mPerViewDescriptorSetLayout.terminate(
-            engine.getDescriptorSetLayoutFactory(), engine.getDriverApi());
-    mDescriptorSetLayout.terminate(
-            engine.getDescriptorSetLayoutFactory(), engine.getDriverApi());
+    DriverApi& driver = engine.getDriverApi();
+    mPerViewDescriptorSetLayout.terminate(engine.getDescriptorSetLayoutFactory(), driver);
+    mDescriptorSetLayout.terminate(engine.getDescriptorSetLayoutFactory(), driver);
 }
 
 void FMaterial::compile(CompilerPriorityQueue priority,

diff --git a/filament/src/details/Skybox.cpp b/filament/src/details/Skybox.cpp
@@ -153,13 +153,13 @@ FMaterial const* FSkybox::createMaterial(FEngine& engine) {
 void FSkybox::terminate(FEngine& engine) noexcept {
     // use Engine::destroy because FEngine::destroy is inlined
     Engine& e = engine;
-    e.destroy(mSkyboxMaterialInstance);
     e.destroy(mSkybox);
+    e.destroy(mSkyboxMaterialInstance);
 
     engine.getEntityManager().destroy(mSkybox);
 
-    mSkyboxMaterialInstance = nullptr;
     mSkybox = {};
+    mSkyboxMaterialInstance = nullptr;
 }
 
 void FSkybox::setLayerMask(uint8_t select, uint8_t values) noexcept {

diff --git a/libs/filamentapp/src/Cube.cpp b/libs/filamentapp/src/Cube.cpp
@@ -140,12 +140,15 @@ void Cube::mapAabb(filament::Engine& engine, filament::Box const& box) {
 Cube::~Cube() {
     mEngine.destroy(mVertexBuffer);
     mEngine.destroy(mIndexBuffer);
-    mEngine.destroy(mMaterialInstanceSolid);
-    mEngine.destroy(mMaterialInstanceWireFrame);
+
     // We don't own the material, only instances
     mEngine.destroy(mSolidRenderable);
     mEngine.destroy(mWireFrameRenderable);
 
+    // material instances must be destroyed after the renderables
+    mEngine.destroy(mMaterialInstanceSolid);
+    mEngine.destroy(mMaterialInstanceWireFrame);
+
     utils::EntityManager& em = utils::EntityManager::get();
     em.destroy(mSolidRenderable);
     em.destroy(mWireFrameRenderable);

diff --git a/libs/gltfio/src/FilamentAsset.cpp b/libs/gltfio/src/FilamentAsset.cpp
@@ -37,12 +37,8 @@ FFilamentAsset::~FFilamentAsset() {
     // Free transient load-time data if they haven't been freed yet.
     releaseSourceData();
 
-    // Destroy all instance objects. Instance entities / components are
-    // destroyed later in this method because they are owned by the asset
-    // (except for the root of the instance).
     for (FFilamentInstance* instance : mInstances) {
         mEntityManager->destroy(instance->mRoot);
-        delete instance;
     }
 
     delete mWireframe;
@@ -76,6 +72,12 @@ FFilamentAsset::~FFilamentAsset() {
         }
     }
 
+    // FilamentInstances need to be destroyed after the renderables have been destroyed
+    // so that there are no dangling MaterialInstance around
+    for (FFilamentInstance* instance : mInstances) {
+        delete instance;
+    }
+
     for (auto vb : mVertexBuffers) {
         mEngine->destroy(vb);
     }