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

[Hendrik Leppkes]

On Tue, Aug 18, 2015 at 10:33 AM, Gwenole Beauchesne <gb.devel at gmail.com> wrote:
> Hi,
>
> 2015-08-17 22:26 GMT+02:00 wm4 <nfxjfg at googlemail.com>:
>> 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.
>
> Yes, and I have reasons to see why this mechanism is needed.
>
> Imagine some old application binaries doesn't allocate the context
> through the supplied helper functions. Without knowing the original
> layout of the user supplied vaapi_context, we could be in a situation
> where we read past the end of the struct. The current patch series
> doesn't present it, but future ones can enlarge the public
> vaapi_context with various other needed fields for configuration.
> Aside of "hwaccel v2 plans", this is an interim solution to be free
> here.
>
> e.g. version 2 fields could include n_scratch_surfaces, version 3
> fields could include pix_fmt (unless it is allowed to the user to
> change sw_pix_fmt), etc. Since that information will be used lazily,
> depending on the flags, having a way to identify the public
> vaapi_context version is desired and necessary I'd say.

But you are adding new fields to the struct now, so any application
that does allocate it manually will already fail.
Its better to just bump the API, rename the struct, and force all
applications to use the allocation function if they want to continue
working in the future. This is the proper way to be forward
compatible.

Then you also don't need to litter the code with version checks everywhere.

- Hendrik