[FFmpeg-devel] [PATCH 4/4] vaapi: add interfaces to properly initialize context.

[wm4]

On Mon, 17 Aug 2015 19:17:53 +0200
Gwenole Beauchesne <gb.devel at gmail.com> wrote:

> Add av_vaapi_context_init() and av_vaapi_context_alloc() helper functions
> so that to properly initialize the vaapi_context structure, and allow for
> future extensions without breaking the API/ABI.
> 
> The new version and flags fields are inserted after the last known user
> initialized field, and actually useful field at all, i.e. VA context_id.
> This is safe because the vaapi_context structure was required to be zero
> initialized in the past. And, since those new helper functions and changes
> cause the AV_VAAPI_CONTEXT_VERSION to be bumped, we can track older struct
> requirements from within libavcodec.
> 
> Besides, it is now required by design, and actual usage, that the vaapi
> context structure be completely initialized once and for all before the
> AVCodecContext.get_format() function ever completes.
> 
> Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne at intel.com>
> ---
>  libavcodec/vaapi.c          | 30 +++++++++++++++++++++++
>  libavcodec/vaapi.h          | 59 +++++++++++++++++++++++++++++++++++++++++----
>  libavcodec/vaapi_internal.h |  1 +
>  3 files changed, 85 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/vaapi.c b/libavcodec/vaapi.c
> index 1ae71a5..4d3e2f3 100644
> --- a/libavcodec/vaapi.c
> +++ b/libavcodec/vaapi.c
> @@ -41,11 +41,41 @@ static void destroy_buffers(VADisplay display, VABufferID *buffers, unsigned int
>      }
>  }
>  
> +/* Allocates a vaapi_context structure */
> +struct vaapi_context *av_vaapi_context_alloc(unsigned int flags)
> +{
> +    struct vaapi_context *vactx;
> +
> +    vactx = av_malloc(sizeof(*vactx));
> +    if (!vactx)
> +        return NULL;
> +
> +    av_vaapi_context_init(vactx, AV_VAAPI_CONTEXT_VERSION, flags);
> +    return vactx;
> +}
> +
> +/* Initializes a vaapi_context structure with safe defaults */
> +void av_vaapi_context_init(struct vaapi_context *vactx, unsigned int version,
> +                           unsigned int flags)
> +{
> +    vactx->display      = NULL;
> +    vactx->config_id    = VA_INVALID_ID;
> +    vactx->context_id   = VA_INVALID_ID;
> +
> +    if (version > 0) {
> +        vactx->version  = version;
> +        vactx->flags    = flags;
> +    }
> +}
> +
>  int ff_vaapi_context_init(AVCodecContext *avctx)
>  {
>      FFVAContext * const vactx = ff_vaapi_get_context(avctx);
>      const struct vaapi_context * const user_vactx = avctx->hwaccel_context;
>  
> +    if (user_vactx->version > 0)
> +        vactx->flags            = user_vactx->flags;
> +
>      vactx->display              = user_vactx->display;
>      vactx->config_id            = user_vactx->config_id;
>      vactx->context_id           = user_vactx->context_id;
> diff --git a/libavcodec/vaapi.h b/libavcodec/vaapi.h
> index 4448a2e..1f032a0 100644
> --- a/libavcodec/vaapi.h
> +++ b/libavcodec/vaapi.h
> @@ -40,14 +40,16 @@
>   * @{
>   */
>  
> +#define AV_VAAPI_CONTEXT_VERSION 1
> +
>  /**
>   * This structure is used to share data between the FFmpeg library and
>   * the client video application.
> - * This shall be zero-allocated and available as
> - * AVCodecContext.hwaccel_context. All user members can be set once
> - * during initialization or through each AVCodecContext.get_buffer()
> - * function call. In any case, they must be valid prior to calling
> - * decoding functions.
> + *
> + * This shall be initialized with av_vaapi_context_init(), or
> + * indirectly through av_vaapi_context_alloc(), and made available as
> + * AVCodecContext.hwaccel_context. All user members must be properly
> + * initialized before AVCodecContext.get_format() completes.
>   */
>  struct vaapi_context {
>      /**
> @@ -74,6 +76,29 @@ struct vaapi_context {
>       */
>      uint32_t context_id;
>  
> +    /**
> +     * This field must be set to AV_VAAPI_CONTEXT_VERSION
> +     *
> +     * @since Version 1.
> +     *
> +     * - encoding: unused
> +     * - decoding: Set by user, through av_vaapi_context_init()
> +     */
> +    unsigned int version;

Not sure if I see any point in this. Normally, backwards-compatible ABI
additions can add fields in FFmpeg, but the API user doesn't get a way
to check whether a field is really there.

Or in other words, the level of ABI compatibility we target is that
newer libav* libs work with old application binaries, but not the other
way around.

> +
> +    /**
> +     * A bit field configuring the internal context used by libavcodec
> +     *
> +     * This is a combination of flags from common AV_HWACCEL_FLAG_xxx and
> +     * from VA-API specific AV_VAAPI_FLAG_xxx.
> +     *
> +     * @since Version 1.
> +     *
> +     * - encoding: unused
> +     * - decoding: Set by user, through av_vaapi_context_init()
> +     */
> +    unsigned int flags;

I'd say just leave it private, and the user can only initialize it with
av_vaapi_context_init().

> +
>  #if FF_API_VAAPI_CONTEXT
>      /**
>       * VAPictureParameterBuffer ID
> @@ -184,6 +209,30 @@ struct vaapi_context {
>  #endif
>  };
>  
> +/**
> + * Allocates a vaapi_context structure.
> + *
> + * This function allocates and initializes a vaapi_context with safe
> + * defaults, e.g. with proper invalid ids for VA config and context.
> + *
> + * The resulting structure can be deallocated with av_freep().
> + *
> + * @param[in]  flags    zero, or a combination of AV_HWACCEL_FLAG_xxx or
> + *     AV_VAAPI_FLAG_xxx flags OR'd altogether.
> + * @return Newly-allocated struct vaapi_context, or NULL on failure
> + */
> +struct vaapi_context *av_vaapi_context_alloc(unsigned int flags);
> +
> +/**
> + * Initializes a vaapi_context structure with safe defaults.
> + *
> + * @param[in]  version  this must be set to AV_VAAPI_CONTEXT_VERSION
> + * @param[in]  flags    zero, or a combination of AV_HWACCEL_FLAG_xxx or
> + *     AV_VAAPI_FLAG_xxx flags OR'd altogether.
> + */
> +void av_vaapi_context_init(struct vaapi_context *vactx, unsigned int version,
> +                           unsigned int flags);
> +
>  /* @} */
>  
>  #endif /* AVCODEC_VAAPI_H */
> diff --git a/libavcodec/vaapi_internal.h b/libavcodec/vaapi_internal.h
> index 29f46ab..a7c69e6 100644
> --- a/libavcodec/vaapi_internal.h
> +++ b/libavcodec/vaapi_internal.h
> @@ -36,6 +36,7 @@
>   */
>  
>  typedef struct {
> +    unsigned int flags;                 ///< Configuration flags
>      VADisplay display;                  ///< Windowing system dependent handle
>      VAConfigID config_id;               ///< Configuration ID
>      VAContextID context_id;             ///< Context ID (video decode pipeline)

This looks fine and all... but in the future it'd be nice if we had
something like av_vdpau_bind_context(), which actually takes care of
mapping the codec profiles and initializing the decoder.

I'm not sure through how many API iterations I want to go through
myself... (for vdpau hwaccel, we had at least 3 APIs now.)

If it's not much trouble, maybe you could consider going into this
direction?

The patch looks good, but a clear documentation whether the user is nor
is not allowed to allocate the struct directly is missing. If it's not
allowed, this would be a breaking API change, but maybe nothing which
requires a major bump.

doc/APIchanges needs additions in any case. (Patches 1 and 2 also do,
I forgot to mention this.)