diff mbox series

[FFmpeg-devel,PATCHv2,2/3] libfdk-aacdec: Use the decoder's default level limiter settings

Message ID 20200205130652.75949-1-martin@martin.st
State Accepted
Headers show
Series None | expand

Commit Message

Martin Storsjö Feb. 5, 2020, 1:06 p.m. UTC 
It was disabled by default in 2dbd35b00c6433e587d5f44d5dbc8972ebbaa88e
as it added delay, but now we compensate for the delay properly
by offsetting timestamps.
---
 libavcodec/libfdk-aacdec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

James Almer Feb. 7, 2020, 5:57 p.m. UTC | #1 
On 2/5/2020 10:06 AM, Martin Storsjö wrote:
> It was disabled by default in 2dbd35b00c6433e587d5f44d5dbc8972ebbaa88e
> as it added delay, but now we compensate for the delay properly
> by offsetting timestamps.
> ---
>  libavcodec/libfdk-aacdec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/libfdk-aacdec.c b/libavcodec/libfdk-aacdec.c
> index d9b080cf3e..b43226f515 100644
> --- a/libavcodec/libfdk-aacdec.c
> +++ b/libavcodec/libfdk-aacdec.c
> @@ -81,7 +81,8 @@ static const AVOption fdk_aac_dec_options[] = {
>      { "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
> -    { "level_limit", "Signal level limiting", OFFSET(level_limit), AV_OPT_TYPE_INT, { .i64 = 0 }, -1, 1, AD },
> +    { "level_limit", "Signal level limiting",
> +                     OFFSET(level_limit),    AV_OPT_TYPE_BOOL,  { .i64 = -1 }, -1, 1, AD },
>  #endif
>  #if FDKDEC_VER_AT_LEAST(3, 0) // 3.0.0
>      { "drc_effect","Dynamic Range Control: effect type, where e.g. [0] is none and [6] is general",

Meant to reply to this patch, sorry. Quoting here:

> You need to check that level_limit != -1, same as all the drc_* options
> next to it in fdk_aac_decode_init(). Otherwise you'll call
> aacDecoder_SetParam() with that value that's only meant to be
> interpreted by this wrapper as "Let the decoder use its default".
Martin Storsjö Feb. 7, 2020, 6:04 p.m. UTC | #2 
On Fri, 7 Feb 2020, James Almer wrote:

> On 2/5/2020 10:06 AM, Martin Storsjö wrote:
>> It was disabled by default in 2dbd35b00c6433e587d5f44d5dbc8972ebbaa88e
>> as it added delay, but now we compensate for the delay properly
>> by offsetting timestamps.
>> ---
>>  libavcodec/libfdk-aacdec.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/libavcodec/libfdk-aacdec.c b/libavcodec/libfdk-aacdec.c
>> index d9b080cf3e..b43226f515 100644
>> --- a/libavcodec/libfdk-aacdec.c
>> +++ b/libavcodec/libfdk-aacdec.c
>> @@ -81,7 +81,8 @@ static const AVOption fdk_aac_dec_options[] = {
>>      { "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
>> -    { "level_limit", "Signal level limiting", OFFSET(level_limit), AV_OPT_TYPE_INT, { .i64 = 0 }, -1, 1, AD },
>> +    { "level_limit", "Signal level limiting",
>> +                     OFFSET(level_limit),    AV_OPT_TYPE_BOOL,  { .i64 = -1 }, -1, 1, AD },
>>  #endif
>>  #if FDKDEC_VER_AT_LEAST(3, 0) // 3.0.0
>>      { "drc_effect","Dynamic Range Control: effect type, where e.g. [0] is none and [6] is general",
>
> Meant to reply to this patch, sorry. Quoting here:
>
>> You need to check that level_limit != -1, same as all the drc_* options
>> next to it in fdk_aac_decode_init(). Otherwise you'll call
>> aacDecoder_SetParam() with that value that's only meant to be
>> interpreted by this wrapper as "Let the decoder use its default".

The code already has this treatment of -1, see line 296 in current master.

// Martin
James Almer Feb. 7, 2020, 6:05 p.m. UTC | #3 
On 2/7/2020 3:04 PM, Martin Storsjö wrote:
> On Fri, 7 Feb 2020, James Almer wrote:
> 
>> On 2/5/2020 10:06 AM, Martin Storsjö wrote:
>>> It was disabled by default in 2dbd35b00c6433e587d5f44d5dbc8972ebbaa88e
>>> as it added delay, but now we compensate for the delay properly
>>> by offsetting timestamps.
>>> ---
>>>  libavcodec/libfdk-aacdec.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/libfdk-aacdec.c b/libavcodec/libfdk-aacdec.c
>>> index d9b080cf3e..b43226f515 100644
>>> --- a/libavcodec/libfdk-aacdec.c
>>> +++ b/libavcodec/libfdk-aacdec.c
>>> @@ -81,7 +81,8 @@ static const AVOption fdk_aac_dec_options[] = {
>>>      { "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
>>> -    { "level_limit", "Signal level limiting", OFFSET(level_limit),
>>> AV_OPT_TYPE_INT, { .i64 = 0 }, -1, 1, AD },
>>> +    { "level_limit", "Signal level limiting",
>>> +                     OFFSET(level_limit),    AV_OPT_TYPE_BOOL,  {
>>> .i64 = -1 }, -1, 1, AD },
>>>  #endif
>>>  #if FDKDEC_VER_AT_LEAST(3, 0) // 3.0.0
>>>      { "drc_effect","Dynamic Range Control: effect type, where e.g.
>>> [0] is none and [6] is general",
>>
>> Meant to reply to this patch, sorry. Quoting here:
>>
>>> You need to check that level_limit != -1, same as all the drc_* options
>>> next to it in fdk_aac_decode_init(). Otherwise you'll call
>>> aacDecoder_SetParam() with that value that's only meant to be
>>> interpreted by this wrapper as "Let the decoder use its default".
> 
> The code already has this treatment of -1, see line 296 in current master.

That's for drc_level. level_limit is line 311.
Martin Storsjö Feb. 7, 2020, 6:08 p.m. UTC | #4 
On Fri, 7 Feb 2020, Martin Storsjö wrote:

> On Fri, 7 Feb 2020, James Almer wrote:
>
>> On 2/5/2020 10:06 AM, Martin Storsjö wrote:
>>> It was disabled by default in 
> 2dbd35b00c6433e587d5f44d5dbc8972ebbaa88e
>>> as it added delay, but now we compensate for the delay 
> properly
>>> by offsetting timestamps.
>>> ---
>>>  libavcodec/libfdk-aacdec.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/libavcodec/libfdk-aacdec.c 
> b/libavcodec/libfdk-aacdec.c
>>> index d9b080cf3e..b43226f515 100644
>>> --- a/libavcodec/libfdk-aacdec.c
>>> +++ b/libavcodec/libfdk-aacdec.c
>>> @@ -81,7 +81,8 @@ static const AVOption 
> fdk_aac_dec_options[] = {
>>>      { "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
>>> -    { "level_limit", "Signal level limiting", 
> OFFSET(level_limit), AV_OPT_TYPE_INT, { .i64 = 0 }, -1, 1, AD 
> },
>>> +    { "level_limit", "Signal level limiting",
>>> +                     OFFSET(level_limit), 
> AV_OPT_TYPE_BOOL,  { .i64 = -1 }, -1, 1, AD },
>>>  #endif
>>>  #if FDKDEC_VER_AT_LEAST(3, 0) // 3.0.0
>>>      { "drc_effect","Dynamic Range Control: effect type, 
> where e.g. [0] is none and [6] is general",
>>
>> Meant to reply to this patch, sorry. Quoting here:
>>
>>> You need to check that level_limit != -1, same as all the 
> drc_* options
>>> next to it in fdk_aac_decode_init(). Otherwise you'll call
>>> aacDecoder_SetParam() with that value that's only meant to 
> be
>>> interpreted by this wrapper as "Let the decoder use its 
> default".
>
> The code already has this treatment of -1, see line 296 in 
> current master.

Sorry, I misread, that was a different option.

But -1 is actually treated by the decoder lib as explicit auto mode, which 
probably also is the default setting if the setting isn't set.

// Martin
James Almer Feb. 7, 2020, 6:18 p.m. UTC | #5 
On 2/7/2020 3:08 PM, Martin Storsjö wrote:
> On Fri, 7 Feb 2020, Martin Storsjö wrote:
> 
>> On Fri, 7 Feb 2020, James Almer wrote:
>>
>>> On 2/5/2020 10:06 AM, Martin Storsjö wrote:
>>>> It was disabled by default in 
>> 2dbd35b00c6433e587d5f44d5dbc8972ebbaa88e
>>>> as it added delay, but now we compensate for the delay 
>> properly
>>>> by offsetting timestamps.
>>>> ---
>>>>  libavcodec/libfdk-aacdec.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/libfdk-aacdec.c 
>> b/libavcodec/libfdk-aacdec.c
>>>> index d9b080cf3e..b43226f515 100644
>>>> --- a/libavcodec/libfdk-aacdec.c
>>>> +++ b/libavcodec/libfdk-aacdec.c
>>>> @@ -81,7 +81,8 @@ static const AVOption 
>> fdk_aac_dec_options[] = {
>>>>      { "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
>>>> -    { "level_limit", "Signal level limiting", 
>> OFFSET(level_limit), AV_OPT_TYPE_INT, { .i64 = 0 }, -1, 1, AD },
>>>> +    { "level_limit", "Signal level limiting",
>>>> +                     OFFSET(level_limit), 
>> AV_OPT_TYPE_BOOL,  { .i64 = -1 }, -1, 1, AD },
>>>>  #endif
>>>>  #if FDKDEC_VER_AT_LEAST(3, 0) // 3.0.0
>>>>      { "drc_effect","Dynamic Range Control: effect type, 
>> where e.g. [0] is none and [6] is general",
>>>
>>> Meant to reply to this patch, sorry. Quoting here:
>>>
>>>> You need to check that level_limit != -1, same as all the 
>> drc_* options
>>>> next to it in fdk_aac_decode_init(). Otherwise you'll call
>>>> aacDecoder_SetParam() with that value that's only meant to 
>> be
>>>> interpreted by this wrapper as "Let the decoder use its 
>> default".
>>
>> The code already has this treatment of -1, see line 296 in current
>> master.
> 
> Sorry, I misread, that was a different option.
> 
> But -1 is actually treated by the decoder lib as explicit auto mode,
> which probably also is the default setting if the setting isn't set.

Ok, then LGTM.

> 
> // 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 d9b080cf3e..b43226f515 100644
--- a/libavcodec/libfdk-aacdec.c
+++ b/libavcodec/libfdk-aacdec.c
@@ -81,7 +81,8 @@  static const AVOption fdk_aac_dec_options[] = {
     { "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
-    { "level_limit", "Signal level limiting", OFFSET(level_limit), AV_OPT_TYPE_INT, { .i64 = 0 }, -1, 1, AD },
+    { "level_limit", "Signal level limiting",
+                     OFFSET(level_limit),    AV_OPT_TYPE_BOOL,  { .i64 = -1 }, -1, 1, AD },
 #endif
 #if FDKDEC_VER_AT_LEAST(3, 0) // 3.0.0
     { "drc_effect","Dynamic Range Control: effect type, where e.g. [0] is none and [6] is general",