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

[Paulo Vitor Magacho da Silva]

2014-07-27 19:24 GMT+01:00 Jean-Baptiste Kempf <jb at videolan.org>:

> 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...


Will fix.


>


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

Still need to figure out to handle this without using global variable.


>
> > +    // destroy_native_crash_handler(env);
>
> idem.
>

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 ?
>

I think that could be like that also.


>
> >  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?
>

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...
>

Will think of how to replace this. But I think for now we can go with this
(if it's working of course).


>
> > --- 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?
>

Yes. This is the idea. Do we need more ?


>
> > -    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?
>

This is just for now not to break VLC application. The VLC app needs to be
refactored to support multiple player instances.


>
> > -    private void applyEqualizer()
> > +    protected void applyEqualizer()
>
> How is that related to the rest?
>

Native code calls this method. And if it's left as private LibVLC cannot be
extended.


>
> With my kindest regards,
>
> --
> Jean-Baptiste Kempf
> http://www.jbkempf.com/ - +33 672 704 734
> Sent from my Electronic Device
> _______________________________________________
> Android mailing list
> Android at videolan.org
> https://mailman.videolan.org/listinfo/android
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/android/attachments/20140727/fbe25d07/attachment-0001.html>