From 9c2f3bcf79e2d177e77eec8151521db41565c5fb Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Wed, 25 Oct 2017 17:00:57 +0100 Subject: [PATCH] deviceinfo: Do the device instance querying on a separate thread. The queries for GL information was corrupting the currently bound EGL context. Instead of jumping through hoops to store and restore the context, just do the query on a separate throw-away thread. Also clean up the messy logic in `Spy::get()` which was unnecessarily complicated and can now be simplified due to the explicit installation of the interceptor. Fixes some traces where contexts were corrupted when traced. --- core/os/device/deviceinfo/cc/android/jni.cpp | 4 +- .../os/device/deviceinfo/cc/android/query.cpp | 28 ++++++- core/os/device/deviceinfo/cc/query.cpp | 25 ++++-- gapii/cc/spy.cpp | 77 +++++++------------ 4 files changed, 75 insertions(+), 59 deletions(-) diff --git a/core/os/device/deviceinfo/cc/android/jni.cpp b/core/os/device/deviceinfo/cc/android/jni.cpp index c0d793418b..7e7437daed 100644 --- a/core/os/device/deviceinfo/cc/android/jni.cpp +++ b/core/os/device/deviceinfo/cc/android/jni.cpp @@ -21,7 +21,9 @@ extern "C" { jbyteArray Java_com_google_android_gapid_DeviceInfoService_getDeviceInfo(JNIEnv* env) { - auto device_instance = get_device_instance(env); + JavaVM* vm = nullptr; + env->GetJavaVM(&vm); + auto device_instance = get_device_instance(vm); auto out = env->NewByteArray(device_instance.size); auto data = reinterpret_cast(device_instance.data); env->SetByteArrayRegion(out, 0, device_instance.size, data); diff --git a/core/os/device/deviceinfo/cc/android/query.cpp b/core/os/device/deviceinfo/cc/android/query.cpp index 82310fe5a9..4b04d39dea 100644 --- a/core/os/device/deviceinfo/cc/android/query.cpp +++ b/core/os/device/deviceinfo/cc/android/query.cpp @@ -272,7 +272,29 @@ bool createContext(void* platform_data) { return false; \ } - JNIEnv* env = reinterpret_cast(platform_data); + JavaVM* vm = reinterpret_cast(platform_data); + JNIEnv* env = nullptr; + auto res = vm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6); + bool shouldDetach = false; + switch (res) { + case JNI_OK: + break; + case JNI_EDETACHED: + res = vm->AttachCurrentThread(&env, nullptr); + if (res != 0) { + snprintf(gContext.mError, sizeof(gContext.mError), + "Failed to attach thread to JavaVM. (%d)", res); + destroyContext(); + return false; + } + shouldDetach = true; + break; + default: + snprintf(gContext.mError, sizeof(gContext.mError), + "Failed to get Java env. (%d)", res); + destroyContext(); + return false; + } Class build(env, "android/os/Build"); CHECK(build.get_field("SUPPORTED_ABIS", gContext.mSupportedABIs)); @@ -285,6 +307,10 @@ bool createContext(void* platform_data) { CHECK(version.get_field("RELEASE", gContext.mOSName)); CHECK(version.get_field("SDK_INT", gContext.mOSVersion)); + if (shouldDetach) { + vm->DetachCurrentThread(); + } + #undef CHECK if (gContext.mSupportedABIs.size() > 0) { diff --git a/core/os/device/deviceinfo/cc/query.cpp b/core/os/device/deviceinfo/cc/query.cpp index f725c0cb27..79e109e590 100644 --- a/core/os/device/deviceinfo/cc/query.cpp +++ b/core/os/device/deviceinfo/cc/query.cpp @@ -18,6 +18,8 @@ #include +#include + namespace { inline bool isLittleEndian() { @@ -41,16 +43,12 @@ device::DataTypeLayout* new_dt_layout() { return out; } -} // anonymous namespace - -namespace query { - -device::Instance* getDeviceInstance(void* platform_data) { +void buildDeviceInstance(void* platform_data, device::Instance** out) { using namespace device; using namespace google::protobuf::io; if (!query::createContext(platform_data)) { - return nullptr; + return; } // OS @@ -126,6 +124,21 @@ device::Instance* getDeviceInstance(void* platform_data) { query::destroyContext(); + *out = instance; +} + +} // anonymous namespace + +namespace query { + +device::Instance* getDeviceInstance(void* platform_data) { + device::Instance* instance = nullptr; + + // buildDeviceInstance on a seperate thread to avoid EGL screwing with the + // currently bound context. + std::thread thread(buildDeviceInstance, platform_data, &instance); + thread.join(); + return instance; } diff --git a/gapii/cc/spy.cpp b/gapii/cc/spy.cpp index b43541a1ae..3dcd3058c7 100644 --- a/gapii/cc/spy.cpp +++ b/gapii/cc/spy.cpp @@ -63,25 +63,8 @@ jint JNI_OnLoad(JavaVM *vm, void *reserved) { gapii::Spy::get(); // Construct the spy. return JNI_VERSION_1_6; } -void* queryPlatformData() { - JNIEnv* env = nullptr; - - auto res = gJavaVM->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6); - switch (res) { - case JNI_OK: - break; - case JNI_EDETACHED: - res = gJavaVM->AttachCurrentThread(&env, nullptr); - if (res != 0) { - GAPID_FATAL("Failed to attach thread to JavaVM. (%d)", res); - } - break; - default: - GAPID_FATAL("Failed to get Java env. (%d)", res); - } - GAPID_INFO("queryPlatformData() env = %p", env); - return env; -} + +void* queryPlatformData() { return gJavaVM; } #else // TARGET_OS == GAPID_OS_ANDROID void* queryPlatformData() { return nullptr; } #endif // TARGET_OS == GAPID_OS_ANDROID @@ -137,25 +120,15 @@ thread_local gapii::CallObserver* gContext = nullptr; namespace gapii { Spy* Spy::get() { - bool init; - { - std::lock_guard lock(gMutex); - init = !gSpy; - if (init) { - GAPID_INFO("Constructing spy..."); - gSpy.reset(new Spy()); - GAPID_INFO("Registering spy symbols..."); - for (int i = 0; kGLESExports[i].mName != NULL; ++i) { - gSpy->RegisterSymbol(kGLESExports[i].mName, kGLESExports[i].mFunc); - } + std::lock_guard lock(gMutex); + if (!gSpy) { + GAPID_INFO("Constructing spy..."); + gSpy.reset(new Spy()); + GAPID_INFO("Registering spy symbols..."); + for (int i = 0; kGLESExports[i].mName != NULL; ++i) { + gSpy->RegisterSymbol(kGLESExports[i].mName, kGLESExports[i].mFunc); } } - if (init) { - auto s = gSpy.get(); - s->enter("writeHeader", 0); - s->writeHeader(); - s->exit(); - } return gSpy.get(); } @@ -204,6 +177,20 @@ Spy::Spy() mSuspendCaptureFrames.store((header.mFlags & ConnectionHeader::FLAG_DEFER_START)? kSuspendIndefinitely: mSuspendCaptureFrames.load()); + set_valid_apis(header.mAPIs); + GAPID_ERROR("APIS %08x", header.mAPIs); + GAPID_INFO("GAPII connection established. Settings:"); + GAPID_INFO("Observe framebuffer every %d frames", mObserveFrameFrequency); + GAPID_INFO("Observe framebuffer every %d draws", mObserveDrawFrequency); + GAPID_INFO("Disable precompiled shaders: %s", mDisablePrecompiledShaders ? "true" : "false"); + + mEncoder = gapii::PackEncoder::create(mConnection); + + // writeHeader needs to come before the installer is created as the + // deviceinfo queries want to call into EGL / GL commands which will be + // patched. + writeHeader(); + #if TARGET_OS == GAPID_OS_ANDROID if (strlen(header.mLibInterceptorPath) > 0) { gInstaller = std::unique_ptr(new Installer(header.mLibInterceptorPath)); @@ -214,15 +201,6 @@ Spy::Spy() } #endif // TARGET_OS == GAPID_OS_ANDROID - set_valid_apis(header.mAPIs); - GAPID_ERROR("APIS %08x", header.mAPIs); - GAPID_INFO("GAPII connection established. Settings:"); - GAPID_INFO("Observe framebuffer every %d frames", mObserveFrameFrequency); - GAPID_INFO("Observe framebuffer every %d draws", mObserveDrawFrequency); - GAPID_INFO("Disable precompiled shaders: %s", mDisablePrecompiledShaders ? "true" : "false"); - - mEncoder = gapii::PackEncoder::create(mConnection); - auto context = enter("init", 0); GlesSpy::init(); VulkanSpy::init(); @@ -245,15 +223,11 @@ Spy::Spy() } void Spy::writeHeader() { - if (!query::createContext(queryPlatformData())) { - GAPID_ERROR("query::createContext() errored: %s", query::contextError()); - } capture::Header file_header; file_header.set_version(CurrentCaptureVersion); file_header.set_allocated_device(query::getDeviceInstance(queryPlatformData())); file_header.set_allocated_abi(query::currentABI()); mEncoder->object(&file_header); - query::destroyContext(); } void Spy::resolveImports() { @@ -310,9 +284,10 @@ EGLContext Spy::eglCreateContext(CallObserver* observer, EGLDisplay display, EGL auto res = GlesSpy::eglCreateContext(observer, display, config, share_context, attrib_vector.data()); // NB: The getters modify the std::map, so this log must be last. - GAPID_INFO("eglCreateContext requested: GL %i.%i, profile 0x%x, flags 0x%x", + GAPID_INFO("eglCreateContext requested: GL %i.%i, profile 0x%x, flags 0x%x -> %p", attribs[EGL_CONTEXT_MAJOR_VERSION_KHR], attribs[EGL_CONTEXT_MINOR_VERSION_KHR], - attribs[EGL_CONTEXT_OPENGL_PROFILE_MASK_KHR], attribs[EGL_CONTEXT_FLAGS_KHR]); + attribs[EGL_CONTEXT_OPENGL_PROFILE_MASK_KHR], attribs[EGL_CONTEXT_FLAGS_KHR], + res); return res; }