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 874874541F for ; Thu, 30 Mar 2023 20:30:22 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id EEABA68C1FC; Thu, 30 Mar 2023 23:30:19 +0300 (EEST) Received: from mail8.parnet.fi (mail8.parnet.fi [77.234.108.134]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C4E9768C1D9 for ; Thu, 30 Mar 2023 23:30:13 +0300 (EEST) Received: from mail9.parnet.fi (mail9.parnet.fi [77.234.108.21]) by mail8.parnet.fi with ESMTP id 32UKUC3M013504-32UKUC3N013504; Thu, 30 Mar 2023 23:30:12 +0300 Received: from foo.martin.st (host-97-187.parnet.fi [77.234.97.187]) by mail9.parnet.fi (Postfix) with ESMTPS id 097F1A1469; Thu, 30 Mar 2023 23:30:11 +0300 (EEST) Date: Thu, 30 Mar 2023 23:30:11 +0300 (EEST) From: =?ISO-8859-15?Q?Martin_Storsj=F6?= To: FFmpeg development discussions and patches In-Reply-To: <20230329155933.2138-1-info@raphael.schlarb.one> Message-ID: <7db40e0-1b9f-b14c-51-10e38ee25a9@martin.st> References: <20230327143705.2371-1-info@raphael.schlarb.one> <20230329155933.2138-1-info@raphael.schlarb.one> MIME-Version: 1.0 X-FE-Policy-ID: 3:14:2:SYSTEM Subject: Re: [FFmpeg-devel] [PATCH v3] avcodec/libfdk-accenc: Add option to set frame length when encoding with libfdk_aac 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 Cc: Raphael Schlarb Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Wed, 29 Mar 2023, Raphael Schlarb wrote: > Some specifications require the size of ld/eld frames to be 480 samples > instead of the default 512. libfdk_aac provides an option to set an alternative > frame size, but it's not exposed via the ffmpeg interface. > This patch adds a frame_length option to solve this problem. > --- > Added missing context to commit message > libavcodec/libfdk-aacenc.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/libavcodec/libfdk-aacenc.c b/libavcodec/libfdk-aacenc.c > index eb97e0fb41..93c26ee082 100644 > --- a/libavcodec/libfdk-aacenc.c > +++ b/libavcodec/libfdk-aacenc.c > @@ -55,6 +55,7 @@ typedef struct AACContext { > int metadata_mode; > AACENC_MetaData metaDataSetup; > int delay_sent; > + int frame_length; > > AudioFrameQueue afq; > } AACContext; > @@ -78,6 +79,7 @@ static const AVOption aac_enc_options[] = { > { "comp_profile", "The desired compression profile for AAC DRC", offsetof(AACContext, comp_profile), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 256, AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_ENCODING_PARAM }, > { "comp_target_ref", "Expected target reference level at decoder side in dB (for clipping prevention/limiter)", offsetof(AACContext, comp_target_ref), AV_OPT_TYPE_INT, { .i64 = 0.0 }, -31.75, 0, AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_ENCODING_PARAM }, > { "prog_ref", "The program reference level or dialog level in dB", offsetof(AACContext, prog_ref), AV_OPT_TYPE_INT, { .i64 = 0.0 }, -31.75, 0, AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_ENCODING_PARAM }, > + { "frame_length", "The desired frame length", offsetof(AACContext, frame_length), AV_OPT_TYPE_INT, { .i64 = 1024 }, 120, 1024, AV_OPT_FLAG_AUDIO_PARAM | AV_OPT_FLAG_ENCODING_PARAM }, > FF_AAC_PROFILE_OPTS > { NULL } > }; > @@ -166,6 +168,18 @@ static av_cold int aac_encode_init(AVCodecContext *avctx) > } > } > > + if (s->frame_length == 1024 && > + (aot == FF_PROFILE_AAC_LD + 1 || aot == FF_PROFILE_AAC_ELD + 1)) { > + s->frame_length = 512; > + } I'm not a fan of having to trying to guess the default like this here. > + > + if ((err = aacEncoder_SetParam(s->handle, AACENC_GRANULE_LENGTH, > + s->frame_length)) != AACENC_OK) { > + av_log(avctx, AV_LOG_ERROR, "Unable to set granule length: %s\n", > + aac_get_error(err)); > + goto error; > + } Instead of always, unconditionally setting this parameter, can't we make it optional - set the default value of the option to e.g. -1 or something like that, and only set the parameter if the user actually has set it? That way there's no risk of any behaviour change introduced by this patch in any current existing case, and any issues brought by the parameter only affect users that actually do set the option. // Martin _______________________________________________ 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".