[vlc-devel] [PATCH] vout/android: utils: don't cache JNIEnv

[Thomas Guillem]

Hello Zhao,

Sorry for the very late reply, I totally forgot your patch.

On Fri, Oct 19, 2018, at 14:51, Zhao Zhili wrote:
> Sometimes android_getEnvCommon() is called in a thread created by the
> libvlc user. If the user called DetachCurrentThread first (for example,
> use non-optimized RAII strategy to AttachCurrentThread/DetachCurrentThread
> each time), then the cached JNIEnv become invalid.
> 
> (*jvm)->GetEnv is about 3 times slower than pthread_getspecific in my
> test case. Since android_getEnv isn't called in a inner loop, it's not a
> bottleneck.

android_getEnv is called for each audio samples (every 20ms approximately) and could be called for each picture upload (when doing mediacodec interop with opengl).

Maybe these modules should cache the ENV themselves. That way, we can apply your patch.

> ---
>  modules/video_output/android/utils.c | 66 ++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 37 deletions(-)
> 
> diff --git a/modules/video_output/android/utils.c 
> b/modules/video_output/android/utils.c
> index de65746..24df7b6 100644
> --- a/modules/video_output/android/utils.c
> +++ b/modules/video_output/android/utils.c
> @@ -299,27 +299,23 @@ LoadNativeWindowAPI(AWindowHandler *p_awh)
>   * Andoid JNIEnv helper
>   */
>  
> -static pthread_key_t jni_env_key;
> -static pthread_once_t jni_env_key_once = PTHREAD_ONCE_INIT;
> +static pthread_key_t jni_jvm_key;
> +static pthread_once_t jni_jvm_key_once = PTHREAD_ONCE_INIT;
>  
>  /* This function is called when a thread attached to the Java VM is canceled or
>   * exited */
>  static void
>  jni_detach_thread(void *data)
>  {
> -    JNIEnv *env = data;
> -    JavaVM *jvm;
> -
> -    (*env)->GetJavaVM(env, &jvm);
> -    assert(jvm);
> +    JavaVM *jvm = data;
>      (*jvm)->DetachCurrentThread(jvm);
>  }
>  
> -static void jni_env_key_create()
> +static void jni_jvm_key_create()
>  {
>      /* Create a TSD area and setup a destroy callback when a thread that
> -     * previously set the jni_env_key is canceled or exited */
> -    pthread_key_create(&jni_env_key, jni_detach_thread);
> +     * previously set the jni_jvm_key is canceled or exited */
> +    pthread_key_create(&jni_jvm_key, jni_detach_thread);
>  }
>  
>  static JNIEnv *
> @@ -327,39 +323,35 @@ android_getEnvCommon(vlc_object_t *p_obj, JavaVM 
> *jvm, const char *psz_name)
>  {
>      assert((p_obj && !jvm) || (!p_obj && jvm));
>  
> -    JNIEnv *env;
> -
> -    pthread_once(&jni_env_key_once, jni_env_key_create);
> -    env = pthread_getspecific(jni_env_key);
> -    if (env == NULL)
> +    if (!jvm)
>      {
> +        jvm = var_InheritAddress(p_obj, "android-jvm");
>          if (!jvm)
> -            jvm = var_InheritAddress(p_obj, "android-jvm");
> +            return NULL;
> +    }
>  
> -        if (!jvm)
> +    JNIEnv *env = NULL;
> +    pthread_once(&jni_jvm_key_once, jni_jvm_key_create);
> +    /* if GetEnv returns JNI_OK, the thread is already attached to the
> +     * JavaVM, so we are already in a java thread, and we don't have to
> +     * setup any destroy callbacks */
> +    if ((*jvm)->GetEnv(jvm, (void **)&env, JNI_VERSION_1_2) != JNI_OK)
> +    {
> +        /* attach the thread to the Java VM */
> +        JavaVMAttachArgs args;
> +
> +        args.version = JNI_VERSION_1_2;
> +        args.name = psz_name;
> +        args.group = NULL;
> +
> +        if ((*jvm)->AttachCurrentThread(jvm, &env, &args) != JNI_OK)
>              return NULL;
>  
> -        /* if GetEnv returns JNI_OK, the thread is already attached to the
> -         * JavaVM, so we are already in a java thread, and we don't have to
> -         * setup any destroy callbacks */
> -        if ((*jvm)->GetEnv(jvm, (void **)&env, JNI_VERSION_1_2) != JNI_OK)
> +        /* save jvm which will be used to detach current thread */
> +        if (pthread_setspecific(jni_jvm_key, jvm) != 0)
>          {
> -            /* attach the thread to the Java VM */
> -            JavaVMAttachArgs args;
> -
> -            args.version = JNI_VERSION_1_2;
> -            args.name = psz_name;
> -            args.group = NULL;
> -
> -            if ((*jvm)->AttachCurrentThread(jvm, &env, &args) != JNI_OK)
> -                return NULL;
> -
> -            /* Set the attached env to the thread-specific data area (TSD) */
> -            if (pthread_setspecific(jni_env_key, env) != 0)
> -            {
> -                (*jvm)->DetachCurrentThread(jvm);
> -                return NULL;
> -            }
> +            (*jvm)->DetachCurrentThread(jvm);
> +            return NULL;
>          }
>      }
>  
> -- 
> 2.9.5
> 
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel