On 16/02/16 11:38, Diego Biurrun wrote:
> On Mon, Feb 15, 2016 at 11:24:31PM +0000, Mark Thompson wrote:
>> --- a/configure
>> +++ b/configure
>> @@ -4712,6 +4713,10 @@ enabled vaapi && enabled xlib &&
>>
>> +enabled vaapi &&
>> + check_code cc "va/va.h" "vaCreateSurfaces(0, 0, 0, 0, 0, 0, 0, 0)" &&
>> + enable vaapi_recent
>
> check_func_headers
No: this is checking the signature, not the existence. Old libva which we don't
want to support has vaCreateSurfaces() with six arguments.
>> --- a/libavutil/Makefile
>> +++ b/libavutil/Makefile
>> @@ -109,6 +109,7 @@ OBJS = adler32.o
>> \
>> OBJS-$(CONFIG_LZO) += lzo.o
>> OBJS-$(CONFIG_CUDA) += hwcontext_cuda.o
>> OBJS-$(CONFIG_VDPAU) += hwcontext_vdpau.o
>> +OBJS-$(CONFIG_VAAPI_RECENT) += hwcontext_vaapi.o
>
> order
Ok.
>> --- a/libavutil/hwcontext_internal.h
>> +++ b/libavutil/hwcontext_internal.h
>> @@ -88,5 +88,6 @@ struct AVHWFramesInternal {
>>
>> extern const HWContextType ff_hwcontext_type_cuda;
>> extern const HWContextType ff_hwcontext_type_vdpau;
>> +extern const HWContextType ff_hwcontext_type_vaapi;
>
> same
Ok.
>> --- /dev/null
>> +++ b/libavutil/hwcontext_vaapi.c
>> @@ -0,0 +1,715 @@
>> +
>> +static struct {
>> + unsigned int fourcc;
>> + unsigned int rt_format;
>> + enum AVPixelFormat pix_fmt;
>> +} vaapi_format_map[] = {
>> +#define MAP(va, rt, av) { \
>> + VA_FOURCC_ ## va, \
>> + VA_RT_FORMAT_ ## rt, \
>> + AV_PIX_FMT_ ## av \
>> + }
>> + MAP(NV12, YUV420, NV12),
>> + MAP(IYUV, YUV420, YUV420P),
>> + MAP(YV12, YUV420, YUV420P), // With U/V planes swapped.
>> + MAP(YV16, YUV422, YUV422P),
>> + MAP(UYVY, YUV422, UYVY422),
>> + //MAP(P010, YUV420_10BPP, P010),
>> + MAP(Y800, YUV400, GRAY8),
>> + MAP(BGRA, RGB32, BGRA),
>> + //MAP(BGRX, RGB32, BGR0),
>> + MAP(RGBA, RGB32, RGBA),
>> + //MAP(RGBX, RGB32, RGB0),
>> +#undef MAP
>> +};
>
> I'd find this more readable with the macro outside of the struct definition.
Sure.
>> + // No vpp. We might still be able to do something useful if
>> + // codecs are support, so try to make the simplest decoder we
>> + // can to query instead.
>
> supportED
Ok.
>> + av_log(hwdev, AV_LOG_ERROR, "Failed to query surface attributes: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>
> VAStatus is a typedef for int? I suspect you need to cast vas here.
VAStatus is int, and libva can't change it without breaking their ABI.
(Similarly, VASurfaceID is unsigned int.)
>> + av_log(hwdev, AV_LOG_ERROR, "Failed to query surface attributes: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>
> same
>
>> + ctx->format_count = 0;
>> + ctx->format_list = av_malloc(pix_fmt_count *
>> sizeof(VAAPISurfaceFormat));
>
> nit: align
Ok.
>> + fail:
>> + if (ctx->format_list)
>> + av_freep(&ctx->format_list);
>> + if (image_list)
>> + av_free(image_list);
>> + if (attr_list)
>> + av_free(attr_list);
>
> The NULL checks before calling av_free() are unnecessary.
Hmm, true. I would prefer to leave them to clarify when the deallocation has to
happen (if something more complex than just free() needs to be done, say), but I
can remove them if you feel strongly about it.
>> +static void vaapi_buffer_free(void *opaque, uint8_t *data)
>> +{
>> + AVHWFramesContext *hwfc = opaque;
>> + AVVAAPIDeviceContext *hwctx = hwfc->device_ctx->hwctx;
>> + VASurfaceID surface_id;
>> + VAStatus vas;
>> +
>> + surface_id = (VASurfaceID)(uintptr_t)data;
>
> Gah, why the double cast?
Casting a pointer to a narrower integer type is dubious in C (can be undefined
behaviour, even on unsigned integers). It shouldn't directly matter here
because it originally came from the target type, but gcc does warn (VASurfaceID
is unsigned int, so narrower given 64-bit pointers) and I'd prefer not to have
the warning.
>> + vas = vaDestroySurfaces(hwctx->display, &surface_id, 1);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(hwfc, AV_LOG_ERROR, "Failed to create surface %#x: "
>> + "%d (%s).\n", surface_id, vas, vaErrorStr(vas));
>
> see above
>
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(hwfc, AV_LOG_ERROR, "Failed to create surface: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>
> see above
>
>> + av_log(hwfc, AV_LOG_DEBUG, "Created surface %#x.\n", surface_id);
>
> same
>
>> + ref = av_buffer_create((uint8_t*)(uintptr_t)surface_id,
>> + sizeof(surface_id), &vaapi_buffer_free,
>> + hwfc, AV_BUFFER_FLAG_READONLY);
>
> same question about the cast
While this one is ok, it feels better to be exactly consistent with the cast in
the other direction.
>> + av_log(hwfc, AV_LOG_DEBUG, "Direct mapping disabled: "
>> + "deriving image does not work: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>
> see above
>
>> +static int vaapi_frames_init(AVHWFramesContext *hwfc)
>> +{
>> + if (!hwfc->pool) {
>> + // Being able to feed additional config attributes into here (or
>> + // replace these defaults) would be nice?
>> + ctx->attributes = av_malloc(2 * sizeof(VASurfaceAttrib));
>
> Check the malloc, it might crash dereferencing a NULL pointer two lines
> below otherwise.
Ok.
>> + ctx->attributes[0] = (VASurfaceAttrib) {
>
> The cast seems pointless, the type is already VASurfaceAttrib.
That's a compound literal, not a cast or initialisation.
>> + fail:
>> + if (avfc->surface_ids)
>> + av_freep(&avfc->surface_ids);
>> + if (ctx->attributes)
>> + av_freep(&ctx->attributes);
>
> No need for the NULL checks.
>
>> +static int vaapi_transfer_get_formats(AVHWFramesContext *hwfc,
>> + enum AVHWFrameTransferDirection dir,
>> + enum AVPixelFormat **formats)
>> +{
>> + VAAPIDeviceContext *ctx = hwfc->device_ctx->internal->priv;
>> + enum AVPixelFormat *pix_fmts, preferred_format;
>> + int i, k;
>> +
>> + preferred_format = hwfc->sw_format;
>> +
>> + pix_fmts = av_malloc((ctx->format_count + 1) * sizeof(enum
>> AVPixelFormat));
>
> sizeof(*pix_fmts)
Ok.
>> +static void vaapi_unmap_frame(void *opaque, uint8_t *data)
>> +{
>> + VAAPISurfaceMap *map = (VAAPISurfaceMap*)data;
>> + surface_id = (VASurfaceID)(uintptr_t)src->data[3];
>
> Trying to avoid warnings?
Yes, see above.
>> +static int vaapi_map_frame(AVHWFramesContext *hwfc,
>> + AVFrame *dst, const AVFrame *src, int flags)
>> +{
>> + void *address = 0;
>
> NULL
Ok.
>> + surface_id = (VASurfaceID)(uintptr_t)src->data[3];
>
> same
>
>> + map->source = src;
>> + map->flags = flags;
>> + map->image.image_id = VA_INVALID_ID;
>
> nit: align =
Ok.
>> +static int vaapi_transfer_data_from(AVHWFramesContext *hwfc,
>> + AVFrame *dst, const AVFrame *src)
>> +{
>> + fail:
>> + if (map)
>> + av_frame_free(&map);
>> + return err;
>> +}
>> +
>> +static int vaapi_transfer_data_to(AVHWFramesContext *hwfc,
>> + AVFrame *dst, const AVFrame *src)
>> +{
>> + fail:
>> + if (map)
>> + av_frame_free(&map);
>> + return err;
>> +}
>
> pointless NULL checks
>
>> --- /dev/null
>> +++ b/libavutil/hwcontext_vaapi.h
>> @@ -0,0 +1,33 @@
>> +
>> +#ifndef AVUTIL_HWCONTEXT_VAAPI_H
>> +#define AVUTIL_HWCONTEXT_VAAPI_H
>> +
>> +#include <va/va.h>
>
> This header needs to be added to SKIPHEADERS in the Makefile.
Ok.
> Diego
Thanks for the review :)
- Mark
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
