[vlc-devel] [RFC PATCH 6/6] omxil: add android hw buffers support (zero copy)

[Martin Storsjö]

On Mon, 30 Jun 2014, Thomas Guillem wrote:

> Activated if cfg "omxil-dr" is true. If extra android symbols are not found or
> if HwBuffer_init() fails: fall back to non direct buffer mode
> ---
> Fixed patch to use pInputPortPrivate for hwbuf FIFO. No problem to use the same
> pointer than the OmxPort FIFO since theses FIFO are not used at the same time.
> modules/codec/omxil/omxil.c |  643 +++++++++++++++++++++++++++++++++++++++----
> modules/codec/omxil/omxil.h |   32 +++
> 2 files changed, 615 insertions(+), 60 deletions(-)

This is only a first review - I'll have more comments on the code later 
when I've actually been able to test it to see how it behaves.


> diff --git a/modules/codec/omxil/omxil.c b/modules/codec/omxil/omxil.c
> index 0f75a92..9c69a6b 100644
> --- a/modules/codec/omxil/omxil.c
> +++ b/modules/codec/omxil/omxil.c
> @@ -28,6 +28,9 @@
> # include "config.h"
> #endif
>
> +#include <dlfcn.h>
> +
> +#include <jni.h>

You'll need to hide the jni.h include behind #ifdef HAVE_IOMX, since a 
normal non-IOMX build (e.g. as for raspberry pi) won't necessarily have 
jni.h. To test this, you can either do a plain linux (or perhaps OS X as 
well) build of vlc, and add --enable-omxil there, and see if it builds. Or 
comment out jni.h here and "../../video_output/android/utils.h" in 
omxil.h, and find what other parts you need to hide to make the rest of it 
build.

> @@ -959,7 +1045,8 @@ static int OpenGeneric( vlc_object_t *p_this, bool b_encode )
>     p_sys->in.p_fmt = &p_dec->fmt_in;
>     OMX_FIFO_INIT (&p_sys->out.fifo, pInputPortPrivate );
>     OMX_BUFFER_COUNT_INIT( &p_sys->out.omx_count);
> -    p_sys->out.b_direct = false;
> +    p_sys->out.b_direct = jni_IsVideoPlayerActivityCreated() && var_InheritBool(p_dec, CFG_PREFIX "dr");
> +    p_sys->out.b_direct = true;

This doesn't seem like what you intended

