* Re: [FFmpeg-devel] [PATCH] lavu: add VKAPI hwcontext implementation
[not found] ` <CAJHBTCCVNkiV-v4Cinj4To8njM0NTEnjao0DpAouG_M-aSY_rA@mail.gmail.com>
@ 2022-02-24 17:48 ` Dennis Mungai
0 siblings, 0 replies; only message in thread
From: Dennis Mungai @ 2022-02-24 17:48 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Wed, 17 Mar 2021 at 00:14, Suji Velupillai <suji.velupillai@broadcom.com>
wrote:
> Thank you Mark for your feedback. Please see inline
>
> On Fri, Mar 12, 2021 at 1:14 PM Mark Thompson <sw@jkqxz.net> wrote:
>
> > On 11/03/2021 22:09, suji.velupillai@broadcom.com wrote:
> > > From: Suji Velupillai <suji.velupillai@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@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
>
>
Hello there,
Any updates on this?
Warm regards,
Dennis.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] only message in thread