[FFmpeg-devel] [PATCH v4 2/5] ffmpeg: VAAPI hwaccel helper and related initialisation

wm4 nfxjfg at googlemail.com
Sun Jan 24 15:52:44 CET 2016


On Sun, 24 Jan 2016 14:20:54 +0000
Mark Thompson <sw at jkqxz.net> 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.


More information about the ffmpeg-devel mailing list