[v2,2/3] omx: Add support for broadcom OMX on raspberry pi

diff mbox

The raspberry pi uses the alternative API/ABI for OMX; this makes
such builds incompatible with all the normal OpenMAX implementations.
Since this can't easily be detected at configure time (one can
build for raspberry pi's OMX just fine using the generic, pristine
Khronos OpenMAX IL headers, no need for their own extensions),
require a separate configure switch for it instead.

The broadcom host library can't be unloaded once loaded and started;
the deinit function that it provides is a no-op, and after started,
it has got background threads running, so dlclosing it makes it
crash.
---
Factorized the component name check.
---
 configure        | 12 +++++++++++-
 libavcodec/omx.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 55 insertions(+), 7 deletions(-)

On Thu, Apr 07, 2016 at 02:43:48PM +0300, Martin Storsjö wrote:
> --- a/configure
> +++ b/configure
> @@ -226,6 +226,7 @@ External library support:
>    --enable-mmal            enable decoding via MMAL [no]
>    --enable-nvenc           enable encoding via NVENC [no]
>    --enable-omx             enable encoding via OpenMAX IL [no]
> +  --enable-rpi-omx         enable encoding via OpenMAX IL for Raspberry Pi [no]
>    --enable-openssl         enable openssl [no]
>    --enable-x11grab         enable X11 grabbing (legacy) [no]
>    --enable-zlib            enable zlib [autodetect]
> @@ -1262,6 +1263,7 @@ EXTERNAL_LIBRARY_LIST="
>  FEATURE_LIST="
>      hardcoded_tables
> +    rpi_omx
>      runtime_cpudetect

omx_rpi would make more sense IMO.

Diego

On 07/04/16 12:43, Martin Storsjö wrote:
> ...
> @@ -4608,7 +4610,15 @@ enabled mmal              && { check_lib interface/mmal/mmal.h mmal_port_connect
>                                      check_lib interface/mmal/mmal.h mmal_port_connect ; }
>                                  check_lib interface/mmal/mmal.h mmal_port_connect ; } ||
>                                 die "ERROR: mmal not found"; }
> -enabled omx               && { check_header OMX_Core.h || die "ERROR: OpenMAX IL headers not found"; }
> +if enabled rpi_omx; then
> +    enable omx
> +    add_cflags -DOMX_SKIP64BIT

Weird extra cflag to everything in the build?  IMO this would be better as another config option.

