[FFmpeg-devel] [PATCH] avcodec/aac_parser: fix aac profile probing

[FFmpeg-devel] [PATCH] avcodec/aac_parser: fix aac profile probing

[Nicolas Gaullier]

Reverts 64bb91fd3b5a00a8849531c7e8dd207f2a626096 except the
key_frame setting for aac.
See also 696ea1c2236842572df88d573e24a39be3f19c98.
Don't force the profile as it may override the correct value
set by the decoder (HE/HEv2).

Fixes #11600

Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
---
 libavcodec/aac_ac3_parser.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
index e10ce13a3b..1dcc2e735f 100644
--- a/libavcodec/aac_ac3_parser.c
+++ b/libavcodec/aac_ac3_parser.c
@@ -147,15 +147,13 @@ get_next:
         } else {
 #if CONFIG_AAC_PARSER
             AACADTSHeaderInfo hdr;
-            GetBitContext gb;
 
-            init_get_bits8(&gb, buf, buf_size);
             if (buf_size < AV_AAC_ADTS_HEADER_SIZE ||
-                ff_adts_header_parse(&gb, &hdr) < 0)
+                ff_adts_header_parse_buf(buf, &hdr) < 0)
                 return i;
 
-            avctx->profile = hdr.object_type - 1;
-            s1->key_frame = (avctx->profile == AV_PROFILE_AAC_USAC) ? get_bits1(&gb) : 1;
+            /* ADTS does not support USAC */
+            s1->key_frame = 1;
             bit_rate = hdr.bit_rate;
 #endif
         }
-- 
2.47.2

_______________________________________________
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] [PATCH] avcodec/aac_parser: fix aac profile probing

[Nicolas Gaullier]

On 6/17/25 13:41, Nicolas Gaullier wrote:
> Reverts 64bb91fd3b5a00a8849531c7e8dd207f2a626096 except the
> key_frame setting for aac.
> See also 696ea1c2236842572df88d573e24a39be3f19c98.
> Don't force the profile as it may override the correct value
> set by the decoder (HE/HEv2).
>
> Fixes #11600
>
> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
> ---
>   libavcodec/aac_ac3_parser.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
> index e10ce13a3b..1dcc2e735f 100644
> --- a/libavcodec/aac_ac3_parser.c
> +++ b/libavcodec/aac_ac3_parser.c
> @@ -147,15 +147,13 @@ get_next:
>           } else {
>   #if CONFIG_AAC_PARSER
>               AACADTSHeaderInfo hdr;
> -            GetBitContext gb;
>   
> -            init_get_bits8(&gb, buf, buf_size);
>               if (buf_size < AV_AAC_ADTS_HEADER_SIZE ||
> -                ff_adts_header_parse(&gb, &hdr) < 0)
> +                ff_adts_header_parse_buf(buf, &hdr) < 0)
>                   return i;
>   
> -            avctx->profile = hdr.object_type - 1;
> -            s1->key_frame = (avctx->profile == AV_PROFILE_AAC_USAC) ? get_bits1(&gb) : 1;
> +            /* ADTS does not support USAC */
> +            s1->key_frame = 1;
>               bit_rate = hdr.bit_rate;
>   #endif
>           }

Ping ?

https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14810

_______________________________________________
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] avcodec/aac_parser: fix aac profile probing

[Baptiste Coudurier]

Hi Nicolas

> On Jun 26, 2025, at 4:06 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
> 
> On 6/17/25 13:41, Nicolas Gaullier wrote:
>> Reverts 64bb91fd3b5a00a8849531c7e8dd207f2a626096 except the
>> key_frame setting for aac.
>> See also 696ea1c2236842572df88d573e24a39be3f19c98.
>> Don't force the profile as it may override the correct value
>> set by the decoder (HE/HEv2).
>> 
>> Fixes #11600
>> 
>> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
>> ---
>>  libavcodec/aac_ac3_parser.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>> 
>> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
>> index e10ce13a3b..1dcc2e735f 100644
>> --- a/libavcodec/aac_ac3_parser.c
>> +++ b/libavcodec/aac_ac3_parser.c
>> @@ -147,15 +147,13 @@ get_next:
>>          } else {
>>  #if CONFIG_AAC_PARSER
>>              AACADTSHeaderInfo hdr;
>> -            GetBitContext gb;
>>  -            init_get_bits8(&gb, buf, buf_size);
>>              if (buf_size < AV_AAC_ADTS_HEADER_SIZE ||
>> -                ff_adts_header_parse(&gb, &hdr) < 0)
>> +                ff_adts_header_parse_buf(buf, &hdr) < 0)
>>                  return i;
>>  -            avctx->profile = hdr.object_type - 1;
>> -            s1->key_frame = (avctx->profile == AV_PROFILE_AAC_USAC) ? get_bits1(&gb) : 1;
>> +            /* ADTS does not support USAC */
>> +            s1->key_frame = 1;
>>              bit_rate = hdr.bit_rate;
>>  #endif
>>          }
> 
> Ping ?
> 
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14810
> 

