Thank you for the review, please see inline. On Thu, Mar 11, 2021 at 2:56 PM Lynne <d...@lynne.ee> wrote:
> 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. > > 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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
smime.p7s
Description: S/MIME Cryptographic Signature
_______________________________________________ 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".
