From d784ce311f839d3288f716a8ddbf2f963bc61010 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 8 Oct 2024 20:39:14 -0700 Subject: [PATCH] gles: add a flag to enable the validation of the nativewindow We are seeing a cluster of crashes that could be due to using an EGLSurface whose ANativeWindow has become invalid. This could happen if we continued to use (i.e. draw with) an EGLSurface after SurfaceHolder::onSurfaceDestroyed() has returned. This new flag enables an assertion that the native window is valid at the time of makeCurrent(), which happens early in the frame. BUG=[330392256] --- .../filament-android/src/main/cpp/Engine.cpp | 3 +- .../com/google/android/filament/Engine.java | 21 ++++-- filament/backend/include/backend/Platform.h | 7 ++ .../backend/platforms/PlatformEGLAndroid.h | 9 +++ .../src/opengl/platforms/PlatformEGL.cpp | 2 + .../opengl/platforms/PlatformEGLAndroid.cpp | 65 +++++++++++++++++-- filament/include/filament/Engine.h | 8 ++- filament/src/details/Engine.cpp | 4 +- 8 files changed, 105 insertions(+), 14 deletions(-) diff --git a/android/filament-android/src/main/cpp/Engine.cpp b/android/filament-android/src/main/cpp/Engine.cpp index 314472557f..7de25a58ba 100644 --- a/android/filament-android/src/main/cpp/Engine.cpp +++ b/android/filament-android/src/main/cpp/Engine.cpp @@ -525,7 +525,7 @@ extern "C" JNIEXPORT void JNICALL Java_com_google_android_filament_Engine_nSetBu jlong resourceAllocatorCacheSizeMB, jlong resourceAllocatorCacheMaxAge, jboolean disableHandleUseAfterFreeCheck, jint preferredShaderLanguage, - jboolean forceGLES2Context) { + jboolean forceGLES2Context, jboolean assertNativeWindowIsValid) { Engine::Builder* builder = (Engine::Builder*) nativeBuilder; Engine::Config config = { .commandBufferSizeMB = (uint32_t) commandBufferSizeMB, @@ -542,6 +542,7 @@ extern "C" JNIEXPORT void JNICALL Java_com_google_android_filament_Engine_nSetBu .disableHandleUseAfterFreeCheck = (bool) disableHandleUseAfterFreeCheck, .preferredShaderLanguage = (Engine::Config::ShaderLanguage) preferredShaderLanguage, .forceGLES2Context = (bool) forceGLES2Context, + .assertNativeWindowIsValid = (bool) assertNativeWindowIsValid, }; builder->config(&config); } diff --git a/android/filament-android/src/main/java/com/google/android/filament/Engine.java b/android/filament-android/src/main/java/com/google/android/filament/Engine.java index 3b869c779d..a433a55d8c 100644 --- a/android/filament-android/src/main/java/com/google/android/filament/Engine.java +++ b/android/filament-android/src/main/java/com/google/android/filament/Engine.java @@ -229,7 +229,7 @@ public class Engine { config.resourceAllocatorCacheSizeMB, config.resourceAllocatorCacheMaxAge, config.disableHandleUseAfterFreeCheck, config.preferredShaderLanguage.ordinal(), - config.forceGLES2Context); + config.forceGLES2Context, config.assertNativeWindowIsValid); return this; } @@ -418,12 +418,12 @@ public class Engine { */ public long stereoscopicEyeCount = 2; - /* + /** * @Deprecated This value is no longer used. */ public long resourceAllocatorCacheSizeMB = 64; - /* + /** * This value determines how many frames texture entries are kept for in the cache. This * is a soft limit, meaning some texture older than this are allowed to stay in the cache. * Typically only one texture is evicted per frame. @@ -431,12 +431,12 @@ public class Engine { */ public long resourceAllocatorCacheMaxAge = 1; - /* + /** * Disable backend handles use-after-free checks. */ public boolean disableHandleUseAfterFreeCheck = false; - /* + /** * Sets a preferred shader language for Filament to use. * * The Metal backend supports two shader languages: MSL (Metal Shading Language) and @@ -458,12 +458,19 @@ public class Engine { }; public ShaderLanguage preferredShaderLanguage = ShaderLanguage.DEFAULT; - /* + /** * When the OpenGL ES backend is used, setting this value to true will force a GLES2.0 * context if supported by the Platform, or if not, will have the backend pretend * it's a GLES2 context. Ignored on other backends. */ public boolean forceGLES2Context = false; + + /** + * Assert the native window associated to a SwapChain is valid when calling makeCurrent(). + * This is only supported for: + * - PlatformEGLAndroid + */ + public boolean assertNativeWindowIsValid = false; } private Engine(long nativeEngine, Config config) { @@ -1409,7 +1416,7 @@ public class Engine { long resourceAllocatorCacheSizeMB, long resourceAllocatorCacheMaxAge, boolean disableHandleUseAfterFreeCheck, int preferredShaderLanguage, - boolean forceGLES2Context); + boolean forceGLES2Context, boolean assertNativeWindowIsValid); private static native void nSetBuilderFeatureLevel(long nativeBuilder, int ordinal); private static native void nSetBuilderSharedContext(long nativeBuilder, long sharedContext); private static native void nSetBuilderPaused(long nativeBuilder, boolean paused); diff --git a/filament/backend/include/backend/Platform.h b/filament/backend/include/backend/Platform.h index af18b1488a..f9adc51a62 100644 --- a/filament/backend/include/backend/Platform.h +++ b/filament/backend/include/backend/Platform.h @@ -91,6 +91,13 @@ public: * Sets the technique for stereoscopic rendering. */ StereoscopicType stereoscopicType = StereoscopicType::NONE; + + /** + * Assert the native window associated to a SwapChain is valid when calling makeCurrent(). + * This is only supported for: + * - PlatformEGLAndroid + */ + bool assertNativeWindowIsValid = false; }; Platform() noexcept; diff --git a/filament/backend/include/backend/platforms/PlatformEGLAndroid.h b/filament/backend/include/backend/platforms/PlatformEGLAndroid.h index c3cc7da89d..bc7d0c1f2f 100644 --- a/filament/backend/include/backend/platforms/PlatformEGLAndroid.h +++ b/filament/backend/include/backend/platforms/PlatformEGLAndroid.h @@ -89,6 +89,11 @@ protected: */ AcquiredImage transformAcquiredImage(AcquiredImage source) noexcept override; +protected: + bool makeCurrent(ContextType type, + SwapChain* drawSwapChain, + SwapChain* readSwapChain) noexcept override; + private: struct InitializeJvmForPerformanceManagerIfNeeded { InitializeJvmForPerformanceManagerIfNeeded(); @@ -102,6 +107,10 @@ private: using clock = std::chrono::high_resolution_clock; clock::time_point mStartTimeOfActualWork; + + void* mNativeWindowLib = nullptr; + int32_t (*ANativeWindow_getBuffersDefaultDataSpace)(ANativeWindow* window) = nullptr; + bool mAssertNativeWindowIsValid = false; }; } // namespace filament::backend diff --git a/filament/backend/src/opengl/platforms/PlatformEGL.cpp b/filament/backend/src/opengl/platforms/PlatformEGL.cpp index 0a8f638a73..38b8b9d504 100644 --- a/filament/backend/src/opengl/platforms/PlatformEGL.cpp +++ b/filament/backend/src/opengl/platforms/PlatformEGL.cpp @@ -553,6 +553,8 @@ void PlatformEGL::destroySwapChain(Platform::SwapChain* swapChain) noexcept { if (swapChain) { SwapChainEGL const* const sc = static_cast(swapChain); if (sc->sur != EGL_NO_SURFACE) { + // - if EGL_KHR_surfaceless_context is supported, mEGLDummySurface is EGL_NO_SURFACE. + // - this is actually a bit too aggressive, but it is a rare operation. egl.makeCurrent(mEGLDummySurface, mEGLDummySurface); eglDestroySurface(mEGLDisplay, sc->sur); delete sc; diff --git a/filament/backend/src/opengl/platforms/PlatformEGLAndroid.cpp b/filament/backend/src/opengl/platforms/PlatformEGLAndroid.cpp index 577b66fb31..f7dce97903 100644 --- a/filament/backend/src/opengl/platforms/PlatformEGLAndroid.cpp +++ b/filament/backend/src/opengl/platforms/PlatformEGLAndroid.cpp @@ -25,12 +25,14 @@ #include "ExternalStreamManagerAndroid.h" #include +#include #include #include #include #include +#include #include #include @@ -42,7 +44,9 @@ #include #include +#include +#include #include #include @@ -80,8 +84,6 @@ UTILS_PRIVATE PFNEGLGETFRAMETIMESTAMPSANDROIDPROC eglGetFrameTimestampsANDROID = } using namespace glext; -using EGLStream = Platform::Stream; - // --------------------------------------------------------------------------------------------- PlatformEGLAndroid::InitializeJvmForPerformanceManagerIfNeeded::InitializeJvmForPerformanceManagerIfNeeded() { @@ -112,6 +114,13 @@ PlatformEGLAndroid::PlatformEGLAndroid() noexcept mOSVersion = length >= 0 ? atoi(scratch) : 1; } + mNativeWindowLib = dlopen("libnativewindow.so", RTLD_LOCAL | RTLD_NOW); + if (mNativeWindowLib) { + ANativeWindow_getBuffersDefaultDataSpace = + (int32_t(*)(ANativeWindow*))dlsym(mNativeWindowLib, + "ANativeWindow_getBuffersDefaultDataSpace"); + } + // This disables an ANGLE optimization on ARM, which turns out to be more costly for us // see b/229017581 // We need to do this before we create the GL context. @@ -125,14 +134,60 @@ PlatformEGLAndroid::PlatformEGLAndroid() noexcept setenv("ANGLE_FEATURE_OVERRIDES_DISABLED", "preferSubmitAtFBOBoundary", false); } -PlatformEGLAndroid::~PlatformEGLAndroid() noexcept = default; - +PlatformEGLAndroid::~PlatformEGLAndroid() noexcept { + if (mNativeWindowLib) { + dlclose(mNativeWindowLib); + } +} void PlatformEGLAndroid::terminate() noexcept { ExternalStreamManagerAndroid::destroy(&mExternalStreamManager); PlatformEGL::terminate(); } +static constexpr const std::string_view kNativeWindowInvalidMsg = + "ANativeWindow is invalid. It probably has been destroyed. EGL surface = "; + +bool PlatformEGLAndroid::makeCurrent(ContextType type, + SwapChain* drawSwapChain, + SwapChain* readSwapChain) noexcept { + + // fast & safe path + if (UTILS_LIKELY(!mAssertNativeWindowIsValid)) { + return PlatformEGL::makeCurrent(type, drawSwapChain, readSwapChain); + } + + SwapChainEGL const* const dsc = static_cast(drawSwapChain); + if (ANativeWindow_getBuffersDefaultDataSpace) { + // anw can be nullptr if we're using a pbuffer surface + if (UTILS_LIKELY(dsc->nativeWindow)) { + // this a proxy of is_valid() + auto result = ANativeWindow_getBuffersDefaultDataSpace(dsc->nativeWindow); + FILAMENT_CHECK_POSTCONDITION(result >= 0) << kNativeWindowInvalidMsg << dsc->sur; + } + } else { + // If we don't have ANativeWindow_getBuffersDefaultDataSpace, we revert to using the + // private query() call. + // Shadow version if the real ANativeWindow, so we can access the query() hook. Query + // has existed since forever, probably Android 1.0. + struct NativeWindow { + // is valid query enum value + enum { IS_VALID = 17 }; + uint64_t pad[18]; + int (* query)(ANativeWindow const*, int, int*); + } const* pWindow = reinterpret_cast(dsc->nativeWindow); + int isValid = 0; + if (UTILS_LIKELY(pWindow->query)) { // just in case it's nullptr + int const err = pWindow->query(dsc->nativeWindow, NativeWindow::IS_VALID, &isValid); + if (UTILS_LIKELY(err >= 0)) { // in case the IS_VALID enum is not recognized + // query call succeeded + FILAMENT_CHECK_POSTCONDITION(isValid) << kNativeWindowInvalidMsg << dsc->sur; + } + } + } + return PlatformEGL::makeCurrent(type, drawSwapChain, readSwapChain); +} + void PlatformEGLAndroid::beginFrame( int64_t monotonic_clock_ns, int64_t refreshIntervalNs, @@ -189,6 +244,8 @@ Driver* PlatformEGLAndroid::createDriver(void* sharedContext, "eglGetFrameTimestampsANDROID"); } + mAssertNativeWindowIsValid = driverConfig.assertNativeWindowIsValid; + return driver; } diff --git a/filament/include/filament/Engine.h b/filament/include/filament/Engine.h index 2686f25ea1..311cf2e5a0 100644 --- a/filament/include/filament/Engine.h +++ b/filament/include/filament/Engine.h @@ -380,8 +380,14 @@ public: * it's a GLES2 context. Ignored on other backends. */ bool forceGLES2Context = false; - }; + /** + * Assert the native window associated to a SwapChain is valid when calling makeCurrent(). + * This is only supported for: + * - PlatformEGLAndroid + */ + bool assertNativeWindowIsValid = false; + }; #if UTILS_HAS_THREADING using CreateCallback = void(void* UTILS_NULLABLE user, void* UTILS_NONNULL token); diff --git a/filament/src/details/Engine.cpp b/filament/src/details/Engine.cpp index b1aaa5c9f5..e62a94d030 100644 --- a/filament/src/details/Engine.cpp +++ b/filament/src/details/Engine.cpp @@ -107,7 +107,8 @@ Engine* FEngine::create(Engine::Builder const& builder) { .disableParallelShaderCompile = instance->getConfig().disableParallelShaderCompile, .disableHandleUseAfterFreeCheck = instance->getConfig().disableHandleUseAfterFreeCheck, .forceGLES2Context = instance->getConfig().forceGLES2Context, - .stereoscopicType = instance->getConfig().stereoscopicType, + .stereoscopicType = instance->getConfig().stereoscopicType, + .assertNativeWindowIsValid = instance->getConfig().assertNativeWindowIsValid, }; instance->mDriver = platform->createDriver(sharedContext, driverConfig); @@ -705,6 +706,7 @@ int FEngine::loop() { .disableHandleUseAfterFreeCheck = mConfig.disableHandleUseAfterFreeCheck, .forceGLES2Context = mConfig.forceGLES2Context, .stereoscopicType = mConfig.stereoscopicType, + .assertNativeWindowIsValid = mConfig.assertNativeWindowIsValid, }; mDriver = mPlatform->createDriver(mSharedGLContext, driverConfig);