How hard is it to detect HEAACv2 in the parser ? Maybe only set it if profile is unknown?
It’s usually nice when parser can set profile.

— 
Baptiste
_______________________________________________
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] avcodec/aac_parser: fix aac profile probing

[Nicolas Gaullier]

On 6/26/25 19:30, Baptiste Coudurier wrote:
> Hi Nicolas
>> On Jun 26, 2025, at 4:06 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
>>
>> On 6/17/25 13:41, Nicolas Gaullier wrote:
>>> Reverts 64bb91fd3b5a00a8849531c7e8dd207f2a626096 except the
>>> key_frame setting for aac.
>>> See also 696ea1c2236842572df88d573e24a39be3f19c98.
>>> Don't force the profile as it may override the correct value
>>> set by the decoder (HE/HEv2).
>>>
>>> Fixes #11600
>>>
>>> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
>>> ---
>>>   libavcodec/aac_ac3_parser.c | 8 +++-----
>>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
>>> index e10ce13a3b..1dcc2e735f 100644
>>> --- a/libavcodec/aac_ac3_parser.c
>>> +++ b/libavcodec/aac_ac3_parser.c
>>> @@ -147,15 +147,13 @@ get_next:
>>>           } else {
>>>   #if CONFIG_AAC_PARSER
>>>               AACADTSHeaderInfo hdr;
>>> -            GetBitContext gb;
>>>   -            init_get_bits8(&gb, buf, buf_size);
>>>               if (buf_size < AV_AAC_ADTS_HEADER_SIZE ||
>>> -                ff_adts_header_parse(&gb, &hdr) < 0)
>>> +                ff_adts_header_parse_buf(buf, &hdr) < 0)
>>>                   return i;
>>>   -            avctx->profile = hdr.object_type - 1;
>>> -            s1->key_frame = (avctx->profile == AV_PROFILE_AAC_USAC) ? get_bits1(&gb) : 1;
>>> +            /* ADTS does not support USAC */
>>> +            s1->key_frame = 1;
>>>               bit_rate = hdr.bit_rate;
>>>   #endif
>>>           }
>> Ping ?
>>
>> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14810
>>
> How hard is it to detect HEAACv2 in the parser ? Maybe only set it if profile is unknown?
> It’s usually nice when parser can set profile.
>
> —
> Baptiste

Yes, you're right, I had the same feeling when reverting this...

It does not seem so straightforward, because aac seems very modular 
while not using signaling, but it must be doable someway.

I will dig into this and see if I can come up with something satisfactory...

I think it would be an additional patch, and this one should remain the 
basic revert which, mostly, simply removes dead code.

Maybe it would be desirable to follow this patch immediately with a new 
patch that could restore the "profile setting" thing correctly, but that 
would add some undetermined delay, so I would say it is best to apply 
this patch on its own.

Nicolas

_______________________________________________
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] avcodec/aac_parser: fix aac profile probing

[Baptiste Coudurier]

