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

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

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