From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 7D0524A5DA for ; Thu, 2 May 2024 20:27:14 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 252C668D85A; Thu, 2 May 2024 23:27:12 +0300 (EEST) Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 884FE68CD1B for ; Thu, 2 May 2024 23:27:06 +0300 (EEST) Received: by mail-wm1-f52.google.com with SMTP id 5b1f17b1804b1-41a72f3a20dso58865065e9.0 for ; Thu, 02 May 2024 13:27:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jkqxz-net.20230601.gappssmtp.com; s=20230601; t=1714681626; x=1715286426; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=OMYL9ASPhc2B0LjxqDZcpiv6uVpAMOAyyvcfUGZt0pU=; b=ZUw12bxkf7OtbhtT2FAUaPFeurZ+OjJmr96D4B5E8wnm2cWe2WUOjK6zsrG7acQD/+ vsyE/zUPvDTpfd6nrG3hIcvbRzTzbh1DB3DeVYd9bQWOBfeDfTc+BdsYiEkebHUedgde mHQLwwAtPFBaHkbl+7CgscBVZYvBsv2+P1OEA2mLafLuAcF0OBB1YklyZMthDaMIzrSO 46wixBcmzO+XH9tAoW1K+LHblnNM04CmAIpVjkCKePKLCqw51tDy1hQ49fatNC5+uewq RfR4RCgM8POi0Ej+/xvl56arsxOCAQwO0WFfj8iU4ZDJ3xNmYW+jkeI0iCav1DiumD2/ f32A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714681626; x=1715286426; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=OMYL9ASPhc2B0LjxqDZcpiv6uVpAMOAyyvcfUGZt0pU=; b=REZ4nweLw759IEnMQ7csDdiUtOENf0Pvb/amX1kvzjSli/1gQgj+fDPL1F0fZVClaS la3y5iugPUaY4BiqbD4T5tok4aEKEdtb2+tkIi/cnkE+2XHVIFX+pRnf2gylwVh76Jp2 MQFqRv7xs8QusyslBXKU9SrJwN0FX3zriU8kZB7bAvaMGWmDdCSNq/WcT00Bodv0HM3p 7YP+5MOXkn0VBi8AcoZ5sUQtK0rMu4V2BTcKa+rO50uSuMjTa47gJ9CMGdL+we3hjqSE /VQqVa/cFsppS7TXro8IpyxZ0ve29rosMFSt9aHTUgKCzUml7x5OIzu0vN7c/GSE9GfG cX2Q== X-Gm-Message-State: AOJu0YyDa2IH6+tYJZFVx692Ol3Tjgog6Fk2hIFQumdiTm6mxfl1MOrQ KMWD4GeJj5w4hIto98Ui+2YspDYNqjIyifPilIWqaU+2XiSr9lBRnqyiUhuC2rj70xi5+gdpEZa T X-Google-Smtp-Source: AGHT+IFUr5/qiLS6LTTP5UvwUGYVLkM87mxVK4/+DoNCBEMiZgbF/iGQcRHNapH0p+wJxDMtFSxfFQ== X-Received: by 2002:a05:600c:4fc3:b0:418:969a:b316 with SMTP id o3-20020a05600c4fc300b00418969ab316mr636014wmq.1.1714681625732; Thu, 02 May 2024 13:27:05 -0700 (PDT) Received: from [192.168.0.15] (cpc92302-cmbg19-2-0-cust1183.5-4.cable.virginm.net. [82.1.212.160]) by smtp.gmail.com with ESMTPSA id b20-20020a05600c4e1400b0041892857924sm3108363wmq.36.2024.05.02.13.27.05 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 02 May 2024 13:27:05 -0700 (PDT) Message-ID: <8520efd4-70d1-4f07-99d1-29e18471aa2c@jkqxz.net> Date: Thu, 2 May 2024 21:27:32 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20240501183809.1060-1-ovchinnikov.dmitrii@gmail.com> From: Mark Thompson In-Reply-To: <20240501183809.1060-1-ovchinnikov.dmitrii@gmail.com> Subject: Re: [FFmpeg-devel] [PATCH 01/10, v2] avutil: add hwcontext_amf. X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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 > +#include > +#include > + > +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-request@ffmpeg.org with subject "unsubscribe".