> On Jun 27, 2025, at 5:01 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
> 
> On 6/26/25 19:30, Baptiste Coudurier wrote:
>> Hi Nicolas
>>> On Jun 26, 2025, at 4:06 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
>>> 
>>> On 6/17/25 13:41, Nicolas Gaullier wrote:
>>>> Reverts 64bb91fd3b5a00a8849531c7e8dd207f2a626096 except the
>>>> key_frame setting for aac.
>>>> See also 696ea1c2236842572df88d573e24a39be3f19c98.
>>>> Don't force the profile as it may override the correct value
>>>> set by the decoder (HE/HEv2).
>>>> 
>>>> Fixes #11600
>>>> 
>>>> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
>>>> ---
>>>>  libavcodec/aac_ac3_parser.c | 8 +++-----
>>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
>>>> index e10ce13a3b..1dcc2e735f 100644
>>>> --- a/libavcodec/aac_ac3_parser.c
>>>> +++ b/libavcodec/aac_ac3_parser.c
>>>> @@ -147,15 +147,13 @@ get_next:
>>>>          } else {
>>>>  #if CONFIG_AAC_PARSER
>>>>              AACADTSHeaderInfo hdr;
>>>> -            GetBitContext gb;
>>>>  -            init_get_bits8(&gb, buf, buf_size);
>>>>              if (buf_size < AV_AAC_ADTS_HEADER_SIZE ||
>>>> -                ff_adts_header_parse(&gb, &hdr) < 0)
>>>> +                ff_adts_header_parse_buf(buf, &hdr) < 0)
>>>>                  return i;
>>>>  -            avctx->profile = hdr.object_type - 1;
>>>> -            s1->key_frame = (avctx->profile == AV_PROFILE_AAC_USAC) ? get_bits1(&gb) : 1;
>>>> +            /* ADTS does not support USAC */
>>>> +            s1->key_frame = 1;
>>>>              bit_rate = hdr.bit_rate;
>>>>  #endif
>>>>          }
>>> Ping ?
>>> 
>>> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14810
>>> 
>> How hard is it to detect HEAACv2 in the parser ? Maybe only set it if profile is unknown?
>> It’s usually nice when parser can set profile.
>> 
>> —
>> Baptiste
> 
> Yes, you're right, I had the same feeling when reverting this...
> 
> It does not seem so straightforward, because aac seems very modular while not using signaling, but it must be doable someway.
> 
> I will dig into this and see if I can come up with something satisfactory...
> 
> I think it would be an additional patch, and this one should remain the basic revert which, mostly, simply removes dead code.
> 
> Maybe it would be desirable to follow this patch immediately with a new patch that could restore the "profile setting" thing correctly, but that would add some undetermined delay, so I would say it is best to apply this patch on its own.

Yeah, does adding “if avctx->profile == UNKNOWN” fix the ticket?

— 
Baptiste Coudurier


_______________________________________________
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] [EXTERNE] Re: [PATCH] avcodec/aac_parser: fix aac profile probing

[Nicolas Gaullier]

On 6/28/25 03:54, Baptiste Coudurier wrote:
>> On Jun 27, 2025, at 5:01 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
>>
>> On 6/26/25 19:30, Baptiste Coudurier wrote:
>>> Hi Nicolas
>>>> On Jun 26, 2025, at 4:06 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
>>>>
>>>> On 6/17/25 13:41, Nicolas Gaullier wrote:
>>>>> Reverts 64bb91fd3b5a00a8849531c7e8dd207f2a626096 except the
>>>>> key_frame setting for aac.
>>>>> See also 696ea1c2236842572df88d573e24a39be3f19c98.
>>>>> Don't force the profile as it may override the correct value
>>>>> set by the decoder (HE/HEv2).
>>>>>
>>>>> Fixes #11600
>>>>>
>>>>> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
>>>>> ---
>>>>>   libavcodec/aac_ac3_parser.c | 8 +++-----
>>>>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
>>>>> index e10ce13a3b..1dcc2e735f 100644
>>>>> --- a/libavcodec/aac_ac3_parser.c
>>>>> +++ b/libavcodec/aac_ac3_parser.c
>>>>> @@ -147,15 +147,13 @@ get_next:
>>>>>           } else {
>>>>>   #if CONFIG_AAC_PARSER
>>>>>               AACADTSHeaderInfo hdr;
>>>>> -            GetBitContext gb;
>>>>>   -            init_get_bits8(&gb, buf, buf_size);
>>>>>               if (buf_size < AV_AAC_ADTS_HEADER_SIZE ||
>>>>> -                ff_adts_header_parse(&gb, &hdr) < 0)
>>>>> +                ff_adts_header_parse_buf(buf, &hdr) < 0)
>>>>>                   return i;
>>>>>   -            avctx->profile = hdr.object_type - 1;
>>>>> -            s1->key_frame = (avctx->profile == AV_PROFILE_AAC_USAC) ? get_bits1(&gb) : 1;
>>>>> +            /* ADTS does not support USAC */
>>>>> +            s1->key_frame = 1;
>>>>>               bit_rate = hdr.bit_rate;
>>>>>   #endif
>>>>>           }
>>>> Ping ?
>>>>
>>>> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14810
>>>>
>>> How hard is it to detect HEAACv2 in the parser ? Maybe only set it if profile is unknown?
>>> It’s usually nice when parser can set profile.
>>>
>>> —
>>> Baptiste
>> Yes, you're right, I had the same feeling when reverting this...
>>
>> It does not seem so straightforward, because aac seems very modular while not using signaling, but it must be doable someway.
>>
>> I will dig into this and see if I can come up with something satisfactory...
>>
>> I think it would be an additional patch, and this one should remain the basic revert which, mostly, simply removes dead code.
>>
>> Maybe it would be desirable to follow this patch immediately with a new patch that could restore the "profile setting" thing correctly, but that would add some undetermined delay, so I would say it is best to apply this patch on its own.
> Yeah, does adding “if avctx->profile == UNKNOWN” fix the ticket?

