On 30/01/17 09:32, wm4 wrote:
> 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?
We now require it in order to get the surface formats. It fails on OOM or if
the query function returns failure, so I think depending on it is reasonable.
(Though if you want the VDPAU wrapper to continue to work that might require
more hackery...)
>> - } 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?
Current Mesa decodes H.265 Main10 into 8-bit surfaces, and this code is trying
to satisfy that case and also be ready to pick 10-bit surfaces when those are
available.
(I was very much trying to avoid listing the formats explicitly.)
> 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.)
No, and I don't think there needs to be. The user is asking for the decoder to
choose something from its own supported formats, and it does. If that happens
to be a format they can't cope with because they have additional constraints
then they needed to make it manually.
>> +
>> + // 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.)
(Yes. Once the Completely General Official Method Of Setting Surface Pool
Sizes Everywhere exists then we will use it here too.)
>> + // 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.
The make_config() function may also feed into an intended "test decoder
capabilities" API...
>>
>> 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.)
Yeah, added to match the device check below, but not strictly needed.
>> +
>> 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?
It depends on results of make_config(), which we only want to call once.
>> +
>> 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.
The API requires us to call get_format(), and that implies that we have to
clear AVCodecContext.hw_frames_ctx (for current users, that API feature is
already fixed).
So, this code could cache the frames context that it allocated last time and
then reuse it if the user offers the same device and the parameters are
identical? I think that would work, but it would also transiently allocate
more memory than it otherwise would when things change - is that ok?
Thanks,
- Mark
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
