[Android] [PATCH 3/3] Support for multiple player instances.

[Jean-Baptiste Kempf]

Thanks a lot for the work.
It's a hard one, especially, since it was not correctly designed on day
one...

On 27 Jul, Paulo Vitor Magacho da Silva wrote :
> +    // initialize android_surface_value_t structure
> +    android_surf_value_t *android_surface = (android_surf_value_t *) malloc(sizeof(android_surf_value_t));

Unchecked malloc...

> -    init_native_crash_handler(env, thiz);
> +    // init_native_crash_handler(env, thiz);

Why?

> +    // destroy_native_crash_handler(env);

idem.

>  void Java_org_videolan_libvlc_LibVLC_detachSurface(JNIEnv *env, jobject thiz) {
> -    pthread_mutex_lock(&vout_android_lock);
> -    vout_android_surf = NULL;
> -    if (vout_android_gui != NULL)
> -        (*env)->DeleteGlobalRef(env, vout_android_gui);
> -    if (vout_android_java_surf != NULL)
> -        (*env)->DeleteGlobalRef(env, vout_android_java_surf);
> -    vout_android_gui = NULL;
> -    vout_android_java_surf = NULL;
> -    pthread_mutex_unlock(&vout_android_lock);
> +    android_surf_value_t * android_surface = (android_surf_value_t *)(intptr_t)getLong(env, thiz, "mAndroidSurfaceValue");
> +
> +    pthread_mutex_lock(&android_surface->vout_android_lock);
> +    android_surface->vout_android_surf = NULL;
> +    if (android_surface->vout_android_gui != NULL)
> +        (*env)->DeleteGlobalRef(env, android_surface->vout_android_gui);
> +    if (android_surface->vout_android_java_surf != NULL)
> +        (*env)->DeleteGlobalRef(env, android_surface->vout_android_java_surf);
> +    android_surface->vout_android_gui = NULL;
> +    android_surface->vout_android_java_surf = NULL;
> +    pthread_mutex_unlock(&android_surface->vout_android_lock);
>  }

Wouldn't it better to pass the surface in parameter to the function ?

>  void Java_org_videolan_libvlc_LibVLC_attachSubtitlesSurface(JNIEnv *env, jobject thiz, jobject surf) {
> -    pthread_mutex_lock(&vout_android_lock);
> -    vout_android_subtitles_surf = (*env)->NewGlobalRef(env, surf);
> -    pthread_cond_signal(&vout_android_surf_attached);
> -    pthread_mutex_unlock(&vout_android_lock);
> +    android_surf_value_t * android_surface = (android_surf_value_t *)(intptr_t)getLong(env, thiz, "mAndroidSurfaceValue");
> +
> +    pthread_mutex_lock(&android_surface->vout_android_lock);
> +    android_surface->vout_android_subtitles_surf = (*env)->NewGlobalRef(env, surf);
> +    pthread_cond_signal(&android_surface->vout_android_surf_attached);
> +    pthread_mutex_unlock(&android_surface->vout_android_lock);
>  }
>  
>  void Java_org_videolan_libvlc_LibVLC_detachSubtitlesSurface(JNIEnv *env, jobject thiz) {
> -    pthread_mutex_lock(&vout_android_lock);
> -    (*env)->DeleteGlobalRef(env, vout_android_subtitles_surf);
> -    vout_android_subtitles_surf = NULL;
> -    pthread_mutex_unlock(&vout_android_lock);
> +    android_surf_value_t * android_surface = (android_surf_value_t *)(intptr_t)getLong(env, thiz, "mAndroidSurfaceValue");
> +
> +    pthread_mutex_lock(&android_surface->vout_android_lock);
> +    if (android_surface->vout_android_subtitles_surf != NULL)
> +        (*env)->DeleteGlobalRef(env, android_surface->vout_android_subtitles_surf);
> +    android_surface->vout_android_subtitles_surf = NULL;
> +    pthread_mutex_unlock(&android_surface->vout_android_lock);
>  }

idem?

> +typedef struct android_surf_value_t {
> +    pthread_mutex_t vout_android_lock;
> +    pthread_cond_t vout_android_surf_attached;
> +    void *vout_android_surf;
> +    void *vout_android_gui;
> +    jobject vout_android_java_surf;
> +    jobject vout_android_subtitles_surf;
> +    bool vout_video_player_activity_created;
> +} android_surf_value_t;
>  

I'm not such a fan of this...

> --- a/vlc-android/src/org/videolan/libvlc/LibVLC.java
> +++ b/vlc-android/src/org/videolan/libvlc/LibVLC.java
> @@ -51,6 +51,9 @@ public class LibVLC {
>      private int mInternalMediaPlayerIndex = 0; // Read-only, reserved for JNI
>      private long mInternalMediaPlayerInstance = 0; // Read-only, reserved for JNI
>  
> +    // Android surface structure
> +    private long mAndroidSurfaceValue = 0; // Read-only, reserved for JNI

That means only one surface per libvlc instance?
  
> -    public static synchronized void restart(Context context) {
> +    public static synchronized void restartInstance(Context context) {
>          if (sInstance != null) {
>              try {
>                  sInstance.destroy();
> @@ -235,6 +237,15 @@ public class LibVLC {
>          }
>      }
>  
> +    public void restart(Context context) {
> +        try {
> +            this.destroy();
> +            this.init(context);
> +        } catch (LibVlcException lve) {
> +            Log.e(TAG, "Unable to reinit libvlc: " + lve);
> +        }
> +    }

So, there is restart and restartInstance? Isn't that, a bit confusing?

> -    private void applyEqualizer()
> +    protected void applyEqualizer()

How is that related to the rest?

With my kindest regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device