>     p_sys->out.b_flushed = true;
>     p_sys->out.p_fmt = &p_dec->fmt_out;
>     p_sys->ports = 2;
> @@ -1139,6 +1226,372 @@ static int OpenGeneric( vlc_object_t *p_this, bool b_encode )
>     return VLC_EGENERIC;
> }
>
> +static void HwBuffer_ChangeState( decoder_t *p_dec, OmxPort *p_port,
> +                                  int i_index, int i_state )
> +{
> +    p_port->p_hwbuf->i_states[i_index] = i_state;
> +    if( i_state == BUF_STATE_OWNED )
> +        p_port->p_hwbuf->i_owned++;
> +    else
> +        p_port->p_hwbuf->i_owned--;
> +
> +#ifdef OMXIL_EXTRA_DEBUG
> +    msg_Dbg( p_dec, "buffer[%d]: state -> %d, owned buffers: %u",
> +             i_index, i_state, p_port->p_hwbuf->i_owned );
> +#endif
> +}
> +
> +static void HwBuffer_Init( decoder_t *p_dec, OmxPort *p_port )
> +{
> +    VLC_UNUSED( p_dec );
> +
> +    if( !p_port->b_direct )
> +        return;
> +
> +    msg_Dbg( p_dec, "HwBuffer_Init");
> +
> +    p_port->p_hwbuf = calloc(1, sizeof(HwBuffer));
> +    if( !p_port->p_hwbuf )
> +    {
> +        goto error;
> +    }
> +    OMX_FIFO_INIT (&p_port->p_hwbuf->fifo, pInputPortPrivate );
> +    vlc_cond_init (&p_port->p_hwbuf->wait);
> +    p_port->p_hwbuf->p_library = LoadNativeWindowAPI(&p_port->p_hwbuf->native_window);
> +    if( p_port->p_hwbuf->p_library )
> +    {
> +        void *surf = jni_LockAndGetAndroidJavaSurface();
> +
> +        if( surf ) {
> +            JNIEnv *p_env;
> +
> +            (*myVm)->AttachCurrentThread(myVm, &p_env, NULL);
> +            p_port->p_hwbuf->window = p_port->p_hwbuf->native_window.winFromSurface(p_env, surf);
> +            (*myVm)->DetachCurrentThread(myVm);
> +            pf_omx_hwbuffer_connect(p_port->p_hwbuf->window);
> +        }

You should probably check whether pf_omx_hwbuffer_connect is non-null 
before trying to do this at all (or move the checks below up before and 
just check p_port->p_hwbuf->window after this)

> +        jni_UnlockAndroidSurface();
> +    }
> +    if( !(pf_enable_graphic_buffers && pf_get_graphic_buffer_usage &&
> +          pf_omx_hwbuffer_connect && pf_omx_hwbuffer_disconnect &&
> +          pf_omx_hwbuffer_setup && pf_omx_hwbuffer_setcrop &&
> +          pf_omx_hwbuffer_dequeue && pf_omx_hwbuffer_queue &&
> +          pf_omx_hwbuffer_cancel) ||
> +        !((OMX_COMPONENTTYPE*)p_port->omx_handle)->UseBuffer ||
> +        !p_port->p_hwbuf->window )
> +    {
> +        msg_Warn( p_dec, "direct output port enabled but can't found "

s/found/find/

> +                          "extra symbols, switch back to non direct" );
> +        goto error;
> +    }
> +
> +    msg_Dbg( p_dec, "direct output port enabled" );
> +    return;
> +error:
> +    /* if HwBuffer_Init fails, we can fall back to non direct buffers */
> +    HwBuffer_Destroy( p_dec, p_port );
> +}
> +
> +static void HwBuffer_Destroy( decoder_t *p_dec, OmxPort *p_port )
> +{
> +    if( p_port->p_hwbuf )
> +    {
> +        if( p_port->p_hwbuf->p_library )
> +        {
> +            if( p_port->p_hwbuf->window )
> +            {
> +                HwBuffer_FreeBuffers( p_dec, p_port );
> +                pf_omx_hwbuffer_disconnect(p_port->p_hwbuf->window);
> +                p_port->p_hwbuf->native_window.winRelease( p_port->p_hwbuf->window );
> +            }
> +            dlclose( p_port->p_hwbuf->p_library );
> +        }
> +
> +        vlc_cond_destroy( &p_port->p_hwbuf->wait );
> +        OMX_FIFO_DESTROY( &p_port->p_hwbuf->fifo );
> +        free( p_port->p_hwbuf );
> +        p_port->p_hwbuf = NULL;
> +    }
> +    p_port->b_direct = false;
> +}
> +
> +static int HwBuffer_AllocateBuffers( decoder_t *p_dec, OmxPort *p_port )
> +{
> +    OMX_PARAM_PORTDEFINITIONTYPE *def = &p_port->definition;
> +    unsigned int min_undequeued = 0;
> +    unsigned int num_frames = p_port->definition.nBufferCountActual;
> +
> +    if( !p_port->p_hwbuf )
> +        return 0;
> +

To make it work on the Galaxy S3 (Exynos based), I had to add something 
similar to this here:

+    decoder_sys_t *p_sys = p_dec->p_sys;
+    int colorFormat = def->format.video.eColorFormat;

      if( !p_port->p_hwbuf )
          return 0;

+    if( !strncmp( p_sys->psz_component, "OMX.SEC.", 8 ) ) {
+        switch (colorFormat) {
+        case OMX_COLOR_FormatYUV420SemiPlanar:
+            colorFormat = 0x105; // HAL_PIXEL_FORMAT_YCbCr_420_SP
+            break;
+        case OMX_COLOR_FormatYUV420Planar:
+            colorFormat = 0x101; // HAL_PIXEL_FORMAT_YCbCr_420_P
+            break;
+        }
+    }
+
      if( pf_omx_hwbuffer_setup(p_port->p_hwbuf->window,
                            def->format.video.nFrameWidth,
                            def->format.video.nFrameHeight,
-                          def->format.video.eColorFormat,
+                          colorFormat,

I found the necessary constants in some cyanogenmod source at some point, 
but I'm not sure how to know exactly which devices need this override (or 
even worse, which firmware versions).


I'll defer reviewing the rest of it until either of us actually gets it 
working on some device where I can test it.

// Martin