[FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA

[FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA

[JonHGee]

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);
-- 
2.39.2.637.g21b0678d19-goog

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

Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA

[Timo Rothenpieler]

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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA

[Gyan Doshi]

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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA

[Timo Rothenpieler]

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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA

[Gyan Doshi]

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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA

[Timo Rothenpieler]

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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA

[Jonathan Gee]

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 <timo@rothenpieler.org>
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-request@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-request@ffmpeg.org with subject "unsubscribe".