From 82623c8fd4bb674e161ea9528f4b0c9fc11f1d2c Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 17 Sep 2021 11:04:53 -0700 Subject: [PATCH] IBLPrefilter relied on finalizer which could cause crashes The finalizer can't be used to dispose of filament resource, because it runs on its own thread. Added a destroy() method to all IBLPrefilter related classes that must be called once the object is no longer needed. note: Resources will be freed eventually anyways when the Engine itself is destroyed. fixes #4633 --- .../filament/utils/IBLPrefilterContext.java | 83 +++++++++---------- .../android/filament/gltf/MainActivity.kt | 4 + .../IBLPrefilterContext.h | 8 +- libs/iblprefilter/src/IBLPrefilterContext.cpp | 7 +- 4 files changed, 52 insertions(+), 50 deletions(-) diff --git a/android/filament-utils-android/src/main/java/com/google/android/filament/utils/IBLPrefilterContext.java b/android/filament-utils-android/src/main/java/com/google/android/filament/utils/IBLPrefilterContext.java index faa71a5a21..0387d39e98 100644 --- a/android/filament-utils-android/src/main/java/com/google/android/filament/utils/IBLPrefilterContext.java +++ b/android/filament-utils-android/src/main/java/com/google/android/filament/utils/IBLPrefilterContext.java @@ -32,9 +32,13 @@ import com.google.android.filament.Texture; * Texture equirect = HDRLoader.createTexture("foo.hdr"); * Texture skyboxTexture = equirectangularToCubemap.run(equirect); * engine.destroy(equirect); + * equirectangularToCubemap.destroy(); * * specularFilter = new IBLPrefilterContext.SpecularFilter(context); * Texture reflections = specularFilter.run(skyboxTexture); + * specularFilter.destroy(); + * + * context.destroy(); * * IndirectLight ibl = IndirectLight.Builder() * .reflections(reflections) @@ -43,81 +47,74 @@ import com.google.android.filament.Texture; * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ */ public class IBLPrefilterContext { - private final long mNativeObject; + private long mNativeObject; public IBLPrefilterContext(Engine engine) { mNativeObject = nCreate(engine.getNativeObject()); if (mNativeObject == 0) throw new IllegalStateException("Couldn't create IBLPrefilterContext"); } - @Override - protected void finalize() throws Throwable { - nDestroy(mNativeObject); - super.finalize(); + public void destroy() { + nDestroy(getNativeObject()); + mNativeObject = 0; } public static class EquirectangularToCubemap { - @SuppressWarnings({"FieldCanBeLocal", "UnusedDeclaration"}) // Keep to finalize native resources - private final HelperFinalizer mFinalizer; - private final long mNativeHelper; + private long mNativeObject; public EquirectangularToCubemap(IBLPrefilterContext context) { - mNativeHelper = nCreateEquirectHelper(context.mNativeObject); - mFinalizer = new HelperFinalizer(mNativeHelper); + mNativeObject = nCreateEquirectHelper(context.getNativeObject()); } public Texture run(Texture equirect) { - long nativeTexture = nEquirectHelperRun(mNativeHelper, equirect.getNativeObject()); + long nativeTexture = nEquirectHelperRun(getNativeObject(), equirect.getNativeObject()); return new Texture(nativeTexture); } - private static class HelperFinalizer { - private final long mNativeObject; + public void destroy() { + nDestroyEquirectHelper(getNativeObject()); + mNativeObject = 0; + } - HelperFinalizer(long nativeObject) { mNativeObject = nativeObject; } - - @Override - public void finalize() { - try { - super.finalize(); - } catch (Throwable t) { // Ignore - } finally { - nDestroyEquirectHelper(mNativeObject); - } + protected long getNativeObject() { + if (mNativeObject == 0) { + throw new IllegalStateException("Calling method on destroyed EquirectangularToCubemap"); } + return mNativeObject; } } public static class SpecularFilter { - @SuppressWarnings({"FieldCanBeLocal", "UnusedDeclaration"}) // Keep to finalize native resources - private final HelperFinalizer mFinalizer; - private final long mNativeHelper; + private long mNativeObject; public SpecularFilter(IBLPrefilterContext context) { - mNativeHelper = nCreateSpecularFilter(context.mNativeObject); - mFinalizer = new HelperFinalizer(mNativeHelper); + mNativeObject = nCreateSpecularFilter(context.getNativeObject()); } public Texture run(Texture skybox) { - long nativeTexture = nSpecularFilterRun(mNativeHelper, skybox.getNativeObject()); + long nativeTexture = nSpecularFilterRun(getNativeObject(), skybox.getNativeObject()); return new Texture(nativeTexture); } - private static class HelperFinalizer { - private final long mNativeObject; - - HelperFinalizer(long nativeObject) { mNativeObject = nativeObject; } - - @Override - public void finalize() { - try { - super.finalize(); - } catch (Throwable t) { // Ignore - } finally { - nDestroySpecularFilter(mNativeObject); - } - } + public void destroy() { + nDestroySpecularFilter(getNativeObject()); + mNativeObject = 0; } + + protected long getNativeObject() { + if (mNativeObject == 0) { + throw new IllegalStateException("Calling method on destroyed SpecularFilter"); + } + return mNativeObject; + } + } + + + protected long getNativeObject() { + if (mNativeObject == 0) { + throw new IllegalStateException("Calling method on destroyed IBLPrefilterContext"); + } + return mNativeObject; } private static native long nCreate(long nativeEngine); diff --git a/android/samples/sample-gltf-viewer/src/main/java/com/google/android/filament/gltf/MainActivity.kt b/android/samples/sample-gltf-viewer/src/main/java/com/google/android/filament/gltf/MainActivity.kt index 2f69d02776..76e1b0f428 100644 --- a/android/samples/sample-gltf-viewer/src/main/java/com/google/android/filament/gltf/MainActivity.kt +++ b/android/samples/sample-gltf-viewer/src/main/java/com/google/android/filament/gltf/MainActivity.kt @@ -196,6 +196,10 @@ class MainActivity : Activity() { val sky = Skybox.Builder().environment(skyboxTexture).build(engine) + specularFilter.destroy(); + equirectToCubemap.destroy(); + context.destroy(); + modelViewer.scene.skybox = sky modelViewer.scene.indirectLight = ibl } diff --git a/libs/iblprefilter/include/filament-iblprefilter/IBLPrefilterContext.h b/libs/iblprefilter/include/filament-iblprefilter/IBLPrefilterContext.h index 560005f8c6..15cd681021 100644 --- a/libs/iblprefilter/include/filament-iblprefilter/IBLPrefilterContext.h +++ b/libs/iblprefilter/include/filament-iblprefilter/IBLPrefilterContext.h @@ -62,7 +62,7 @@ public: * Creates an IBLPrefilter context. * @param engine filament engine to use */ - IBLPrefilterContext(filament::Engine& engine); + explicit IBLPrefilterContext(filament::Engine& engine); /** * Destroys all GPU resources created during initialization. @@ -75,7 +75,7 @@ public: // movable IBLPrefilterContext(IBLPrefilterContext&& rhs) noexcept; - IBLPrefilterContext& operator=(IBLPrefilterContext&& rhs); + IBLPrefilterContext& operator=(IBLPrefilterContext&& rhs) noexcept; // ------------------------------------------------------------------------------------------- @@ -98,7 +98,7 @@ public: EquirectangularToCubemap(EquirectangularToCubemap const&) = delete; EquirectangularToCubemap& operator=(EquirectangularToCubemap const&) = delete; EquirectangularToCubemap(EquirectangularToCubemap&& rhs) noexcept; - EquirectangularToCubemap& operator=(EquirectangularToCubemap&& rhs); + EquirectangularToCubemap& operator=(EquirectangularToCubemap&& rhs) noexcept; /** * Converts an equirectangular image to a cubemap. @@ -175,7 +175,7 @@ public: SpecularFilter(SpecularFilter const&) = delete; SpecularFilter& operator=(SpecularFilter const&) = delete; SpecularFilter(SpecularFilter&& rhs) noexcept; - SpecularFilter& operator=(SpecularFilter&& rhs); + SpecularFilter& operator=(SpecularFilter&& rhs) noexcept; /** * Generates a prefiltered cubemap. diff --git a/libs/iblprefilter/src/IBLPrefilterContext.cpp b/libs/iblprefilter/src/IBLPrefilterContext.cpp index cc64634636..83fb60ef02 100644 --- a/libs/iblprefilter/src/IBLPrefilterContext.cpp +++ b/libs/iblprefilter/src/IBLPrefilterContext.cpp @@ -142,7 +142,7 @@ IBLPrefilterContext::IBLPrefilterContext(IBLPrefilterContext&& rhs) noexcept this->operator=(std::move(rhs)); } -IBLPrefilterContext& IBLPrefilterContext::operator=(IBLPrefilterContext&& rhs) { +IBLPrefilterContext& IBLPrefilterContext::operator=(IBLPrefilterContext&& rhs) noexcept { using std::swap; if (this != & rhs) { swap(mRenderer, rhs.mRenderer); @@ -182,7 +182,7 @@ IBLPrefilterContext::EquirectangularToCubemap::EquirectangularToCubemap( IBLPrefilterContext::EquirectangularToCubemap& IBLPrefilterContext::EquirectangularToCubemap::operator=( - IBLPrefilterContext::EquirectangularToCubemap&& rhs) { + IBLPrefilterContext::EquirectangularToCubemap&& rhs) noexcept { using std::swap; if (this != &rhs) { swap(mEquirectMaterial, rhs.mEquirectMaterial); @@ -332,7 +332,8 @@ IBLPrefilterContext::SpecularFilter::SpecularFilter(SpecularFilter&& rhs) noexce this->operator=(std::move(rhs)); } -IBLPrefilterContext::SpecularFilter& IBLPrefilterContext::SpecularFilter::operator=(SpecularFilter&& rhs) { +IBLPrefilterContext::SpecularFilter& +IBLPrefilterContext::SpecularFilter::operator=(SpecularFilter&& rhs) noexcept { using std::swap; if (this != & rhs) { swap(mKernelMaterial, rhs.mKernelMaterial);