[FFmpeg-devel] [PATCH v3 2/5] ffmpeg: initialisation code for VAAPI, hwaccel helper

wm4 nfxjfg at googlemail.com
Tue Jan 19 21:02:17 CET 2016


On Tue, 19 Jan 2016 13:54:25 +0000
Mark Thompson <sw at jkqxz.net> wrote:

> >> +    surface = (AVVAAPISurface*)input_frame->buf[0]->data;  
> > 
> > I haven't paid attention to this before. Shouldn't this be a vaapi
> > surface ID according to how libavcodec works?  
> 
> data[3] is the VAAPI surface ID, as used in libavcodec.
> 
> Here we need to carry more information around in order to have enough context to map/unmap the surface.

Still slightly questionable. Does this mean the user can't create
surfaces and pass them to an API which happens to expect this set?

Also, it would be cleaner to set this on a data pointer too. For
example AVFrame.data[1]. (In fact, it would be pretty sane to do it
this way. This also fulfills the AVFrame "rule" that the memory
referenced by data pointers should be covered by AVFrame.buf. We even
thought about doing this in an uniform way across all hwaccels, so that
every hwaccel would use the same struct type.)

> >> +    av_log(ctx, AV_LOG_DEBUG, "Retrieve data from surface %#x (format %#x).\n",
> >> +           surface->id, output->av_format);
> >> +
> >> +    av_vaapi_lock_hardware_context(ctx->hardware_context);
> >> +
> >> +    if(output->av_format == AV_PIX_FMT_VAAPI) {
> >> +        copying = 0;
> >> +        av_log(ctx, AV_LOG_VERBOSE, "Surface %#x retrieved without copy.\n",
> >> +               surface->id);  
> > 
> > I'm not sure if this (and the one below) deserves a log message, and at
> > a verbose level at that. If you really want this, a trace level might be
> > better.  
> 
> This was mainly so that at -v 55 I could see what was going on without being totally spammed (well, several lines per frame).
> 
> I'll demote these (and others in the codecs which are saying the same thing).

In general, I'm not very fond of all these log calls (they just make
the code harder to read once you're done with the implementation), but
I'll leave this to you to decide. (Or maybe others have opinions.)

> >> +
> >> +    } else {
> >> +        err = ff_vaapi_map_surface(surface, 1);
> >> +        if(err) {
> >> +            av_log(ctx, AV_LOG_ERROR, "Failed to map surface %#x.",
> >> +                   surface->id);
> >> +            goto fail;
> >> +        }
> >> +
> >> +        copying = 1;
> >> +        av_log(ctx, AV_LOG_VERBOSE, "Surface %#x mapped: image %#x data %p.\n",
> >> +               surface->id, surface->image.image_id, surface->mapped_address);
> >> +    }
> >> +
> >> +    // The actual frame need not fill the surface.
> >> +    av_assert0(input_frame->width  <= output->width);
> >> +    av_assert0(input_frame->height <= output->height);
> >> +
> >> +    output_frame = &ctx->output_frame;
> >> +    output_frame->width  = input_frame->width;
> >> +    output_frame->height = input_frame->height;
> >> +    output_frame->format = output->av_format;
> >> +
> >> +    if(copying) {
> >> +        err = av_frame_get_buffer(output_frame, 32);
> >> +        if(err < 0) {
> >> +            av_log(ctx, AV_LOG_ERROR, "Failed to get output buffer: %d (%s).\n",
> >> +                   err, av_err2str(err));
> >> +            goto fail_unmap;
> >> +        }
> >> +
> >> +        err = ff_vaapi_copy_from_surface(output_frame, surface);
> >> +        if(err < 0) {
> >> +            av_log(ctx, AV_LOG_ERROR, "Failed to copy frame data: %d (%s).\n",
> >> +                   err, av_err2str(err));
> >> +            goto fail_unmap;
> >> +        }
> >> +
> >> +    } else {
> >> +        // Just copy the hidden ID field.
> >> +        output_frame->data[3] = input_frame->data[3];
> >> +    }
> >> +
> >> +    err = av_frame_copy_props(output_frame, input_frame);
> >> +    if(err < 0) {
> >> +        av_log(ctx, AV_LOG_ERROR, "Failed to copy frame props: %d (%s).\n",
> >> +               err, av_err2str(err));
> >> +        goto fail_unmap;
> >> +    }
> >> +
> >> +    av_frame_unref(input_frame);
> >> +    av_frame_move_ref(input_frame, output_frame);
> >> +
> >> +  fail_unmap:
> >> +    if(copying)
> >> +        ff_vaapi_unmap_surface(surface, 0);
> >> +  fail:
> >> +    av_vaapi_unlock_hardware_context(ctx->hardware_context);
> >> +    return err;
> >> +}  
> > 
> > This whole thing could be a utility function. (Libav was planning to do
> > this.)  
> 
> Unsure, I'll think about it along with AVFrame redesign.  If nothing else, the map/copy/unmap could be abstracted out.
> 
> >> +
> >> +static const struct {
> >> +    VAProfile from;
> >> +    VAProfile to;
> >> +} vaapi_profile_compatibility[] = {
> >> +#define FT(f, t) { VAProfile ## f, VAProfile ## t }
> >> +    FT(MPEG2Simple,             MPEG2Main   ),
> >> +    FT(H263Baseline,     MPEG4AdvancedSimple),
> >> +    FT(MPEG4Simple,      MPEG4AdvancedSimple),
> >> +    FT(MPEG4AdvancedSimple,     MPEG4Main   ),
> >> +    FT(H264ConstrainedBaseline, H264Baseline),
> >> +    FT(H264Baseline,            H264Main    ), // (Not quite true.)
> >> +    FT(H264Main,                H264High    ),
> >> +    FT(VC1Simple,               VC1Main     ),
> >> +    FT(VC1Main,                 VC1Advanced ),
> >> +#undef FT  
> > 
> > I think all libva drivers report these properly, so this shouldn't be
> > needed. Also, you can't decode Baseline with Main?
> > 
> > The only exception might be constrained baseline. AFAIK this profile was
> > to libva later and before that had to be decoded with Main, so if
> > constrained baseline is not reported by libva, Main should be used.  
> 
> Ok, I didn't know libva did that for you.  Will change.

From what I know it does. Except the constrained baseline case, at
least with older drivers or libva.

> >> +};
> >> +
> >> +static int vaapi_find_next_compatible(VAProfile profile)
> >> +{
> >> +    int i;
> >> +    for(i = 0; i < FF_ARRAY_ELEMS(vaapi_profile_compatibility); i++) {
> >> +        if(vaapi_profile_compatibility[i].from == profile)
> >> +            return vaapi_profile_compatibility[i].to;
> >> +    }
> >> +    return VAProfileNone;
> >> +}
> >> +
> >> +static const struct {
> >> +    enum AVCodecID codec_id;
> >> +    int codec_profile;
> >> +    VAProfile va_profile;
> >> +} vaapi_profile_map[] = {
> >> +#define MAP(c, p, v) { AV_CODEC_ID_ ## c, FF_PROFILE_ ## p, VAProfile ## v }
> >> +    MAP(MPEG2VIDEO,  MPEG2_SIMPLE,    MPEG2Simple ),
> >> +    MAP(MPEG2VIDEO,  MPEG2_MAIN,      MPEG2Main   ),
> >> +    MAP(H263,        UNKNOWN,         H263Baseline),
> >> +    MAP(MPEG4,       MPEG4_SIMPLE,    MPEG4Simple ),
> >> +    MAP(MPEG4,       MPEG4_ADVANCED_SIMPLE,
> >> +                               MPEG4AdvancedSimple),
> >> +    MAP(MPEG4,       MPEG4_MAIN,      MPEG4Main   ),
> >> +    MAP(H264,        H264_CONSTRAINED_BASELINE,
> >> +                           H264ConstrainedBaseline),
> >> +    MAP(H264,        H264_BASELINE,   H264Baseline),
> >> +    MAP(H264,        H264_MAIN,       H264Main    ),
> >> +    MAP(H264,        H264_HIGH,       H264High    ),
> >> +    MAP(HEVC,        HEVC_MAIN,       HEVCMain    ),
> >> +    MAP(WMV3,        VC1_SIMPLE,      VC1Simple   ),
> >> +    MAP(WMV3,        VC1_MAIN,        VC1Main     ),
> >> +    MAP(WMV3,        VC1_COMPLEX,     VC1Advanced ),
> >> +    MAP(WMV3,        VC1_ADVANCED,    VC1Advanced ),
> >> +    MAP(VC1,         VC1_SIMPLE,      VC1Simple   ),
> >> +    MAP(VC1,         VC1_MAIN,        VC1Main     ),
> >> +    MAP(VC1,         VC1_COMPLEX,     VC1Advanced ),
> >> +    MAP(VC1,         VC1_ADVANCED,    VC1Advanced ),
> >> +    MAP(MJPEG,       UNKNOWN,         JPEGBaseline),
> >> +    MAP(VP8,         UNKNOWN,        VP8Version0_3),
> >> +    MAP(VP9,         VP9_0,           VP9Profile0 ),
> >> +#undef MAP
> >> +};
> >> +
> >> +static VAProfile vaapi_find_profile(const AVCodecContext *avctx)
> >> +{
> >> +    VAProfile result = VAProfileNone;
> >> +    int i;
> >> +    for(i = 0; i < FF_ARRAY_ELEMS(vaapi_profile_map); i++) {
> >> +        if(avctx->codec_id != vaapi_profile_map[i].codec_id)
> >> +            continue;
> >> +        result = vaapi_profile_map[i].va_profile;
> >> +        if(avctx->profile == vaapi_profile_map[i].codec_profile)
> >> +            break;
> >> +        // If there isn't an exact match, we will choose the last (highest)
> >> +        // profile in the mapping table.  
> > 
> > Shouldn't it fail? the decoder will return garbage or worse if the
> > video uses features not present in the selected profile.  
> 
> I was thinking that it should at least try because it might work (profile overdeclared or support underdeclared because of incomplete conformance).
> 
> Making it just fail is fair enough, though.
> 
> >> +    }
> >> +    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 },
> >> +};
> >> +
> >> +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;
> >> +}
> >> +
> >> +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 best_profile, profile;
> >> +        int profile_count;
> >> +        VAProfile *profile_list;
> >> +
> >> +        best_profile = vaapi_find_profile(avctx);
> >> +        if(best_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));  
> > 
> > Unchecked memory allocation?  
> 
> Will fix.
> 
> >> +
> >> +        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);
> >> +        }
> >> +
> >> +        profile = best_profile;
> >> +        while(profile != VAProfileNone) {
> >> +            for(i = 0; i < profile_count; i++) {
> >> +                if(profile_list[i] == profile)
> >> +                    break;
> >> +            }
> >> +            if(i < profile_count)
> >> +                break;
> >> +
> >> +            av_log(ctx, AV_LOG_DEBUG, "Hardware does not support codec "
> >> +                   "profile: %s / %d -> VAProfile %d.\n",
> >> +                   avcodec_get_name(avctx->codec_id), avctx->profile,
> >> +                   profile);
> >> +            profile = vaapi_find_next_compatible(profile);
> >> +        }
> >> +
> >> +        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 if(profile == best_profile) {
> >> +            av_log(ctx, AV_LOG_INFO, "Hardware supports exact codec: "
> >> +                   "%s / %d -> VAProfile %d.\n",
> >> +                   avcodec_get_name(avctx->codec_id), avctx->profile,
> >> +                   profile);
> >> +        } else {
> >> +            av_log(ctx, AV_LOG_INFO, "Hardware supports compatible 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;
> >> +    }
> >> +
> >> +    // 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_INFO, "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));
> >> +
> >> +        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_INFO, "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_INFO, "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);
> >> +    }
> >> +
> >> +    // 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.
> >> +    {
> >> +        // Unfortunately, we need an active codec pipeline to do this properly
> >> +        // using vaQuerySurfaceAttributes().  For now, just assume the values
> >> +        // we got passed are ok.
> >> +        output->width  = avctx->coded_width;
> >> +        output->height = avctx->coded_height;  
> > 
> > Not sure I understand...  
> 
> We want to check whether the decoder actually supports the stream at this resolution, but we can't query it without an active decoder pipeline configuration, which doesn't exist yet.
> 
> It would be nice to do this here so it can be diagnosed clearly ("hardware does not support this stream at resolution WxH") rather than failing later in a probably opaque way, but it doesn't really
> matter that much.

Then this check could be added elsewhere? (I think we should have such
a check, because otherwise the hardware/driver will probably do its
dreaded "return garbage or worse" thing.)

> >> +    }
> >> +  
> > 
> > Coding-style-wise, these { ... } statements are weird and AFAIK not
> > found anywhere else in ffmpeg.  
> 
> I like them as a separator?  Purely stylistic, though - I can remove them all if you object (they appear in other parts of this patch, too).
> 