Not exactly, I mean it would indeed fix the ticket but that would not 
make the parser succeed in setting the correct profile - even if using 
an FFMAX to be able to "upgrade" the profile gradually.

I just checked how it works currently with the decoder, just by 
debugging a simple HEAACv2 raw sample of my own. There are three steps 
in three different code sections, here it is:

1: aacdec.c#2412 : ac->avctx->profile = ac->oc[1].m4ac.object_type - 1; 
=> set AAC LC (similar as current parser code)

2: aacdec.c#1946 : ac->avctx->profile = AV_PROFILE_AAC_HE;

3: aacsbr_templace.c#978: ac->avctx->profile = AV_PROFILE_AAC_HE_V2;

[NB: in aacdec.c#1941, the corresponding line "ac->avctx->profile = 
AV_PROFILE_AAC_HE_V2;" is not reach.]

Well, it seems far from easy. I think it would require a very good 
knowledge of the code to have the parser correctly set the profile. And 
at the end, on my side, I don't think I will have time for this in the 
next weeks.


So, I think we really have to simply remove this line which sets the 
profile in the parser: there is nothing good with it.

And for the rest of the patch, see "aot = get_bits(gbc, 2)" in 
ff_adts_header_parse(), so this is only about removing dead code (unless 
a future patch would resurrect it, but it is far from certain).

Nicolas

_______________________________________________
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] [EXTERNE] Re: [PATCH] avcodec/aac_parser: fix aac profile probing

[Baptiste Coudurier]

