From 1b2ab097192f0d648b99bf86f20d0557e005f70e Mon Sep 17 00:00:00 2001 From: Powei Feng Date: Thu, 27 Feb 2025 12:14:22 -0800 Subject: [PATCH] android: prevent leak of cubemap for IndirectLight/Skybox (#8470) We break existing API by returning both the IndirectLight and the texture. Same thing with Skybox. Fix one bug where the float array in getSphericalHarmonics wasn't getting written out. Fixes #6412 --- NEW_RELEASE_NOTES.md | 2 + .../src/main/cpp/Utils.cpp | 44 +++++++------------ .../android/filament/utils/KTX1Loader.kt | 30 +++++++++---- .../android/filament/utils/ModelViewer.kt | 15 ++++++- .../android/filament/gltf/MainActivity.kt | 8 +++- 5 files changed, 59 insertions(+), 40 deletions(-) diff --git a/NEW_RELEASE_NOTES.md b/NEW_RELEASE_NOTES.md index 4a1a9c7fa7..b0b65cb5d2 100644 --- a/NEW_RELEASE_NOTES.md +++ b/NEW_RELEASE_NOTES.md @@ -7,3 +7,5 @@ for next branch cut* header. appropriate header in [RELEASE_NOTES.md](./RELEASE_NOTES.md). ## Release notes for next branch cut + +- android: breaking changes to API KTX1Loader::createIndirectLight and KTX1Loader::createSkybox diff --git a/android/filament-utils-android/src/main/cpp/Utils.cpp b/android/filament-utils-android/src/main/cpp/Utils.cpp index 7dfdaeb65b..bf14e8aae9 100644 --- a/android/filament-utils-android/src/main/cpp/Utils.cpp +++ b/android/filament-utils-android/src/main/cpp/Utils.cpp @@ -43,37 +43,25 @@ static jlong nCreateKTXTexture(JNIEnv* env, jclass, }, bundle); } -static jlong nCreateIndirectLight(JNIEnv* env, jclass, - jlong nativeEngine, jobject javaBuffer, jint remaining, jboolean srgb) { +static jlong nCreateIndirectLight(JNIEnv* env, jclass, jlong nativeEngine, jlong ktxTexture, + jfloatArray sphericalHarmonics) { Engine* engine = (Engine*) nativeEngine; - AutoBuffer buffer(env, javaBuffer, remaining); - Ktx1Bundle* bundle = new Ktx1Bundle((const uint8_t*) buffer.getData(), buffer.getSize()); - Texture* cubemap = Ktx1Reader::createTexture(engine, *bundle, srgb, [](void* userdata) { - Ktx1Bundle* bundle = (Ktx1Bundle*) userdata; - delete bundle; - }, bundle); - - float3 harmonics[9]; - bundle->getSphericalHarmonics(harmonics); - - IndirectLight* indirectLight = IndirectLight::Builder() - .reflections(cubemap) - .irradiance(3, harmonics) - .intensity(30000) - .build(*engine); + Texture* cubemap = (Texture*) ktxTexture; + jfloat* harmonics = env->GetFloatArrayElements(sphericalHarmonics, nullptr); + IndirectLight* indirectLight = + IndirectLight::Builder() + .reflections(cubemap) + .irradiance(3, reinterpret_cast(harmonics)) + .intensity(30000) + .build(*engine); + env->ReleaseFloatArrayElements(sphericalHarmonics, harmonics, JNI_ABORT); return (jlong) indirectLight; } -static jlong nCreateSkybox(JNIEnv* env, jclass, - jlong nativeEngine, jobject javaBuffer, jint remaining, jboolean srgb) { +static jlong nCreateSkybox(JNIEnv* env, jclass, jlong nativeEngine, jlong ktxTexture) { Engine* engine = (Engine*) nativeEngine; - AutoBuffer buffer(env, javaBuffer, remaining); - Ktx1Bundle* bundle = new Ktx1Bundle((const uint8_t*) buffer.getData(), buffer.getSize()); - Texture* cubemap = Ktx1Reader::createTexture(engine, *bundle, srgb, [](void* userdata) { - Ktx1Bundle* bundle = (Ktx1Bundle*) userdata; - delete bundle; - }, bundle); + Texture* cubemap = (Texture*) ktxTexture; return (jlong) Skybox::Builder().environment(cubemap).showSun(true).build(*engine); } @@ -86,7 +74,7 @@ static jboolean nGetSphericalHarmonics(JNIEnv* env, jclass, jobject javaBuffer, const auto success = bundle.getSphericalHarmonics( reinterpret_cast(outSphericalHarmonics) ); - env->ReleaseFloatArrayElements(outSphericalHarmonics_, outSphericalHarmonics, JNI_ABORT); + env->ReleaseFloatArrayElements(outSphericalHarmonics_, outSphericalHarmonics, 0); return success ? JNI_TRUE : JNI_FALSE; } @@ -104,8 +92,8 @@ JNIEXPORT jint JNI_OnLoad(JavaVM* vm, void*) { if (ktxloaderClass == nullptr) return JNI_ERR; static const JNINativeMethod ktxMethods[] = { {(char*)"nCreateKTXTexture", (char*)"(JLjava/nio/Buffer;IZ)J", reinterpret_cast(nCreateKTXTexture)}, - {(char*)"nCreateIndirectLight", (char*)"(JLjava/nio/Buffer;IZ)J", reinterpret_cast(nCreateIndirectLight)}, - {(char*)"nCreateSkybox", (char*)"(JLjava/nio/Buffer;IZ)J", reinterpret_cast(nCreateSkybox)}, + {(char*)"nCreateIndirectLight", (char*)"(JJ[F)J", reinterpret_cast(nCreateIndirectLight)}, + {(char*)"nCreateSkybox", (char*)"(JJ)J", reinterpret_cast(nCreateSkybox)}, {(char*)"nGetSphericalHarmonics", (char*)"(Ljava/nio/Buffer;I[F)Z", reinterpret_cast(nGetSphericalHarmonics)}, }; rc = env->RegisterNatives(ktxloaderClass, ktxMethods, sizeof(ktxMethods) / sizeof(JNINativeMethod)); diff --git a/android/filament-utils-android/src/main/java/com/google/android/filament/utils/KTX1Loader.kt b/android/filament-utils-android/src/main/java/com/google/android/filament/utils/KTX1Loader.kt index d221e59cc6..3deb17e86c 100644 --- a/android/filament-utils-android/src/main/java/com/google/android/filament/utils/KTX1Loader.kt +++ b/android/filament-utils-android/src/main/java/com/google/android/filament/utils/KTX1Loader.kt @@ -34,6 +34,12 @@ object KTX1Loader { var srgb = false } + class IndirectLightBundle(val indirectLight: IndirectLight? = null, val cubemap: Texture? = null) { + } + + class SkyboxBundle(val skybox: Skybox? = null, val cubemap: Texture? = null) { + } + /** * Consumes the content of a KTX file and produces a [Texture] object. * @@ -56,10 +62,15 @@ object KTX1Loader { * @param options Loader options. * @return The resulting Filament texture, or null on failure. */ - fun createIndirectLight(engine: Engine, buffer: Buffer, options: Options = Options()): IndirectLight { + fun createIndirectLight(engine: Engine, buffer: Buffer, options: Options = Options()): IndirectLightBundle { val nativeEngine = engine.nativeObject - val nativeIndirectLight = nCreateIndirectLight(nativeEngine, buffer, buffer.remaining(), options.srgb) - return IndirectLight(nativeIndirectLight) + val sphericalHarmonics = getSphericalHarmonics(buffer) + if (sphericalHarmonics == null) { + return IndirectLightBundle() + } + val ktxTexture = createTexture(engine, buffer, options) + val nativeIndirectLight = nCreateIndirectLight(nativeEngine, ktxTexture.nativeObject, sphericalHarmonics) + return IndirectLightBundle(IndirectLight(nativeIndirectLight), ktxTexture) } /** @@ -70,10 +81,11 @@ object KTX1Loader { * @param options Loader options. * @return The resulting Filament texture, or null on failure. */ - fun createSkybox(engine: Engine, buffer: Buffer, options: Options = Options()): Skybox { + fun createSkybox(engine: Engine, buffer: Buffer, options: Options = Options()): SkyboxBundle { val nativeEngine = engine.nativeObject - val nativeSkybox = nCreateSkybox(nativeEngine, buffer, buffer.remaining(), options.srgb) - return Skybox(nativeSkybox) + val ktxTexture = createTexture(engine, buffer, options) + val nativeSkybox = nCreateSkybox(nativeEngine, ktxTexture.nativeObject) + return SkyboxBundle(Skybox(nativeSkybox), ktxTexture) } /** @@ -89,7 +101,7 @@ object KTX1Loader { } private external fun nCreateKTXTexture(nativeEngine: Long, buffer: Buffer, remaining: Int, srgb: Boolean): Long - private external fun nCreateIndirectLight(nativeEngine: Long, buffer: Buffer, remaining: Int, srgb: Boolean): Long + private external fun nCreateIndirectLight(nativeEngine: Long, ktxTexture: Long, sphericalHarmonics: FloatArray) : Long private external fun nGetSphericalHarmonics(buffer: Buffer, remaining: Int, outSphericalHarmonics: FloatArray): Boolean - private external fun nCreateSkybox(nativeEngine: Long, buffer: Buffer, remaining: Int, srgb: Boolean): Long -} \ No newline at end of file + private external fun nCreateSkybox(nativeEngine: Long, ktxTexture: Long) : Long +} diff --git a/android/filament-utils-android/src/main/java/com/google/android/filament/utils/ModelViewer.kt b/android/filament-utils-android/src/main/java/com/google/android/filament/utils/ModelViewer.kt index 2f11466b49..90e42656d5 100644 --- a/android/filament-utils-android/src/main/java/com/google/android/filament/utils/ModelViewer.kt +++ b/android/filament-utils-android/src/main/java/com/google/android/filament/utils/ModelViewer.kt @@ -98,6 +98,9 @@ class ModelViewer( val renderer: Renderer @Entity val light: Int + var indirectLightCubemap: Texture? = null + var skyboxCubemap: Texture? = null + private lateinit var displayHelper: DisplayHelper private lateinit var cameraManipulator: Manipulator private lateinit var gestureDetector: GestureDetector @@ -332,6 +335,16 @@ class ModelViewer( materialProvider.destroy() resourceLoader.destroy() + if (indirectLightCubemap != null) { + engine.destroyTexture(indirectLightCubemap!!) + indirectLightCubemap = null + } + + if (skyboxCubemap != null) { + engine.destroyTexture(skyboxCubemap!!) + skyboxCubemap = null + } + engine.destroyEntity(light) engine.destroyRenderer(renderer) engine.destroyView(this@ModelViewer.view) @@ -421,4 +434,4 @@ class ModelViewer( companion object { private val kDefaultObjectPosition = Float3(0.0f, 0.0f, -4.0f) } -} \ No newline at end of file +} 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 102c4944e6..8322cbbb88 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 @@ -156,12 +156,16 @@ class MainActivity : Activity() { val scene = modelViewer.scene val ibl = "default_env" readCompressedAsset("envs/$ibl/${ibl}_ibl.ktx").let { - scene.indirectLight = KTX1Loader.createIndirectLight(engine, it) + val bundle = KTX1Loader.createIndirectLight(engine, it) + scene.indirectLight = bundle.indirectLight + modelViewer.indirectLightCubemap = bundle.cubemap scene.indirectLight!!.intensity = 30_000.0f viewerContent.indirectLight = modelViewer.scene.indirectLight } readCompressedAsset("envs/$ibl/${ibl}_skybox.ktx").let { - scene.skybox = KTX1Loader.createSkybox(engine, it) + val bundle = KTX1Loader.createSkybox(engine, it) + scene.skybox = bundle.skybox + modelViewer.skyboxCubemap = bundle.cubemap } }