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

[Baptiste Coudurier <baptiste.coudurier-at-gmail.com@ffmpeg.org>]

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