Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA
On 22.02.2023 03:12, JonHGee wrote: libavcodec/libfdk-aacenc: VBR mode currently does not account for scaling when using -aq options with libfdk, resulting in clamping to vbr mode 5 whenever a value >0 is provided. Adjusting for the scaling factor for proper VBR support. --- libavcodec/libfdk-aacenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/libfdk-aacenc.c b/libavcodec/libfdk-aacenc.c index 54549de473..da211baf51 100644 --- a/libavcodec/libfdk-aacenc.c +++ b/libavcodec/libfdk-aacenc.c @@ -230,7 +230,7 @@ static av_cold int aac_encode_init(AVCodecContext *avctx) } if (avctx->flags & AV_CODEC_FLAG_QSCALE || s->vbr) { -int mode = s->vbr ? s->vbr : avctx->global_quality; +int mode = s->vbr ? s->vbr : avctx->global_quality / FF_QP2LAMBDA; if (mode < 1 || mode > 5) { av_log(avctx, AV_LOG_WARNING, "VBR quality %d out of range, should be 1-5\n", mode); Won't this break every existing command line and API client that has passed a value according to the current scale? Also, what binds stronger here? It this "(s->vbr ? s->vbr : avctx->global_quality) / FF_QP2LAMBDA" or "s->vbr ? s->vbr : (avctx->global_quality / FF_QP2LAMBDA)"? In any case, this does not look correct to me. Where would the sudden multiplication with FF_QP2LAMBDA come from in the first place? ___ 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".
Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA
On 2023-02-22 09:39 pm, Timo Rothenpieler wrote: On 22.02.2023 03:12, JonHGee wrote: libavcodec/libfdk-aacenc: VBR mode currently does not account for scaling when using -aq options with libfdk, resulting in clamping to vbr mode 5 whenever a value >0 is provided. Adjusting for the scaling factor for proper VBR support. --- libavcodec/libfdk-aacenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/libfdk-aacenc.c b/libavcodec/libfdk-aacenc.c index 54549de473..da211baf51 100644 --- a/libavcodec/libfdk-aacenc.c +++ b/libavcodec/libfdk-aacenc.c @@ -230,7 +230,7 @@ static av_cold int aac_encode_init(AVCodecContext *avctx) } if (avctx->flags & AV_CODEC_FLAG_QSCALE || s->vbr) { - int mode = s->vbr ? s->vbr : avctx->global_quality; + int mode = s->vbr ? s->vbr : avctx->global_quality / FF_QP2LAMBDA; if (mode < 1 || mode > 5) { av_log(avctx, AV_LOG_WARNING, "VBR quality %d out of range, should be 1-5\n", mode); Won't this break every existing command line and API client that has passed a value according to the current scale? Also, what binds stronger here? It this "(s->vbr ? s->vbr : avctx->global_quality) / FF_QP2LAMBDA" or "s->vbr ? s->vbr : (avctx->global_quality / FF_QP2LAMBDA)"? In any case, this does not look correct to me. Where would the sudden multiplication with FF_QP2LAMBDA come from in the first place? From fftools\ffmpeg_mux_init.c 619: ost->enc_ctx->global_quality = FF_QP2LAMBDA * qscale Regards, Gyan ___ 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".
Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA
On 22.02.2023 17:33, Gyan Doshi wrote: From fftools\ffmpeg_mux_init.c 619: ost->enc_ctx->global_quality = FF_QP2LAMBDA * qscale Regards, Gyan But that's only if you set the old qscale CLI options. If you set the global_quality option directly, there is no factor applied. By dividing by FF_QP2LAMBDA you break setting this value via global_quality, and instead add support for setting it via the old and pretty much abandoned qscale interface. Which is an API break. ___ 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".
Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA
On 2023-02-22 10:12 pm, Timo Rothenpieler wrote: On 22.02.2023 17:33, Gyan Doshi wrote: From fftools\ffmpeg_mux_init.c 619: ost->enc_ctx->global_quality = FF_QP2LAMBDA * qscale Regards, Gyan But that's only if you set the old qscale CLI options. If you set the global_quality option directly, there is no factor applied. By dividing by FF_QP2LAMBDA you break setting this value via global_quality, and instead add support for setting it via the old and pretty much abandoned qscale interface. FWIW, that's what LAME does. libavcodec\libmp3lame.c 119: lame_set_VBR_quality(s->gfp, avctx->global_quality / (float)FF_QP2LAMBDA); global_quality semantics seem overloaded. Maybe we should just redirect user to priv vbr and error out in fdk init. Regards, Gyan ___ 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".
Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA
On 22.02.2023 17:46, Gyan Doshi wrote: On 2023-02-22 10:12 pm, Timo Rothenpieler wrote: On 22.02.2023 17:33, Gyan Doshi wrote: From fftools\ffmpeg_mux_init.c 619: ost->enc_ctx->global_quality = FF_QP2LAMBDA * qscale Regards, Gyan But that's only if you set the old qscale CLI options. If you set the global_quality option directly, there is no factor applied. By dividing by FF_QP2LAMBDA you break setting this value via global_quality, and instead add support for setting it via the old and pretty much abandoned qscale interface. FWIW, that's what LAME does. libavcodec\libmp3lame.c 119: lame_set_VBR_quality(s->gfp, avctx->global_quality / (float)FF_QP2LAMBDA); global_quality semantics seem overloaded. Maybe we should just redirect user to priv vbr and error out in fdk init. Regards, Gyan It's pretty much a matter of "what did this code always do in the past". It then got to stick to it, cause otherwise we break downstream API and CLI consumers. The mp3lame code is probably old enough that qscale was still the default at the time. Nowadays it's global_quality. Both of those options mapping to the same field in avctx, one with a magic factor applies, is definitely and oddity. ___ 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".
Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA
I had noticed fdk is specifically looking for the qscale flag, and otherwise does not do anything with global_quality. I suppose the change is risky for anyone who is setting both global quality and qscale, but with the current code, it seems incorrect to have a conditional based on the scaled option and not account for the scaling. if (avctx->flags & AV_CODEC_FLAG_QSCALE || s->vbr) { int mode = s->vbr ? s->vbr : avctx->global_quality; On Wed, Feb 22, 2023 at 9:16 AM Timo Rothenpieler wrote: > On 22.02.2023 17:46, Gyan Doshi wrote: > > > > > > On 2023-02-22 10:12 pm, Timo Rothenpieler wrote: > >> On 22.02.2023 17:33, Gyan Doshi wrote: > >>> From > >>> > >>> fftools\ffmpeg_mux_init.c > >>> 619:ost->enc_ctx->global_quality = FF_QP2LAMBDA * qscale > >>> > >>> Regards, > >>> Gyan > >> > >> But that's only if you set the old qscale CLI options. > >> If you set the global_quality option directly, there is no factor > >> applied. > >> > >> By dividing by FF_QP2LAMBDA you break setting this value via > >> global_quality, and instead add support for setting it via the old and > >> pretty much abandoned qscale interface. > > > > FWIW, that's what LAME does. > > > > libavcodec\libmp3lame.c > > 119:lame_set_VBR_quality(s->gfp, avctx->global_quality / > > (float)FF_QP2LAMBDA); > > > > global_quality semantics seem overloaded. Maybe we should just redirect > > user to priv vbr and error out in fdk init. > > > > Regards, > > Gyan > > It's pretty much a matter of "what did this code always do in the past". > It then got to stick to it, cause otherwise we break downstream API and > CLI consumers. > > The mp3lame code is probably old enough that qscale was still the > default at the time. > Nowadays it's global_quality. > > Both of those options mapping to the same field in avctx, one with a > magic factor applies, is definitely and oddity. > ___ > 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". > ___ 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".