[FFmpeg-devel] [PATCH] lavu: add VKAPI hwcontext implementation

[Suji Velupillai]

Thank you for the review, please see inline.

On Thu, Mar 11, 2021 at 2:56 PM Lynne <dev at lynne.ee> wrote:

> Mar 11, 2021, 23:09 by suji.velupillai at broadcom.com:
>
> > From: Suji Velupillai <suji.velupillai at 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.
>
> Yes, this hardware has fully open sourced drivers and utilities.
The hardware is currently available for early access customers now.  In
future, it will be more widely available.


> > +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.
>

Sorry I missed that, will fix it.


>
> > +    // 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.
>

Okay thank you.

>
>
> > +/**
> > + * 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.
>
> Okay, will do.

>
> > +    /**
> > +     * 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...
>
>
Okay will remove the above functions.


> > +    /**
> > +     * 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.
>

Make sense, can I change it to vkapi instead to be consistent?


>
> > 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].
>

Actually there is no particular reason for it to be [3], looking through
code initially,
some using [3] such as VAAPI, QSV, DXVA2, etc. So we used the same, but now
I see
others using [0] such as DRM, VULKAN.
I'll change it [0], which makes sense.

Thank you

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4218 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210311/41096f36/attachment.bin>