[FFmpeg-devel] [PATCH v4 1/5] libavutil: VAAPI infrastructure

wm4 nfxjfg at googlemail.com
Sun Jan 24 15:43:37 CET 2016


On Sun, 24 Jan 2016 13:44:47 +0000
Mark Thompson <sw at jkqxz.net> wrote:

> ...
> >> +
> >> +    vas = vaSyncSurface(hw_ctx->display, surface->id);  
> > 
> > Is this strictly needed? (I don't know.)  
> 
> It's definitely needed in some cases - the hwaccel decoder does not call vaSyncSurface(), so we have to somewhere on the path to copying the data out.

OK then.

> 
> It could be separated, but if someone else has already called it then it's idempotent so it seems sensible to leave it here,
> 
> >> +    if(vas != VA_STATUS_SUCCESS) {
> >> +        av_log(0, AV_LOG_ERROR, "Failed to sync surface "
> >> +               "%#x: %d (%s).\n", surface->id, vas, vaErrorStr(vas));
> >> +        err = AVERROR_EXTERNAL;
> >> +        goto fail;
> >> +    }
> >> +
> >> +    if(derive) {
> >> +        vas = vaDeriveImage(hw_ctx->display,
> >> +                            surface->id, image);
> >> +        if(vas != VA_STATUS_SUCCESS) {
> >> +            av_log(0, AV_LOG_ERROR, "Failed to derive image from surface "
> >> +                   "%#x: %d (%s).\n", surface->id, vas, vaErrorStr(vas));  
> > 
> > Is it guaranteed that surface->id is an unsigned int and VAStatus is
> > int? I know the typedefs resolve to these types, but is there a chance
> > a future libva version will change them?
> > 
> > Also, this message should probably not be an error, but a verbose at
> > most. (Since it can gracefully fallback to the code path below.)  
> 
> I've assumed unsigned int / int throughout.
> 
> The alternative is adding PRIxVASurfaceID and PRIdVAStatus macros which look at libva version, which rather seems like overkill.
> 
> Anyway, VASurfaceID == unsigned int at least is already baked into their ABI (it's in a lot of the parameter structures).

If we care, we should do something that can't break if libva ever
changes it, and e.g. cast the values when passing them to printf.

Anyway, it's probably fine to ignore this, since libva would have to
break their ABI anyway.

> >> +            derive = 0;
> >> +        }
> >> +    }
> >> +    if(!derive) {
> >> +        vas = vaCreateImage(hw_ctx->display, &config->image_format,
> >> +                            config->width, config->height, image);
> >> +        if(vas != VA_STATUS_SUCCESS) {
> >> +            av_log(0, AV_LOG_ERROR, "Failed to create image for surface "
> >> +                   "%#x: %d (%s).\n", surface->id, vas, vaErrorStr(vas));
> >> +            err = AVERROR_EXTERNAL;
> >> +            goto fail;
> >> +        }
> >> +
> >> +        if(get) {
> >> +            vas = vaGetImage(hw_ctx->display, surface->id, 0, 0,
> >> +                             config->width, config->height, image->image_id);
> >> +            if(vas != VA_STATUS_SUCCESS) {
> >> +                av_log(0, AV_LOG_ERROR, "Failed to get image for surface "
> >> +                       "%#x: %d (%s).\n", surface->id, vas, vaErrorStr(vas));
> >> +                err = AVERROR_EXTERNAL;
> >> +                goto fail_image;
> >> +            }
> >> +        }
> >> +    }
> >> +
> >> +    av_assert0(image->format.fourcc == config->image_format.fourcc);  
> > 
> > I wouldn't use assert to check expectations from API return values.  
> 
> Sure, it can just be a different failure case.
> 
> >> +
> >> +    vas = vaMapBuffer(hw_ctx->display, image->buf, &address);
> >> +    if(vas != VA_STATUS_SUCCESS) {
> >> +        av_log(0, AV_LOG_ERROR, "Failed to map image from surface "
> >> +               "%#x: %d (%s).\n", surface->id, vas, vaErrorStr(vas));
> >> +        err = AVERROR_EXTERNAL;
> >> +        goto fail_image;
> >> +    }
> >> +
> >> +    surface->mapped_address = address;
> >> +
> >> +    for(i = 0; i < image->num_planes; i++) {
> >> +        frame->data[i] = (uint8_t*)address + image->offsets[i];
> >> +        frame->linesize[i] = image->pitches[i];
> >> +    }
> >> +
> >> +    av_vaapi_unlock_hardware_context(hw_ctx);
> >> +    return 0;
> >> +
> >> +  fail_image:
> >> +    vas = vaDestroyImage(hw_ctx->display, surface->image.image_id);
> >> +    if(vas != VA_STATUS_SUCCESS) {
> >> +        av_log(0, AV_LOG_ERROR, "Failed to destroy image for surface "
> >> +               "%#x: %d (%s).\n", surface->id, vas, vaErrorStr(vas));
> >> +    }
> >> +  fail:
> >> +    av_vaapi_unlock_hardware_context(hw_ctx);
> >> +    return err;
> >> +}  
> > 
> > So this API function uses a per-surface image struct and other fields.
> > What happens if multiple threads try to map it? Does libva even allow
> > this in theory? (If not, this isn't a concern. But maybe the function
> > could complain loudly if such an use is detected.)  
> 
> Don't do that?  The mapping information is saved so it could work in theory (needs a map-refcount on the surface), but I didn't have a sensible use case so ignored it (you definitely can't map a surface while using it for other operations (even read-only ones like encoding), so there already strong parallelism constraints the user has to be aware of).
> 
> I'll add another error case for it.
> 

OK, fine. (Another case where hwaccel MT decoding won't work.)

> >> +
> >> +static AVFrame *vaapi_make_proxy_frame(const AVFrame *src)
> >> +{
> >> +    AVVAAPISurface *surface = vaapi_get_surface(src);
> >> +    VAImage *image = &surface->image;
> >> +    AVFrame *dst;
> >> +    int i;
> >> +
> >> +    if(!surface->mapped_address) {
> >> +        av_log(0, AV_LOG_ERROR, "Surface %#x is not mapped.",
> >> +               surface->id);
> >> +        return 0;
> >> +    }
> >> +
> >> +    dst = av_frame_alloc();
> >> +    if(!dst)
> >> +        return 0;
> >> +
> >> +    for(i = 0; i < image->num_planes; i++) {
> >> +        dst->data[i] = src->data[i];
> >> +        dst->linesize[i] = src->linesize[i];
> >> +    }
> >> +
> >> +    dst->width  = src->width;
> >> +    dst->height = src->height;
> >> +
> >> +    dst->format = vaapi_pix_fmt(image->format.fourcc);
> >> +    if(image->format.fourcc == VA_FOURCC_YV12) {
> >> +        uint8_t *tmp;
> >> +        tmp = dst->data[1];
> >> +        dst->data[1] = dst->data[2];
> >> +        dst->data[2] = tmp;
> >> +    }
> >> +
> >> +    av_frame_copy_props(dst, src);
> >> +
> >> +    return dst;
> >> +}  
> > 
> > So this just swaps the planes if needed? Maybe it would be better to
> > make av_vaapi_map_frame() do this? (On the other hand, it could be
> > confusing... but I think it'd be better in summary.)  
> 
> map_frame() is meant to be as "raw" as possible so that vaDeriveImage() could work.  This is just rearranging pointers and doesn't affect that, though.
> 
> Unclear.

IMHO it's preferable if the user doesn't have to be aware of the
different yuv420p conventions, and it'd be just better to "fix" this on
mapping. But you can decide.

> >> +
> >> +int av_vaapi_copy_to_surface(AVFrame *dst, const AVFrame *src)
> >> +{
> >> +    AVFrame *proxy;
> >> +    int err;
> >> +
> >> +    if(dst->format != AV_PIX_FMT_VAAPI)
> >> +        return AVERROR(EINVAL);
> >> +
> >> +    err = av_vaapi_map_frame(dst, 0);
> >> +    if(err)
> >> +        return err;
> >> +
> >> +    proxy = vaapi_make_proxy_frame(dst);
> >> +    if(proxy)
> >> +        err = av_frame_copy(proxy, src);
> >> +    else
> >> +        err = AVERROR(ENOMEM);
> >> +
> >> +    av_vaapi_unmap_frame(dst, 1);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +int av_vaapi_copy_from_surface(AVFrame *dst, AVFrame *src)
> >> +{
> >> +    AVFrame *proxy;
> >> +    int err;
> >> +
> >> +    if(src->format != AV_PIX_FMT_VAAPI)
> >> +        return AVERROR(EINVAL);
> >> +
> >> +    err = av_vaapi_map_frame(src, 1);
> >> +    if(err)
> >> +        return err;
> >> +
> >> +    proxy = vaapi_make_proxy_frame(src);
> >> +    if(proxy)
> >> +        err = av_frame_copy(dst, proxy);
> >> +    else
> >> +        err = AVERROR(ENOMEM);
> >> +
> >> +    av_vaapi_unmap_frame(src, 0);
> >> +
> >> +    return err;
> >> +}
> >> +
> >> +static const AVClass vaapi_pipeline_class = {
> >> +    .class_name = "VAAPI/pipeline",
> >> +    .item_name  = av_default_item_name,
> >> +    .version    = LIBAVUTIL_VERSION_INT,
> >> +};
> >> +
> >> +int av_vaapi_pipeline_init(AVVAAPIPipelineContext *ctx,
> >> +                           AVVAAPIHardwareContext *hw_ctx,
> >> +                           AVVAAPIPipelineConfig *config,
> >> +                           AVVAAPISurfacePool *pool)
> >> +{
> >> +    VASurfaceID output_surface_ids[AV_VAAPI_MAX_SURFACES];
> >> +    int output_surface_count;
> >> +    VAStatus vas;
> >> +    int i, err;
> >> +
> >> +    av_vaapi_lock_hardware_context(hw_ctx);
> >> +
> >> +    memset(ctx, 0, sizeof(*ctx));
> >> +    ctx->class = &vaapi_pipeline_class;
> >> +
> >> +    ctx->hardware_context = hw_ctx;
> >> +
> >> +    if(pool) {
> >> +        output_surface_count = pool->frame_count;
> >> +        for(i = 0; i < output_surface_count; i++)
> >> +            output_surface_ids[i] = vaapi_get_surface(pool->frames[i])->id;
> >> +    } else {
> >> +        // An output surface pool need not be supplied if the pipeline
> >> +        // produces no image output (an intra-only codec like JPEG, say).
> >> +
> >> +        output_surface_count = 0;
> >> +    }
> >> +
> >> +    vas = vaCreateConfig(hw_ctx->display, config->profile,
> >> +                         config->entrypoint, config->attributes,
> >> +                         config->attribute_count, &ctx->config_id);
> >> +    if(vas != VA_STATUS_SUCCESS) {
> >> +        av_log(ctx, AV_LOG_ERROR, "Failed to create pipeline configuration: "
> >> +               "%d (%s).\n", vas, vaErrorStr(vas));
> >> +        err = AVERROR(EINVAL);
> >> +        goto fail;
> >> +    }
> >> +
> >> +    vas = vaCreateContext(hw_ctx->display, ctx->config_id,
> >> +                          config->width, config->height,
> >> +                          VA_PROGRESSIVE,
> >> +                          output_surface_ids, output_surface_count,
> >> +                          &ctx->context_id);
> >> +    if(vas != VA_STATUS_SUCCESS) {
> >> +        av_log(ctx, AV_LOG_ERROR, "Failed to create pipeline context: "
> >> +               "%d (%s).\n", vas, vaErrorStr(vas));
> >> +        err = AVERROR(EINVAL);
> >> +        goto fail;
> >> +    }
> >> +
> >> +    av_log(ctx, AV_LOG_DEBUG, "VAAPI pipeline initialised: config %#x "
> >> +           "context %#x.\n", ctx->config_id, ctx->context_id);
> >> +
> >> +    err = 0;
> >> +  fail:
> >> +    av_vaapi_unlock_hardware_context(hw_ctx);
> >> +    return err;
> >> +}
> >> +
> >> +int av_vaapi_pipeline_uninit(AVVAAPIPipelineContext *ctx)
> >> +{
> >> +    VAStatus vas;
> >> +
> >> +    av_vaapi_lock_hardware_context(ctx->hardware_context);
> >> +
> >> +    av_assert0(ctx->hardware_context);
> >> +
> >> +    vas = vaDestroyContext(ctx->hardware_context->display, ctx->context_id);
> >> +    if(vas != VA_STATUS_SUCCESS) {
> >> +        av_log(ctx, AV_LOG_ERROR, "Failed to destroy pipeline context: "
> >> +               "%d (%s).\n", vas, vaErrorStr(vas));
> >> +    }
> >> +
> >> +    vaDestroyConfig(ctx->hardware_context->display, ctx->config_id);
> >> +    if(vas != VA_STATUS_SUCCESS) {
> >> +        av_log(ctx, AV_LOG_ERROR, "Failed to destroy pipeline configuration: "
> >> +               "%d (%s).\n", vas, vaErrorStr(vas));
> >> +    }
> >> +
> >> +    av_vaapi_unlock_hardware_context(ctx->hardware_context);
> >> +
> >> +    return 0;
> >> +}
> >> diff --git a/libavutil/vaapi.h b/libavutil/vaapi.h
> >> new file mode 100644
> >> index 0000000..53c4c7c
> >> --- /dev/null
> >> +++ b/libavutil/vaapi.h
> >> @@ -0,0 +1,115 @@
> >> +/*
> >> + * VAAPI helper functions.
> >> + *
> >> + * Copyright (C) 2016 Mark Thompson <mrt at jkqxz.net>
> >> + *
> >> + * This file is part of FFmpeg.
> >> + *
> >> + * FFmpeg is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2.1 of the License, or (at your option) any later version.
> >> + *
> >> + * FFmpeg is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with FFmpeg; if not, write to the Free Software
> >> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> >> + */
> >> +
> >> +#ifndef LIBAVUTIL_VAAPI_H_
> >> +#define LIBAVUTIL_VAAPI_H_
> >> +
> >> +#include <va/va.h>
> >> +
> >> +#include "pixfmt.h"
> >> +#include "frame.h"
> >> +
> >> +
> >> +typedef struct AVVAAPIHardwareContext {
> >> +    VADisplay display;
> >> +
> >> +    VAConfigID  decoder_pipeline_config_id;
> >> +    VAContextID decoder_pipeline_context_id;
> >> +
> >> +    void (*lock)(void *user_context);
> >> +    void (*unlock)(void *user_context);
> >> +    void *lock_user_context;
> >> +} AVVAAPIHardwareContext;
> >> +
> >> +AVVAAPIHardwareContext *av_vaapi_alloc_hardware_context(void);
> >> +
> >> +void av_vaapi_lock_hardware_context(AVVAAPIHardwareContext *ctx);
> >> +void av_vaapi_unlock_hardware_context(AVVAAPIHardwareContext *ctx);
> >> +
> >> +
> >> +#define AV_VAAPI_MAX_SURFACES 64
> >> +
> >> +typedef struct AVVAAPISurfaceConfig {
> >> +    enum AVPixelFormat av_format;
> >> +    unsigned int rt_format;
> >> +    VAImageFormat image_format;
> >> +
> >> +    unsigned int width;
> >> +    unsigned int height;
> >> +
> >> +    unsigned int attribute_count;
> >> +    VASurfaceAttrib *attributes;
> >> +} AVVAAPISurfaceConfig;
> >> +
> >> +typedef struct AVVAAPISurfacePool {
> >> +    AVVAAPIHardwareContext *hardware_context;
> >> +
> >> +    int frame_count;
> >> +    AVFrame *frames[AV_VAAPI_MAX_SURFACES];
> >> +} AVVAAPISurfacePool;
> >> +
> >> +int av_vaapi_surface_pool_init(AVVAAPISurfacePool *pool,
> >> +                               AVVAAPIHardwareContext *hw_ctx,
> >> +                               AVVAAPISurfaceConfig *config,
> >> +                               int frame_count);
> >> +
> >> +int av_vaapi_surface_pool_uninit(AVVAAPISurfacePool *pool);
> >> +
> >> +int av_vaapi_surface_pool_get(AVVAAPISurfacePool *pool, AVFrame *target);
> >> +
> >> +
> >> +typedef struct AVVAAPIPipelineConfig {
> >> +    VAProfile profile;
> >> +    VAEntrypoint entrypoint;
> >> +
> >> +    unsigned int width;
> >> +    unsigned int height;
> >> +
> >> +    unsigned int attribute_count;
> >> +    VAConfigAttrib *attributes;
> >> +} AVVAAPIPipelineConfig;
> >> +
> >> +typedef struct AVVAAPIPipelineContext {
> >> +    const AVClass *class;
> >> +
> >> +    AVVAAPIHardwareContext *hardware_context;
> >> +
> >> +    VAConfigID config_id;
> >> +    VAContextID context_id;
> >> +} AVVAAPIPipelineContext;
> >> +
> >> +int av_vaapi_pipeline_init(AVVAAPIPipelineContext *ctx,
> >> +                           AVVAAPIHardwareContext *hw_ctx,
> >> +                           AVVAAPIPipelineConfig *config,
> >> +                           AVVAAPISurfacePool *pool);
> >> +
> >> +int av_vaapi_pipeline_uninit(AVVAAPIPipelineContext *ctx);
> >> +
> >> +
> >> +int av_vaapi_map_frame(AVFrame *frame, int get);
> >> +int av_vaapi_unmap_frame(AVFrame *frame, int put);
> >> +
> >> +int av_vaapi_copy_to_surface(AVFrame *dst, const AVFrame *src);
> >> +int av_vaapi_copy_from_surface(AVFrame *dst, AVFrame *src);  
> > 
> > It's still up to debate which of all this should be public API and what
> > not, I think.
> > 
> > If it gets to be public, we should probably take care of making it
> > future-proof. E.g. add alloc functions for structs, so we can add more
> > fields at a later point, without breaking ABI.  
> 
> I don't really like it being public, but the libavcodec/libavfilter dependency is rather forcing.  I'll wait for more thoughts on how this should work.
> 
> (Note that struct AVVAAPISurface is not public here, and doesn't need to be - everything outside the surface pools and mapping only needs the VASurfaceID from data[3].)
> 

OK. Which functions do you think should be public, and which not?


More information about the ffmpeg-devel mailing list