diff mbox series

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

Message ID 20230222021240.2370954-1-JonHGee@gmail.com
State New
Headers show
Series [FFmpeg-devel] libavcodec/libfdk-aacenc: Scale VBR mode with FF_QP2LAMBDA | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

JonHGee Feb. 22, 2023, 2:12 a.m. UTC 
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(-)

Comments

Timo Rothenpieler Feb. 22, 2023, 4:09 p.m. UTC | #1 
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?
Gyan Doshi Feb. 22, 2023, 4:33 p.m. UTC | #2 
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
Timo Rothenpieler Feb. 22, 2023, 4:42 p.m. UTC | #3 
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.
Gyan Doshi Feb. 22, 2023, 4:46 p.m. UTC | #4 
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
Timo Rothenpieler Feb. 22, 2023, 5:15 p.m. UTC | #5 
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.
JonHGee Feb. 22, 2023, 6:49 p.m. UTC | #6 
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".
>
diff mbox series

Patch

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);