Re: [FFmpeg-devel] [PATCH v2 2/6] avformat: add Android content resolver protocol support

[Matthieu Bouron <matthieu.bouron@gmail.com>]

On Sun, Mar 17, 2024 at 12:33:04PM +0800, Zhao Zhili wrote:
> Sorry for the long delay of review.
> 
> > On Mar 5, 2024, at 03:37, Matthieu Bouron <matthieu.bouron@gmail.com> wrote:
> > 
> > On Tue, Feb 27, 2024 at 03:50:38PM +0100, Matthieu Bouron wrote:
> >> Handles Android content-uri starting with content://.
> >> ---
> > 
> > [...]
> > 
> 
> > Handles Android content-uri starting with content://.
> > ---
> >  configure               |   2 +
> >  doc/APIchanges          |   3 +
> >  libavcodec/jni.c        |   1 +
> >  libavformat/Makefile    |   1 +
> >  libavformat/file.c      | 157 ++++++++++++++++++++++++++++++++++++++++
> >  libavformat/protocols.c |   1 +
> >  6 files changed, 165 insertions(+)
> > 
> > diff --git a/configure b/configure
> > index bb5e630bad..790a1df7c8 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3655,6 +3655,8 @@ xcbgrab_indev_suggest="libxcb_shm libxcb_shape libxcb_xfixes"
> >  xv_outdev_deps="xlib_xv xlib_x11 xlib_xext"
> >  
> >  # protocols
> > +android_content_protocol_deps="jni"
> > +android_content_protocol_select="file_protocol"
> >  async_protocol_deps="threads"
> >  bluray_protocol_deps="libbluray"
> >  ffrtmpcrypt_protocol_conflict="librtmp_protocol"
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index 10f6667e9e..258e953ca6 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09
> >  
> >  API changes, most recent first:
> >  
> > +2024-02-xx - xxxxxxxxxx - lavc 60.41.100 - jni.h
> > +  Add av_jni_set_android_app_ctx() and av_jni_get_android_app_ctx().
> 
> Should be added together with patch 1/6.

Fixed.

