[emapp] fixed a crash bug at loading an effect with error (#154)

The commit fixes a crash bug at loading an effect with error by deleting shared specific `ImageContainer::destroy` due to unnecessary destruction of shared image handle. The commit also fixes memory leak of offscreen render target effect set.
hkrn · Jun 14, 2022 · 5093b23 · 5093b23
1 parent 49180ee
commit 5093b23
Show file tree

Hide file tree

Showing 6 changed files with 11 additions and 40 deletions.
diff --git a/emapp/include/emapp/effect/OffscreenRenderTargetImageContainer.h b/emapp/include/emapp/effect/OffscreenRenderTargetImageContainer.h
@@ -27,7 +27,6 @@ class OffscreenRenderTargetImageContainer NANOEM_DECL_SEALED : public RenderTarg
         sg_pixel_format format);
     void resizeWithScale(Effect *effect, const Vector2UI16 &size);
     void destroy(Effect *effect) NANOEM_DECL_NOEXCEPT;
-    void destroy(Effect *effect, sg_image sharedTexture) NANOEM_DECL_NOEXCEPT;
 
     const sg_image_desc &depthStencilImageDescription() const NANOEM_DECL_NOEXCEPT;
     sg_image depthStencilImageHandle() const NANOEM_DECL_NOEXCEPT;