> +fi
> +enabled omx               && { check_header OMX_Core.h ||
> +                                { ! enabled cross_compile && enabled rpi_omx && {
> +                                    add_cflags -isystem/opt/vc/include/IL ; }
> +                                check_header OMX_Core.h ; } ||
> +                               die "ERROR: OpenMAX IL headers not found"; }
>  enabled openssl           && { check_pkg_config openssl openssl/ssl.h SSL_library_init && {
>                                 add_cflags $openssl_cflags && add_extralibs $openssl_libs; }||
>                                 check_lib openssl/ssl.h SSL_library_init -lssl -lcrypto ||
> diff --git a/libavcodec/omx.c b/libavcodec/omx.c
> index fbb6c72..da73762 100644
> --- a/libavcodec/omx.c
> +++ b/libavcodec/omx.c
> @@ -64,10 +64,12 @@ static int64_t from_omx_ticks(OMX_TICKS value) {
>              return AVERROR_UNKNOWN;                                       \
>          }                                                                 \
>      } while (0)
> +#define IS_BROADCOM() av_strstart(s->component_name, "OMX.broadcom.", NULL)
>  
>  typedef struct OMXContext {
>      int users;
>      void *lib;
> +    void *lib2;
>      OMX_ERRORTYPE (*ptr_Init)(void);
>      OMX_ERRORTYPE (*ptr_Deinit)(void);
>      OMX_ERRORTYPE (*ptr_ComponentNameEnum)(OMX_STRING, OMX_U32, OMX_U32);
> @@ -75,6 +77,7 @@ typedef struct OMXContext {
>      OMX_ERRORTYPE (*ptr_FreeHandle)(OMX_HANDLETYPE);
>      OMX_ERRORTYPE (*ptr_GetComponentsOfRole)(OMX_STRING, OMX_U32*, OMX_U8**);
>      OMX_ERRORTYPE (*ptr_GetRolesOfComponent)(OMX_STRING, OMX_U32*, OMX_U8**);
> +    void (*host_init)(void);
>  } OMXContext;
>  
>  static OMXContext *omx_context;
> @@ -87,9 +90,23 @@ static av_cold void *dlsym2(void *handle, const char *symbol, const char *prefix
>      return dlsym(handle, buf);
>  }
>  
> -static av_cold int omx_try_load(void *logctx, const char *libname, const char *prefix)
> +static av_cold int omx_try_load(void *logctx, const char *libname, const char *prefix, const char *libname2)
>  {
>      OMXContext *s = omx_context;
> +    if (libname2) {
> +        s->lib2 = dlopen(libname2, RTLD_NOW | RTLD_GLOBAL);
> +        if (!s->lib2) {
> +            av_log(logctx, AV_LOG_WARNING, "%s not found\n", libname);
> +            return AVERROR_ENCODER_NOT_FOUND;
> +        }
> +        s->host_init = dlsym(s->lib2, "bcm_host_init");
> +        if (!s->host_init) {
> +            av_log(logctx, AV_LOG_WARNING, "bcm_host_init not found\n");
> +            dlclose(s->lib2);
> +            s->lib2 = NULL;
> +            return AVERROR_ENCODER_NOT_FOUND;
> +        }
> +    }
>      s->lib = dlopen(libname, RTLD_NOW | RTLD_GLOBAL);
>      if (!s->lib) {
>          av_log(logctx, AV_LOG_WARNING, "%s not found\n", libname);
> @@ -108,6 +125,9 @@ static av_cold int omx_try_load(void *logctx, const char *libname, const char *p
>          av_log(logctx, AV_LOG_WARNING, "Not all functions found in %s\n", libname);
>          dlclose(s->lib);
>          s->lib = NULL;
> +        if (s->lib2)
> +            dlclose(s->lib2);
> +        s->lib2 = NULL;
>          return AVERROR_ENCODER_NOT_FOUND;
>      }
>      return 0;
> @@ -115,8 +135,12 @@ static av_cold int omx_try_load(void *logctx, const char *libname, const char *p
>  
>  static av_cold int omx_init(void *logctx, const char *libname, const char *prefix) {
>      static const char * const libnames[] = {
> -        "libOMX_Core.so",
> -        "libOmxCore.so",
> +#if CONFIG_RPI_OMX
> +        "/opt/vc/lib/libopenmaxil.so", "/opt/vc/lib/libbcm_host.so",
> +#else
> +        "libOMX_Core.so", NULL,
> +        "libOmxCore.so", NULL,
> +#endif
>          NULL
>      };
>      const char* const* nameptr;
> @@ -136,14 +160,14 @@ static av_cold int omx_init(void *logctx, const char *libname, const char *prefi
>      }
>      omx_context->users = 1;
>      if (libname) {
> -        ret = omx_try_load(logctx, libname, prefix);
> +        ret = omx_try_load(logctx, libname, prefix, NULL);
>          if (ret < 0) {
>              pthread_mutex_unlock(&omx_context_mutex);
>              return ret;
>          }
>      } else {
> -        for (nameptr = libnames; *nameptr; nameptr++)
> -            if (!(ret = omx_try_load(logctx, *nameptr, prefix)))
> +        for (nameptr = libnames; *nameptr; nameptr += 2)
> +            if (!(ret = omx_try_load(logctx, nameptr[0], prefix, nameptr[1])))
>                  break;
>          if (!*nameptr) {
>              pthread_mutex_unlock(&omx_context_mutex);
> @@ -151,6 +175,8 @@ static av_cold int omx_init(void *logctx, const char *libname, const char *prefi
>          }
>      }
>  
> +    if (omx_context->host_init)
> +        omx_context->host_init();
>      omx_context->ptr_Init();
>      pthread_mutex_unlock(&omx_context_mutex);
>      return 0;
> @@ -277,6 +303,12 @@ static av_cold int find_component(void *logctx, const char *role, char *str,
>      char **components;
>      int ret = 0;
>  
> +#if CONFIG_RPI_OMX
> +    if (av_strstart(role, "video_encoder.", NULL)) {
> +        av_strlcpy(str, "OMX.broadcom.video_encode", str_size);
> +        return 0;
> +    }
> +#endif
>      omx_context->ptr_GetComponentsOfRole((OMX_STRING) role, &num, NULL);
>      if (!num) {
>          av_log(logctx, AV_LOG_WARNING, "No component for role %s found\n", role);
> @@ -326,6 +358,8 @@ static av_cold int omx_component_init(AVCodecContext *avctx, const char *role)
>  
>      s->version.s.nVersionMajor = 1;
>      s->version.s.nVersionMinor = 1;
> +    if (IS_BROADCOM())
> +        s->version.s.nRevision = 2;
>  
>      err = omx_context->ptr_GetHandle(&s->handle, s->component_name, s, (OMX_CALLBACKTYPE*) &callbacks);
>      if (err != OMX_ErrorNone) {
> @@ -393,6 +427,10 @@ static av_cold int omx_component_init(AVCodecContext *avctx, const char *role)
>      in_port_params.format.video.eColorFormat = s->color_format;
>      s->stride = avctx->width;
>      s->plane_size = avctx->height;
> +    if (IS_BROADCOM()) {
> +        s->stride = FFALIGN(s->stride, 16);
> +        s->plane_size = FFALIGN(s->plane_size, 16);
> +    }
>      in_port_params.format.video.nStride = s->stride;
>      in_port_params.format.video.nSliceHeight = s->plane_size;
>      if (avctx->framerate.den > 0 && avctx->framerate.num > 0)
> 

All a bit nasty, but I guess it works.

Thanks,

- Mark

On Sat, 9 Apr 2016, Mark Thompson wrote:

> On 07/04/16 12:43, Martin Storsjö wrote:
>> ...
>> @@ -4608,7 +4610,15 @@ enabled mmal              && { check_lib interface/mmal/mmal.h mmal_port_connect
>>                                      check_lib interface/mmal/mmal.h mmal_port_connect ; }
>>                                  check_lib interface/mmal/mmal.h mmal_port_connect ; } ||
>>                                 die "ERROR: mmal not found"; }
>> -enabled omx               && { check_header OMX_Core.h || die "ERROR: OpenMAX IL headers not found"; }
>> +if enabled rpi_omx; then
>> +    enable omx
>> +    add_cflags -DOMX_SKIP64BIT
>
> Weird extra cflag to everything in the build?

Yes; my other option was to have this at the start of omx.c:
---
#include "config.h"

#if CONFIG_RPI_OMX
#define OMX_SKIP64BIT
#endif

#include <OMX_Core.h>
---
That's probably cleaner in one sense, but e.g. if the omx code would be 
split over multiple files, having it in the global cflags feels cleaner.

> IMO this would be better as another config option.

You mean to have a separate --enable-omx-skip64bit? For other cases of 
generic omx, that might make sense (or the user could perhaps just add 
--extra-cflags=-DOMX_SKIP64BIT). But if enabling raspberry pi features it 
really should be automatic at least. The problem with such an option is 
that it is really hard as a user to know whether you need it or not. If 
you need it but don't set it (e.g. on rpi), things generally will work, 
but the timestamps get messed up (the lower 32 bit get shifted up into the 
higher half, and the lower half has garbage, or vice versa).

So until the code is extended/tested to actually concretely work with 
another driver that needs it, I'd be a little hesitant adding such an 
option.

// Martin

On Thu, 7 Apr 2016, Diego Biurrun wrote:

> On Thu, Apr 07, 2016 at 02:43:48PM +0300, Martin Storsjö wrote:
>> --- a/configure
>> +++ b/configure
>> @@ -226,6 +226,7 @@ External library support:
>>    --enable-mmal            enable decoding via MMAL [no]
>>    --enable-nvenc           enable encoding via NVENC [no]
>>    --enable-omx             enable encoding via OpenMAX IL [no]
>> +  --enable-rpi-omx         enable encoding via OpenMAX IL for Raspberry Pi [no]
>>    --enable-openssl         enable openssl [no]
>>    --enable-x11grab         enable X11 grabbing (legacy) [no]
>>    --enable-zlib            enable zlib [autodetect]
>> @@ -1262,6 +1263,7 @@ EXTERNAL_LIBRARY_LIST="
>>  FEATURE_LIST="
>>      hardcoded_tables
>> +    rpi_omx
>>      runtime_cpudetect
>
> omx_rpi would make more sense IMO.

Sure - do others agree as well?

// Martin

On 09/04/16 20:11, Martin Storsjö wrote:
> On Sat, 9 Apr 2016, Mark Thompson wrote:
> 
>> On 07/04/16 12:43, Martin Storsjö wrote:
>>> ...
>>> @@ -4608,7 +4610,15 @@ enabled mmal              && { check_lib interface/mmal/mmal.h mmal_port_connect
>>>                                      check_lib interface/mmal/mmal.h mmal_port_connect ; }
>>>                                  check_lib interface/mmal/mmal.h mmal_port_connect ; } ||
>>>                                 die "ERROR: mmal not found"; }
>>> -enabled omx               && { check_header OMX_Core.h || die "ERROR: OpenMAX IL headers not found"; }
>>> +if enabled rpi_omx; then
>>> +    enable omx
>>> +    add_cflags -DOMX_SKIP64BIT
>>
>> Weird extra cflag to everything in the build?
> 
> Yes; my other option was to have this at the start of omx.c:
> ---
> #include "config.h"
> 
> #if CONFIG_RPI_OMX
> #define OMX_SKIP64BIT
> #endif
> 
> #include <OMX_Core.h>
> ---
> That's probably cleaner in one sense, but e.g. if the omx code would be split over multiple files, having it in the global cflags feels cleaner.
> 
>> IMO this would be better as another config option.
> 
> You mean to have a separate --enable-omx-skip64bit? For other cases of generic omx, that might make sense (or the user could perhaps just add --extra-cflags=-DOMX_SKIP64BIT). But if enabling raspberry pi features it really should be automatic at least. The problem with such an option is that it is really hard as a user to know whether you need it or not. If you need it but don't set it (e.g. on rpi), things generally will work, but the timestamps get messed up (the lower 32 bit get shifted up into the higher half, and the lower half has garbage, or vice versa).

Not quite.  I was thinking of something in CONFIG_EXTRA (CONFIG_OMX_SKIP64BIT, then?), not user-visible and automatically set by CONFIG_RPI_OMX and any other implementations which require it.  Then the code above, testing that option rather than OMX_RPI.

> So until the code is extended/tested to actually concretely work with another driver that needs it, I'd be a little hesitant adding such an option.

Maybe this is the most important point in any case.  I don't really mind, so do whatever you feel is best.

- Mark

On 09/04/16 20:11, Martin Storsjö wrote:
> On Thu, 7 Apr 2016, Diego Biurrun wrote:
> 
>> On Thu, Apr 07, 2016 at 02:43:48PM +0300, Martin Storsjö wrote:
>>> --- a/configure
>>> +++ b/configure
>>> @@ -226,6 +226,7 @@ External library support:
>>>    --enable-mmal            enable decoding via MMAL [no]
>>>    --enable-nvenc           enable encoding via NVENC [no]
>>>    --enable-omx             enable encoding via OpenMAX IL [no]
>>> +  --enable-rpi-omx         enable encoding via OpenMAX IL for Raspberry Pi [no]
>>>    --enable-openssl         enable openssl [no]
>>>    --enable-x11grab         enable X11 grabbing (legacy) [no]
>>>    --enable-zlib            enable zlib [autodetect]
>>> @@ -1262,6 +1263,7 @@ EXTERNAL_LIBRARY_LIST="
>>>  FEATURE_LIST="
>>>      hardcoded_tables
>>> +    rpi_omx
>>>      runtime_cpudetect
>>
>> omx_rpi would make more sense IMO.
> 
> Sure - do others agree as well?
> 

Yes.  (If only for doing the right thing in alphabeticised lists.)

Quoting Martin Storsjö (2016-04-09 21:11:09)
> On Sat, 9 Apr 2016, Mark Thompson wrote:
> 
> > On 07/04/16 12:43, Martin Storsjö wrote:
> >> ...
> >> @@ -4608,7 +4610,15 @@ enabled mmal              && { check_lib interface/mmal/mmal.h mmal_port_connect
> >>                                      check_lib interface/mmal/mmal.h mmal_port_connect ; }
> >>                                  check_lib interface/mmal/mmal.h mmal_port_connect ; } ||
> >>                                 die "ERROR: mmal not found"; }
> >> -enabled omx               && { check_header OMX_Core.h || die "ERROR: OpenMAX IL headers not found"; }
> >> +if enabled rpi_omx; then
> >> +    enable omx
> >> +    add_cflags -DOMX_SKIP64BIT
> >
> > Weird extra cflag to everything in the build?
> 
> Yes; my other option was to have this at the start of omx.c:
> ---
> #include "config.h"
> 
> #if CONFIG_RPI_OMX
> #define OMX_SKIP64BIT
> #endif
> 
> #include <OMX_Core.h>
> ---
> That's probably cleaner in one sense, but e.g. if the omx code would be 
> split over multiple files, having it in the global cflags feels cleaner.

Then you could have it in an omx.h header.