Hi,
2015-08-19 21:57 GMT+02:00 wm4 <nfx...@googlemail.com>:
> On Wed, 19 Aug 2015 19:32:27 +0200
> Gwenole Beauchesne <gb.de...@gmail.com> wrote:
>
>> 2015-08-19 19:19 GMT+02:00 Gwenole Beauchesne <gb.de...@gmail.com>:
>> > Hi,
>> >
>> > 2015-08-19 18:50 GMT+02:00 wm4 <nfx...@googlemail.com>:
>> >> On Wed, 19 Aug 2015 18:01:36 +0200
>> >> Gwenole Beauchesne <gb.de...@gmail.com> wrote:
>> >>
>> >>> Add av_vaapi_set_pipeline_params() interface to initialize the internal
>> >>> FFVAContext with a valid VA display, and optional parameters including
>> >>> a user-supplied VA context id.
>> >>>
>> >>> This is preparatory work for delegating VA pipeline (decoder) creation
>> >>> to libavcodec. Meanwhile, if this new interface is used, the user shall
>> >>> provide the VA context id and a valid AVCodecContext.get_buffer2() hook.
>> >>> Otherwise, the older vaapi_context approach is still supported.
>> >>>
>> >>> This is an API change. The whole vaapi_context structure is no longer
>> >>> needed, and thus deprecated.
>> >>>
>> >>> Signed-off-by: Gwenole Beauchesne <gwenole.beauche...@intel.com>
>> >>> ---
>> >>> doc/APIchanges | 6 ++--
>> >>> libavcodec/vaapi.c | 87
>> >>> ++++++++++++++++++++++++++++++++++++++++-----
>> >>> libavcodec/vaapi.h | 49 +++++++++++++++++++++++--
>> >>> libavcodec/vaapi_internal.h | 4 +++
>> >>> 4 files changed, 134 insertions(+), 12 deletions(-)
>> >>>
>> >>> diff --git a/doc/APIchanges b/doc/APIchanges
>> >>> index aa92b69..fcdaa26 100644
>> >>> --- a/doc/APIchanges
>> >>> +++ b/doc/APIchanges
>> >>> @@ -16,8 +16,10 @@ libavutil: 2014-08-09
>> >>> API changes, most recent first:
>> >>>
>> >>> 2015-xx-xx - lavc 56.58.100 - vaapi.h
>> >>> - Deprecate old VA-API context (vaapi_context) fields that were only
>> >>> - set and used by libavcodec. They are all managed internally now.
>> >>> + Add new API to configure the hwaccel/vaapi pipeline, currently
>> >>> + useful for decoders: av_vaapi_set_pipeline_params()
>> >>> + Deprecate old VA-API context (vaapi_context) structure, which is no
>> >>> + longer used for initializing a VA-API decode pipeline.
>> >>>
>> >>> 2015-xx-xx - lavu 54.31.100 - pixfmt.h
>> >>> Add a unique pixel format for VA-API (AV_PIX_FMT_VAAPI) that
>> >>> diff --git a/libavcodec/vaapi.c b/libavcodec/vaapi.c
>> >>> index c081bb5..afddbb3 100644
>> >>> --- a/libavcodec/vaapi.c
>> >>> +++ b/libavcodec/vaapi.c
>> >>> @@ -41,24 +41,95 @@ static void destroy_buffers(VADisplay display,
>> >>> VABufferID *buffers, unsigned int
>> >>> }
>> >>> }
>> >>>
>> >>> +/** @name VA-API pipeline parameters (internal) */
>> >>> +/**@{*/
>> >>> +/** Pipeline configuration flags
>> >>> (AV_HWACCEL_FLAG_*|AV_VAAPI_PIPELINE_FLAG_*) */
>> >>> +#define AV_VAAPI_PIPELINE_PARAM_FLAGS "flags"
>> >>> +/** User-supplied VA display handle */
>> >>> +#define AV_VAAPI_PIPELINE_PARAM_DISPLAY "display"
>> >>> +/**@}*/
>> >>> +
>> >>> +#define OFFSET(x) offsetof(FFVAContext, x)
>> >>> +static const AVOption FFVAContextOptions[] = {
>> >>> + { AV_VAAPI_PIPELINE_PARAM_FLAGS, "flags", OFFSET(flags),
>> >>> + AV_OPT_TYPE_INT, { .i64 = 0 }, 0, UINT32_MAX },
>> >>> + { AV_VAAPI_PIPELINE_PARAM_DISPLAY, "VA display",
>> >>> OFFSET(user_display),
>> >>> + AV_OPT_TYPE_INT64, { .i64 = 0 }, 0, UINTPTR_MAX },
>> >>> + { AV_VAAPI_PIPELINE_PARAM_CONTEXT, "VA context id",
>> >>> OFFSET(user_context_id),
>> >>> + AV_OPT_TYPE_INT, { .i64 = VA_INVALID_ID }, 0, UINT32_MAX },
>> >>> + { NULL, }
>> >>> +};
>> >>> +#undef OFFSET
>> >>> +
>> >>> +static const AVClass FFVAContextClass = {
>> >>> + .class_name = "FFVAContext",
>> >>> + .item_name = av_default_item_name,
>> >>> + .option = FFVAContextOptions,
>> >>> + .version = LIBAVUTIL_VERSION_INT,
>> >>> +};
>> >>> +
>> >>> +int av_vaapi_set_pipeline_params(AVCodecContext *avctx, VADisplay
>> >>> display,
>> >>> + uint32_t flags, AVDictionary **params)
>> >>> +{
>> >>> + int ret;
>> >>> +
>> >>> + if (!display) {
>> >>> + av_log(avctx, AV_LOG_ERROR, "No valid VA display supplied.\n");
>> >>> + return AVERROR(EINVAL);
>> >>> + }
>> >>> +
>> >>> + if (params && *params)
>> >>> + av_dict_copy(&avctx->internal->hwaccel_config, *params, 0);
>> >>> +
>> >>> + ret = av_dict_set_int(&avctx->internal->hwaccel_config,
>> >>> + AV_VAAPI_PIPELINE_PARAM_FLAGS, flags, 0);
>> >>> + if (ret != 0)
>> >>> + return ret;
>> >>> +
>> >>> + return av_dict_set_int(&avctx->internal->hwaccel_config,
>> >>> + AV_VAAPI_PIPELINE_PARAM_DISPLAY,
>> >>> + (int64_t)(intptr_t)display, 0);
>> >>> +}
>> >>> +
>> >>> 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;
>> >>> + int ret;
>> >>>
>> >>> - if (!user_vactx) {
>> >>> - av_log(avctx, AV_LOG_ERROR, "Hardware acceleration context
>> >>> (hwaccel_context) does not exist.\n");
>> >>> - return AVERROR(ENOSYS);
>> >>> - }
>> >>> + vactx->klass = &FFVAContextClass;
>> >>> + av_opt_set_defaults(vactx);
>> >>>
>> >>> - vactx->display = user_vactx->display;
>> >>> - vactx->config_id = user_vactx->config_id;
>> >>> - vactx->context_id = user_vactx->context_id;
>> >>> +#if FF_API_VAAPI_CONTEXT
>> >>> +FF_DISABLE_DEPRECATION_WARNINGS
>> >>> + if (avctx->hwaccel_context) {
>> >>> + const struct vaapi_context * const user_vactx =
>> >>> avctx->hwaccel_context;
>> >>> + vactx->user_display = (uintptr_t)user_vactx->display;
>> >>> + vactx->user_context_id = user_vactx->context_id;
>> >>> + }
>> >>> +FF_ENABLE_DEPRECATION_WARNINGS
>> >>> +#endif
>> >>>
>> >>> + vactx->context_id = VA_INVALID_ID;
>> >>> vactx->pic_param_buf_id = VA_INVALID_ID;
>> >>> vactx->iq_matrix_buf_id = VA_INVALID_ID;
>> >>> vactx->bitplane_buf_id = VA_INVALID_ID;
>> >>>
>> >>> + ret = av_opt_set_dict(vactx, &avctx->internal->hwaccel_config);
>> >>> + if (ret != 0)
>> >>> + return ret;
>> >>> +
>> >>> + vactx->display = (void
>> >>> *)(uintptr_t)vactx->user_display;
>> >>> + vactx->context_id = vactx->user_context_id;
>> >>> +
>> >>> + if (!vactx->display) {
>> >>> + av_log(avctx, AV_LOG_ERROR, "No valid VA display found.\n");
>> >>> + return AVERROR(EINVAL);
>> >>> + }
>> >>> +
>> >>> + if (vactx->context_id != VA_INVALID_ID && !avctx->get_buffer2) {
>> >>> + av_log(avctx, AV_LOG_ERROR, "No AVCodecContext.get_buffer2()
>> >>> hooked defined.\n");
>> >>> + return AVERROR(EINVAL);
>> >>> + }
>> >>> return 0;
>> >>> }
>> >>>
>> >>> diff --git a/libavcodec/vaapi.h b/libavcodec/vaapi.h
>> >>> index 4448a2e..43f8381 100644
>> >>> --- a/libavcodec/vaapi.h
>> >>> +++ b/libavcodec/vaapi.h
>> >>> @@ -32,7 +32,9 @@
>> >>>
>> >>> #include <stdint.h>
>> >>> #include <libavutil/attributes.h>
>> >>> +#include <va/va.h>
>> >>> #include "version.h"
>> >>> +#include "avcodec.h"
>> >>>
>> >>> /**
>> >>> * @defgroup lavc_codec_hwaccel_vaapi VA API Decoding
>> >>> @@ -48,7 +50,11 @@
>> >>> * during initialization or through each AVCodecContext.get_buffer()
>> >>> * function call. In any case, they must be valid prior to calling
>> >>> * decoding functions.
>> >>> + *
>> >>> + * This structure is deprecated. Please refer to pipeline parameters
>> >>> + * and associated accessors, e.g. av_vaapi_set_pipeline_params().
>> >>> */
>> >>> +#if FF_API_VAAPI_CONTEXT
>> >>> struct vaapi_context {
>> >>> /**
>> >>> * Window system dependent data
>> >>> @@ -56,6 +62,7 @@ struct vaapi_context {
>> >>> * - encoding: unused
>> >>> * - decoding: Set by user
>> >>> */
>> >>> + attribute_deprecated
>> >>> void *display;
>> >>>
>> >>> /**
>> >>> @@ -64,6 +71,7 @@ struct vaapi_context {
>> >>> * - encoding: unused
>> >>> * - decoding: Set by user
>> >>> */
>> >>> + attribute_deprecated
>> >>> uint32_t config_id;
>> >>>
>> >>> /**
>> >>> @@ -72,9 +80,9 @@ struct vaapi_context {
>> >>> * - encoding: unused
>> >>> * - decoding: Set by user
>> >>> */
>> >>> + attribute_deprecated
>> >>> uint32_t context_id;
>> >>>
>> >>> -#if FF_API_VAAPI_CONTEXT
>> >>> /**
>> >>> * VAPictureParameterBuffer ID
>> >>> *
>> >>> @@ -181,8 +189,45 @@ struct vaapi_context {
>> >>> */
>> >>> attribute_deprecated
>> >>> uint32_t slice_data_size;
>> >>> -#endif
>> >>> };
>> >>> +#endif
>> >>> +
>> >>> +/** @name VA-API pipeline parameters */
>> >>> +/**@{*/
>> >>> +/**
>> >>> + * VA context id (uint32_t) [default: VA_INVALID_ID]
>> >>> + *
>> >>> + * This defines the VA context id to use for decoding. If set, then
>> >>> + * the user allocates and owns the handle, and shall supply VA surfaces
>> >>> + * through an appropriate hook to AVCodecContext.get_buffer2().
>> >>> + */
>> >>> +#define AV_VAAPI_PIPELINE_PARAM_CONTEXT "context"
>> >>
>> >> I don't understand this. Wouldn't it also be possible to use a
>> >> user-defined get_buffer2 function, but with a context created by
>> >> libavcodec?
>> >
>> > Historically, you cannot create a decode pipeline without supplying a
>> > fixed number of VA surfaces to vaCreateContext(). You cannot have any
>> > guarantee either that re-creating a VA context with an increased
>> > number of VA surfaces would still get things working. I have an
>> > example of a pretty old and semi-opensource driver that stuffs various
>> > local states into the VA context even if they are VA surface specific.
>
> I see... I guess we still want to support these. Does the vaapi API
> provide a way to know whether this is required?
No, since it's specified to work that way. :)
However, I can tell you that: AFAIK, all VA drivers will work as
described (vaCreateContext() + fixed number of VA surfaces required
for decode) except the open source Intel HD Graphics VA driver.
Though, iirc even with it I had to store extra data in the VA context
but only for H.264 MVC decoding on anything older than Haswell.
>> > If we wanted to call get_buffer2() multiple times to get an initial
>> > set of VA surfaces that you could use to create a VA context, (i) this
>> > would be ugly IMHO, and (ii) there is no guarantee either that the
>> > user get_buffer2() impl wouldn't hang/wait until his pool of free VA
>> > surfaces fills in again.
>
> I don't think it's ugly... seems like a nice (and optional) hack to
> support some older hardware without complicating the normal API too
> much.
>
>> For that very particular situation, I believe we could introduce an
>> AV_GET_BUFFER_FLAG_NOWAIT flag. But yet, we have no guarantee either
>> that the user would actually support this flag if needed. :)
>
> Sounds like a good solution. These surfaces are special as in they need
> to stick around until end of decoding too.
>
> Personally I don't see much value in the new API without having
> libavcodec handle decoder creation (including profile handling).
>
> This could be skewed by my personal experience; most of my own
> hwaccel-specific vaapi code is for decoder creation and profile
> handling. I'd like to do my own surface allocation handling, because
> vaapi doesn't seem to provide a way to retrieve certain surface
> parameters like chroma format and actual surface size. Other API
> users will have similar issues. But maybe I'm somehow misled.
This is exactly what I would like to know, based on experience: why
would you want to go the pain with (i) determine & map surface
parameters to use, (ii) allocate & maintain your own pool of VA
surfaces, (iii) determine & map decoder profile to use if libavcodec
can do all of that for free for you?
As I mentioned, I can concede to:
1. Expose an av_vaapi_get_surface_params() function that gives you the
right surface chroma format and size params to use ;
2. Expose an av_vaapi_get_decoder_params() function that gives you the
right profile to use based on the actual underlying HW capabilities.
With that, this can already reduce quite a bit of code (and possible
errors) in user code and (1) is just a matter of calling
vaCreateSurfaces(), and (2) is just a matter of calling
vaCreateContext(). Once you have that, this is not too far from
calling av_vaapi_set_pipeline_params(). There is still some code to
write for (ii) in user application if the user really wants such
cases.
That's why, I really would like to know why you would want to do it
manually. This is technically possible, but I would like to understand
the real benefits over just having lavc to manage that kind of things
itself. I think that for the "advanced" user like you that really
really wants to allocate your own VA surfaces (but why? again?), you
can still use the old-school way, i.e. supply the VA context yourself
then, but with the new API (hwaccel_context-free).
Having said that, even if I do (1) and (2), they will live in a
separate/subsequent patch as I wanted to move towards this "lavc
managed HW resources" path gradually. i.e. I don't want to change the
comment as this exactly matches what lavc would do/user has to do at
this very precise point in the series.
>> And, I prefer to restrict to clearly defined usages that work, rather
>> than allowing multiple combos that would complexify test coverage... I
>> believe the ones I exposed in the cover letter are the only practical
>> ones to ever be useful, based on a compiled set of feedbacks over past
>> years. But I can be wrong. If so, please tell me why would you
>> particularly want to have lavc allocate the VA context and you
>> providing your own VA surfaces. If creating a VA context is a problem
>> for the user, I certainly can expose a utility function that provides
>> the necessary decode params. Actually, it was in the plan too:
>> av_vaapi_get_decode_params().
>>
>> > Because of that, I believe it's better to restrict the usages to what
>> > it is strictly permitted, for historical reasons. So, either user
>> > creates VA surfaces + VA context, or he lets lavc do so but for both
>> > VA surfaces & VA context as well. Besides, as I further distilled in
>> > one of the patches, lavc naturally works through allocating buffers
>> > itself for SW decoders, that can work, and this is something desired
>> > to achieve with hwaccel too. At least for vaapi. i.e. you'd only need
>> > to provide a VADisplay handle.
>
> I agree, but there are some minor details that perhaps need to be taken
> care of. (See what I wrote above for some remarks.)
>
>> >>> +/**@}*/
>> >>> +
>> >>> +/**
>> >>> + * Defines VA processing pipeline parameters
>> >>> + *
>> >>> + * This function binds the supplied VA @a display to a codec context
>> >>> + * @a avctx.
>> >>> + *
>> >>> + * The user retains full ownership of the display, and thus shall
>> >>> + * ensure the VA-API subsystem was initialized with vaInitialize(),
>> >>> + * make due diligence to keep it live until it is no longer needed,
>> >>> + * and dispose the associated resources with vaTerminate() whenever
>> >>> + * appropriate.
>> >>> + *
>> >>> + * @note This function has no effect if it is called outside of an
>> >>> + * AVCodecContext.get_format() hook.
>> >>> + *
>> >>> + * @param[in] avctx the codec context being used for decoding the
>> >>> stream
>> >>> + * @param[in] display the VA display handle to use for decoding
>> >>> + * @param[in] flags zero or more OR'd AV_HWACCEL_FLAG_* or
>> >>> + * AV_VAAPI_PIPELINE_FLAG_* flags
>> >>> + * @param[in] params optional parameters to configure the pipeline
>> >>> + * @return 0 on success, an AVERROR code on failure.
>> >>> + */
>> >>> +int av_vaapi_set_pipeline_params(AVCodecContext *avctx, VADisplay
>> >>> display,
>> >>> + uint32_t flags, AVDictionary **params);
>> >>>
>> >>> /* @} */
>> >>>
>> >>> diff --git a/libavcodec/vaapi_internal.h b/libavcodec/vaapi_internal.h
>> >>> index 29f46ab..958246c 100644
>> >>> --- a/libavcodec/vaapi_internal.h
>> >>> +++ b/libavcodec/vaapi_internal.h
>> >>> @@ -36,6 +36,10 @@
>> >>> */
>> >>>
>> >>> typedef struct {
>> >>> + const void *klass;
>> >>> + uint32_t flags; ///< Pipeline flags
>> >>> + uint64_t user_display; ///< User-supplied VA display
>> >>> + uint32_t user_context_id; ///< User-supplied VA context ID
>> >>> VADisplay display; ///< Windowing system dependent
>> >>> handle
>> >>> VAConfigID config_id; ///< Configuration ID
>> >>> VAContextID context_id; ///< Context ID (video decode
>> >>> pipeline)
>> >>
>> >> _______________________________________________
>> >> ffmpeg-devel mailing list
>> >> ffmpeg-devel@ffmpeg.org
>> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>> > Regards,
>> > --
>> > 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
--
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