diff --git a/emapp/include/emapp/effect/RenderTargetColorImageContainer.h b/emapp/include/emapp/effect/RenderTargetColorImageContainer.h
@@ -41,7 +41,6 @@ class RenderTargetColorImageContainer : private NonCopyable {
     void setColorImageHandle(sg_image value);
     void setScaleFactor(const Vector2 &value);
     void destroy(Effect *effect) NANOEM_DECL_NOEXCEPT;
-    void destroy(Effect *effect, sg_image sharedTexture) NANOEM_DECL_NOEXCEPT;
 
     RenderTargetMipmapGenerator *mipmapGenerator();
     const char *nameConstString() const NANOEM_DECL_NOEXCEPT;

diff --git a/emapp/src/Effect.cc b/emapp/src/Effect.cc
@@ -5379,13 +5379,10 @@ void
 Effect::destroyAllRenderTargetColorImages(NamedRenderTargetColorImageContainerMap &containers)
 {
     SG_PUSH_GROUPF("Effect::destroyAllRenderTargetColorImages(size=%d)", containers.size());
-    sg_image invalid = { SG_INVALID_ID };
     for (NamedRenderTargetColorImageContainerMap::iterator it = containers.begin(), end = containers.end(); it != end;
          ++it) {
-        RenderTargetColorImageContainer *container = it->second,
-                                        *sharedContainer =
-                                            m_project->findSharedRenderTargetImageContainer(it->first, this);
-        container->destroy(this, sharedContainer ? sharedContainer->colorImageHandle() : invalid);
+        RenderTargetColorImageContainer *container = it->second;
+        container->destroy(this);
         nanoem_delete(container);
     }
     containers.clear();
@@ -5413,10 +5410,8 @@ Effect::destroyAllOffscreenRenderTargetImages(OffscreenRenderTargetImageContaine
     sg_image invalid = { SG_INVALID_ID };
     for (OffscreenRenderTargetImageContainerMap::iterator it = containers.begin(), end = containers.end(); it != end;
          ++it) {
-        const RenderTargetColorImageContainer *sharedContainer =
-            m_project->findSharedRenderTargetImageContainer(it->first, this);
         OffscreenRenderTargetImageContainer *container = it->second;
-        container->destroy(this, sharedContainer ? sharedContainer->colorImageHandle() : invalid);
+        container->destroy(this);
         nanoem_delete(container);
     }
     containers.clear();

diff --git a/emapp/src/Project.cc b/emapp/src/Project.cc
@@ -6431,6 +6431,7 @@ Project::loadOffscreenRenderTargetEffect(Effect *ownerEffect, const IncludeEffec
                     cond.m_hidden = cond.m_none = false;
                     newConditions.push_back(cond);
                     m_allOffscreenRenderTargetEffectSets[ownerEffect].insert(targetEffect);
+                    m_loadedEffectSet.insert(targetEffect);
                 }
                 else {
                     bool hitCache = enableSourceCache && findSourceEffectCache(resolvedURI, output, error);
@@ -6447,6 +6448,7 @@ Project::loadOffscreenRenderTargetEffect(Effect *ownerEffect, const IncludeEffec
                                           URI::lastPathComponent(filename))
                                     : filename);
                             m_allOffscreenRenderTargetEffectSets[ownerEffect].insert(targetEffect);
+                            m_loadedEffectSet.insert(targetEffect);
                             if (enableSourceCache && !hitCache) {
                                 setSourceEffectCache(resolvedURI, output, error);
                             }
@@ -6472,6 +6474,7 @@ Project::loadOffscreenRenderTargetEffect(Effect *ownerEffect, const IncludeEffec
                     if (loadOffscreenRenderTargetEffectFromByteArray(
                             targetEffect, fileURI, condition, bytes, newConditions, progress, error)) {
                         m_allOffscreenRenderTargetEffectSets[ownerEffect].insert(targetEffect);
+                        m_loadedEffectSet.insert(targetEffect);
                     }
                     else {
                         destroyDetachedEffect(targetEffect);
@@ -6529,6 +6532,7 @@ Project::loadOffscreenRenderTargetEffectFromEffectSourceMap(const Effect *ownerE
     }
     if (loaded) {
         m_allOffscreenRenderTargetEffectSets[ownerEffect].insert(targetEffect);
+        m_loadedEffectSet.insert(targetEffect);
     }
     else {
         destroyDetachedEffect(targetEffect);

diff --git a/emapp/src/effect/OffscreenRenderTargetImageContainer.cc b/emapp/src/effect/OffscreenRenderTargetImageContainer.cc
@@ -133,20 +133,6 @@ OffscreenRenderTargetImageContainer::destroy(Effect *effect) NANOEM_DECL_NOEXCEP
     SG_POP_GROUP();
 }
 
-void
-OffscreenRenderTargetImageContainer::destroy(Effect *effect, sg_image sharedTexture) NANOEM_DECL_NOEXCEPT
-{
-    SG_PUSH_GROUPF("effect::OffscreenRenderTargetImageContainer::destroy(name=%s, sharedTexture=%d)", nameConstString(),
-        sharedTexture.id);
-    RenderTargetColorImageContainer::destroy(effect, sharedTexture);
-    destroyAllImages(effect, m_depthStencilMipmapImages);
-    m_depthStencilMipmapImages.clear();
-    effect->removeImageLabel(m_depthStencilImage);
-    sg::destroy_image(m_depthStencilImage);
-    m_depthStencilImage = { SG_INVALID_ID };
-    SG_POP_GROUP();
-}
-
 const sg_image_desc &
 OffscreenRenderTargetImageContainer::depthStencilImageDescription() const NANOEM_DECL_NOEXCEPT
 {

diff --git a/emapp/src/effect/RenderTargetColorImageContainer.cc b/emapp/src/effect/RenderTargetColorImageContainer.cc
@@ -159,8 +159,10 @@ void
 RenderTargetColorImageContainer::destroy(Effect *effect) NANOEM_DECL_NOEXCEPT
 {
     SG_PUSH_GROUPF("effect::RenderTargetColorImageContainer::destroy(name=%s)", m_name.c_str());
-    effect->removeImageLabel(m_colorImage);
-    sg::destroy_image(m_colorImage);
+    if (!m_sharedTexture) {
+        effect->removeImageLabel(m_colorImage);
+        sg::destroy_image(m_colorImage);
+    }
     m_colorImage = { SG_INVALID_ID };
     if (m_mipmapGenerator) {
         m_mipmapGenerator->destroy(effect);
@@ -169,20 +171,6 @@ RenderTargetColorImageContainer::destroy(Effect *effect) NANOEM_DECL_NOEXCEPT
     SG_POP_GROUP();
 }
 
-void
-RenderTargetColorImageContainer::destroy(Effect *effect, sg_image sharedTexture) NANOEM_DECL_NOEXCEPT
-{
-    SG_PUSH_GROUPF("effect::RenderTargetColorImageContainer::destroy(name=%s, sharedTexture=%d)", m_name.c_str(),
-        sharedTexture.id);
-    const bool shared = sg::is_valid(sharedTexture);
-    effect->removeImageLabel(shared ? sharedTexture : m_colorImage);
-    sg::destroy_image(shared ? sharedTexture : m_colorImage);
-    if (!shared) {
-        m_colorImage = { SG_INVALID_ID };
-    }
-    SG_POP_GROUP();
-}
-
 RenderTargetMipmapGenerator *
 RenderTargetColorImageContainer::mipmapGenerator()
 {