Leaving them is not a deal breaker, but IMHO they look a bit strange.
If they're really separate, why not move them to a new function? The
encoder patches also have "if(1) {", which looks even stranger.

> >> +    return 0;
> >> +}
> >> +
> >> +static void vaapi_decode_uninit(AVCodecContext *avctx)
> >> +{
> >> +    InputStream  *ist = avctx->opaque;
> >> +    VAAPIDecoderContext *ctx = ist->hwaccel_ctx;
> >> +
> >> +    if(ctx->codec_initialised) {
> >> +        av_vaapi_lock_hardware_context(ctx->hardware_context);
> >> +        ff_vaapi_pipeline_uninit(&ctx->codec);
> >> +        av_vaapi_unlock_hardware_context(ctx->hardware_context);
> >> +        ctx->codec_initialised = 0;
> >> +    }
> >> +
> >> +    av_free(ctx);
> >> +
> >> +    ist->hwaccel_ctx           = 0;
> >> +    ist->hwaccel_uninit        = 0;
> >> +    ist->hwaccel_get_buffer    = 0;
> >> +    ist->hwaccel_retrieve_data = 0;
> >> +}
> >> +
> >> +
> >> +static AVVAAPIHardwareContext vaapi_context;
> >> +static AVClass *vaapi_log = &vaapi_class;
> >> +
> >> +int vaapi_decode_init(AVCodecContext *avctx)
> >> +{
> >> +    InputStream *ist = avctx->opaque;
> >> +    VAAPIDecoderContext *ctx;
> >> +    int err;
> >> +
> >> +    if(ist->hwaccel_id != HWACCEL_VAAPI)
> >> +        return AVERROR(EINVAL);
> >> +
> >> +    if(ist->hwaccel_ctx) {
> >> +        ctx = ist->hwaccel_ctx;
> >> +
> >> +        av_vaapi_lock_hardware_context(ctx->hardware_context);
> >> +
> >> +        err = ff_vaapi_pipeline_uninit(&ctx->codec);
> >> +        if(err) {
> >> +            av_log(ctx, AV_LOG_ERROR, "Unable to reinit; failed to uninit "
> >> +                   "old codec context: %d (%s).\n", err, av_err2str(err));
> >> +            goto fail;
> >> +        }
> >> +
> >> +    } else {
> >> +        if(vaapi_context.decoder_pipeline_config_id != VA_INVALID_ID) {
> >> +            av_log(&vaapi_log, AV_LOG_ERROR, "Multiple VAAPI decoder "
> >> +                   "pipelines are not supported!\n");
> >> +            return AVERROR(EINVAL);
> >> +        }
> >> +
> >> +        ctx = av_mallocz(sizeof(*ctx));
> >> +        if(!ctx)
> >> +            return AVERROR(ENOMEM);
> >> +        ctx->class = &vaapi_class;
> >> +
> >> +        ctx->hardware_context = &vaapi_context;
> >> +
> >> +        av_vaapi_lock_hardware_context(ctx->hardware_context);
> >> +    }
> >> +
> >> +    err = vaapi_build_decoder_config(ctx, &ctx->config, &ctx->output, avctx);
> >> +    if(err) {
> >> +        av_log(ctx, AV_LOG_ERROR, "No supported configuration for this codec.");
> >> +        goto fail;
> >> +    }
> >> +
> >> +    err = ff_vaapi_pipeline_init(&ctx->codec, ctx->hardware_context,
> >> +                                 &ctx->config, 0, &ctx->output);
> >> +    if(err) {
> >> +        av_log(ctx, AV_LOG_ERROR, "Failed to initialise codec context: "
> >> +               "%d (%s).\n", err, av_err2str(err));
> >> +        goto fail;
> >> +    }
> >> +    ctx->codec_initialised = 1;
> >> +
> >> +    av_vaapi_unlock_hardware_context(ctx->hardware_context);
> >> +
> >> +    av_log(ctx, AV_LOG_DEBUG, "VAAPI decoder (re)init complete.\n");
> >> +
> >> +    ist->hwaccel_ctx           = ctx;
> >> +    ist->hwaccel_uninit        = vaapi_decode_uninit;
> >> +    ist->hwaccel_get_buffer    = vaapi_get_buffer;
> >> +    ist->hwaccel_retrieve_data = vaapi_retrieve_data;
> >> +
> >> +    avctx->hwaccel_context = ctx->hardware_context;
> >> +
> >> +    ctx->hardware_context->decoder_pipeline_config_id  = ctx->codec.config_id;
> >> +    ctx->hardware_context->decoder_pipeline_context_id = ctx->codec.context_id;
> >> +
> >> +    return 0;
> >> +
> >> + fail:
> >> +    av_vaapi_unlock_hardware_context(ctx->hardware_context);
> >> +    vaapi_decode_uninit(avctx);
> >> +    return err;
> >> +}
> >> +
> >> +int vaapi_hardware_set_options(AVDictionary **dict)
> >> +{
> >> +    av_log(&vaapi_log, AV_LOG_INFO, "Setting VAAPI hardware_context.\n");
> >> +    av_dict_set_int(dict, "hardware_context", (int64_t)&vaapi_context, 0);
> >> +    return 0;
> >> +}
> >> +
> >> +
> >> +static AVMutex vaapi_mutex;
> >> +
> >> +static void vaapi_mutex_lock(void)
> >> +{
> >> +    ff_mutex_lock(&vaapi_mutex);
> >> +}
> >> +
> >> +static void vaapi_mutex_unlock(void)
> >> +{
> >> +    ff_mutex_unlock(&vaapi_mutex);
> >> +}
> >> +
> >> +int vaapi_hardware_init(const char *device)
> >> +{
> >> +    VADisplay display;
> >> +    int drm_fd;
> >> +    Display *x11_display;
> >> +    VAStatus vas;
> >> +    int major, minor;
> >> +
> >> +    if(device && device[0] == '/') {
> >> +        // Assume a DRM path.
> >> +
> >> +        drm_fd = open(device, O_RDWR);
> >> +        if(drm_fd < 0) {
> >> +            av_log(&vaapi_log, AV_LOG_ERROR, "Cannot open DRM device %s.\n",
> >> +                   device);
> >> +            return AVERROR(errno);  
> > 
> > I think the av_log call can overwrite the errno.  
> 
> True, will fix.
> 
> >> +        }
> >> +        display = vaGetDisplayDRM(drm_fd);
> >> +        if(!display) {
> >> +            av_log(&vaapi_log, AV_LOG_ERROR, "Cannot open the VA display "
> >> +                   "(from DRM device %s).\n", device);
> >> +            close(drm_fd);
> >> +            return AVERROR(EINVAL);
> >> +        }
> >> +
> >> +    } else {
> >> +        // Assume an X11 display name (or null for default).
> >> +
> >> +        x11_display = XOpenDisplay(device); // device might be NULL.
> >> +        if(!x11_display) {
> >> +            av_log(&vaapi_log, AV_LOG_ERROR, "Cannot open X11 display %s.\n",
> >> +                   XDisplayName(device));
> >> +            return AVERROR(ENOENT);
> >> +        }
> >> +        display = vaGetDisplay(x11_display);
> >> +        if(!display) {
> >> +            av_log(&vaapi_log, AV_LOG_ERROR, "Cannot open the VA display "
> >> +                   "(from X11 display %s).\n", XDisplayName(device));
> >> +            return AVERROR(EINVAL);
> >> +        }
> >> +    }
> >> +
> >> +    vas = vaInitialize(display, &major, &minor);
> >> +    if(vas != VA_STATUS_SUCCESS) {
> >> +        av_log(&vaapi_log, AV_LOG_ERROR, "Failed to initialise VAAPI "
> >> +               "connection: %d (%s).\n", vas, vaErrorStr(vas));
> >> +        return AVERROR(EINVAL);
> >> +    }
> >> +    av_log(&vaapi_log, AV_LOG_INFO, "Initialised VAAPI connection: "
> >> +           "version %d.%d\n", major, minor);
> >> +
> >> +    ff_mutex_init(&vaapi_mutex, 0);
> >> +
> >> +    vaapi_context.display = display;
> >> +    vaapi_context.lock    = &vaapi_mutex_lock;
> >> +    vaapi_context.unlock  = &vaapi_mutex_unlock;
> >> +
> >> +    vaapi_context.decoder_pipeline_config_id  = VA_INVALID_ID;
> >> +    vaapi_context.decoder_pipeline_context_id = VA_INVALID_ID;
> >> +
> >> +    return 0;
> >> +}
> >> +  
> > 
> > In general, I think the vaapi profile selection and decoder creation
> > code could be moved to libavcodec. For vdpau there's already such an
> > API (av_vdpau_bind_context()), which could serve as inspiration. (Of
> > course this is not a required change.)  
> 
> I was mainly following the fact that it isn't there already, I guess.  I think I would prefer to leave it here for now, then possibly move in a following patch which also addresses the hardware
> context structure use in the existing decoders.

Fine.


More information about the ffmpeg-devel mailing list