Mar 11, 2021, 23:09 by suji.velupil...@broadcom.com:
> From: Suji Velupillai <suji.velupil...@broadcom.com>
>
> Initial commit to add VKAPI hardware accelerator implementation.
> The depedency component vkil source code can be obtained from github
> https://github.com/Broadcom/vkil
>
Is this code for hardware that no one will ever be able to get,
like the last time something like this was sent?
We've already had to turn down one such patch which added
support for hardware some company wanted but literally no
one else could get.
The difference is this one is fully open-source as far as the code
goes, since the kernel driver's going to be in 5.12, which is enough
to call it good in my book, though it's still iffy.
> +static int vkapi_transfer_get_formats(AVHWFramesContext *ctx,
> + enum AVHWFrameTransferDirection dir,
> + enum AVPixelFormat **formats)
> +{
> + enum AVPixelFormat *fmts;
> + int ret = 0;
> +
> + // this allocation freed in hwcontext::transfer_data_alloc
> + fmts = av_malloc_array(2, sizeof(*fmts));
> + if (!fmts) {
> + av_log(ctx, AV_LOG_ERROR, "vkapi_transfer_get_formats failed\n");
> + ret = AVERROR(ENOMEM);
> + } else {
> + fmts[0] = ctx->sw_format;
> + fmts[1] = AV_PIX_FMT_NONE;
> + *formats = fmts;
> + }
> +
> + return ret;
> +}
> +
> +static int vkapi_convert_AV2VK_Frame(vkil_buffer_surface *dst,
> + const AVFrame *src)
>
Why the all-caps on AV2VK? And a capital 'F'. Just make it all lowercase,
that is our code style.
> + // populate the vkil_surface structure with the origin pointer on the
> host
> + ret = vkapi_convert_AV2VK_Frame(surface, src);
> + if (ret) {
> + ret = av_image_alloc(tmp_data, linesize, src->width, src->height,
> + src->format, VKIL_BUF_ALIGN);
> + if (ret < 0)
> + goto fail;
> +
> + av_image_copy(tmp_data, linesize, (const uint8_t **)src->data,
> + src->linesize, src->format, src->width, src->height);
> +
> + for (i = 0; i < VKIL_BUF_NPLANES; i++) {
> + surface->plane_top[i]= tmp_data[i];
> + surface->plane_bot[i]= tmp_data[VKIL_BUF_NPLANES + i];
> + surface->stride[i] = linesize[i];
> + }
> + }
> +
> + surface->quality = dst->quality;
> +
> + ilctx = hw_framectx->ilctx;
> + if (!ilctx) {
> + ret = AVERROR(EINVAL);
> + goto fail;
> + }
> +
> + // blocking call as ffmpeg assumes the transfer is complete on return
>
No, it doesn't. Just ref the frame and unref it the next time a user
tries to upload a new frame, and if the previous upload hasn't
finished, block until it has.
That way you can have asynchronous uploading, but
the downloading has to be synchronous. I'm fine with this being
left as-is for now though.
> +/**
> + * Allocated as AVHWDeviceContext.hwctx
> + */
> +typedef struct VKAPIDeviceContext {
> + /**
> + * Holds pointers to hardware specific functions
> + */
> + vkil_api *ilapi;
> + /**
> + * Internal functions definitions
> + */
> + /**
> + * Get the hwprops reference from the AVFrame:data[3]
> + */
> + int (*frame_ref_hwprops)(const AVFrame *frame, void *phw_surface_desc);
>
Definitely doesn't need to be in this code. Just remove it and
duplicate it wherever it's used elsewhere.
> + /**
> + * Set the hwprops into AVFrame:data[3]
> + */
> + int (*frame_set_hwprops)(AVFrame *frame, const vkil_buffer_surface
> *hw_surface_desc);
>
Same as above. Again, all fields are already public.
Users should do this themselves.
> + /**
> + * Get the hwprops from AVFrame:data[3]
> + */
> + int (*frame_get_hwprops)(const AVFrame *frame, vkil_buffer_surface
> *hw_surface_desc);
>
Same.
> + /**
> + * Check if format is in an array
> + */
> + int (*fmt_is_in)(int fmt, const int *fmts);
>
Same...
> + /**
> + * Convert AVPixelFormat to VKAPI equivalent pixel format
> + */
> + int (*av2vk_fmt)(enum AVPixelFormat pixel_format);
>
This function can stay, but it needs no state at all, so just put
it out side of the structure. Name it:
const VkFormat *av_vkil_from_pixfmt(enum AVPixelFormat p);
The *vk* namespace is taken by Vulkan already.
> diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> index 46ef211add..3ae607a3d6 100644
> --- a/libavutil/pixfmt.h
> +++ b/libavutil/pixfmt.h
> @@ -348,6 +348,12 @@ enum AVPixelFormat {
> AV_PIX_FMT_NV24, ///< planar YUV 4:4:4, 24bpp, 1 plane for Y and 1
> plane for the UV components, which are interleaved (first byte U and the
> following byte V)
> AV_PIX_FMT_NV42, ///< as above, but U and V bytes are swapped
>
> + /**
> + * VKAPI hardware acceleration.
> + * data[3] contains a pointer to the vkil_buffer_surface structure
>
Why [3]? Why not [0]? Is this some horrible hack to allow
for some extremely weird software frames with an additional
hardware frame? Or to simplify checking for whether a frame
is software or hardware, despite the fact the format field
already tells you exactly what it is, and planar YUVA frames
will break the check?
I think unless you have very good reasons, this should be [0].
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