> On Jun 30, 2025, at 11:08 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
> 
> On 6/28/25 03:54, Baptiste Coudurier wrote:
>>> On Jun 27, 2025, at 5:01 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
>>> 
>>> On 6/26/25 19:30, Baptiste Coudurier wrote:
>>>> Hi Nicolas
>>>>> On Jun 26, 2025, at 4:06 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
>>>>> 
>>>>> On 6/17/25 13:41, Nicolas Gaullier wrote:
>>>>>> Reverts 64bb91fd3b5a00a8849531c7e8dd207f2a626096 except the
>>>>>> key_frame setting for aac.
>>>>>> See also 696ea1c2236842572df88d573e24a39be3f19c98.
>>>>>> Don't force the profile as it may override the correct value
>>>>>> set by the decoder (HE/HEv2).
>>>>>> 
>>>>>> Fixes #11600
>>>>>> 
>>>>>> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
>>>>>> ---
>>>>>>  libavcodec/aac_ac3_parser.c | 8 +++-----
>>>>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>> 
>>>>>> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
>>>>>> index e10ce13a3b..1dcc2e735f 100644
>>>>>> --- a/libavcodec/aac_ac3_parser.c
>>>>>> +++ b/libavcodec/aac_ac3_parser.c
>>>>>> @@ -147,15 +147,13 @@ get_next:
>>>>>>          } else {
>>>>>>  #if CONFIG_AAC_PARSER
>>>>>>              AACADTSHeaderInfo hdr;
>>>>>> -            GetBitContext gb;
>>>>>>  -            init_get_bits8(&gb, buf, buf_size);
>>>>>>              if (buf_size < AV_AAC_ADTS_HEADER_SIZE ||
>>>>>> -                ff_adts_header_parse(&gb, &hdr) < 0)
>>>>>> +                ff_adts_header_parse_buf(buf, &hdr) < 0)
>>>>>>                  return i;
>>>>>>  -            avctx->profile = hdr.object_type - 1;
>>>>>> -            s1->key_frame = (avctx->profile == AV_PROFILE_AAC_USAC) ? get_bits1(&gb) : 1;
>>>>>> +            /* ADTS does not support USAC */
>>>>>> +            s1->key_frame = 1;
>>>>>>              bit_rate = hdr.bit_rate;
>>>>>>  #endif
>>>>>>          }
>>>>> Ping ?
>>>>> 
>>>>> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14810
>>>>> 
>>>> How hard is it to detect HEAACv2 in the parser ? Maybe only set it if profile is unknown?
>>>> It’s usually nice when parser can set profile.
>>>> 
>>>> —
>>>> Baptiste
>>> Yes, you're right, I had the same feeling when reverting this...
>>> 
>>> It does not seem so straightforward, because aac seems very modular while not using signaling, but it must be doable someway.
>>> 
>>> I will dig into this and see if I can come up with something satisfactory...
>>> 
>>> I think it would be an additional patch, and this one should remain the basic revert which, mostly, simply removes dead code.
>>> 
>>> Maybe it would be desirable to follow this patch immediately with a new patch that could restore the "profile setting" thing correctly, but that would add some undetermined delay, so I would say it is best to apply this patch on its own.
>> Yeah, does adding “if avctx->profile == UNKNOWN” fix the ticket?
> 
> Not exactly, I mean it would indeed fix the ticket but that would not make the parser succeed in setting the correct profile - even if using an FFMAX to be able to "upgrade" the profile gradually.
> 
> I just checked how it works currently with the decoder, just by debugging a simple HEAACv2 raw sample of my own. There are three steps in three different code sections, here it is:
> 
> 1: aacdec.c#2412 : ac->avctx->profile = ac->oc[1].m4ac.object_type - 1; => set AAC LC (similar as current parser code)
> 
> 2: aacdec.c#1946 : ac->avctx->profile = AV_PROFILE_AAC_HE;
> 
> 3: aacsbr_templace.c#978: ac->avctx->profile = AV_PROFILE_AAC_HE_V2;
> 
> [NB: in aacdec.c#1941, the corresponding line "ac->avctx->profile = AV_PROFILE_AAC_HE_V2;" is not reach.]
> 
> Well, it seems far from easy. I think it would require a very good knowledge of the code to have the parser correctly set the profile. And at the end, on my side, I don't think I will have time for this in the next weeks.
> 
> 
> So, I think we really have to simply remove this line which sets the profile in the parser: there is nothing good with it.
> 

It was added so IMHO we can assume it was wanted or needed. I think it’s safer to check for unknown, it fixes the ticket and does not make it worse than it currently is.

— 
Baptiste
_______________________________________________
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] avcodec/aac_parser: fix aac profile probing

[Nicolas Gaullier]

