diff mbox series

[FFmpeg-devel,3/3] libfdk-aacdec: Allow explicitly disabling the DRC reference level option

Message ID 20200205110708.64282-3-martin@martin.st
State New
Headers show
Series [FFmpeg-devel,1/3] libfdk-aacdec: Apply the decoder's output delay on timestamps
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Martin Storsjö Feb. 5, 2020, 11:07 a.m. UTC
Previously, it was always left in the automatic mode, if the option
was set to the only special (negative) value. Now there's two separate
special values for this option, -1 for automatic (metadata based)
and -2 for explicitly disabled.
---
 libavcodec/libfdk-aacdec.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

James Almer Feb. 5, 2020, 1:10 p.m. UTC | #1
On 2/5/2020 8:07 AM, Martin Storsjö wrote:
> Previously, it was always left in the automatic mode, if the option
> was set to the only special (negative) value. Now there's two separate
> special values for this option, -1 for automatic (metadata based)
> and -2 for explicitly disabled.
> ---
>  libavcodec/libfdk-aacdec.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/libfdk-aacdec.c b/libavcodec/libfdk-aacdec.c
> index 32a97958c4..cc50fdce2f 100644
> --- a/libavcodec/libfdk-aacdec.c
> +++ b/libavcodec/libfdk-aacdec.c
> @@ -76,8 +76,8 @@ static const AVOption fdk_aac_dec_options[] = {
>                       OFFSET(drc_boost),      AV_OPT_TYPE_INT,   { .i64 = -1 }, -1, 127, AD, NULL    },
>      { "drc_cut",   "Dynamic Range Control: attenuation factor, where [0] is none and [127] is max compression",
>                       OFFSET(drc_cut),        AV_OPT_TYPE_INT,   { .i64 = -1 }, -1, 127, AD, NULL    },
> -    { "drc_level", "Dynamic Range Control: reference level, quantized to 0.25dB steps where [0] is 0dB and [127] is -31.75dB",
> -                     OFFSET(drc_level),      AV_OPT_TYPE_INT,   { .i64 = -1},  -1, 127, AD, NULL    },
> +    { "drc_level", "Dynamic Range Control: reference level, quantized to 0.25dB steps where [0] is 0dB and [127] is -31.75dB, -1 for auto, and -2 for disabled",

Not against adding this extra explanation here since this one is not a
simple boolean option, but i see there's no entry for fdk-aacdec in
decoders.texy, so could you add one with all the options described in it?

Bump micro, and LGTM.

> +                     OFFSET(drc_level),      AV_OPT_TYPE_INT,   { .i64 = -1},  -2, 127, AD, NULL    },
>      { "drc_heavy", "Dynamic Range Control: heavy compression, where [1] is on (RF mode) and [0] is off",
>                       OFFSET(drc_heavy),      AV_OPT_TYPE_INT,   { .i64 = -1},  -1, 1,   AD, NULL    },
>  #if FDKDEC_VER_AT_LEAST(2, 5) // 2.5.10
> @@ -298,6 +298,12 @@ static av_cold int fdk_aac_decode_init(AVCodecContext *avctx)
>      }
>  
>      if (s->drc_level != -1) {
> +        // This option defaults to -1, i.e. not calling
> +        // aacDecoder_SetParam(AAC_DRC_REFERENCE_LEVEL) at all, which defaults
> +        // to the level from DRC metadata, if available. The user can set
> +        // -drc_level -2, which calls aacDecoder_SetParam(
> +        // AAC_DRC_REFERENCE_LEVEL) with a negative value, which then
> +        // explicitly disables the feature.
>          if (aacDecoder_SetParam(s->handle, AAC_DRC_REFERENCE_LEVEL, s->drc_level) != AAC_DEC_OK) {
>              av_log(avctx, AV_LOG_ERROR, "Unable to set DRC reference level in the decoder\n");
>              return AVERROR_UNKNOWN;
>
Martin Storsjö Feb. 5, 2020, 1:12 p.m. UTC | #2
On Wed, 5 Feb 2020, James Almer wrote:

> On 2/5/2020 8:07 AM, Martin Storsjö wrote:
>> Previously, it was always left in the automatic mode, if the option
>> was set to the only special (negative) value. Now there's two separate
>> special values for this option, -1 for automatic (metadata based)
>> and -2 for explicitly disabled.
>> ---
>>  libavcodec/libfdk-aacdec.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavcodec/libfdk-aacdec.c b/libavcodec/libfdk-aacdec.c
>> index 32a97958c4..cc50fdce2f 100644
>> --- a/libavcodec/libfdk-aacdec.c
>> +++ b/libavcodec/libfdk-aacdec.c
>> @@ -76,8 +76,8 @@ static const AVOption fdk_aac_dec_options[] = {
>>                       OFFSET(drc_boost),      AV_OPT_TYPE_INT,   { .i64 = -1 }, -1, 127, AD, NULL    },
>>      { "drc_cut",   "Dynamic Range Control: attenuation factor, where [0] is none and [127] is max compression",
>>                       OFFSET(drc_cut),        AV_OPT_TYPE_INT,   { .i64 = -1 }, -1, 127, AD, NULL    },
>> -    { "drc_level", "Dynamic Range Control: reference level, quantized to 0.25dB steps where [0] is 0dB and [127] is -31.75dB",
>> -                     OFFSET(drc_level),      AV_OPT_TYPE_INT,   { .i64 = -1},  -1, 127, AD, NULL    },
>> +    { "drc_level", "Dynamic Range Control: reference level, quantized to 0.25dB steps where [0] is 0dB and [127] is -31.75dB, -1 for auto, and -2 for disabled",
>
> Not against adding this extra explanation here since this one is not a
> simple boolean option, but i see there's no entry for fdk-aacdec in
> decoders.texy, so could you add one with all the options described in it?

I'd say that's a separate issue from these patches - I could look into 
doing that, but that would be a later step.

// Martin
James Almer Feb. 5, 2020, 1:13 p.m. UTC | #3
On 2/5/2020 10:12 AM, Martin Storsjö wrote:
> On Wed, 5 Feb 2020, James Almer wrote:
> 
>> On 2/5/2020 8:07 AM, Martin Storsjö wrote:
>>> Previously, it was always left in the automatic mode, if the option
>>> was set to the only special (negative) value. Now there's two separate
>>> special values for this option, -1 for automatic (metadata based)
>>> and -2 for explicitly disabled.
>>> ---
>>>  libavcodec/libfdk-aacdec.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavcodec/libfdk-aacdec.c b/libavcodec/libfdk-aacdec.c
>>> index 32a97958c4..cc50fdce2f 100644
>>> --- a/libavcodec/libfdk-aacdec.c
>>> +++ b/libavcodec/libfdk-aacdec.c
>>> @@ -76,8 +76,8 @@ static const AVOption fdk_aac_dec_options[] = {
>>>                       OFFSET(drc_boost),      AV_OPT_TYPE_INT,   {
>>> .i64 = -1 }, -1, 127, AD, NULL    },
>>>      { "drc_cut",   "Dynamic Range Control: attenuation factor, where
>>> [0] is none and [127] is max compression",
>>>                       OFFSET(drc_cut),        AV_OPT_TYPE_INT,   {
>>> .i64 = -1 }, -1, 127, AD, NULL    },
>>> -    { "drc_level", "Dynamic Range Control: reference level,
>>> quantized to 0.25dB steps where [0] is 0dB and [127] is -31.75dB",
>>> -                     OFFSET(drc_level),      AV_OPT_TYPE_INT,   {
>>> .i64 = -1},  -1, 127, AD, NULL    },
>>> +    { "drc_level", "Dynamic Range Control: reference level,
>>> quantized to 0.25dB steps where [0] is 0dB and [127] is -31.75dB, -1
>>> for auto, and -2 for disabled",
>>
>> Not against adding this extra explanation here since this one is not a
>> simple boolean option, but i see there's no entry for fdk-aacdec in
>> decoders.texy, so could you add one with all the options described in it?
> 
> I'd say that's a separate issue from these patches - I could look into
> doing that, but that would be a later step.

Yes, it's a request, not a blocking comment.

> 
> // 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".
diff mbox series

Patch

diff --git a/libavcodec/libfdk-aacdec.c b/libavcodec/libfdk-aacdec.c
index 32a97958c4..cc50fdce2f 100644
--- a/libavcodec/libfdk-aacdec.c
+++ b/libavcodec/libfdk-aacdec.c
@@ -76,8 +76,8 @@  static const AVOption fdk_aac_dec_options[] = {
                      OFFSET(drc_boost),      AV_OPT_TYPE_INT,   { .i64 = -1 }, -1, 127, AD, NULL    },
     { "drc_cut",   "Dynamic Range Control: attenuation factor, where [0] is none and [127] is max compression",
                      OFFSET(drc_cut),        AV_OPT_TYPE_INT,   { .i64 = -1 }, -1, 127, AD, NULL    },
-    { "drc_level", "Dynamic Range Control: reference level, quantized to 0.25dB steps where [0] is 0dB and [127] is -31.75dB",
-                     OFFSET(drc_level),      AV_OPT_TYPE_INT,   { .i64 = -1},  -1, 127, AD, NULL    },
+    { "drc_level", "Dynamic Range Control: reference level, quantized to 0.25dB steps where [0] is 0dB and [127] is -31.75dB, -1 for auto, and -2 for disabled",
+                     OFFSET(drc_level),      AV_OPT_TYPE_INT,   { .i64 = -1},  -2, 127, AD, NULL    },
     { "drc_heavy", "Dynamic Range Control: heavy compression, where [1] is on (RF mode) and [0] is off",
                      OFFSET(drc_heavy),      AV_OPT_TYPE_INT,   { .i64 = -1},  -1, 1,   AD, NULL    },
 #if FDKDEC_VER_AT_LEAST(2, 5) // 2.5.10
@@ -298,6 +298,12 @@  static av_cold int fdk_aac_decode_init(AVCodecContext *avctx)
     }
 
     if (s->drc_level != -1) {
+        // This option defaults to -1, i.e. not calling
+        // aacDecoder_SetParam(AAC_DRC_REFERENCE_LEVEL) at all, which defaults
+        // to the level from DRC metadata, if available. The user can set
+        // -drc_level -2, which calls aacDecoder_SetParam(
+        // AAC_DRC_REFERENCE_LEVEL) with a negative value, which then
+        // explicitly disables the feature.
         if (aacDecoder_SetParam(s->handle, AAC_DRC_REFERENCE_LEVEL, s->drc_level) != AAC_DEC_OK) {
             av_log(avctx, AV_LOG_ERROR, "Unable to set DRC reference level in the decoder\n");
             return AVERROR_UNKNOWN;