Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GraphicsSpy Layer Fixes for Q and Beyond #2611

Merged
merged 7 commits into from
Feb 27, 2019
Merged

GraphicsSpy Layer Fixes for Q and Beyond #2611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f65a652
Don't require a JNI VM for the Android device info.
pmuetschard Feb 15, 2019
c697aee
Be more consistent with the layer library names.
pmuetschard Feb 15, 2019
6cf78bb
Rename the VkGraphicsSpy layer to simply GraphicsSpy.
pmuetschard Feb 15, 2019
57c3ce7
Remove Windows support from the graphics spy layer.
pmuetschard Feb 15, 2019
47a1592
Fix pointer formatting to be consistent with codebase.
pmuetschard Feb 16, 2019
fb88d65
Implement some functions in GraphicsSpy directly.
pmuetschard Feb 15, 2019
7f7b116
Load libgapii in GraphicsSpy if it has not been loaded already.
pmuetschard Feb 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/device-info/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ int main(int argc, char const *argv[]) {
}
}

device_instance instance = get_device_instance(nullptr);
device_instance instance = get_device_instance();
if (output_binary) {
#if _WIN32
_setmode(_fileno(stdout), _O_BINARY);
Expand Down
4 changes: 1 addition & 3 deletions core/os/device/deviceinfo/cc/android/jni.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ extern "C" {

jbyteArray Java_com_google_android_gapid_DeviceInfoService_getDeviceInfo(
JNIEnv* env) {
JavaVM* vm = nullptr;
env->GetJavaVM(&vm);
auto device_instance = get_device_instance(vm);
auto device_instance = get_device_instance();
auto out = env->NewByteArray(device_instance.size);
auto data = reinterpret_cast<jbyte*>(device_instance.data);
env->SetByteArrayRegion(out, 0, device_instance.size, data);
Expand Down
112 changes: 0 additions & 112 deletions core/os/device/deviceinfo/cc/android/jni_helpers.h
View file
Open in desktop

This file was deleted.

90 changes: 34 additions & 56 deletions core/os/device/deviceinfo/cc/android/query.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

#include "egl_lite.h"
#include "jni_helpers.h"

#include "../query.h"

Expand All @@ -24,9 +23,10 @@
#include "core/cc/log.h"

#include <cstring>
#include <sstream>

#include <android/log.h>
#include <jni.h>
#include <sys/system_properties.h>

#define LOG_ERR(...) \
__android_log_print(ANDROID_LOG_ERROR, "GAPID", __VA_ARGS__);
Expand Down Expand Up @@ -176,7 +176,7 @@ void destroyContext() {
}
}

bool createContext(void* platform_data) {
bool createContext() {
if (gContextRefCount++ > 0) {
return true;
}
Expand All @@ -186,12 +186,6 @@ bool createContext(void* platform_data) {
gContext.mContext = nullptr;
gContext.mNumCores = 0;

if (platform_data == nullptr) {
snprintf(gContext.mError, sizeof(gContext.mError),
"platform_data was nullptr");
return false;
}

#define RESOLVE(name, pfun) \
auto name = reinterpret_cast<pfun>(core::GetGlesProcAddress(#name)); \
GAPID_ASSERT(name != nullptr)
Expand Down Expand Up @@ -271,48 +265,39 @@ bool createContext(void* platform_data) {

#undef CHECK

#define CHECK(x) \
if (!x) { \
snprintf(gContext.mError, sizeof(gContext.mError), "JNI error:\n " #x); \
destroyContext(); \
return false; \
}

JavaVM* vm = reinterpret_cast<JavaVM*>(platform_data);
JNIEnv* env = nullptr;
auto res = vm->GetEnv(reinterpret_cast<void**>(&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;
}
#define GET_PROP(name, trans) \
do { \
char _v[PROP_VALUE_MAX] = {0}; \
if (__system_property_get(name, _v) == 0) { \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC this is typically a method we shouldn't get calling?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just say ben's comment below, this is basically the same.

snprintf(gContext.mError, sizeof(gContext.mError), \
"Failed reading property %s", name); \
destroyContext(); \
return false; \
} \
trans; \
} while (0)

#define GET_STRING_PROP(n, t) GET_PROP(n, t = _v)
#define GET_INT_PROP(n, t) GET_PROP(n, t = atoi(_v))
#define GET_STRING_LIST_PROP(n, t) \
do { \
std::string _l, _t; \
GET_STRING_PROP(n, _l); \
std::istringstream _s(_l); \
while (std::getline(_s, _t, ',')) { \
t.push_back(_t); \
} \
} while (0)

std::string manufacturer;
std::string model;

Class build(env, "android/os/Build");
CHECK(build.get_field("SUPPORTED_ABIS", gContext.mSupportedABIs));
CHECK(build.get_field("HOST", gContext.mHost));
CHECK(build.get_field("SERIAL", gContext.mSerial));
CHECK(build.get_field("MANUFACTURER", manufacturer));
CHECK(build.get_field("MODEL", model));
CHECK(build.get_field("HARDWARE", gContext.mHardware));
CHECK(build.get_field("DISPLAY", gContext.mOSBuild));
GET_STRING_LIST_PROP("ro.product.cpu.abilist", gContext.mSupportedABIs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be sure to test this on older versions of Android and with 3rd party devices. IIRC, there was something preventing me from using system properties to get this info, hence the JNI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://developer.android.com/about/versions/nougat/android-7.0-changes __system_property_get is the function to call. The warning there is simply that there is no standard specifying the names and values of the existing properties. However, the properties I'm querying here are the same as the ones queries by android.os.Build. BTW, the native code behind android.os.Build also uses the __system_property_* functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the issue was with the property names, more that the function was simply not available on all versions of Android.
To be honest, I'm slightly surprised that this is accessible at all, as I thought there was an effort to classify this API as private (non-NDK-public). These release notes appear to suggest otherwise though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are probably referring to this: https://stackoverflow.com/questions/28413530/api-to-get-android-system-properties-is-removed-in-arm64-platforms
As far as I can make out, it may at one point have been the idea to remove them, but that idea seems to have been overturned. There is a lot of Android code that depends on those functions to be present and accessible to apps. With those release notes, they are now guaranteed at the API level we build for.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the 7.0 docs, I would agree with Pascal that these are now officially supported only because of the wording:

Use __system_property_get instead of the private property_get

Which implies that __system_property_get is public. So I think this should be fine.

GET_STRING_PROP("ro.build.host", gContext.mHost);
GET_STRING_PROP("ro.product.manufacturer", manufacturer);
GET_STRING_PROP("ro.product.model", model);
GET_STRING_PROP("ro.hardware", gContext.mHardware);
GET_STRING_PROP("ro.build.display.id", gContext.mOSBuild);

if (model != "") {
if (manufacturer != "") {
Expand All @@ -322,15 +307,8 @@ bool createContext(void* platform_data) {
}
}

Class version(env, "android/os/Build$VERSION");
CHECK(version.get_field("RELEASE", gContext.mOSName));
CHECK(version.get_field("SDK_INT", gContext.mOSVersion));

if (shouldDetach) {
vm->DetachCurrentThread();
}

#undef CHECK
GET_STRING_PROP("ro.build.version.release", gContext.mOSName);
GET_INT_PROP("ro.build.version.sdk", gContext.mOSVersion);

if (gContext.mSupportedABIs.size() > 0) {
auto primaryABI = gContext.mSupportedABIs[0];
Expand Down
4 changes: 2 additions & 2 deletions core/os/device/deviceinfo/cc/instance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@

extern "C" {

device_instance get_device_instance(void* platform_data) {
device_instance get_device_instance() {
device_instance out = {};

query::Option query_opt;
query_opt.vulkan.set_query_layers_and_extensions(true)
.set_query_physical_devices(true);
auto instance = query::getDeviceInstance(query_opt, platform_data);
auto instance = query::getDeviceInstance(query_opt);
if (!instance) {
return out;
}
Expand Down
2 changes: 1 addition & 1 deletion core/os/device/deviceinfo/cc/instance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ typedef struct {
size_t size;
} device_instance;

device_instance get_device_instance(void* platform_data);
device_instance get_device_instance();
const char* get_device_instance_error();
void free_device_instance(device_instance);

Expand Down
2 changes: 1 addition & 1 deletion core/os/device/deviceinfo/cc/linux/query.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ void createGlContext() {
gContext.mPbuffer, gContext.mGlCtx);
}

bool createContext(void* platform_data) {
bool createContext() {
if (gContextRefCount++ > 0) {
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion core/os/device/deviceinfo/cc/osx/query.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ void createGlContext() {
}
}

bool createContext(void* platform_data) {
bool createContext() {
if (gContextRefCount++ > 0) {
return true;
}
Expand Down
9 changes: 4 additions & 5 deletions core/os/device/deviceinfo/cc/query.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,11 @@ void deviceInstanceID(device::Instance* instance) {
delete[] proto_data;
}

void buildDeviceInstance(const query::Option& opt, void* platform_data,
device::Instance** out) {
void buildDeviceInstance(const query::Option& opt, device::Instance** out) {
using namespace device;
using namespace google::protobuf::io;

if (!query::createContext(platform_data)) {
if (!query::createContext()) {
return;
}

Expand Down Expand Up @@ -181,12 +180,12 @@ void buildDeviceInstance(const query::Option& opt, void* platform_data,

namespace query {

device::Instance* getDeviceInstance(const Option& opt, void* platform_data) {
device::Instance* getDeviceInstance(const Option& opt) {
device::Instance* instance = nullptr;

// buildDeviceInstance on a separate thread to avoid EGL screwing with the
// currently bound context.
std::thread thread(buildDeviceInstance, opt, platform_data, &instance);
std::thread thread(buildDeviceInstance, opt, &instance);
thread.join();
return instance;
}
Expand Down
12 changes: 6 additions & 6 deletions core/os/device/deviceinfo/cc/query.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ struct Option {

// getDeviceInstance returns the device::Instance proto message for the
// current device. It must be freed with delete.
device::Instance* getDeviceInstance(const Option& opt, void* platform_data);
device::Instance* getDeviceInstance(const Option& opt);

// updateVulkanPhysicalDevices modifies the given device::Instance by adding
// device::VulkanPhysicalDevice to the device::Instance. If a
Expand All @@ -76,10 +76,10 @@ device::Instance* getDeviceInstance(const Option& opt, void* platform_data);
// ID re-hashed with the new content. Returns true if Vulkan physical device
// info is fetched successfully and device::Instance updated, otherwise returns
// false and keeps the device::Instance unchanged. Caution: When called with
// VkGraphicsSpy layer loaded i.e. during tracing, the function pointer to a
// layer under VkGraphicsSpy must be passed in. Resolving the Vulkan function
// addresses from loader will cause a infinite calling stack and may deadlock
// in the loader.
// GraphicsSpy layer loaded i.e. during tracing, the function pointer to a layer
// under GraphicsSpy must be passed in. Resolving the Vulkan function addresses
// from loader will cause a infinite calling stack and may deadlock in the
// loader.
bool updateVulkanDriver(
device::Instance* inst, size_t vk_inst_handle = 0,
std::function<void*(size_t, const char*)> get_inst_proc_addr = nullptr);
Expand Down Expand Up @@ -112,7 +112,7 @@ bool hasGLorGLES();
// The functions below are used by getDeviceInstance(), and are implemented
// in the target-dependent sub-directories.

bool createContext(void* platform_data);
bool createContext();
const char* contextError();
void destroyContext();

Expand Down
2 changes: 1 addition & 1 deletion core/os/device/deviceinfo/cc/vk.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ bool vkLayersAndExtensions(
auto& l = inst_layer_props[i];
uint32_t ext_count = 0;
// Skip our layers.
if (!strcmp(l.layerName, "VkGraphicsSpy") ||
if (!strcmp(l.layerName, "GraphicsSpy") ||
!strcmp(l.layerName, "VirtualSwapchain")) {
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion core/os/device/deviceinfo/cc/windows/query.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ void createGlContext() {
return;
}

bool createContext(void* platform_data) {
bool createContext() {
if (gContextRefCount++ > 0) {
return true;
}
Expand Down
Loading