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 8785B4A5C0 for ; Tue, 2 Jul 2024 19:08:44 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id B7CBA68D945; Tue, 2 Jul 2024 22:08:41 +0300 (EEST) Received: from mail-ed1-f43.google.com (mail-ed1-f43.google.com [209.85.208.43]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 2C27C68D8DA for ; Tue, 2 Jul 2024 22:08:35 +0300 (EEST) Received: by mail-ed1-f43.google.com with SMTP id 4fb4d7f45d1cf-58b447c5112so1394873a12.3 for ; Tue, 02 Jul 2024 12:08:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1719947314; x=1720552114; darn=ffmpeg.org; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=sjuR+lLdK3ggHARWbqfUvxDXR1MKt+vRD5WES86fkTM=; b=TCpsf/Fi116zqZjsWqvuswC6uAAHN4HV75HB59Q8PtnucqNMObiCtFDHY0qkYIzzwC AXWBdCQElgUtEpN9hvNEi8aWCFYoSRqUlkgL2lRoaMeZC0EWqpFYmGdndTLplcHYpHYU c9BeetGomNvlMR2IlMYa6fXivYZkhJ8iy6a+Kv1wFUoG8/cTgyZ6iilgOS5jXi2PUOoD vN92T1ZeOYlJQWlAOQZAIEwnYsa2QSh56HU4vvl0hpAy0+w+lgCvmuOKkhMg1sGulXzs 3OBD7agnXxM/Bi5/TSueqkj73DOZ+k0FDedWDoCYQ7rO94ZBVebmQKza2MeNxU8Igrs5 +/lA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719947314; x=1720552114; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=sjuR+lLdK3ggHARWbqfUvxDXR1MKt+vRD5WES86fkTM=; b=H6fdr9/BTWemTIzZxq8kaX3O+pwGmuxN5Geoaf9iHqIA49FIjxBXGdkYpPtGs94kFT TS0vuShNV3dfy6stzQH8MDny7NGFvBfW0Qqv3z6tTAdV0VmjzzVo43y7+z2TXvCg2aki hmiLyGZtkR8nJKhBfTMTVO0O22o/02/EPCpSVj/mI60PG/hZlkuirh+/NUImiTNdXGMM 18col8vTIy+WMP5tZDwzeW+r2ibZOIwpUgJ2EEmdAL+iXJ/hncHG/5qEtAyZZj+5PsRy yCnq6DyyvJkHaHZ0LOWnR9n/dfKLeKurfPdNlh/VAXlxCBE5Iv7sNud4OB2XFwreMSMi o2dQ== X-Gm-Message-State: AOJu0Yx9tFs4aiLS3ahmIg8yfxyrR8dWQUW5Eg5HPT8i7WSXES2j9oDm QzXygYL0HBHFxSlSy+e2GmUhaC3hNXe6XKL/n2VKHxQioYvaezJqIL47KAo/tdMDde9AEHbq3tU zZX/Pue2bpZ65ONrRcf4nXu3PtbnEUw== X-Google-Smtp-Source: AGHT+IHMg8SvMH1soapUbUKuUUEY9pgPbS+BxlxT42RRy55XhoIK/RdvCf6TM5sctem3PmIv5yYM13V6VukwmhBDYx0= X-Received: by 2002:a05:6402:34d5:b0:57c:7471:a0dd with SMTP id 4fb4d7f45d1cf-5879f4a56f0mr8364558a12.12.1719947313939; Tue, 02 Jul 2024 12:08:33 -0700 (PDT) MIME-Version: 1.0 References: <20240528154807.1151-1-tong1.wu@intel.com> <20240528154807.1151-15-tong1.wu@intel.com> <3c6857f5-5c29-440c-a13d-8130296f14be@lynne.ee> <04370e78-0311-4c42-a065-890889b7d6c5@lynne.ee> In-Reply-To: From: Sean McGovern Date: Tue, 2 Jul 2024 15:08:23 -0400 Message-ID: To: FFmpeg development discussions and patches X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [FFmpeg-devel] [PATCH v12 15/15] avcodec/hw_base_encode: add avctx pointer for FFHWBaseEncodeContext 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: Hi Tong, On Mon, Jun 3, 2024, 05:23 Wu, Tong1 wrote: > >From: Lynne > >Sent: Wednesday, May 29, 2024 8:02 AM > >To: Wu, Tong1 ; FFmpeg development discussions and > >patches > >Subject: Re: [FFmpeg-devel] [PATCH v12 15/15] avcodec/hw_base_encode: add > >avctx pointer for FFHWBaseEncodeContext > > > >On 29/05/2024 00:53, Wu, Tong1 wrote: > >>> From: ffmpeg-devel On Behalf Of > >Lynne > >>> via ffmpeg-devel > >>> Sent: Wednesday, May 29, 2024 1:08 AM > >>> To: ffmpeg-devel@ffmpeg.org > >>> Cc: Lynne > >>> Subject: Re: [FFmpeg-devel] [PATCH v12 15/15] avcodec/hw_base_encode: > >add > >>> avctx pointer for FFHWBaseEncodeContext > >>> > >>> On 28/05/2024 17:48, tong1.wu-at-intel.com@ffmpeg.org wrote: > >>>> From: Tong Wu > >>>> > >>>> An avctx pointer is added to FFHWBaseEncodeContext. This is to make > >>>> FFHWBaseEncodeContext a standalone component for ff_hw_base_* > >>> functions. > >>>> This patch also removes some unnecessary AVCodecContext arguments. > >>>> > >>>> Signed-off-by: Tong Wu > >>>> --- > >>>> libavcodec/d3d12va_encode.c | 6 +++--- > >>>> libavcodec/hw_base_encode.c | 31 +++++++++++++------------------ > >>>> libavcodec/hw_base_encode.h | 8 +++++--- > >>>> libavcodec/vaapi_encode.c | 6 +++--- > >>>> 4 files changed, 24 insertions(+), 27 deletions(-) > >>>> > >>>> diff --git a/libavcodec/d3d12va_encode.c b/libavcodec/d3d12va_encode.c > >>>> index 0fbf8eb07c..6d3a53c6ca 100644 > >>>> --- a/libavcodec/d3d12va_encode.c > >>>> +++ b/libavcodec/d3d12va_encode.c > >>>> @@ -1351,7 +1351,7 @@ static int > >>> d3d12va_encode_create_recon_frames(AVCodecContext *avctx) > >>>> enum AVPixelFormat recon_format; > >>>> int err; > >>>> > >>>> - err = ff_hw_base_get_recon_format(avctx, NULL, &recon_format); > >>>> + err = ff_hw_base_get_recon_format(base_ctx, NULL, &recon_format); > >>>> if (err < 0) > >>>> return err; > >>>> > >>>> @@ -1398,7 +1398,7 @@ int ff_d3d12va_encode_init(AVCodecContext > >>> *avctx) > >>>> int err; > >>>> HRESULT hr; > >>>> > >>>> - err = ff_hw_base_encode_init(avctx); > >>>> + err = ff_hw_base_encode_init(avctx, base_ctx); > >>>> if (err < 0) > >>>> goto fail; > >>>> > >>>> @@ -1552,7 +1552,7 @@ int ff_d3d12va_encode_close(AVCodecContext > >>> *avctx) > >>>> D3D12_OBJECT_RELEASE(ctx->video_device3); > >>>> D3D12_OBJECT_RELEASE(ctx->device3); > >>>> > >>>> - ff_hw_base_encode_close(avctx); > >>>> + ff_hw_base_encode_close(base_ctx); > >>>> > >>>> return 0; > >>>> } > >>>> diff --git a/libavcodec/hw_base_encode.c b/libavcodec/hw_base_encode.c > >>>> index 92f69bb78c..88efdf672c 100644 > >>>> --- a/libavcodec/hw_base_encode.c > >>>> +++ b/libavcodec/hw_base_encode.c > >>>> @@ -94,14 +94,13 @@ static void > >>> hw_base_encode_remove_refs(FFHWBaseEncodePicture *pic, int level) > >>>> pic->ref_removed[level] = 1; > >>>> } > >>>> > >>>> -static void hw_base_encode_set_b_pictures(AVCodecContext *avctx, > >>>> +static void hw_base_encode_set_b_pictures(FFHWBaseEncodeContext > >*ctx, > >>>> FFHWBaseEncodePicture > *start, > >>>> FFHWBaseEncodePicture > *end, > >>>> FFHWBaseEncodePicture > *prev, > >>>> int current_depth, > >>>> FFHWBaseEncodePicture > **last) > >>>> { > >>>> - FFHWBaseEncodeContext *ctx = avctx->priv_data; > >>>> FFHWBaseEncodePicture *pic, *next, *ref; > >>>> int i, len; > >>>> > >>>> @@ -148,20 +147,19 @@ static void > >>> hw_base_encode_set_b_pictures(AVCodecContext *avctx, > >>>> hw_base_encode_add_ref(pic, ref, 0, 1, 0); > >>>> > >>>> if (i > 1) > >>>> - hw_base_encode_set_b_pictures(avctx, start, pic, pic, > >>>> + hw_base_encode_set_b_pictures(ctx, start, pic, pic, > >>>> current_depth + 1, > &next); > >>>> else > >>>> next = pic; > >>>> > >>>> - hw_base_encode_set_b_pictures(avctx, pic, end, next, > >>>> + hw_base_encode_set_b_pictures(ctx, pic, end, next, > >>>> current_depth + 1, last); > >>>> } > >>>> } > >>>> > >>>> -static void hw_base_encode_add_next_prev(AVCodecContext *avctx, > >>>> +static void hw_base_encode_add_next_prev(FFHWBaseEncodeContext > >>> *ctx, > >>>> FFHWBaseEncodePicture > *pic) > >>>> { > >>>> - FFHWBaseEncodeContext *ctx = avctx->priv_data; > >>>> int i; > >>>> > >>>> if (!pic) > >>>> @@ -333,12 +331,12 @@ static int > >>> hw_base_encode_pick_next(AVCodecContext *avctx, > >>>> } > >>>> > >>>> if (b_counter > 0) { > >>>> - hw_base_encode_set_b_pictures(avctx, start, pic, pic, 1, > >>>> + hw_base_encode_set_b_pictures(ctx, start, pic, pic, 1, > >>>> &prev); > >>>> } else { > >>>> prev = pic; > >>>> } > >>>> - hw_base_encode_add_next_prev(avctx, prev); > >>>> + hw_base_encode_add_next_prev(ctx, prev); > >>>> > >>>> return 0; > >>>> } > >>>> @@ -687,9 +685,9 @@ int > >ff_hw_base_init_gop_structure(AVCodecContext > >>> *avctx, uint32_t ref_l0, uint32 > >>>> return 0; > >>>> } > >>>> > >>>> -int ff_hw_base_get_recon_format(AVCodecContext *avctx, const void > >>> *hwconfig, enum AVPixelFormat *fmt) > >>>> +int ff_hw_base_get_recon_format(FFHWBaseEncodeContext *ctx, const > >>> void *hwconfig, > >>>> + enum AVPixelFormat *fmt) > >>>> { > >>>> - FFHWBaseEncodeContext *ctx = avctx->priv_data; > >>>> AVHWFramesConstraints *constraints = NULL; > >>>> enum AVPixelFormat recon_format; > >>>> int err, i; > >>>> @@ -722,14 +720,14 @@ int > >>> ff_hw_base_get_recon_format(AVCodecContext *avctx, const void > >*hwconfig, > >>> enu > >>>> // No idea what to use; copy input format. > >>>> recon_format = ctx->input_frames->sw_format; > >>>> } > >>>> - av_log(avctx, AV_LOG_DEBUG, "Using %s as format of " > >>>> + av_log(ctx->avctx, AV_LOG_DEBUG, "Using %s as format of " > >>>> "reconstructed frames.\n", > av_get_pix_fmt_name(recon_format)); > >>>> > >>>> if (ctx->surface_width < constraints->min_width || > >>>> ctx->surface_height < constraints->min_height || > >>>> ctx->surface_width > constraints->max_width || > >>>> ctx->surface_height > constraints->max_height) { > >>>> - av_log(avctx, AV_LOG_ERROR, "Hardware does not support > encoding > >at > >>> " > >>>> + av_log(ctx->avctx, AV_LOG_ERROR, "Hardware does not support > >>> encoding at " > >>>> "size %dx%d (constraints: width %d-%d height > %d-%d).\n", > >>>> ctx->surface_width, ctx->surface_height, > >>>> constraints->min_width, constraints->max_width, > >>>> @@ -756,10 +754,9 @@ int > >>> ff_hw_base_encode_free(FFHWBaseEncodePicture *pic) > >>>> return 0; > >>>> } > >>>> > >>>> -int ff_hw_base_encode_init(AVCodecContext *avctx) > >>>> +int ff_hw_base_encode_init(AVCodecContext *avctx, > >>> FFHWBaseEncodeContext *ctx) > >>>> { > >>>> - FFHWBaseEncodeContext *ctx = avctx->priv_data; > >>>> - > >>>> + ctx->avctx = avctx; > >>>> ctx->frame = av_frame_alloc(); > >>>> if (!ctx->frame) > >>>> return AVERROR(ENOMEM); > >>>> @@ -789,10 +786,8 @@ int ff_hw_base_encode_init(AVCodecContext > >>> *avctx) > >>>> return 0; > >>>> } > >>>> > >>>> -int ff_hw_base_encode_close(AVCodecContext *avctx) > >>>> +int ff_hw_base_encode_close(FFHWBaseEncodeContext *ctx) > >>>> { > >>>> - FFHWBaseEncodeContext *ctx = avctx->priv_data; > >>>> - > >>>> av_fifo_freep2(&ctx->encode_fifo); > >>>> > >>>> av_frame_free(&ctx->frame); > >>>> diff --git a/libavcodec/hw_base_encode.h > >b/libavcodec/hw_base_encode.h > >>>> index 15ef3d7ac6..13c1fc0f69 100644 > >>>> --- a/libavcodec/hw_base_encode.h > >>>> +++ b/libavcodec/hw_base_encode.h > >>>> @@ -116,6 +116,7 @@ typedef struct FFHWEncodePictureOperation { > >>>> > >>>> typedef struct FFHWBaseEncodeContext { > >>>> const AVClass *class; > >>>> + AVCodecContext *avctx; > >>>> > >>>> // Hardware-specific hooks. > >>>> const struct FFHWEncodePictureOperation *op; > >>>> @@ -222,13 +223,14 @@ int > >>> ff_hw_base_encode_receive_packet(AVCodecContext *avctx, AVPacket > >*pkt); > >>>> int ff_hw_base_init_gop_structure(AVCodecContext *avctx, uint32_t > >ref_l0, > >>> uint32_t ref_l1, > >>>> int flags, int > prediction_pre_only); > >>>> > >>>> -int ff_hw_base_get_recon_format(AVCodecContext *avctx, const void > >>> *hwconfig, enum AVPixelFormat *fmt); > >>>> +int ff_hw_base_get_recon_format(FFHWBaseEncodeContext *ctx, const > >>> void *hwconfig, > >>>> + enum AVPixelFormat *fmt); > >>>> > >>>> int ff_hw_base_encode_free(FFHWBaseEncodePicture *pic); > >>>> > >>>> -int ff_hw_base_encode_init(AVCodecContext *avctx); > >>>> +int ff_hw_base_encode_init(AVCodecContext *avctx, > >>> FFHWBaseEncodeContext *ctx); > >>>> > >>>> -int ff_hw_base_encode_close(AVCodecContext *avctx); > >>>> +int ff_hw_base_encode_close(FFHWBaseEncodeContext *ctx); > >>>> > >>>> #define HW_BASE_ENCODE_COMMON_OPTIONS \ > >>>> { "idr_interval", \ > >>>> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c > >>>> index b35a23e852..0693e77548 100644 > >>>> --- a/libavcodec/vaapi_encode.c > >>>> +++ b/libavcodec/vaapi_encode.c > >>>> @@ -2059,7 +2059,7 @@ static av_cold int > >>> vaapi_encode_create_recon_frames(AVCodecContext *avctx) > >>>> } > >>>> hwconfig->config_id = ctx->va_config; > >>>> > >>>> - err = ff_hw_base_get_recon_format(avctx, (const void*)hwconfig, > >>> &recon_format); > >>>> + err = ff_hw_base_get_recon_format(base_ctx, (const > void*)hwconfig, > >>> &recon_format); > >>>> if (err < 0) > >>>> goto fail; > >>>> > >>>> @@ -2106,7 +2106,7 @@ av_cold int > >ff_vaapi_encode_init(AVCodecContext > >>> *avctx) > >>>> VAStatus vas; > >>>> int err; > >>>> > >>>> - err = ff_hw_base_encode_init(avctx); > >>>> + err = ff_hw_base_encode_init(avctx, base_ctx); > >>>> if (err < 0) > >>>> goto fail; > >>>> > >>>> @@ -2313,7 +2313,7 @@ av_cold int > >>> ff_vaapi_encode_close(AVCodecContext *avctx) > >>>> av_freep(&ctx->codec_sequence_params); > >>>> av_freep(&ctx->codec_picture_params); > >>>> > >>>> - ff_hw_base_encode_close(avctx); > >>>> + ff_hw_base_encode_close(base_ctx); > >>>> > >>>> return 0; > >>>> } > >>> > >>> Err, you missed ff_hw_base_encode_set_output_property, > >>> ff_hw_base_encode_receive_packet and ff_hw_base_init_gop_structure? > >>> > >>> Rest looks better. > >> > >> ff_hw_base_encode_receive_packet is directly bind to int > >(*receive_packet)(struct AVCodecContext *avctx, struct AVPacket *avpkt); I > >thought maybe this should not be changed? For the other two functions, > >components in avctx are used. If they need to be changed, I could either > take > >both FFHWBaseEncodeContext and AVCodecContext as arguments or use ctx- > >>avctx->* to get the components which one do you think better. Thank you. > > > >Take both FFHWBaseEncodeContext and AVCodecContext as arguments, and > >use > >it to get fields such as gop_size, max_b_frames and so on. > > > >*Do not use* avctx->priv_data anywhere in hw_base_encode. Any usage of > >it has to be switched to a user-given FFHWBaseEncodeContext via a > >function argument. > > > >ff_hw_base_encode_receive_packet needs to be switched to > >(FFHWBaseEncodeContext, AVCodecContext...) as well, as it uses > >avctx->priv_data. > > > >Hardware encoders should have small wrappers for receive_packet which > >would call ff_hw_base_encode_receive_packet. It's not much, just 3 lines. > > > >Hardware encoders could still use FFHWBaseEncodeContext as their own > >main encode context, but they would no longer be required to use it as > >their main context. It'll make sense with some code: > > > >For example, d3d12va_encode's wrapper function would be: > >static int receive_packet(avctx...) > >{ > > return ff_hw_base_encode_receive_packet(avctx->priv_data, avctx...) > >} > > > >Vulkan's wrapper would, on the other hand look like: > >static int receive_packet(avctx...) > >{ > > VulkanEncodeContext *vkenc = avctx->priv_data; > > return ff_hw_base_encode_receive_packet(vkenc->hw_base, avctx...) > >} > > > >You should also rename FFHWBaseEncodeContext.avctx to > >FFHWBaseEncodeContext.log_ctx with a type of void *, and use it for > >av_log where needed. It would be set to avctx in > >ff_hw_base_encode_init(), but encoders could override it if they needed > >to by modifying the context. > >If frame threading is ever implemented, this change would make it > >easier, as the context for each call would be exlicitly known. > > > > Updated in v13. I've added wrappers in vaapi_encode.c and d3d12_encode.c > to make it available for all the codecs(vaapi_encode_* and > d3d12va_encode_hevc). Thanks. > _______________________________________________ > 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". > Something in this set broke the 'check headers' FATE node. Can you please look at it? Thanks, Sean McGovern > _______________________________________________ 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".