On 10/03/16 14:27, Anton Khirnov wrote:
> Quoting Mark Thompson (2016-03-08 00:21:45)
>> ---
>> configure | 4 +
>> libavutil/Makefile | 3 +
>> libavutil/hwcontext.c | 3 +
>> libavutil/hwcontext.h | 1 +
>> libavutil/hwcontext_internal.h | 1 +
>> libavutil/hwcontext_vaapi.c | 822
>> +++++++++++++++++++++++++++++++++++++++++
>> libavutil/hwcontext_vaapi.h | 70 ++++
>> 7 files changed, 904 insertions(+)
>> create mode 100644 libavutil/hwcontext_vaapi.c
>> create mode 100644 libavutil/hwcontext_vaapi.h
>>
>> ...
>> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
>> ...
>> +
>> +static enum AVPixelFormat vaapi_pix_fmt_from_fourcc(unsigned int fourcc)
>> +{
>> + int i;
>> + for (i = 0; i < FF_ARRAY_ELEMS(vaapi_format_map); i++)
>> + if (vaapi_format_map[i].fourcc == fourcc)
>> + return vaapi_format_map[i].pix_fmt;
>> + return AV_PIX_FMT_NONE;
>> +}
>> +
>> +static unsigned int vaapi_fourcc_from_pix_fmt(enum AVPixelFormat pix_fmt)
>
> This function seems to be unused.
It is, it was left in for symmetry and possible later use. Still, I guess it's
sufficiently trivial that I might as well just remove it for now to get rid of
the compiler warning.
>> +{
>> + int i;
>> + for (i = 0; i < FF_ARRAY_ELEMS(vaapi_format_map); i++)
>> + if (vaapi_format_map[i].pix_fmt == pix_fmt)
>> + return vaapi_format_map[i].fourcc;
>> + return 0;
>> +}
>> +
>> ...
>> +
>> +static int vaapi_frames_get_constraints(AVHWDeviceContext *hwdev,
>> + const void *hwconfig,
>> + AVHWFramesConstraints *constraints)
>> +{
>> + AVVAAPIDeviceContext *hwctx = hwdev->hwctx;
>> + const AVVAAPIHWConfig *config = hwconfig;
>> + AVVAAPIHWConfig *tmp_config;
>> + VASurfaceAttrib *attr_list = NULL;
>> + VAStatus vas;
>> + enum AVPixelFormat pix_fmt;
>> + unsigned int fourcc;
>> + int err, i, j, attr_count, pix_fmt_count;
>> +
>> + if (!hwconfig) {
>> + // No configuration was provided, so we create a temporary pipeline
>> + // configuration in order to query all supported image formats.
>> +
>> + tmp_config = av_mallocz(sizeof(*config));
>> + if (!tmp_config)
>> + return AVERROR(ENOMEM);
>> +
>> + vas = vaCreateConfig(hwctx->display,
>> + VAProfileNone, VAEntrypointVideoProc,
>> + 0, 0, &tmp_config->config_id);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + // No vpp. We might still be able to do something useful if
>> + // codecs are supported, so try to make the most-commonly
>> + // supported decoder configuration we can to query instead.
>> + vas = vaCreateConfig(hwctx->display,
>> + VAProfileH264ConstrainedBaseline,
>> + VAEntrypointVLD, 0, 0,
>> + &tmp_config->config_id);
>> + if (vas != VA_STATUS_SUCCESS)
>> + return AVERROR(ENOSYS);
>
> leaking tmp_config here
Fixed.
> Also, one of those zeros should be NULL I think, some compilers might
> complain about that.
Ok.
>> + }
>> +
>> + config = tmp_config;
>> + }
>> +
>> + attr_count = 0;
>> + vas = vaQuerySurfaceAttributes(hwctx->display, config->config_id,
>> + 0, &attr_count);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(hwdev, AV_LOG_ERROR, "Failed to query surface attributes: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>> + err = AVERROR(ENOSYS);
>> + goto fail;
>> + }
>> +
>> + attr_list = av_malloc(attr_count * sizeof(*attr_list));
>> + if (!attr_list) {
>> + err = AVERROR(ENOMEM);
>> + goto fail;
>> + }
>> +
>> + vas = vaQuerySurfaceAttributes(hwctx->display, config->config_id,
>> + attr_list, &attr_count);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(hwdev, AV_LOG_ERROR, "Failed to query surface attributes: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>> + err = AVERROR(ENOSYS);
>> + goto fail;
>> + }
>> +
>> + pix_fmt_count = 0;
>> + for (i = 0; i < attr_count; i++) {
>> + switch (attr_list[i].type) {
>> + case VASurfaceAttribPixelFormat:
>> + fourcc = attr_list[i].value.value.i;
>> + pix_fmt = vaapi_pix_fmt_from_fourcc(fourcc);
>> + if (pix_fmt != AV_PIX_FMT_NONE) {
>> + ++pix_fmt_count;
>> + } else {
>> + // Something unsupported - ignore.
>> + }
>> + break;
>> + case VASurfaceAttribMinWidth:
>> + constraints->min_width = attr_list[i].value.value.i;
>> + break;
>> + case VASurfaceAttribMinHeight:
>> + constraints->min_height = attr_list[i].value.value.i;
>> + break;
>> + case VASurfaceAttribMaxWidth:
>> + constraints->max_width = attr_list[i].value.value.i;
>> + break;
>> + case VASurfaceAttribMaxHeight:
>> + constraints->max_height = attr_list[i].value.value.i;
>> + break;
>> + }
>> + }
>> + if (pix_fmt_count == 0) {
>> + // Nothing usable found. Presumably there exists something which
>> + // works, so leave the set null to indicate unknown.
>> + constraints->valid_sw_formats = NULL;
>> + } else {
>> + constraints->valid_sw_formats = av_malloc_array(pix_fmt_count + 1,
>> + sizeof(pix_fmt));
>> + if (!constraints->valid_sw_formats) {
>> + err = AVERROR(ENOMEM);
>> + goto fail;
>> + }
>> +
>> + for (i = j = 0; i < attr_count; i++) {
>> + if (attr_list[i].type != VASurfaceAttribPixelFormat)
>> + continue;
>> + fourcc = attr_list[i].value.value.i;
>> + pix_fmt = vaapi_pix_fmt_from_fourcc(fourcc);
>> + if (pix_fmt != AV_PIX_FMT_NONE)
>> + constraints->valid_sw_formats[j++] = pix_fmt;
>> + }
>> + av_assert0(j == pix_fmt_count);
>> + constraints->valid_sw_formats[j] = AV_PIX_FMT_NONE;
>> + }
>> +
>> + constraints->valid_hw_formats = av_malloc_array(2, sizeof(pix_fmt));
>> + if (!constraints->valid_hw_formats) {
>> + err = AVERROR(ENOMEM);
>> + goto fail;
>> + }
>> + constraints->valid_hw_formats[0] = AV_PIX_FMT_VAAPI;
>> + constraints->valid_hw_formats[1] = AV_PIX_FMT_NONE;
>> +
>> + av_freep(&attr_list);
>> + if (!hwconfig) {
>> + vaDestroyConfig(hwctx->display, tmp_config->config_id);
>> + av_freep(&tmp_config);
>> + }
>> +
>> + return 0;
>> +
>> + fail:
>> + av_freep(constraints->valid_sw_formats);
>
> I don't think you need to free this. Especially as you don't free the
> formats either.
Yeah, it isn't needed now that there is an explicit destructor. I'll remove it.
>> + av_freep(&attr_list);
>> + if (!hwconfig) {
>> + vaDestroyConfig(hwctx->display, tmp_config->config_id);
>> + av_freep(&tmp_config);
>> + }
>> + return err;
>> +}
>> +
>> ...
>> +
>> +static AVBufferRef *vaapi_pool_alloc(void *opaque, int size)
>> +{
>> + AVHWFramesContext *hwfc = opaque;
>> + VAAPIFramesContext *ctx = hwfc->internal->priv;
>> + AVVAAPIDeviceContext *hwctx = hwfc->device_ctx->hwctx;
>> + AVVAAPIFramesContext *avfc = hwfc->hwctx;
>> + VASurfaceID surface_id;
>> + VAStatus vas;
>> + AVBufferRef *ref;
>> +
>> + vas = vaCreateSurfaces(hwctx->display, ctx->rt_format,
>> + hwfc->width, hwfc->height,
>> + &surface_id, 1,
>> + ctx->attributes, ctx->nb_attributes);
>> + if (vas != VA_STATUS_SUCCESS) {
>> + av_log(hwfc, AV_LOG_ERROR, "Failed to create surface: "
>> + "%d (%s).\n", vas, vaErrorStr(vas));
>> + return NULL;
>> + }
>> + av_log(hwfc, AV_LOG_DEBUG, "Created surface %#x.\n", surface_id);
>> +
>> + ref = av_buffer_create((uint8_t*)(uintptr_t)surface_id,
>> + sizeof(surface_id), &vaapi_buffer_free,
>> + hwfc, AV_BUFFER_FLAG_READONLY);
>> + if (!ref) {
>> + vaDestroySurfaces(hwctx->display, &surface_id, 1);
>> + return NULL;
>> + }
>> +
>> + if (hwfc->initial_pool_size > 0) {
>> + av_assert0(avfc->nb_surfaces < hwfc->initial_pool_size);
>> + avfc->surface_ids[avfc->nb_surfaces] = surface_id;
>> + }
>> + ++avfc->nb_surfaces;
>> + if (avfc->nb_surfaces == 1) {
>> + // Test whether vaDeriveImage() works for this type of surface.
>> + VAImageFormat *expected_format;
>> + VAImage test_image;
>> + int err;
>> +
>> + ctx->derive_works = 0;
>
> I think this is unsafe, since this variable is also accessed in
> mapping the frame, which can easily happen concurrently with this
> function. So it would be better to make this test in frames_init.
> Also, this function won't be called at all if the caller supplies his
> own frame pool. The mapping function should work even then.
So call get_buffer() at the end of frames_init() and try the mapping there?
That might be slightly surprising to the user supplying their own pool, but
otherwise seems entirely reasonable.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
