[FFmpeg-devel] [PATCH] lavu: add VKAPI hwcontext implementation
Suji Velupillai
suji.velupillai at broadcom.com
Tue Mar 16 23:14:23 EET 2021
Thank you Mark for your feedback. Please see inline
On Fri, Mar 12, 2021 at 1:14 PM Mark Thompson <sw at jkqxz.net> wrote:
> On 11/03/2021 22:09, suji.velupillai at broadcom.com wrote:
> > 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
> >
> > Signed-off-by: Suji Velupillai <suji.velupillai at broadcom.com>
> > ---
> > configure | 8 +-
> > doc/APIchanges | 4 +
> > libavutil/Makefile | 2 +
> > libavutil/hwcontext.c | 4 +
> > libavutil/hwcontext.h | 1 +
> > libavutil/hwcontext_internal.h | 1 +
> > libavutil/hwcontext_vkapi.c | 522 +++++++++++++++++++++++++++++++++
> > libavutil/hwcontext_vkapi.h | 104 +++++++
> > libavutil/pixdesc.c | 4 +
> > libavutil/pixfmt.h | 6 +
> > 10 files changed, 655 insertions(+), 1 deletion(-)
> > create mode 100644 libavutil/hwcontext_vkapi.c
> > create mode 100644 libavutil/hwcontext_vkapi.h
>
> Where has the "vkapi" name come from? It seems to be consistently called
> "vkil" in that repository.
valkyrie is the hardware name. vkil refers to VK Interface Layer. vkapi is
the name given to all ffmpeg API's.
>
If you are making up the name for this, please consider making it less
> confusing:
> * The standard Vulkan API already effectively owns the "vk" namespace
> prefix, and colliding with that in a related project is unhelpful.
> * Indeed, Vulkan already uses the "VKAPI" name in its headers when
> marking ABIs (see <
> https://github.com/ARM-software/vulkan-sdk/blob/master/include/vulkan/vk_platform.h#L35
> >).
> * Current search results for "vkapi" show it is also used by APIs for the
> VK social network.
>
Okay I will rename all with VKAPI and VK_* reference to avoid
conflicts/confusion.
Kernel driver code consistently used "bcm_vk", also Google search points
correctly to the kernel driver for this card.
Would it be okay to switch to bcm_vk instead of vkapi and vk?
>
> > ... > diff --git a/libavutil/hwcontext_vkapi.h
> b/libavutil/hwcontext_vkapi.h
> > new file mode 100644
> > index 0000000000..096602b42e
> > --- /dev/null
> > +++ b/libavutil/hwcontext_vkapi.h
> > @@ -0,0 +1,104 @@
> > +/*
> > + * Copyright (c) 2018 Broadcom
> > + *
> > + * 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 AVUTIL_HWCONTEXT_VKAPI_H
> > +#define AVUTIL_HWCONTEXT_VKAPI_H
> > +
> > +#include <vkil_api.h>
> > +
> > +#define VKAPI_METADATA_PLANE 4
> > +
> > +/**
> > + * @file
> > + * API-specific header for AV_HWDEVICE_TYPE_VKAPI.
> > + */
> > +
> > +/**
> > + * 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);
> > + /**
> > + * Set the hwprops into AVFrame:data[3]
> > + */
> > + int (*frame_set_hwprops)(AVFrame *frame, const vkil_buffer_surface
> *hw_surface_desc);
> > + /**
> > + * Get the hwprops from AVFrame:data[3]
> > + */
> > + int (*frame_get_hwprops)(const AVFrame *frame, vkil_buffer_surface
> *hw_surface_desc);
> > + /**
> > + * Check if format is in an array
> > + */
> > + int (*fmt_is_in)(int fmt, const int *fmts);
> > + /**
> > + * Convert AVPixelFormat to VKAPI equivalent pixel format
> > + */
> > + int (*av2vk_fmt)(enum AVPixelFormat pixel_format);
> > + /**
> > + * Get no of buffer count reference in the hardware pool
> > + */
> > + int (*get_pool_occupancy)(AVHWFramesContext *ctx);
> > +} VKAPIDeviceContext;
>
> This structure has two uses:
>
> * To be filled by the user when they already have an instance of the
> device and want to use it with libav*.
> * To provide information to the user about an instance of the device
> created inside libav*.
>
> To that end, the "ilapi" field makes sense to me (the user provides their
> vkil API handle), but they also need to provide some sort of reference to
> the actual device (a vkil_context handle, maybe?).
>
> I don't think any of the other fields make sense; it looks like you are
> trying to expose some internals in a confusing way - why would a user
> creating this structure want to set those function pointers?
>
> Agreed, Lynne also pointed this out, so I removed it and moved it within
the library that really needs the functions.
> > +/**
> > + * Contains color information for hardware
> > + */
> > +typedef struct VKAPIColorContext {
> > + enum AVColorRange range;
> > + enum AVColorPrimaries primaries;
> > + enum AVColorTransferCharacteristic trc;
> > + enum AVColorSpace space;
> > + enum AVChromaLocation chroma_location;
> > +} VKAPIColorContext;
> > +
> > +/**
> > + * Allocated as AVHWFramesContext.hwctx
> > + */
> > +typedef struct VKAPIFramesContext {
> > + /**
> > + * Handle to a hardware frame context
> > + */
> > + uint32_t handle;
> > + /**
> > + * Hardware component port associated to the frame context
> > + */
> > + uint32_t port_id;
> > + uint32_t extra_port_id;
> > + /**
> > + * Color information
> > + */
> > + VKAPIColorContext color;
> > + /**
> > + * ilcontext associated to the frame context
> > + */
> > + vkil_context *ilctx;
> > +} VKAPIFramesContext;
> > +
> > +#endif /* AVUTIL_HWCONTEXT_VKAPI_H */
>
> The vkil_context looks like it should be part of the device.
>
> So sort of handle information for the context of the frame makes sense, ok.
>
> The port_id fields don't seem to be used at all in your implementation,
> which strongly suggests that they should not be here.
>
> The colour information you are including here is generally represented
> per-frame, so attaching it to a frame context seems dubious.
Both of those fields are used in the libavcodec, but it makes sense to be
within the codecs struct itself. I'll review the code and change it.
> > 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
> > + */
> > + AV_PIX_FMT_VKAPI,
>
> As already noted, please use data[0] (data[3] is unhelpfully baked into
> some older formats for historical reasons, apologies for any confusion
> there).
>
> No problem. Thank you for clarification, agreed, I'll change it to
data[0].
Also, you seem to have references to an additional field store in data[4],
> the meaning of which needs to be documented here as well.
>
Okay
>
>
> To understand what is going on with this hwcontext it would be very
> helpful to see some components using it (even in prototype form).
>
We have working encoder/decoder and scaler functionalities for this
hardware in ffmpeg framework, which I will be sending it for review. I
thought to get the hwcontext in first for feedback. It needs some clean up
and changes in code based on this patch review also. I will do that as soon
as possible.
>
> Thanks,
>
> - Mark
> _______________________________________________
> 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/20210316/7978b9c1/attachment.bin>
More information about the ffmpeg-devel
mailing list