On 01/05/2024 19:38, Dmitrii Ovchinnikov wrote: > Adds hwcontext_amf, which allows to use shared AMF > context for the encoder, decoder and AMF-based filters, > without copy to the host memory. > It will also allow you to use some optimizations in > the interaction of components (for example, SAV) and make a more > manageable and optimal setup for using GPU devices with AMF > in the case of a fully AMF pipeline. > It will be a significant performance uplift when full AMF pipeline > with filters is used. > > We also plan to add Compression artefact removal filter in near feature. > v2: cleanup header files, fixed naming and formats > --- > libavutil/Makefile | 4 + > libavutil/hwcontext.c | 4 + > libavutil/hwcontext.h | 1 + > libavutil/hwcontext_amf.c | 588 +++++++++++++++++++++++++++++ > libavutil/hwcontext_amf.h | 41 ++ > libavutil/hwcontext_amf_internal.h | 77 ++++ > libavutil/hwcontext_internal.h | 1 + > libavutil/pixdesc.c | 4 + > libavutil/pixfmt.h | 5 + > 9 files changed, 725 insertions(+) > create mode 100644 libavutil/hwcontext_amf.c > create mode 100644 libavutil/hwcontext_amf.h > create mode 100644 libavutil/hwcontext_amf_internal.h > > ... > diff --git a/libavutil/hwcontext_amf.c b/libavutil/hwcontext_amf.c > new file mode 100644 > index 0000000000..8edfb20fbb > --- /dev/null > +++ b/libavutil/hwcontext_amf.c > ... > + > +static void amf_dummy_free(void *opaque, uint8_t *data) > +{ > + > +} > + > +static AVBufferRef *amf_pool_alloc(void *opaque, size_t size) > +{ > + AVHWFramesContext *hwfc = (AVHWFramesContext *)opaque; > + AVBufferRef *buf; > + > + buf = av_buffer_create(NULL, NULL, amf_dummy_free, hwfc, > AV_BUFFER_FLAG_READONLY); > + if (!buf) { > + av_log(hwfc, AV_LOG_ERROR, "Failed to create buffer for AMF > context.\n"); > + return NULL; > + } > + return buf; > +}
This seems to have forgotten to actually allocate anything? It will always assign NULL to frame->data[0] below. > + > +static int amf_frames_init(AVHWFramesContext *ctx) > +{ > + int i; > + > + for (i = 0; i < FF_ARRAY_ELEMS(supported_formats); i++) { > + if (ctx->sw_format == supported_formats[i]) > + break; > + } > + if (i == FF_ARRAY_ELEMS(supported_formats)) { > + av_log(ctx, AV_LOG_ERROR, "Pixel format '%s' is not supported\n", > + av_get_pix_fmt_name(ctx->sw_format)); > + return AVERROR(ENOSYS); > + } > + > + ffhwframesctx(ctx)->pool_internal = > + av_buffer_pool_init2(sizeof(AMFSurface), ctx, > + amf_pool_alloc, NULL); > + > + > + return 0; > +} > + > +static int amf_get_buffer(AVHWFramesContext *ctx, AVFrame *frame) > +{ > + frame->buf[0] = av_buffer_pool_get(ctx->pool); > + if (!frame->buf[0]) > + return AVERROR(ENOMEM); > + > + frame->data[0] = frame->buf[0]->data; > + frame->format = AV_PIX_FMT_AMF_SURFACE; > + frame->width = ctx->width; > + frame->height = ctx->height; > + return 0; > +} > + > ... > + > +static int amf_transfer_data_to(AVHWFramesContext *ctx, AVFrame *dst, > + const AVFrame *src) > +{ > + AMFSurface* surface = (AMFSurface*)dst->data[0]; > + AMFPlane *plane; > + uint8_t *dst_data[4]; > + int dst_linesize[4]; > + int planes; > + int i; > + int w = FFMIN(dst->width, src->width); > + int h = FFMIN(dst->height, src->height); > + > + planes = (int)surface->pVtbl->GetPlanesCount(surface); > + av_assert0(planes < FF_ARRAY_ELEMS(dst_data)); > + > + for (i = 0; i < planes; i++) { > + plane = surface->pVtbl->GetPlaneAt(surface, i); > + dst_data[i] = plane->pVtbl->GetNative(plane); > + dst_linesize[i] = plane->pVtbl->GetHPitch(plane); > + } > + av_image_copy(dst_data, dst_linesize, > + (const uint8_t**)src->data, src->linesize, src->format, > + w, h); > + > + return 0; > +} > +static int amf_transfer_data_from(AVHWFramesContext *ctx, AVFrame *dst, > + const AVFrame *src) > +{ > + AMFSurface* surface = (AMFSurface*)src->data[0]; > + AMFPlane *plane; > + uint8_t *src_data[4]; > + int src_linesize[4]; > + int planes; > + int i; > + int w = FFMIN(dst->width, src->width); > + int h = FFMIN(dst->height, src->height); > + int ret; > + > + ret = surface->pVtbl->Convert(surface, AMF_MEMORY_HOST); > + AMF_RETURN_IF_FALSE(ctx, ret == AMF_OK, AVERROR_UNKNOWN, > "Convert(amf::AMF_MEMORY_HOST) failed with error %d\n", AVERROR_UNKNOWN); > + > + planes = (int)surface->pVtbl->GetPlanesCount(surface); > + av_assert0(planes < FF_ARRAY_ELEMS(src_data)); > + > + for (i = 0; i < planes; i++) { > + plane = surface->pVtbl->GetPlaneAt(surface, i); > + src_data[i] = plane->pVtbl->GetNative(plane); > + src_linesize[i] = plane->pVtbl->GetHPitch(plane); > + } > + av_image_copy(dst->data, dst->linesize, > + (const uint8_t **)src_data, src_linesize, dst->format, > + w, h); > + surface->pVtbl->Release(surface); > + return 0; > +} This makes it look like you really wanted to implement map_from, not transfer_data_from. > ...> diff --git a/libavutil/hwcontext_amf.h b/libavutil/hwcontext_amf.h > new file mode 100644 > index 0000000000..36f13890a0 > --- /dev/null > +++ b/libavutil/hwcontext_amf.h > @@ -0,0 +1,41 @@ > +/* > + * 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_AMF_H > +#define AVUTIL_HWCONTEXT_AMF_H > + > +#include "pixfmt.h" > + > +#include "libavformat/avformat.h" libavutil cannot depend on libavformat, that would be circular. > +#include "libavutil/hwcontext.h" > + > +typedef struct AVAMFDeviceContextInternal AVAMFDeviceContextInternal; > + > +/** > + * This struct is allocated as AVHWDeviceContext.hwctx > + */ > + > +typedef struct AVAMFDeviceContext { > + AVBufferRef *internal; So what does a user of this hwcontext set this pointer to? > +} AVAMFDeviceContext; And what should the user be returning for a user-created pool of frames? > + > +enum AMF_SURFACE_FORMAT av_amf_av_to_amf_format(enum AVPixelFormat fmt); > +enum AVPixelFormat av_amf_to_av_format(enum AMF_SURFACE_FORMAT fmt); > + > +#endif /* AVUTIL_HWCONTEXT_AMF_H */ > diff --git a/libavutil/hwcontext_amf_internal.h > b/libavutil/hwcontext_amf_internal.h > new file mode 100644 > index 0000000000..023dc8b4bb > --- /dev/null > +++ b/libavutil/hwcontext_amf_internal.h > @@ -0,0 +1,77 @@ > +/* > + * 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_AMF_INTERNAL_H > +#define AVUTIL_HWCONTEXT_AMF_INTERNAL_H > +#include <AMF/core/Factory.h> > +#include <AMF/core/Context.h> > +#include <AMF/core/Trace.h> > + > +typedef struct AVAMFDeviceContextInternal { > + amf_handle library; ///< handle to DLL library > + AMFFactory *factory; ///< pointer to AMF factory > + AMFDebug *debug; ///< pointer to AMF debug interface > + AMFTrace *trace; ///< pointer to AMF trace interface > + > + amf_uint64 version; ///< version of AMF runtime > + AMFContext *context; ///< AMF context > + AMF_MEMORY_TYPE mem_type; > +} AVAMFDeviceContextInternal; Some of these details look like they should be in the public hwcontext so that a user can create one. Maybe just the AMFContext? I'm not sure of the details. > + > +typedef struct AmfTraceWriter { > + AMFTraceWriterVtbl *vtbl; > + void *avctx; > + void *avcl; > +} AmfTraceWriter; > + > +extern AmfTraceWriter av_amf_trace_writer; Mutable variables are not allowed in the libraries, external mutable variables doubly so. > + > +int av_amf_context_init(AVAMFDeviceContextInternal* internal, void* avcl); > +int av_amf_create_context( AVAMFDeviceContextInternal * internal, > + void* avcl, > + const char *device, > + AVDictionary *opts, int flags); > +int av_amf_context_internal_create(AVAMFDeviceContextInternal * internal, > + void* avcl, > + const char *device, > + AVDictionary *opts, int flags); > +void av_amf_context_internal_free(void *opaque, uint8_t *data); > +int av_amf_context_derive(AVAMFDeviceContextInternal * internal, > + AVHWDeviceContext *child_device_ctx, > AVDictionary *opts, > + int flags); No. Moving your public functions to an "internal" header is not helpful. All of these appear to be exposing internals of the hwcontext functions, so should be implemented through them. > + > + > +/** > +* Error handling helper > +*/ > +#define AMF_RETURN_IF_FALSE(avctx, exp, ret_value, /*message,*/ ...) \ > + if (!(exp)) { \ > + av_log(avctx, AV_LOG_ERROR, __VA_ARGS__); \ > + return ret_value; \ > + } > + > +#define AMF_GOTO_FAIL_IF_FALSE(avctx, exp, ret_value, /*message,*/ ...) \ > + if (!(exp)) { \ > + av_log(avctx, AV_LOG_ERROR, __VA_ARGS__); \ > + ret = ret_value; \ > + goto fail; \ > + } > + > +#define AMF_TIME_BASE_Q (AVRational){1, AMF_SECOND} > +#endif /* AVUTIL_HWCONTEXT_AMF_INTERNAL_H */ > \ No newline at end of file Free review comment from your git client. > ... Thanks, - Mark _______________________________________________ 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".