On 6/28/25 03:54, Baptiste Coudurier wrote:
>> On Jun 27, 2025, at 5:01 AM, Nicolas Gaullier <nicolas.gaullier at 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 at 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 at 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