Hi,
2015-08-17 22:26 GMT+02:00 wm4 <nfx...@googlemail.com>:
> On Mon, 17 Aug 2015 19:17:53 +0200
> Gwenole Beauchesne <gb.de...@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.beauche...@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.
>> +
>> + /**
>> + * 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().
OK. This works for me too.
>
>> +
>> #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.
That's absolutely part of the plan, but not necessarily called the same way.
av_vaapi_get_decode_params() + some other function currently internal.
I don't want a monster function that maps + allocates the decoder at
once. Internally, we'd use two functions anyway, so we might decide on
whether to expose them too.
> 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.)
I don't necessarily want to support piecemal and complex use cases.
Either we want to delegate all VA allocations to libavcodec, or the
user takes care of it. At best, I foresee the following cases:
- User allocates and manages everything ;
- User allocates the VADisplay only and libavcodec handles the rest ;
- libavcodec manages all allocations, but limited to VA/DRM display.
Next, we could possibly move to "hwaccel v2" and completely nuke
vaapi_context away if we supply the desired AVOptions. We'll discuss
at VDD, but iirc, Luca was also interested in that option but others
objected.
> 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.
It is allowed, as long as he bothers initializing Version properly.
Otherwise, we would derive to compat/old mode of operation.
> doc/APIchanges needs additions in any case. (Patches 1 and 2 also do,
> I forgot to mention this.)
OK, haven't noticed this "new" file from 2009, I will amend this too.
Thanks,
--
Gwenole Beauchesne
Intel Corporation SAS / 2 rue de Paris, 92196 Meudon Cedex, France
Registration Number (RCS): Nanterre B 302 456 199
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
