On Sun, 24 Jan 2016 14:20:54 +0000
Mark Thompson <[email protected]> wrote:
> ...
> >> +static int vaapi_get_buffer(AVCodecContext *s, AVFrame *frame, int flags)
> >> +{
> >> + InputStream *ist = s->opaque;
> >> + VAAPIDecoderContext *ctx = ist->hwaccel_ctx;
> >> + AVFrame *new_frame;
> >> +
> >> + av_assert0(frame->format == AV_PIX_FMT_VAAPI);
> >
> > This can actually change on software decoding fallback, I think?
>
> But this function won't be called in that case?
>
> It works on the switching cases in FATE, at least:
> h264-reinit-small_420_8-to-large_444_10 and
> h264-reinit-small_420_9-to-small_420_8 both pass.
OK, didn't realize ffmpeg.c already dispatches the call is needed.
> >> +
> >> + new_frame = av_frame_alloc();
> >> + if(!new_frame)
> >> + return AVERROR(ENOMEM);
> >> +
> >> + av_vaapi_surface_pool_get(&ctx->pool, new_frame);
> >> +
> >> + av_log(ctx, AV_LOG_DEBUG, "Decoder given reference to surface %#x.\n",
> >> + (VASurfaceID)new_frame->data[3]);
> >> +
> >> + av_frame_copy_props(new_frame, frame);
> >> + av_frame_unref(frame);
> >> + av_frame_move_ref(frame, new_frame);
> >
> > What are these acrobatics for? Why not use frame directly?
>
> The AVFrame supplied by the decoder already has a lot of the metadata filled
> in, but the surface pool needs to make a new reference to its own AVFrame.
>
> Without this, you get display order == decode order (that was fun to track
> down).
I still don't understand - AVFrames aren't referenced, AVBufferRefs
are.
> ...
> >> + // If there isn't an exact match, we will choose the last
> >> (highest)
> >> + // profile in the mapping table. This is unfortunate because it
> >> + // could well be wrong, but it improves compatibility because
> >> streams
> >> + // and decoders do not necessarily conform to the requirements.
> >> + // (Without this, a lot of streams in FATE are discarded when then
> >> + // could have worked - H.264 extended profile which actually
> >> decodes
> >> + // with main, for example.)
> >
> > I still think this should not be done by default, and instead a user
> > option should enable it.
>
> I guess that works if the error message suggests the option they could try.
> "-lax-codec-profile-constraints"? Other hwaccels might want it too.
Yes, something like this. At least vdpau already checks it pretty
strictly (even verifies the level).
> >> + }
> >> + return result;
> >> +}
> >> +
> >> +static const struct {
> >> + enum AVPixelFormat pix_fmt;
> >> + unsigned int fourcc;
> >> +} vaapi_image_formats[] = {
> >> + { AV_PIX_FMT_NV12, VA_FOURCC_NV12 },
> >> + { AV_PIX_FMT_YUV420P, VA_FOURCC_YV12 },
> >> + { AV_PIX_FMT_GRAY8, VA_FOURCC_Y800 },
> >> +};
> >> +
> >> +static int vaapi_get_pix_fmt(unsigned int fourcc)
> >> +{
> >> + int i;
> >> + for(i = 0; i < FF_ARRAY_ELEMS(vaapi_image_formats); i++)
> >> + if(vaapi_image_formats[i].fourcc == fourcc)
> >> + return vaapi_image_formats[i].pix_fmt;
> >> + return 0;
> >> +}
> >
> > Again wondering why there's not just a single mapping table in the
> > libavutil code. (Just a suggestion; this is ok too.)
>
> Ok, that does make sense to go with the mapping now. I'll change it.
>
> >> +
> >> +static int vaapi_build_decoder_config(VAAPIDecoderContext *ctx,
> >> + AVVAAPIPipelineConfig *config,
> >> + AVVAAPISurfaceConfig *output,
> >> + AVCodecContext *avctx)
> >> +{
> >> + VAStatus vas;
> >> + int i;
> >> +
> >> + memset(config, 0, sizeof(*config));
> >> +
> >> + // Pick codec profile to use.
> >> + {
> >> + VAProfile profile;
> >> + int profile_count;
> >> + VAProfile *profile_list;
> >> +
> >> + profile = vaapi_find_profile(avctx);
> >> + if(profile == VAProfileNone) {
> >> + av_log(ctx, AV_LOG_ERROR, "VAAPI does not support codec
> >> %s.\n",
> >> + avcodec_get_name(avctx->codec_id));
> >> + return AVERROR(EINVAL);
> >> + }
> >> +
> >> + profile_count = vaMaxNumProfiles(ctx->hardware_context->display);
> >> + profile_list = av_calloc(profile_count, sizeof(VAProfile));
> >> + if(!profile_list)
> >> + return AVERROR(ENOMEM);
> >> +
> >> + vas = vaQueryConfigProfiles(ctx->hardware_context->display,
> >> + profile_list, &profile_count);
> >> + if(vas != VA_STATUS_SUCCESS) {
> >> + av_log(ctx, AV_LOG_ERROR, "Failed to query profiles: %d
> >> (%s).\n",
> >> + vas, vaErrorStr(vas));
> >> + av_free(profile_list);
> >> + return AVERROR(EINVAL);
> >> + }
> >> +
> >> + while(1) {
> >> + for(i = 0; i < profile_count; i++) {
> >> + if(profile_list[i] == profile)
> >> + break;
> >> + }
> >> + if(i < profile_count)
> >> + break;
> >> +
> >> + if(profile == VAProfileH264Baseline ||
> >> + profile == VAProfileH264ConstrainedBaseline) {
> >> + // Promote both of these to improve compatibility.
> >> + profile = VAProfileH264Main;
> >> + continue;
> >> + }
> >> +
> >> + profile = VAProfileNone;
> >> + break;
> >> + }
> >> +
> >> + av_free(profile_list);
> >> +
> >> + if(profile == VAProfileNone) {
> >> + av_log(ctx, AV_LOG_ERROR, "Hardware does not support codec: "
> >> + "%s / %d.\n", avcodec_get_name(avctx->codec_id),
> >> + avctx->profile);
> >> + return AVERROR(EINVAL);
> >> + } else {
> >> + av_log(ctx, AV_LOG_DEBUG, "Hardware supports codec: "
> >> + "%s / %d -> VAProfile %d.\n",
> >> + avcodec_get_name(avctx->codec_id), avctx->profile,
> >> + profile);
> >> + }
> >> +
> >> + config->profile = profile;
> >> + config->entrypoint = VAEntrypointVLD;
> >> + }
> >> +
> >> + // Decide on the internal chroma format.
> >> + {
> >> + VAConfigAttrib attr;
> >> +
> >> + // Currently the software only supports YUV420, so just make sure
> >> + // that the hardware we have does too.
> >> +
> >> + memset(&attr, 0, sizeof(attr));
> >> + attr.type = VAConfigAttribRTFormat;
> >> + vas = vaGetConfigAttributes(ctx->hardware_context->display,
> >> config->profile,
> >> + VAEntrypointVLD, &attr, 1);
> >> + if(vas != VA_STATUS_SUCCESS) {
> >> + av_log(ctx, AV_LOG_ERROR, "Failed to fetch config attributes:
> >> "
> >> + "%d (%s).\n", vas, vaErrorStr(vas));
> >> + return AVERROR(EINVAL);
> >> + }
> >> + if(!(attr.value & VA_RT_FORMAT_YUV420)) {
> >> + av_log(ctx, AV_LOG_ERROR, "Hardware does not support required
> >> "
> >> + "chroma format (%#x).\n", attr.value);
> >> + return AVERROR(EINVAL);
> >> + }
> >> +
> >> + output->rt_format = VA_RT_FORMAT_YUV420;
> >
> > I'm confused, shouldn't this set it to something retrieved by the
> > function above?
>
> Read again - it's just a check, the code errors out on all other
> possibilities.
>
Oh, ok.
> This section is only present because more could be added. Currently the only
> other possibility in the Intel driver is greyscale for H.264 decoding only,
> so it didn't seem worth it now.
>
What about conversion to RGB? I think 10 bit will also have a different
RT format, not sure.
> >> + }
> >> +
> >> + // Decide on the image format.
> >> + if(avctx->pix_fmt == AV_PIX_FMT_VAAPI) {
> >> + // We are going to be passing through a VAAPI surface directly:
> >> + // they will stay as whatever opaque internal format for that
> >> time,
> >> + // and we never need to make VAImages from them.
> >> +
> >> + av_log(ctx, AV_LOG_DEBUG, "Using VAAPI opaque output format.\n");
> >> +
> >> + output->av_format = AV_PIX_FMT_VAAPI;
> >> + memset(&output->image_format, 0, sizeof(output->image_format));
> >> +
> >> + } else {
> >> + int image_format_count;
> >> + VAImageFormat *image_format_list;
> >> + int pix_fmt;
> >> +
> >> + // We might want to force a change to the output format here
> >> + // if we are intending to use VADeriveImage?
> >> +
> >> + image_format_count =
> >> vaMaxNumImageFormats(ctx->hardware_context->display);
> >> + image_format_list = av_calloc(image_format_count,
> >> + sizeof(VAImageFormat));
> >> + if(!image_format_list)
> >> + return AVERROR(ENOMEM);
> >> +
> >> + vas = vaQueryImageFormats(ctx->hardware_context->display,
> >> image_format_list,
> >> + &image_format_count);
> >> + if(vas != VA_STATUS_SUCCESS) {
> >> + av_log(ctx, AV_LOG_ERROR, "Failed to query image formats: "
> >> + "%d (%s).\n", vas, vaErrorStr(vas));
> >> + return AVERROR(EINVAL);
> >> + }
> >> +
> >> + for(i = 0; i < image_format_count; i++) {
> >> + pix_fmt = vaapi_get_pix_fmt(image_format_list[i].fourcc);
> >> + if(pix_fmt == AV_PIX_FMT_NONE)
> >> + continue;
> >> + if(pix_fmt == avctx->pix_fmt)
> >> + break;
> >> + }
> >> + if(i < image_format_count) {
> >> + av_log(ctx, AV_LOG_DEBUG, "Using desired output format %s "
> >> + "(%#x).\n", av_get_pix_fmt_name(pix_fmt),
> >> + image_format_list[i].fourcc);
> >> + } else {
> >> + for(i = 0; i < image_format_count; i++) {
> >> + pix_fmt = vaapi_get_pix_fmt(image_format_list[i].fourcc);
> >> + if(pix_fmt != AV_PIX_FMT_NONE)
> >> + break;
> >> + }
> >> + if(i >= image_format_count) {
> >> + av_log(ctx, AV_LOG_ERROR, "No supported output format
> >> found.\n");
> >> + av_free(image_format_list);
> >> + return AVERROR(EINVAL);
> >> + }
> >> + av_log(ctx, AV_LOG_DEBUG, "Using alternate output format %s "
> >> + "(%#x).\n", av_get_pix_fmt_name(pix_fmt),
> >> + image_format_list[i].fourcc);
> >> + }
> >> +
> >> + output->av_format = pix_fmt;
> >> + memcpy(&output->image_format, &image_format_list[i],
> >> + sizeof(VAImageFormat));
> >> +
> >> + av_free(image_format_list);
> >> + }
> >
> > Seems to pick a format randomly (from the unordered image_format_list).
> > Maybe it'd just be better to force NV12, and if that doesn't work,
> > retry with yuv420p, or so.
>
> It picks the format you asked for (avctx->pix_fmt), or if not present takes
> the first usable one on the list. In practice you ask for YUV420P and
> probably get it, with NV12 otherwise.
>
The first from the list returned by libva is still arbitrary.
What's the format "you asked for"? Does this refer to the -pix_fmt
command line option, or whatever it was?
> >> +
> >> + // Decide how many reference frames we need.
> >> + {
> >> + // We should be able to do this in a more sensible way by looking
> >> + // at how many reference frames the input stream requires.
> >> + //output->count = DEFAULT_SURFACES;
> >> + }
> >> +
> >> + // Test whether the width and height are within allowable limits.
> >> + {
> >> + // It would be nice if we could check this here before starting
> >> + // anything, but unfortunately we need an active VA config to
> >> test.
> >> + // Hence just copy. If it isn't supproted, the pipeline
> >> + // initialisation below will fail below instead.
> >> + config->width = output->width = avctx->coded_width;
> >> + config->height = output->height = avctx->coded_height;
> >> + }
> >
> > Forgot what the conclusion about this was last time.
>
> Check later, and the pipeline initialisation will fail. I forgot to add the
> explicit check (call vaQuerySurfaceAttributes()) to improve the message, I'll
> do that now.
>
Hm, ok.
> ...
> There is currently has no uninitialisation on any of this (vaTerminate, close
> X display or drm fd) because I don't have a sensible place to call it from.
> I'm hoping that's ok in the ffmpeg application.
Should be fine.
_______________________________________________
ffmpeg-devel mailing list
[email protected]
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
