Message ID | 20171214170611.GA5481@tsuri.lan |
---|---|
State | Accepted |
Commit | f3cffd121b990717996d8ddd646bd555c1db135b |
Headers | show |
On Thu, 14 Dec 2017 18:06:11 +0100 Matthieu Bouron <matthieu.bouron@gmail.com> wrote: > On Thu, Dec 14, 2017 at 01:02:49PM +0100, wm4 wrote: > > On Thu, 14 Dec 2017 12:53:38 +0100 > > Matthieu Bouron <matthieu.bouron@gmail.com> wrote: > > > > > On Thu, Dec 14, 2017 at 12:21:28PM +0100, wm4 wrote: > > > > On Thu, 14 Dec 2017 11:09:13 +0100 > > > > Matthieu Bouron <matthieu.bouron@gmail.com> wrote: > > > > > > > > > --- > > > > > libavcodec/mediacodec_wrapper.c | 262 +++++++++++----------------------------- > > > > > 1 file changed, 70 insertions(+), 192 deletions(-) > > > > > > > > > > diff --git a/libavcodec/mediacodec_wrapper.c b/libavcodec/mediacodec_wrapper.c > > > > > index f34450a6d8..4660e895ca 100644 > > > > > --- a/libavcodec/mediacodec_wrapper.c > > > > > +++ b/libavcodec/mediacodec_wrapper.c > > > > > @@ -1132,200 +1132,78 @@ fail: > > > > > > > > > > > > > +#define DECLARE_FF_AMEDIACODEC_CREATE_FUNC(func, jfunc) \ > > > > > +FFAMediaCodec* ff_AMediaCodec_##func(const char *arg) \ > > > > > +{ \ > > > > > + int ret = -1; \ > > > > > + JNIEnv *env = NULL; \ > > > > > + FFAMediaCodec *codec = NULL; \ > > > > > + jstring jarg = NULL; \ > > > > > + jobject object = NULL; \ > > > > > + \ > > > > > + codec = av_mallocz(sizeof(FFAMediaCodec)); \ > > > > > + if (!codec) { \ > > > > > + return NULL; \ > > > > > + } \ > > > > > + codec->class = &amediacodec_class; \ > > > > > + \ > > > > > + env = ff_jni_get_env(codec); \ > > > > > + if (!env) { \ > > > > > + av_freep(&codec); \ > > > > > + return NULL; \ > > > > > + } \ > > > > > + \ > > > > > + if (ff_jni_init_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec) < 0) { \ > > > > > + goto fail; \ > > > > > + } \ > > > > > + \ > > > > > + jarg = ff_jni_utf_chars_to_jstring(env, arg, codec); \ > > > > > + if (!jarg) { \ > > > > > + goto fail; \ > > > > > + } \ > > > > > + \ > > > > > + object = (*env)->CallStaticObjectMethod(env, \ > > > > > + codec->jfields.mediacodec_class, \ > > > > > + codec->jfields.jfunc, \ > > > > > + jarg); \ > > > > > + if (ff_jni_exception_check(env, 1, codec) < 0) { \ > > > > > + goto fail; \ > > > > > + } \ > > > > > + \ > > > > > + codec->object = (*env)->NewGlobalRef(env, object); \ > > > > > + if (!codec->object) { \ > > > > > + goto fail; \ > > > > > + } \ > > > > > + \ > > > > > + if (codec_init_static_fields(codec) < 0) { \ > > > > > + goto fail; \ > > > > > + } \ > > > > > + \ > > > > > + if (codec->jfields.get_input_buffer_id && codec->jfields.get_output_buffer_id) { \ > > > > > + codec->has_get_i_o_buffer = 1; \ > > > > > + } \ > > > > > + \ > > > > > + ret = 0; \ > > > > > +fail: \ > > > > > + if (jarg) { \ > > > > > + (*env)->DeleteLocalRef(env, jarg); \ > > > > > + } \ > > > > > + \ > > > > > + if (object) { \ > > > > > + (*env)->DeleteLocalRef(env, object); \ > > > > > + } \ > > > > > + \ > > > > > + if (ret < 0) { \ > > > > > + ff_jni_reset_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec); \ > > > > > + av_freep(&codec); \ > > > > > + } \ > > > > > + \ > > > > > + return codec; \ > > > > > } > > > > > > > > > > > > > Such long macros are very ugly. And the only macro argument that is > > > > used in the body is jfunc. Would it be possible to move most of the > > > > macro into a proper function, with jfunc as pointer argument or so? > > > > > > I agree with you that such macros are ugly. > > > > > > I used this approach as the jfunc value cannot be known before > > > ff_jni_init_jfields is called. > > > > > > I will try another approach and send a new patch. > > > > Right, it seems it's not as simple as passing a pointer. Maybe > > offsetof() could help. > > Patch updated. I did not use offsetof as it felt a bit weird to pass an > offset of a struct to a function to call the right method. > Looks much nicer!
On Thu, Dec 14, 2017 at 06:38:48PM +0100, wm4 wrote: > On Thu, 14 Dec 2017 18:06:11 +0100 > Matthieu Bouron <matthieu.bouron@gmail.com> wrote: > > > On Thu, Dec 14, 2017 at 01:02:49PM +0100, wm4 wrote: > > > On Thu, 14 Dec 2017 12:53:38 +0100 > > > Matthieu Bouron <matthieu.bouron@gmail.com> wrote: > > > > > > > On Thu, Dec 14, 2017 at 12:21:28PM +0100, wm4 wrote: > > > > > On Thu, 14 Dec 2017 11:09:13 +0100 > > > > > Matthieu Bouron <matthieu.bouron@gmail.com> wrote: > > > > > > > > > > > --- > > > > > > libavcodec/mediacodec_wrapper.c | 262 +++++++++++----------------------------- > > > > > > 1 file changed, 70 insertions(+), 192 deletions(-) > > > > > > > > > > > > diff --git a/libavcodec/mediacodec_wrapper.c b/libavcodec/mediacodec_wrapper.c > > > > > > index f34450a6d8..4660e895ca 100644 > > > > > > --- a/libavcodec/mediacodec_wrapper.c > > > > > > +++ b/libavcodec/mediacodec_wrapper.c > > > > > > @@ -1132,200 +1132,78 @@ fail: > > > > > > > > > > > > > > > > +#define DECLARE_FF_AMEDIACODEC_CREATE_FUNC(func, jfunc) \ > > > > > > +FFAMediaCodec* ff_AMediaCodec_##func(const char *arg) \ > > > > > > +{ \ > > > > > > + int ret = -1; \ > > > > > > + JNIEnv *env = NULL; \ > > > > > > + FFAMediaCodec *codec = NULL; \ > > > > > > + jstring jarg = NULL; \ > > > > > > + jobject object = NULL; \ > > > > > > + \ > > > > > > + codec = av_mallocz(sizeof(FFAMediaCodec)); \ > > > > > > + if (!codec) { \ > > > > > > + return NULL; \ > > > > > > + } \ > > > > > > + codec->class = &amediacodec_class; \ > > > > > > + \ > > > > > > + env = ff_jni_get_env(codec); \ > > > > > > + if (!env) { \ > > > > > > + av_freep(&codec); \ > > > > > > + return NULL; \ > > > > > > + } \ > > > > > > + \ > > > > > > + if (ff_jni_init_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec) < 0) { \ > > > > > > + goto fail; \ > > > > > > + } \ > > > > > > + \ > > > > > > + jarg = ff_jni_utf_chars_to_jstring(env, arg, codec); \ > > > > > > + if (!jarg) { \ > > > > > > + goto fail; \ > > > > > > + } \ > > > > > > + \ > > > > > > + object = (*env)->CallStaticObjectMethod(env, \ > > > > > > + codec->jfields.mediacodec_class, \ > > > > > > + codec->jfields.jfunc, \ > > > > > > + jarg); \ > > > > > > + if (ff_jni_exception_check(env, 1, codec) < 0) { \ > > > > > > + goto fail; \ > > > > > > + } \ > > > > > > + \ > > > > > > + codec->object = (*env)->NewGlobalRef(env, object); \ > > > > > > + if (!codec->object) { \ > > > > > > + goto fail; \ > > > > > > + } \ > > > > > > + \ > > > > > > + if (codec_init_static_fields(codec) < 0) { \ > > > > > > + goto fail; \ > > > > > > + } \ > > > > > > + \ > > > > > > + if (codec->jfields.get_input_buffer_id && codec->jfields.get_output_buffer_id) { \ > > > > > > + codec->has_get_i_o_buffer = 1; \ > > > > > > + } \ > > > > > > + \ > > > > > > + ret = 0; \ > > > > > > +fail: \ > > > > > > + if (jarg) { \ > > > > > > + (*env)->DeleteLocalRef(env, jarg); \ > > > > > > + } \ > > > > > > + \ > > > > > > + if (object) { \ > > > > > > + (*env)->DeleteLocalRef(env, object); \ > > > > > > + } \ > > > > > > + \ > > > > > > + if (ret < 0) { \ > > > > > > + ff_jni_reset_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec); \ > > > > > > + av_freep(&codec); \ > > > > > > + } \ > > > > > > + \ > > > > > > + return codec; \ > > > > > > } > > > > > > > > > > > > > > > > Such long macros are very ugly. And the only macro argument that is > > > > > used in the body is jfunc. Would it be possible to move most of the > > > > > macro into a proper function, with jfunc as pointer argument or so? > > > > > > > > I agree with you that such macros are ugly. > > > > > > > > I used this approach as the jfunc value cannot be known before > > > > ff_jni_init_jfields is called. > > > > > > > > I will try another approach and send a new patch. > > > > > > Right, it seems it's not as simple as passing a pointer. Maybe > > > offsetof() could help. > > > > Patch updated. I did not use offsetof as it felt a bit weird to pass an > > offset of a struct to a function to call the right method. > > > > Looks much nicer! I will push the patchset in a few hours if there is no objection.
On Fri, Dec 15, 2017 at 04:13:22PM +0100, Matthieu Bouron wrote: > On Thu, Dec 14, 2017 at 06:38:48PM +0100, wm4 wrote: > > On Thu, 14 Dec 2017 18:06:11 +0100 > > Matthieu Bouron <matthieu.bouron@gmail.com> wrote: > > > > > On Thu, Dec 14, 2017 at 01:02:49PM +0100, wm4 wrote: > > > > On Thu, 14 Dec 2017 12:53:38 +0100 > > > > Matthieu Bouron <matthieu.bouron@gmail.com> wrote: > > > > > > > > > On Thu, Dec 14, 2017 at 12:21:28PM +0100, wm4 wrote: > > > > > > On Thu, 14 Dec 2017 11:09:13 +0100 > > > > > > Matthieu Bouron <matthieu.bouron@gmail.com> wrote: > > > > > > > > > > > > > --- > > > > > > > libavcodec/mediacodec_wrapper.c | 262 +++++++++++----------------------------- > > > > > > > 1 file changed, 70 insertions(+), 192 deletions(-) > > > > > > > > > > > > > > diff --git a/libavcodec/mediacodec_wrapper.c b/libavcodec/mediacodec_wrapper.c > > > > > > > index f34450a6d8..4660e895ca 100644 > > > > > > > --- a/libavcodec/mediacodec_wrapper.c > > > > > > > +++ b/libavcodec/mediacodec_wrapper.c > > > > > > > @@ -1132,200 +1132,78 @@ fail: > > > > > > > > > > > > > > > > > > > +#define DECLARE_FF_AMEDIACODEC_CREATE_FUNC(func, jfunc) \ > > > > > > > +FFAMediaCodec* ff_AMediaCodec_##func(const char *arg) \ > > > > > > > +{ \ > > > > > > > + int ret = -1; \ > > > > > > > + JNIEnv *env = NULL; \ > > > > > > > + FFAMediaCodec *codec = NULL; \ > > > > > > > + jstring jarg = NULL; \ > > > > > > > + jobject object = NULL; \ > > > > > > > + \ > > > > > > > + codec = av_mallocz(sizeof(FFAMediaCodec)); \ > > > > > > > + if (!codec) { \ > > > > > > > + return NULL; \ > > > > > > > + } \ > > > > > > > + codec->class = &amediacodec_class; \ > > > > > > > + \ > > > > > > > + env = ff_jni_get_env(codec); \ > > > > > > > + if (!env) { \ > > > > > > > + av_freep(&codec); \ > > > > > > > + return NULL; \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + if (ff_jni_init_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec) < 0) { \ > > > > > > > + goto fail; \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + jarg = ff_jni_utf_chars_to_jstring(env, arg, codec); \ > > > > > > > + if (!jarg) { \ > > > > > > > + goto fail; \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + object = (*env)->CallStaticObjectMethod(env, \ > > > > > > > + codec->jfields.mediacodec_class, \ > > > > > > > + codec->jfields.jfunc, \ > > > > > > > + jarg); \ > > > > > > > + if (ff_jni_exception_check(env, 1, codec) < 0) { \ > > > > > > > + goto fail; \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + codec->object = (*env)->NewGlobalRef(env, object); \ > > > > > > > + if (!codec->object) { \ > > > > > > > + goto fail; \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + if (codec_init_static_fields(codec) < 0) { \ > > > > > > > + goto fail; \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + if (codec->jfields.get_input_buffer_id && codec->jfields.get_output_buffer_id) { \ > > > > > > > + codec->has_get_i_o_buffer = 1; \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + ret = 0; \ > > > > > > > +fail: \ > > > > > > > + if (jarg) { \ > > > > > > > + (*env)->DeleteLocalRef(env, jarg); \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + if (object) { \ > > > > > > > + (*env)->DeleteLocalRef(env, object); \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + if (ret < 0) { \ > > > > > > > + ff_jni_reset_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec); \ > > > > > > > + av_freep(&codec); \ > > > > > > > + } \ > > > > > > > + \ > > > > > > > + return codec; \ > > > > > > > } > > > > > > > > > > > > > > > > > > > Such long macros are very ugly. And the only macro argument that is > > > > > > used in the body is jfunc. Would it be possible to move most of the > > > > > > macro into a proper function, with jfunc as pointer argument or so? > > > > > > > > > > I agree with you that such macros are ugly. > > > > > > > > > > I used this approach as the jfunc value cannot be known before > > > > > ff_jni_init_jfields is called. > > > > > > > > > > I will try another approach and send a new patch. > > > > > > > > Right, it seems it's not as simple as passing a pointer. Maybe > > > > offsetof() could help. > > > > > > Patch updated. I did not use offsetof as it felt a bit weird to pass an > > > offset of a struct to a function to call the right method. > > > > > > > Looks much nicer! > > I will push the patchset in a few hours if there is no objection. > Patch applied.
diff --git a/libavcodec/mediacodec_wrapper.c b/libavcodec/mediacodec_wrapper.c index f34450a6d8..329a5eb896 100644 --- a/libavcodec/mediacodec_wrapper.c +++ b/libavcodec/mediacodec_wrapper.c @@ -1132,13 +1132,18 @@ fail: return ret; } -FFAMediaCodec* ff_AMediaCodec_createCodecByName(const char *name) +#define CREATE_CODEC_BY_NAME 0 +#define CREATE_DECODER_BY_TYPE 1 +#define CREATE_ENCODER_BY_TYPE 2 + +static inline FFAMediaCodec *codec_create(int method, const char *arg) { int ret = -1; JNIEnv *env = NULL; FFAMediaCodec *codec = NULL; - jstring codec_name = NULL; + jstring jarg = NULL; jobject object = NULL; + jmethodID create_id = NULL; codec = av_mallocz(sizeof(FFAMediaCodec)); if (!codec) { @@ -1156,77 +1161,23 @@ FFAMediaCodec* ff_AMediaCodec_createCodecByName(const char *name) goto fail; } - codec_name = ff_jni_utf_chars_to_jstring(env, name, codec); - if (!codec_name) { - goto fail; - } - - object = (*env)->CallStaticObjectMethod(env, codec->jfields.mediacodec_class, codec->jfields.create_by_codec_name_id, codec_name); - if (ff_jni_exception_check(env, 1, codec) < 0) { - goto fail; - } - - codec->object = (*env)->NewGlobalRef(env, object); - if (!codec->object) { - goto fail; - } - - if (codec_init_static_fields(codec) < 0) { - goto fail; - } - - if (codec->jfields.get_input_buffer_id && codec->jfields.get_output_buffer_id) { - codec->has_get_i_o_buffer = 1; - } - - ret = 0; -fail: - if (codec_name) { - (*env)->DeleteLocalRef(env, codec_name); - } - - if (object) { - (*env)->DeleteLocalRef(env, object); - } - - if (ret < 0) { - ff_jni_reset_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec); - av_freep(&codec); - } - - return codec; -} - -FFAMediaCodec* ff_AMediaCodec_createDecoderByType(const char *mime) -{ - int ret = -1; - JNIEnv *env = NULL; - FFAMediaCodec *codec = NULL; - jstring mime_type = NULL; - jobject object = NULL; - - codec = av_mallocz(sizeof(FFAMediaCodec)); - if (!codec) { - return NULL; - } - codec->class = &amediacodec_class; - - env = ff_jni_get_env(codec); - if (!env) { - av_freep(&codec); - return NULL; - } - - if (ff_jni_init_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec) < 0) { + jarg = ff_jni_utf_chars_to_jstring(env, arg, codec); + if (!jarg) { goto fail; } - mime_type = ff_jni_utf_chars_to_jstring(env, mime, codec); - if (!mime_type) { - goto fail; + switch (method) { + case CREATE_CODEC_BY_NAME: create_id = codec->jfields.create_by_codec_name_id; break; + case CREATE_DECODER_BY_TYPE: create_id = codec->jfields.create_decoder_by_type_id; break; + case CREATE_ENCODER_BY_TYPE: create_id = codec->jfields.create_encoder_by_type_id; break; + default: + av_assert0(0); } - object = (*env)->CallStaticObjectMethod(env, codec->jfields.mediacodec_class, codec->jfields.create_decoder_by_type_id, mime_type); + object = (*env)->CallStaticObjectMethod(env, + codec->jfields.mediacodec_class, + create_id, + jarg); if (ff_jni_exception_check(env, 1, codec) < 0) { goto fail; } @@ -1246,8 +1197,8 @@ FFAMediaCodec* ff_AMediaCodec_createDecoderByType(const char *mime) ret = 0; fail: - if (mime_type) { - (*env)->DeleteLocalRef(env, mime_type); + if (jarg) { + (*env)->DeleteLocalRef(env, jarg); } if (object) { @@ -1262,70 +1213,15 @@ fail: return codec; } -FFAMediaCodec* ff_AMediaCodec_createEncoderByType(const char *mime) -{ - int ret = -1; - JNIEnv *env = NULL; - FFAMediaCodec *codec = NULL; - jstring mime_type = NULL; - jobject object = NULL; +#define DECLARE_FF_AMEDIACODEC_CREATE_FUNC(name, method) \ +FFAMediaCodec *ff_AMediaCodec_##name(const char *arg) \ +{ \ + return codec_create(method, arg); \ +} \ - codec = av_mallocz(sizeof(FFAMediaCodec)); - if (!codec) { - return NULL; - } - codec->class = &amediacodec_class; - - env = ff_jni_get_env(codec); - if (!env) { - av_freep(&codec); - return NULL; - } - - if (ff_jni_init_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec) < 0) { - goto fail; - } - - mime_type = ff_jni_utf_chars_to_jstring(env, mime, codec); - if (!mime_type) { - goto fail; - } - - object = (*env)->CallStaticObjectMethod(env, codec->jfields.mediacodec_class, codec->jfields.create_encoder_by_type_id, mime_type); - if (ff_jni_exception_check(env, 1, codec) < 0) { - goto fail; - } - - codec->object = (*env)->NewGlobalRef(env, object); - if (!codec->object) { - goto fail; - } - - if (codec_init_static_fields(codec) < 0) { - goto fail; - } - - if (codec->jfields.get_input_buffer_id && codec->jfields.get_output_buffer_id) { - codec->has_get_i_o_buffer = 1; - } - - ret = 0; -fail: - if (mime_type) { - (*env)->DeleteLocalRef(env, mime_type); - } - - if (object) { - (*env)->DeleteLocalRef(env, object); - } - - if (ret < 0) { - ff_jni_reset_jfields(env, &codec->jfields, jni_amediacodec_mapping, 1, codec); - av_freep(&codec); - } - - return codec; -} +DECLARE_FF_AMEDIACODEC_CREATE_FUNC(createCodecByName, CREATE_CODEC_BY_NAME) +DECLARE_FF_AMEDIACODEC_CREATE_FUNC(createDecoderByType, CREATE_DECODER_BY_TYPE) +DECLARE_FF_AMEDIACODEC_CREATE_FUNC(createEncoderByType, CREATE_ENCODER_BY_TYPE) int ff_AMediaCodec_delete(FFAMediaCodec* codec) {