[FFmpeg-devel] [PATCH 4/4] vaapi: add interfaces to properly initialize context.
wm4
nfxjfg at googlemail.com
Tue Aug 18 11:34:00 CEST 2015
On Tue, 18 Aug 2015 10:33:20 +0200
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.
Well, this is just how we do things, and until now we didn't add such
version checks. As I've wrote in the previous mail, ABI compatibility
is restricted to keep old programs linked against newer libav* working.
And yes, this means the current vaapi_context can't actually be
extended unconditionally. VDPAU had the same problem; it was changed to
require the user to allocate the context, but AFAIK this mattered only
if another API function introduced at the same time was used, so it
still remained ABI compatible. We can do the same here, but probably
need a flag in the internal context.
Besides, such version checks don't even work. For example, consider:
int flags = 0;
if (some_struct->version > 4)
flags = some_struct->flags;
The compiler could legally turn this into:
int flags = some_struct->flags;
int version = some_struct->version;
if (!(version > 4))
flags = 0;
There's no guarantee the version check will actually avoid access to
the struct.
> >> +
> >> + /**
> >> + * 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.
Sounds good.
> 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.
As long as the codec details are isolated...
> > 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.
Luca doesn't seem to entirely understand how the interaction with the
windowing system has to work.
> > 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,
More information about the ffmpeg-devel
mailing list