> 
> > +
> >  2024-02-26 - xxxxxxxxxx - lavf 60.22.101 - avformat.h
> >    AV_DISPOSITION_DEPENDENT may now also be used for video streams
> >    intended to be merged with other video streams for presentation.
> > diff --git a/libavcodec/jni.c b/libavcodec/jni.c
> > index cfe95bd1ec..1193c608c3 100644
> > --- a/libavcodec/jni.c
> > +++ b/libavcodec/jni.c
> > @@ -35,6 +35,7 @@
> >  #include "ffjni.h"
> >  
> >  static void *java_vm;
> > +static void *android_app_ctx;
> >  static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> >  
> >  int av_jni_set_java_vm(void *vm, void *log_ctx)
> > diff --git a/libavformat/Makefile b/libavformat/Makefile
> > index 4a380668bd..08fe98a535 100644
> > --- a/libavformat/Makefile
> > +++ b/libavformat/Makefile
> > @@ -657,6 +657,7 @@ OBJS-$(CONFIG_LIBOPENMPT_DEMUXER)        += libopenmpt.o
> >  OBJS-$(CONFIG_VAPOURSYNTH_DEMUXER)       += vapoursynth.o
> >  
> >  # protocols I/O
> > +OBJS-$(CONFIG_ANDROID_CONTENT_PROTOCOL)  += file.o
> >  OBJS-$(CONFIG_ASYNC_PROTOCOL)            += async.o
> >  OBJS-$(CONFIG_APPLEHTTP_PROTOCOL)        += hlsproto.o
> >  OBJS-$(CONFIG_BLURAY_PROTOCOL)           += bluray.o
> > diff --git a/libavformat/file.c b/libavformat/file.c
> > index 64df7ff6fb..1b2b69f090 100644
> > --- a/libavformat/file.c
> > +++ b/libavformat/file.c
> > @@ -40,6 +40,12 @@
> >  #include <stdlib.h>
> >  #include "os_support.h"
> >  #include "url.h"
> > +#if CONFIG_ANDROID_CONTENT_PROTOCOL
> > +#include <jni.h>
> > +#include "libavcodec/jni.h"
> > +#include "libavcodec/ffjni.c"
> > +#endif
> > +
> >  
> >  /* Some systems may not have S_ISFIFO */
> >  #ifndef S_ISFIFO
> > @@ -101,6 +107,21 @@ typedef struct FileContext {
> >      int64_t initial_pos;
> >  } FileContext;
> >  
> > +
> > +#if CONFIG_ANDROID_CONTENT_PROTOCOL
> > +static const AVOption android_content_options[] = {
> > +    { "blocksize", "set I/O operation maximum block size", offsetof(FileContext, blocksize), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 1, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM },
> > +    { NULL }
> > +};
> > +
> > +static const AVClass android_content_class = {
> > +    .class_name = "android_content",
> > +    .item_name  = av_default_item_name,
> > +    .option     = android_content_options,
> > +    .version    = LIBAVUTIL_VERSION_INT,
> > +};
> > +#endif
> > +
> >  static const AVOption file_options[] = {
> >      { "truncate", "truncate existing files on write", offsetof(FileContext, trunc), AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, AV_OPT_FLAG_ENCODING_PARAM },
> >      { "blocksize", "set I/O operation maximum block size", offsetof(FileContext, blocksize), AV_OPT_TYPE_INT, { .i64 = INT_MAX }, 1, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM },
> > @@ -524,3 +545,139 @@ const URLProtocol ff_fd_protocol = {
> >  };
> >  
> >  #endif /* CONFIG_FD_PROTOCOL */
> > +
> > +#if CONFIG_ANDROID_CONTENT_PROTOCOL
> > +
> > +struct JFields {
> > +    jclass uri_class;
> > +    jmethodID parse_id;
> > +
> > +    jclass context_class;
> > +    jmethodID get_content_resolver_id;
> > +
> > +    jclass content_resolver_class;
> > +    jmethodID open_file_descriptor_id;
> > +
> > +    jclass parcel_file_descriptor_class;
> > +    jmethodID detach_fd_id;
> > +};
> 
> typedef struct is the preferred coding style of FFmpeg, it’s mentioned in
> developer.texi.

Fixed.

> 
> > +
> > +#define OFFSET(x) offsetof(struct JFields, x)
> > +static const struct FFJniField jfields_mapping[] = {
> > +    { "android/net/Uri", NULL, NULL, FF_JNI_CLASS, OFFSET(uri_class), 1 },
> > +        { "android/net/Uri", "parse", "(Ljava/lang/String;)Landroid/net/Uri;", FF_JNI_STATIC_METHOD, OFFSET(parse_id), 1 },
> > +
> > +    { "android/content/Context", NULL, NULL, FF_JNI_CLASS, OFFSET(context_class), 1 },
> > +        { "android/content/Context", "getContentResolver", "()Landroid/content/ContentResolver;", FF_JNI_METHOD, OFFSET(get_content_resolver_id), 1 },
> > +
> > +    { "android/content/ContentResolver", NULL, NULL, FF_JNI_CLASS, OFFSET(content_resolver_class), 1 },
> > +        { "android/content/ContentResolver", "openFileDescriptor", "(Landroid/net/Uri;Ljava/lang/String;)Landroid/os/ParcelFileDescriptor;", FF_JNI_METHOD, OFFSET(open_file_descriptor_id), 1 },
> > +
> > +    { "android/os/ParcelFileDescriptor", NULL, NULL, FF_JNI_CLASS, OFFSET(parcel_file_descriptor_class), 1 },
> > +        { "android/os/ParcelFileDescriptor", "detachFd", "()I", FF_JNI_METHOD, OFFSET(detach_fd_id), 1 },
> > +
> > +    { NULL }
> > +};
> > +#undef OFFSET
> > +
> > +static int android_content_open(URLContext *h, const char *filename, int flags)
> > +{
> > +    FileContext *c = h->priv_data;
> > +    int fd, ret;
> > +    const char *mode_str = "r";
> > +
> > +    JNIEnv *env;
> > +    struct JFields jfields = { 0 };
> > +    jobject application_context = NULL;
> > +    jobject url = NULL;
> > +    jobject mode = NULL;
> > +    jobject uri = NULL;
> > +    jobject content_resolver = NULL;
> > +    jobject parcel_file_descriptor = NULL;
> > +
> > +    env = ff_jni_get_env(c);
> > +    if (!env) {
> > +        return AVERROR(EINVAL);
> > +    }
> > +
> > +    ret = ff_jni_init_jfields(env, &jfields, jfields_mapping, 0, c);
> > +    if (ret < 0) {
> > +        av_log(c, AV_LOG_ERROR, "failed to initialize jni fields\n");
> > +        return ret;
> > +    }
> > +
> > +    application_context = av_jni_get_android_app_ctx();
> > +    if (!application_context) {
> > +        av_log(c, AV_LOG_ERROR, "application context is not set\n");
> > +        ret = AVERROR_EXTERNAL;
> > +        goto done;
> > +    }
> > +
> > +    url = ff_jni_utf_chars_to_jstring(env, filename, c);
> > +    if (!url) {
> > +        ret = AVERROR_EXTERNAL;
> > +        goto done;
> > +    }
> > +
> > +    if (flags & AVIO_FLAG_WRITE && flags & AVIO_FLAG_READ)
> > +        mode_str = "rw";
> > +    else if (flags & AVIO_FLAG_WRITE)
> > +        mode_str = "w";
> > +
> > +    mode = ff_jni_utf_chars_to_jstring(env, mode_str, c);
> > +    if (!mode) {
> > +        ret = AVERROR_EXTERNAL;
> > +        goto done;
> > +    }
> > +
> > +    uri = (*env)->CallStaticObjectMethod(env, jfields.uri_class, jfields.parse_id, url);
> > +    ret = ff_jni_exception_check(env, 1, c);
> > +    if (ret < 0)
> > +        goto done;
> > +
> > +    content_resolver = (*env)->CallObjectMethod(env, application_context, jfields.get_content_resolver_id);
> > +    ret = ff_jni_exception_check(env, 1, c);
> > +    if (ret < 0)
> > +        goto done;
> > +
> > +    parcel_file_descriptor = (*env)->CallObjectMethod(env, content_resolver, jfields.open_file_descriptor_id, uri, mode);
> > +    ret = ff_jni_exception_check(env, 1, c);
> > +    if (ret < 0)
> > +        goto done;
> > +
> > +    fd = (*env)->CallIntMethod(env, parcel_file_descriptor, jfields.detach_fd_id);
> > +    ret = ff_jni_exception_check(env, 1, c);
> > +    if (ret < 0)
> > +        goto done;
> > +
> > +#if HAVE_SETMODE
> > +    setmode(fd, O_BINARY);
> > +#endif
> 
> Since it’s android specific protocol, the conditional compile is always false, isn’t it?

You're right. Removed.

> 
> > +    c->fd = fd;
> > +    h->is_streamed = 0;
> 
> Firstly, is_streamed is default to false, so the above line is unnecessary.
> Secondly, I’m not sure whether it’s always seeable.

Added a check.

Thanks for the review, I've sent an updated patch series (v4).

Best regards,
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".