Use a better method for handling callback references in AVIOContext.
authorMichael Zucchi <michael@swordfish.com.au>
Tue, 14 May 2019 12:53:25 +0000 (22:23 +0930)
committerMichael Zucchi <michael@swordfish.com.au>
Tue, 14 May 2019 12:53:25 +0000 (22:23 +0930)
Also fix a potential leak with custom i/o handlers.

src/notzed.jjmpeg/classes/au/notzed/jjmpeg/AVIOContext.java
src/notzed.jjmpeg/jni/jj-aviocontext.c
src/notzed.jjmpeg/jni/jj-aviocontext.def

index ea1f893..94118ac 100644 (file)
@@ -26,11 +26,12 @@ import java.nio.ByteBuffer;
 public class AVIOContext extends AVOptions implements AVIOContextBits {
 
        /**
-        * Maintains references required for callbacks.
+        * Maintain a reference to an interrupt or i/o handler if set.
         * <p>
-        * This is a {@link au.notzed.nativez.ReferenceZ} object.
+        * The C uses a weak reference and this makes sure it stays around
+        * while {@code this} does.
         */
-       Object opaque;
+       Object handler;
 
        //
        public static final int AVSEEK_SET = 0;
index 51725b4..cd7fadd 100644 (file)
@@ -22,6 +22,9 @@
 #include "jjmpeg.h"
 #include "jj-aviocontext.h"
 
+//#define D(x) do { x; fflush(stdout); } while(0)
+#define D(x)
+
 jint AVIOContext_OnLoad(JavaVM *vmi, JNIEnv *env) {
        return nativez_ResolveReferences(env, java_names, &java)
                || nativez_ResolveFunctions(env, jj_libtable, fn_names, &fn);
@@ -30,73 +33,76 @@ jint AVIOContext_OnLoad(JavaVM *vmi, JNIEnv *env) {
 /* ********************************************************************** */
 
 /*
- * All the callbacks use this structure to find the java instance
- * which implement it.
+ * All callbacks have a weak reference to their Java handler passed
+ * in as opaque.
+ *
+ * It must get a localref before using it as per JNI spec Weak Global
+ * References (Clarification June 2001).
  *
- * AVIOContext.open() has a further complication in that there is
- * nowhere to store a pointer to this.  To get around this if an
- * interrupt callback is supplied it is referenced in an instance of a
- * nativez.ReferenceZ object and set on the java AVIOContext.opaque
- * field.
+ * When AVIOContext is created a reference is also set on the object
+ * which means the callback must live at least as long as the avio.
  */
-struct jj_io {
-       // AVIOHandler or AVIOInterrupt depending on how it was created
-       jobject jhandler;
-};
 
 static int jj_read_packet(void *opaque, uint8_t *buf, int buf_size) {
        JNIEnv *env = nativez_AttachCurrentThreadAsDaemon();
        
        if (env) {
-               struct jj_io *jjio = opaque;
-               jvalue args[1];
+               jobject jhandler = (*env)->NewLocalRef(env, opaque);
 
-               args[0].l = nativez_NewDirectBuffer(env, buf, buf_size);
+               if (jhandler) {
+                       jvalue args[1];
 
-               return (*env)->CallIntMethodA(env, jjio->jhandler, AVIOHandler_readPacket_l, args);
-       } else {
-               return AVERROR(EIO);
+                       args[0].l = nativez_NewDirectBuffer(env, buf, buf_size);
+
+                       return (*env)->CallIntMethodA(env, jhandler, AVIOHandler_readPacket_l, args);
+               }
        }
+       return AVERROR(EIO);
 }
 
 static int jj_write_packet(void *opaque, uint8_t *buf, int buf_size) {
        JNIEnv *env = nativez_AttachCurrentThreadAsDaemon();
 
        if (env) {
-               struct jj_io *jjio = opaque;
-               jvalue args[1];
+               jobject jhandler = (*env)->NewLocalRef(env, opaque);
 
-               args[0].l = nativez_NewDirectBuffer(env, buf, buf_size);
+               if (jhandler) {
+                       jvalue args[1];
 
-               return (*env)->CallIntMethodA(env, jjio->jhandler, AVIOHandler_writePacket_l, args);
-       } else {
-               return AVERROR(EIO);
+                       args[0].l = nativez_NewDirectBuffer(env, buf, buf_size);
+                       
+                       return (*env)->CallIntMethodA(env, jhandler, AVIOHandler_writePacket_l, args);
+               }
        }
+       return AVERROR(EIO);
 }
 
 static int64_t jj_seek(void *opaque, int64_t offset, int whence) {
        JNIEnv *env = nativez_AttachCurrentThreadAsDaemon();
 
        if (env) {
-               struct jj_io *jjio = opaque;
-               jvalue args[2];
+               jobject jhandler = (*env)->NewLocalRef(env, opaque);
 
-               args[0].j = offset;
-               args[1].i = whence;
+               if (jhandler) {
+                       jvalue args[2];
 
-               return (*env)->CallLongMethodA(env, jjio->jhandler, AVIOHandler_seek_ji, args);
-       } else {
-               return AVERROR(EIO);
+                       args[0].j = offset;
+                       args[1].i = whence;
+
+                       return (*env)->CallLongMethodA(env, jhandler, AVIOHandler_seek_ji, args);
+               }
        }
+       return AVERROR(EIO);
 }
 
 static int jj_interrupt(void *opaque) {
        JNIEnv *env = nativez_AttachCurrentThreadAsDaemon();
 
        if (env) {
-               struct jj_io *jjio = opaque;
-
-               return (*env)->CallBooleanMethodA(env, jjio->jhandler, AVIOInterrupt_isInterrupted_, NULL);
+               jobject jhandler = (*env)->NewLocalRef(env, opaque);
+               if (jhandler)
+                       return (*env)->CallBooleanMethodA(env, jhandler, AVIOInterrupt_isInterrupted_, NULL);
+               return 0;
        } else {
                return 1;
        }
@@ -107,13 +113,16 @@ JNIEXPORT void JNICALL Java_au_notzed_jjmpeg_AVIOContext_release
        AVIOContext *ic = (void *)(uintptr_t)jic;
 
        if (ic->read_packet == jj_read_packet) {
-               struct jj_io *jjio = ic->opaque;
+               //jobject jhandler = ic->opaque;
+
+               D(printf("%p: close custom avio\n", ic));
 
-               (*env)->DeleteGlobalRef(env, jjio->jhandler);
+               //(*env)->DeleteGlobalRef(env, jhandler);
                DLCALL(av_free)(ic->buffer);
                DLCALL(avio_context_free)(&ic);
-               free(jjio);
        } else {
+               D(printf("%p: close ffmpeg avio\n", ic));
+
                DLCALL(avio_close)(ic);
        }
 }
@@ -122,23 +131,12 @@ JNIEXPORT jobject JNICALL Java_au_notzed_jjmpeg_AVIOContext_open
 (JNIEnv *env, jclass jc, jstring jurl, jint flags, jobject jcb, jobject joptions) {
        AVIOContext *io = NULL;
        int res;
-       struct jj_io *jjio = NULL;
        AVIOInterruptCB cb = { 0, 0 };
        jobject jio = NULL;
-       jobject jrefs = NULL;
-       
-       jjio = malloc(sizeof(*jjio));
-       if (!jjio) {
-               nativez_ThrowOutOfMemoryError(env, "Allocating handler");
-               return NULL;
-       }
        
        if (jcb) {
-               // FIXME: check returns
-               jrefs = ReferenceZ_create(env, &jcb, 1);
-               jjio->jhandler = (*env)->NewGlobalRef(env, jcb);
                cb.callback = jj_interrupt;
-               cb.opaque = jjio;
+               cb.opaque = (*env)->NewWeakGlobalRef(env, jcb);
        }
        
        AVDictionary *options = AVDictionary_get(env, joptions);
@@ -155,40 +153,32 @@ JNIEXPORT jobject JNICALL Java_au_notzed_jjmpeg_AVIOContext_open
        jio = NativeZ_create(env, jc, io);
        if (!jio)
                goto fail;
-
        
-       (*env)->SetObjectField(env, jio, AVIOContext_opaque, jrefs);
+       (*env)->SetObjectField(env, jio, AVIOContext_handler, jcb);
 
        return jio;
  fail:
+       D(printf("open failed %d\n", res));
        nativez_ReleaseString(env, jurl, url);
-       if (jjio->jhandler)
-               (*env)->DeleteGlobalRef(env, jjio->jhandler);
-       free(jjio);
-
        return NULL;
 }
 
 JNIEXPORT jobject JNICALL Java_au_notzed_jjmpeg_AVIOContext_alloc
 (JNIEnv *env, jclass jc, jint buffer_size, jint jflags, jobject jhandler) {
        AVIOContext *io = NULL;
-       struct jj_io *jjio;
        uint8_t *buffer;
+       jobject ghandler;
        jobject jio = NULL;
 
-       jjio = malloc(sizeof(*jjio));
-       if (!jjio)
-               goto fail_nojjio;
-
        buffer = DLCALL(av_malloc)(buffer_size);
        if (!buffer)
                goto fail_nobuffer;
        
-       jjio->jhandler = (*env)->NewGlobalRef(env, jhandler);
-               
+       ghandler = (*env)->NewWeakGlobalRef(env, jhandler);
+
        io = DLCALL(avio_alloc_context)(buffer, buffer_size,
                                        (jflags & au_notzed_jjmpeg_AVIOContext_AVIO_WRITABLE) != 0,
-                                       jjio,
+                                       ghandler,
                                        jj_read_packet,
                                        jj_write_packet,
                                        jj_seek);
@@ -201,13 +191,14 @@ JNIEXPORT jobject JNICALL Java_au_notzed_jjmpeg_AVIOContext_alloc
        if (!jio)
                goto fail;
 
+       (*env)->SetObjectField(env, jio, AVIOContext_handler, jhandler);
+
+       D(printf("%p: AVIOContext.alloc\n", io));
+
        return jio;
  fail:
-       (*env)->DeleteGlobalRef(env, jjio->jhandler);
-       free(buffer);
+       DLCALL(av_free)(buffer);
  fail_nobuffer:
-       free(jjio);
- fail_nojjio:
        nativez_ThrowOutOfMemoryError(env, "Allocating io context");
        return NULL;
 }
index fec52b1..3106a91 100644 (file)
@@ -13,7 +13,7 @@ header avutil libavutil/mem.h {
 }
 
 java AVIOContext au/notzed/jjmpeg/AVIOContext {
-     opaque, Ljava/lang/Object;
+     handler, Ljava/lang/Object;
 }
 
 java AVIOInterrupt au/notzed/jjmpeg/AVIOContext$AVIOInterrupt {