Android getSafeAreaRect() Wrong values

[AlexandreK38]

• axmol version: current dev
• devices test on:
  • Xiaomi Redmi Note 13 Pro, running Android 13
  • Pixel 6, running Android 14
• developing environments
  • NDK version: r26

Steps to Reproduce:

1. Call Director getSafeAreaRect() and note the given values.
2. Call Director getSafeAreaRect() once again and check for values, these are not the same as previous call.

The cause comes from the static var below, the first time the JNI is called and values computed and stored in the static array, but the next call the JNI is not called and the values are 0.

static int* cornerRadii =
            JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getDeviceCornerRadii");

static int* safeInsets =
            JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getSafeInsets");

It seems to an issue with static variable in java.
The fix is just to remove the static keyword.

[AlexandreK38]

@rh101 Hi! I saw you did the PR #1934, didn't you notice this behavior?

[rh101]

@rh101 Hi! I saw you did the PR #1934, didn't you notice this behavior?

I recall making the change to support the rounded corners, and added the call:
static int* cornerRadii = JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getDeviceCornerRadii");

The static int* safeInsets isn't new; it's been that way since Cocso2d-x.

I didn't notice any issues at the time, but have you by any chance stepped through the code to see why the values are different? Is it happening on specific devices?

[AlexandreK38]

I recall making the change to support the rounded corners, and added the call: static int* cornerRadii = JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getDeviceCornerRadii");

The static int* safeInsets isn't new; it's been that way since Cocso2d-x.

I didn't notice any issues at the time, but have you by any chance stepped through the code to see why the values are different? Is it happening on specific devices?

I tried on 2 devices and same behavior. For safeInsets it's true that it was like this before your changes, and the issue was already there. On my devices I have 0, so even if the second and later calls are wrong I didn't notice.

It's your cornerRadii that made me look what the values were as they changed:
First call I have 70 for the rounded corners, for all the 4 corners
Second and later calls I have 0, and it's not calling the AxmolEngine.getDeviceCornerRadii.

I noticed that the cornerRadii address is still the same between calls. The fact that declaring static make the call only once to the function or initializer, so the static int* keeps the address given in return by the JNI to access the Java int*; it was valid at the first call but after the JNI address is no more valid.

As side note, I hope there isn't many calls to the JNI stored in static var with pointer in Axmol, because this could cause much troubles

[rh101]

I noticed that the cornerRadii address is still the same between calls. The fact that declaring static make the call only once to the function or initializer, so the static int* keeps the address given in return by the JNI to access the Java int*; it was valid at the first call but after the JNI address is no more valid.

I'll check that out later today.

As side note, I hope there isn't many calls to the JNI stored in static var with pointer in Axmol, because this could cause much troubles

The only reason I can think of for the use of the statics is to avoid JNI calls for data that should not be changing is most likely because they're expensive to make. The pointer is pointing to an address that should not change (from my understanding), because of the use of the final int[] in the Java code etc.. Isn't that the case?

[AlexandreK38]

The only reason I can think of for the use of the statics is to avoid JNI calls for data that should not be changing is most likely because they're expensive to make. The pointer is pointing to an address that should not change (from my understanding), because of the use of the final int[] in the Java code etc.. Isn't that the case?

Yeah for sure it’s to save JNI calls; I also thought that at first and tried to store as static member in AxmolEngine the int[] but no changes, but what was strange is that I got 0 after even if it could be from anywhere in the memory. Then I looked at JniHelper::callStaticIntArrayMethod and understood:

axmol/core/platform/android/jni/JniHelper.h

Line 271 in f320ba1

static int* callStaticIntArrayMethod(const char* className, const char* methodName, Ts&&... xs)

The function declares a static int ret[32] and then memcopy the result got from java into this array… But any code calling this method will use the same ret array in memory as it’s static…. and the last call will override the previous result… and what is the other call right after? The safeInsets ones, that returns 0. That explains why I have always 0 after.
The static jnihelper callStaticIntArrayMethod and callStaticFloatArrayMethod methods work but only once. I guess they are not used more than once, and for the Int one it was the case until your PR added a second call. Quite a dangerous behavior

