Re: [FFmpeg-devel] [PATCH v3] avcodec/libfdk-accenc: Add option to set frame length when encoding with libfdk_aac

From: "Martin Storsjö" <martin@martin.st>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: Raphael Schlarb <info@raphael.schlarb.one>
Subject: Re: [FFmpeg-devel] [PATCH v3] avcodec/libfdk-accenc: Add option to set frame length when encoding with libfdk_aac
Date: Thu, 30 Mar 2023 23:30:11 +0300 (EEST)
Message-ID: <7db40e0-1b9f-b14c-51-10e38ee25a9@martin.st> (raw)
In-Reply-To: <20230329155933.2138-1-info@raphael.schlarb.one>

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".