From 8a6e1af28a9876120239a42fbf3d384fa9d0d1c7 Mon Sep 17 00:00:00 2001 From: Michael Zucchi Date: Tue, 14 May 2019 22:23:25 +0930 Subject: [PATCH] Use a better method for handling callback references in AVIOContext. Also fix a potential leak with custom i/o handlers. --- .../classes/au/notzed/jjmpeg/AVIOContext.java | 7 +- src/notzed.jjmpeg/jni/jj-aviocontext.c | 123 ++++++++---------- src/notzed.jjmpeg/jni/jj-aviocontext.def | 2 +- 3 files changed, 62 insertions(+), 70 deletions(-) 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. *

- * 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 { -- 2.39.5