From: Michael Zucchi <michael@swordfish.com.au>
Date: Tue, 14 May 2019 12:53:25 +0000 (+0930)
Subject: Use a better method for handling callback references in AVIOContext.
X-Git-Url: https://code.zedzone.au/cvs?a=commitdiff_plain;h=8a6e1af28a9876120239a42fbf3d384fa9d0d1c7;p=jjmpeg

Use a better method for handling callback references in AVIOContext.
Also fix a potential leak with custom i/o handlers.
---

diff --git a/src/notzed.jjmpeg/classes/au/notzed/jjmpeg/AVIOContext.java b/src/notzed.jjmpeg/classes/au/notzed/jjmpeg/AVIOContext.java
index ea1f893..94118ac 100644
--- a/src/notzed.jjmpeg/classes/au/notzed/jjmpeg/AVIOContext.java
+++ b/src/notzed.jjmpeg/classes/au/notzed/jjmpeg/AVIOContext.java
@@ -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;
diff --git a/src/notzed.jjmpeg/jni/jj-aviocontext.c b/src/notzed.jjmpeg/jni/jj-aviocontext.c
index 51725b4..cd7fadd 100644
--- a/src/notzed.jjmpeg/jni/jj-aviocontext.c
+++ b/src/notzed.jjmpeg/jni/jj-aviocontext.c
@@ -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;
 }
diff --git a/src/notzed.jjmpeg/jni/jj-aviocontext.def b/src/notzed.jjmpeg/jni/jj-aviocontext.def
index fec52b1..3106a91 100644
--- a/src/notzed.jjmpeg/jni/jj-aviocontext.def
+++ b/src/notzed.jjmpeg/jni/jj-aviocontext.def
@@ -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 {