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

[Nicolas Gaullier <nicolas.gaullier@cji.paris>]

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