On 6/30/25 21:03, Baptiste Coudurier wrote:
>> On Jun 30, 2025, at 11:08 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
>>
>> On 6/28/25 03:54, Baptiste Coudurier wrote:
>>>> On Jun 27, 2025, at 5:01 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
>>>>
>>>> On 6/26/25 19:30, Baptiste Coudurier wrote:
>>>>> Hi Nicolas
>>>>>> On Jun 26, 2025, at 4:06 AM, Nicolas Gaullier <nicolas.gaullier@cji.paris> wrote:
>>>>>>
>>>>>> On 6/17/25 13:41, Nicolas Gaullier wrote:
>>>>>>> Reverts 64bb91fd3b5a00a8849531c7e8dd207f2a626096 except the
>>>>>>> key_frame setting for aac.
>>>>>>> See also 696ea1c2236842572df88d573e24a39be3f19c98.
>>>>>>> Don't force the profile as it may override the correct value
>>>>>>> set by the decoder (HE/HEv2).
>>>>>>>
>>>>>>> Fixes #11600
>>>>>>>
>>>>>>> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris>
>>>>>>> ---
>>>>>>>   libavcodec/aac_ac3_parser.c | 8 +++-----
>>>>>>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
>>>>>>> index e10ce13a3b..1dcc2e735f 100644
>>>>>>> --- a/libavcodec/aac_ac3_parser.c
>>>>>>> +++ b/libavcodec/aac_ac3_parser.c
>>>>>>> @@ -147,15 +147,13 @@ get_next:
>>>>>>>           } else {
>>>>>>>   #if CONFIG_AAC_PARSER
>>>>>>>               AACADTSHeaderInfo hdr;
>>>>>>> -            GetBitContext gb;
>>>>>>>   -            init_get_bits8(&gb, buf, buf_size);
>>>>>>>               if (buf_size < AV_AAC_ADTS_HEADER_SIZE ||
>>>>>>> -                ff_adts_header_parse(&gb, &hdr) < 0)
>>>>>>> +                ff_adts_header_parse_buf(buf, &hdr) < 0)
>>>>>>>                   return i;
>>>>>>>   -            avctx->profile = hdr.object_type - 1;
>>>>>>> -            s1->key_frame = (avctx->profile == AV_PROFILE_AAC_USAC) ? get_bits1(&gb) : 1;
>>>>>>> +            /* ADTS does not support USAC */
>>>>>>> +            s1->key_frame = 1;
>>>>>>>               bit_rate = hdr.bit_rate;
>>>>>>>   #endif
>>>>>>>           }
>>>>>> Ping ?
>>>>>>
>>>>>> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14810
>>>>>>
>>>>> How hard is it to detect HEAACv2 in the parser ? Maybe only set it if profile is unknown?
>>>>> It’s usually nice when parser can set profile.
>>>>>
>>>>> —
>>>>> Baptiste
>>>> Yes, you're right, I had the same feeling when reverting this...
>>>>
>>>> It does not seem so straightforward, because aac seems very modular while not using signaling, but it must be doable someway.
>>>>
>>>> I will dig into this and see if I can come up with something satisfactory...
>>>>
>>>> I think it would be an additional patch, and this one should remain the basic revert which, mostly, simply removes dead code.
>>>>
>>>> Maybe it would be desirable to follow this patch immediately with a new patch that could restore the "profile setting" thing correctly, but that would add some undetermined delay, so I would say it is best to apply this patch on its own.
>>> Yeah, does adding “if avctx->profile == UNKNOWN” fix the ticket?
>> Not exactly, I mean it would indeed fix the ticket but that would not make the parser succeed in setting the correct profile - even if using an FFMAX to be able to "upgrade" the profile gradually.
>>
>> I just checked how it works currently with the decoder, just by debugging a simple HEAACv2 raw sample of my own. There are three steps in three different code sections, here it is:
>>
>> 1: aacdec.c#2412 : ac->avctx->profile = ac->oc[1].m4ac.object_type - 1; => set AAC LC (similar as current parser code)
>>
>> 2: aacdec.c#1946 : ac->avctx->profile = AV_PROFILE_AAC_HE;
>>
>> 3: aacsbr_templace.c#978: ac->avctx->profile = AV_PROFILE_AAC_HE_V2;
>>
>> [NB: in aacdec.c#1941, the corresponding line "ac->avctx->profile = AV_PROFILE_AAC_HE_V2;" is not reach.]
>>
>> Well, it seems far from easy. I think it would require a very good knowledge of the code to have the parser correctly set the profile. And at the end, on my side, I don't think I will have time for this in the next weeks.
>>
>>
>> So, I think we really have to simply remove this line which sets the profile in the parser: there is nothing good with it.
>>
> It was added so IMHO we can assume it was wanted or needed. I think it’s safer to check for unknown, it fixes the ticket and does not make it worse than it currently is.

I think it wasn't the original intention. If you take a look at the 
serie that was applied (git log 
f265f9c9d04863180503707bfad285f48e6bf080), it seems that the target was 
to remove the "keyframe-only" from aac (to support usac), this is what 
64bb91fd3b5a00a8849531c7e8dd207f2a626096 "prepares" as stated in its 
msg. Moreover, a trace of a remaining local variable "profile" was 
removed by 7a5d6690fc54ef354b30cb2236b7127928356f01, so it suggests the 
opportunity to set avctx came "later", it was secondary.

Anyway, I can just check for unknown if you prefer. In this case, we can 
either keep a single "partial revert" patch, or split it into two 
patches: the first to just fix the ticket thanks to the "check unknown", 
and the second to remove the dead usac code. IMHO, two patches would be 
clearer.

Nicolas

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