I also wonder why it’s hard coded 32 as maximum array length. We can’t remove the static keyword for ret because we will return a local var address, we need another way to return the array, for example a std::vector<int>

[rh101]

The function declares a static int ret[32] and then memcopy the result got from java into this array… But any code calling this method will use the same ret array in memory as it’s static….

I had absolutely no idea that this was the case, given that the method is generic. Checking back how long it's been in there, again from Cocos2d-x:
https://github.com/cocos2d/cocos2d-x/blob/76903dee64046c7bfdba50790be283484b4be271/cocos/platform/android/jni/JniHelper.h#L199

I guess they are not used more than once, and for the Int one it was the case until your PR added a second call. Quite a dangerous behavior

That is exactly the reason. Thanks for looking into the issue.

A few possible ways to fix this come to mind:

1. It returns a new std::vector each time after copying the data from the java side into it, which also means the caller can now know the true size of the data, because at the moment they cannot; the array size is assumed, which is potentially dangerous.
2. It is left as-is, but the data from it is copied over as exactly 32 ints by the caller, but with this, the method should be renamed to reflect the hard-coded size. The caller should no longer hold a pointer, but copy the data, and handle its own caching of the data rather than the pointer.

No matter what, the code calling callStaticIntArrayMethod should be the one responsible for caching the data, not the pointer to the data.

Any other ideas on how this should be implemented?

EDIT: The safe area and corner radii are affected by the layout mode set by the application. Would the layout mode ever change at runtime (check display-cutout and rounded-corners)? If they can change, then caching the values is pointless, and we would have to make the JNI calls on every request for the values.

[rh101]

Possible solution:

template <typename... Ts>
static axstd::pod_vector<int32_t> callStaticIntArrayMethod(const char* className, const char* methodName, Ts&&... xs)
{
    ax::JniMethodInfo t;
    const char* signature = jni::TypeSignature<jni::Array<jint>(std::decay_t<Ts>...)>{}();
    if (ax::JniHelper::getStaticMethodInfo(t, className, methodName, signature))
    {
        LocalRefMapType localRefs;
        jintArray array =
            (jintArray)t.env->CallStaticObjectMethod(t.classID, t.methodID, convert(localRefs, t, xs)...);

        if (array == nullptr)
        {
            t.env->DeleteLocalRef(t.classID);
            deleteLocalRefs(t.env, localRefs);
            return {};
        }

        jsize len = t.env->GetArrayLength(array);
        axstd::pod_vector<int32_t> result(len);
        jint* elems = t.env->GetIntArrayElements(array, 0);
        if (elems)
        {
            memcpy(result.data(), elems, sizeof(int32_t) * len);
            t.env->ReleaseIntArrayElements(array, elems, 0);
        };
        t.env->DeleteLocalRef(t.classID);
        deleteLocalRefs(t.env, localRefs);
        return result;
    }
    else
    {
        reportError(className, methodName, signature);
    }
    return {};
}

then in GLViewImpl-android.cpp:

static axstd::pod_vector<int32_t> cornerRadii = JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getDeviceCornerRadii");

and
static axstd::pod_vector<int32_t> safeInsets = JniHelper::callStaticIntArrayMethod("org/axmol/lib/AxmolEngine", "getSafeInsets");

Instead of checking for a nullptr, we simply check for the actual size we need:
if (safeInsets.size() >= 4) etc. etc.

This way the caller is responsible for caching the values, as it is not the responsibility of the JNI helper methods to be caching values.

[rh101]

@AlexandreK38 Check #2178 please. This is all assuming that the values of the safe area should never change for the duration of the application, and that the app layout is only set once on start-up. The functionality hasn't changed from how it is right now, except the bug is fixed.

[AlexandreK38]

@rh101 I never saw axstd::pod_vector type until now but yeah it's all good for me 👍

EDIT: The safe area and corner radii are affected by the layout mode set by the application. Would the layout mode ever change at runtime (check display-cutout and rounded-corners)? If they can change, then caching the values is pointless, and we would have to make the JNI calls on every request for the values.

The cut out is changed in the AppActivity in the onCreate, but I suppose we don't change it after. I don't on my side, we can assume it is not changed, as you did.