[vlc-devel] [PATCH 3/3] mediacodec: implementation of MediaCodec direct rendering based on the work by Martin Storsjö.

[Martin Storsjö]

On Mon, 18 Nov 2013, Felix Abecassis wrote:

> Somehow the new files were not added to the patch after rebasing.
> Resending.
>
> The decoder stores opaque buffers in the p_sys member of the picture
> and the vout uses a callback from the decoder to render these
> buffers. When the decoder flushes or closes, all the currently in
> flight pictures (filled by the decoder but not displayed yet) need to
> be invalidated. A mutex is required in order to prevent the vout from using
> destroyed MediaCodec buffers.
> ---
> modules/codec/Makefile.am                |   2 +-
> modules/codec/omxil/android_mediacodec.c | 222 +++++++++++++++++++++++++++----
> modules/codec/omxil/android_opaque.c     |  30 +++++
> modules/codec/omxil/android_opaque.h     |  44 ++++++
> modules/video_output/Modules.am          |   1 +
> modules/video_output/android/opaque.c    | 204 ++++++++++++++++++++++++++++
> modules/video_output/android/surface.c   |   3 +
> 7 files changed, 477 insertions(+), 29 deletions(-)
> create mode 100644 modules/codec/omxil/android_opaque.c
> create mode 100644 modules/codec/omxil/android_opaque.h
> create mode 100644 modules/video_output/android/opaque.c
>

> @@ -392,9 +440,79 @@ static void CloseDecoder(vlc_object_t *p_this)
>
>     free(p_sys->name);
>     ArchitectureSpecificCopyHooksDestroy(p_sys->pixel_format, &p_sys->architecture_specific_data);
> +    free(p_sys->inflight_picture);
>     free(p_sys);
> }
>
> +/*****************************************************************************
> + * vout callbacks
> + *****************************************************************************/
> +static void DisplayBuffer(picture_sys_t* p_picsys, bool b_render)
> +{
> +    decoder_t *p_dec = p_picsys->p_dec;
> +    decoder_sys_t *p_sys = p_dec->p_sys;
> +
> +    if (!p_picsys->b_valid)
> +        return;
> +
> +    vlc_mutex_lock(get_android_opaque_mutex());
> +
> +    /* Picture might have been invalidated while waiting on the mutex. */
> +    if (!p_picsys->b_valid)
> +    {
> +        vlc_mutex_unlock(get_android_opaque_mutex());
> +        return;
> +    }
> +
> +    uint32_t i_index = p_picsys->i_index;
> +    p_sys->inflight_picture[i_index] = NULL;
> +
> +    /* Release the MediaCodec buffer. */
> +    JNIEnv *env = NULL;
> +    (*myVm)->AttachCurrentThread(myVm, &env, NULL);
> +    (*env)->CallVoidMethod(env, p_sys->codec, p_sys->release_output_buffer, i_index, b_render);
> +    jthrowable exception = (*env)->ExceptionOccurred(env);
> +    if (exception != NULL) {
> +        jclass illegalStateException = (*env)->FindClass(env, "java/lang/IllegalStateException");
> +        if ((*env)->IsInstanceOf(env, exception, illegalStateException)) {
> +            msg_Err(p_dec, "Codec error (IllegalStateException) in MediaCodec.releaseOutputBuffer (ReleaseOutputBuffer)");
> +            (*env)->ExceptionClear(env);
> +            (*env)->DeleteLocalRef(env, illegalStateException);
> +        }

Why do you need to explicitly check for this particular exception class? 
This is the most likely exception to be thrown (most of the MediaCodec 
methods throw this when something unexpected happens). This is the only 
place where we handle exceptions where we actually check what class it is. 
If we don't do ExceptionClear, the JNI env will abort on the next JNI 
call. Not sure if that's better or worse than just ignoring the exception 
and hoping that it will sort itself out (at least exiting cleanly). The 
code would at least be shorter and simpler without the explicit check.



>             return;
>         } else if (index == INFO_OUTPUT_BUFFERS_CHANGED) {
>             msg_Dbg(p_dec, "output buffers changed");
>             (*env)->DeleteGlobalRef(env, p_sys->output_buffers);
> +
>             p_sys->output_buffers = (*env)->CallObjectMethod(env, p_sys->codec,
>                                                              p_sys->get_output_buffers);
>             p_sys->output_buffers = (*env)->NewGlobalRef(env, p_sys->output_buffers);
> +
> +            vlc_mutex_lock(get_android_opaque_mutex());
> +            free(p_sys->inflight_picture);
> +            p_sys->i_output_buffers = (*env)->GetArrayLength(env, p_sys->output_buffers);
> +            p_sys->inflight_picture = calloc(1, sizeof(picture_t*) * p_sys->i_output_buffers);
> +            vlc_mutex_unlock(get_android_opaque_mutex());
> +
>         } else if (index == INFO_OUTPUT_FORMAT_CHANGED) {
>
>             jobject format = (*env)->CallObjectMethod(env, p_sys->codec, p_sys->get_output_format);
> @@ -468,7 +623,13 @@ static void GetOutput(decoder_t *p_dec, JNIEnv *env, picture_t **pp_pic)
>             int crop_bottom     = GET_INTEGER(format, "crop-bottom");
>
>             const char *name = "unknown";
> -            GetVlcChromaFormat(p_sys->pixel_format, &p_dec->fmt_out.i_codec, &name);
> +            if (p_sys->direct_rendering)
> +            {
> +                jni_SetAndroidSurfaceSizeEnv(env, width, height, width, height, 1, 1);
> +            }
> +            else
> +                GetVlcChromaFormat(p_sys->pixel_format, &p_dec->fmt_out.i_codec, &name);
> +

nit: While VLC generally has favoured this indentation style, it's not the 
general norm all over the place. Most of this file places the braces on 
the same line as the if/else, so it would be more consistent with the rest 
of the file in that form.

> diff --git a/modules/codec/omxil/android_opaque.c b/modules/codec/omxil/android_opaque.c
> new file mode 100644
> index 0000000..7a06842
> --- /dev/null
> +++ b/modules/codec/omxil/android_opaque.c
> @@ -0,0 +1,30 @@
> +
> +#include "android_opaque.h"
> +
> +vlc_mutex_t* get_android_opaque_mutex()
> +{
> +    static vlc_mutex_t s_mutex = VLC_STATIC_MUTEX;
> +    return &s_mutex;
> +}

This is a little bit hackish - having one plugin define a public symbol 
that other plugins directly link against. Yes, it works on android right 
now when all the plugins are statically linked into one big blob, but it 
violates general VLC plugin designs. (No, I don't have other suggestions 
either.)


In general the patch looks ok once these details have been 
fixed/discussed/agreed upon, but the deadlock on shutdown that I saw would 
have to be fixed.

// Martin