On Sun, 29 Jan 2017 19:57:13 +0000
Mark Thompson <[email protected]> wrote:
> In this case, the user only supplies a device and the frame context
> is allocated internally by lavc.
> ---
> libavcodec/vaapi_decode.c | 132
> ++++++++++++++++++++++++++++++++++++++++------
> libavcodec/vaapi_decode.h | 3 ++
> 2 files changed, 118 insertions(+), 17 deletions(-)
>
> diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c
> index 42f03ab14..3ba07908f 100644
> --- a/libavcodec/vaapi_decode.c
> +++ b/libavcodec/vaapi_decode.c
> @@ -18,6 +18,7 @@
>
> #include "libavutil/avassert.h"
> #include "libavutil/common.h"
> +#include "libavutil/pixdesc.h"
>
> #include "avcodec.h"
> #include "internal.h"
> @@ -280,6 +281,7 @@ static int vaapi_decode_make_config(AVCodecContext *avctx)
> const AVCodecDescriptor *codec_desc;
> VAProfile profile, *profile_list = NULL;
> int profile_count, exact_match, alt_profile;
> + const AVPixFmtDescriptor *sw_desc, *desc;
>
> // Allowing a profile mismatch can be useful because streams may
> // over-declare their required capabilities - in particular, many
> @@ -373,7 +375,9 @@ static int vaapi_decode_make_config(AVCodecContext *avctx)
> goto fail;
> }
>
> - hwconfig = av_hwdevice_hwconfig_alloc(ctx->frames->device_ref);
> + hwconfig = av_hwdevice_hwconfig_alloc(avctx->hw_device_ctx ?
> + avctx->hw_device_ctx :
> + ctx->frames->device_ref);
> if (!hwconfig) {
> err = AVERROR(ENOMEM);
> goto fail;
> @@ -381,24 +385,77 @@ static int vaapi_decode_make_config(AVCodecContext
> *avctx)
> hwconfig->config_id = ctx->va_config;
>
> constraints =
> - av_hwdevice_get_hwframe_constraints(ctx->frames->device_ref,
> + av_hwdevice_get_hwframe_constraints(avctx->hw_device_ctx ?
> + avctx->hw_device_ctx :
> + ctx->frames->device_ref,
> hwconfig);
> if (!constraints) {
> - // Ignore.
Is it required now? Does this have any consequences that could arise in
practice?
> - } else {
> - if (avctx->coded_width < constraints->min_width ||
> - avctx->coded_height < constraints->min_height ||
> - avctx->coded_width > constraints->max_width ||
> - avctx->coded_height > constraints->max_height) {
> - av_log(avctx, AV_LOG_ERROR, "Hardware does not support image "
> - "size %dx%d (constraints: width %d-%d height %d-%d).\n",
> - avctx->coded_width, avctx->coded_height,
> - constraints->min_width, constraints->max_width,
> - constraints->min_height, constraints->max_height);
> - err = AVERROR(EINVAL);
> - goto fail;
> + err = AVERROR(ENOMEM);
> + goto fail;
> + }
> +
> + if (avctx->coded_width < constraints->min_width ||
> + avctx->coded_height < constraints->min_height ||
> + avctx->coded_width > constraints->max_width ||
> + avctx->coded_height > constraints->max_height) {
> + av_log(avctx, AV_LOG_ERROR, "Hardware does not support image "
> + "size %dx%d (constraints: width %d-%d height %d-%d).\n",
> + avctx->coded_width, avctx->coded_height,
> + constraints->min_width, constraints->max_width,
> + constraints->min_height, constraints->max_height);
> + err = AVERROR(EINVAL);
> + goto fail;
> + }
> + if (!constraints->valid_sw_formats ||
> + constraints->valid_sw_formats[0] == AV_PIX_FMT_NONE) {
> + av_log(avctx, AV_LOG_ERROR, "Hardware does not offer any "
> + "usable surface formats.\n");
> + err = AVERROR(EINVAL);
> + goto fail;
> + }
> +
> + // Find the first format in the list which matches the expected
> + // bit depth and subsampling. If none are found (this can happen
> + // when 10-bit streams are decoded to 8-bit surfaces, for example)
> + // then just take the first format on the list.
> + ctx->surface_format = constraints->valid_sw_formats[0];
> + sw_desc = av_pix_fmt_desc_get(avctx->sw_pix_fmt);
> + for (i = 0; constraints->valid_sw_formats[i] != AV_PIX_FMT_NONE; i++) {
> + desc = av_pix_fmt_desc_get(constraints->valid_sw_formats[i]);
> + if (desc->nb_components != sw_desc->nb_components ||
> + desc->log2_chroma_w != sw_desc->log2_chroma_w ||
> + desc->log2_chroma_h != sw_desc->log2_chroma_h)
> + continue;
> + for (j = 0; j < desc->nb_components; j++) {
> + if (desc->comp[j].depth != sw_desc->comp[j].depth)
> + break;
> }
> + if (j < desc->nb_components)
> + continue;
> + ctx->surface_format = constraints->valid_sw_formats[i];
> + break;
> + }
A bit questionable. Might be better to list the formats explicitly, but
if it works I don't mind either.
Is this even needed? Intuitively, I'd totally expect that the
av_hwdevice_get_hwframe_constraints() API returns the best format
for the given config first, or so. Or can it happen that a decoder
for example supports 10 bit decoding with 8 bit surfaces? Or maybe the
config just isn't specific enough to include these details?
Is there anything that prevents packed formats from being preferred
over planar/semi-planar ones? (I think the Intel decoder still supports
packed, but I'm not sure.)
> +
> + // Start with at least four surfaces.
> + ctx->surface_count = 4;
(This still requires some additional work. The user should be able to
control this. It's probably up to elenril to decide what exactly to do
about this.)
> + // Add per-codec number of surfaces used for storing reference frames.
> + switch (avctx->codec_id) {
> + case AV_CODEC_ID_H264:
> + case AV_CODEC_ID_HEVC:
> + ctx->surface_count += 16;
> + break;
> + case AV_CODEC_ID_VP9:
> + ctx->surface_count += 8;
> + break;
> + case AV_CODEC_ID_VP8:
> + ctx->surface_count += 3;
> + break;
> + default:
> + ctx->surface_count += 2;
> }
> + // Add an additional surface per thread is frame threading is enabled.
> + if (avctx->active_thread_type & FF_THREAD_FRAME)
> + ctx->surface_count += avctx->thread_count;
I find it slightly odd that the surface_count field is always set, even
of the user provides a hw_frames_ctx. But shouldn't be a problem.
>
> av_hwframe_constraints_free(&constraints);
> av_freep(&hwconfig);
> @@ -462,12 +519,32 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
> ctx->frames = (AVHWFramesContext*)avctx->hw_frames_ctx->data;
> ctx->hwfc = ctx->frames->hwctx;
>
> + if (ctx->frames->format != AV_PIX_FMT_VAAPI) {
> + av_log(avctx, AV_LOG_ERROR, "Frames supplied for VAAPI "
> + "decoding must be VAAPI frames (not %d).\n",
> + ctx->frames->format);
> + err = AVERROR(EINVAL);
> + goto fail;
> + }
I guess this test isn't strictly needed for this change, and is just an
additional check of the hw_frames_ctx API usage?
(I don't mind it being included in this patch, just asking.)
> +
> ctx->device = ctx->frames->device_ctx;
> ctx->hwctx = ctx->device->hwctx;
>
> + } else if (avctx->hw_device_ctx) {
> + ctx->device = (AVHWDeviceContext*)avctx->hw_device_ctx->data;
> + ctx->hwctx = ctx->device->hwctx;
> +
> + if (ctx->device->type != AV_HWDEVICE_TYPE_VAAPI) {
> + av_log(avctx, AV_LOG_ERROR, "Device supplied for VAAPI "
> + "decoding must be a VAAPI device (not %d).\n",
> + ctx->device->type);
> + err = AVERROR(EINVAL);
> + goto fail;
> + }
> +
> } else {
> - av_log(avctx, AV_LOG_ERROR, "A hardware frames context is "
> - "required for VAAPI decoding.\n");
> + av_log(avctx, AV_LOG_ERROR, "A hardware device or frames context "
> + "is required for VAAPI decoding.\n");
> err = AVERROR(EINVAL);
> goto fail;
> }
> @@ -486,6 +563,27 @@ int ff_vaapi_decode_init(AVCodecContext *avctx)
> if (err)
> goto fail;
>
> + if (!avctx->hw_frames_ctx) {
> + avctx->hw_frames_ctx = av_hwframe_ctx_alloc(avctx->hw_device_ctx);
> + ctx->frames = (AVHWFramesContext*)avctx->hw_frames_ctx->data;
> +
> + ctx->frames->format = AV_PIX_FMT_VAAPI;
> + ctx->frames->width = avctx->coded_width;
> + ctx->frames->height = avctx->coded_height;
> +
> + ctx->frames->sw_format = ctx->surface_format;
> + ctx->frames->initial_pool_size = ctx->surface_count;
> +
> + err = av_hwframe_ctx_init(avctx->hw_frames_ctx);
> + if (err < 0) {
> + av_log(avctx, AV_LOG_ERROR, "Failed to initialise internal "
> + "frames context: %d.\n", err);
> + goto fail;
> + }
> +
> + ctx->hwfc = ctx->frames->hwctx;
> + }
Could this also go into the if branch checking for avctx->hw_device_ctx
above?
> +
> vas = vaCreateContext(ctx->hwctx->display, ctx->va_config,
> avctx->coded_width, avctx->coded_height,
> VA_PROGRESSIVE,
> diff --git a/libavcodec/vaapi_decode.h b/libavcodec/vaapi_decode.h
> index 08b212d03..0ff400e34 100644
> --- a/libavcodec/vaapi_decode.h
> +++ b/libavcodec/vaapi_decode.h
> @@ -69,6 +69,9 @@ typedef struct VAAPIDecodeContext {
>
> AVHWFramesContext *frames;
> AVVAAPIFramesContext *hwfc;
> +
> + enum AVPixelFormat surface_format;
> + int surface_count;
> } VAAPIDecodeContext;
>
>
Last but not least, I'm wondering if it would be possible to not unref
and recreate the hw frames pool if the surface format/size doesn't
change. This would be useful for e.g. seeks. Seeks imply a
avcodec_flush_buffers() call, which destroys the hwdec state and causes
get_format() to be called again.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
