[FFmpeg-devel] libavformat/hlsenc.c: Populate OTI using AAC profile in write_codec_attr.

This patch populates the third entry for HLS codec attribute using the
AAC profile.

The HLS specifications[1] require this digit to be the Object Type ID as
referred to in table 1.3 of ISO/IEC 14496-3:2009[2].

The numerical constants in the code refer to these OTIs minus one, as
documeted in commit 372597e[3], confirmed by comparing the values in the
code with the values in the table mentioned above.

Links:
1: https://datatracker.ietf.org/doc/html/rfc6381#section-3.3
2: https://csclub.uwaterloo.ca/~ehashman/ISO14496-3-2009.pdf
3: https://github.com/FFmpeg/FFmpeg/commit/372597e5381c097455a7b73849254d56083eb056

---
 libavformat/hlsenc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Hey there!

Le ven. 22 déc. 2023 à 09:09, Romain Beauxis <toots@rastageeks.org> a écrit :
>
> This patch populates the third entry for HLS codec attribute using the
> AAC profile.
>
> The HLS specifications[1] require this digit to be the Object Type ID as
> referred to in table 1.3 of ISO/IEC 14496-3:2009[2].
>
> The numerical constants in the code refer to these OTIs minus one, as
> documented in commit 372597e[3], confirmed by comparing the values in the
> code with the values in the table mentioned above.
>
> Links:
> 1: https://datatracker.ietf.org/doc/html/rfc6381#section-3.3
> 2: https://csclub.uwaterloo.ca/~ehashman/ISO14496-3-2009.pdf
> 3: https://github.com/FFmpeg/FFmpeg/commit/372597e5381c097455a7b73849254d56083eb056

Anyone interested? I think that this is a pretty straight-forward
change that could potentially qualify as a bugfix for 6.1.1, after
all, this generates incorrect HLS playlist descriptions..

-- Romain

> ---
>  libavformat/hlsenc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 7049956dd7..2551bac6ae 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -418,8 +418,10 @@ static void write_codec_attr(AVStream *st, VariantStream *vs)
>      } else if (st->codecpar->codec_id == AV_CODEC_ID_MP3) {
>          snprintf(attr, sizeof(attr), "mp4a.40.34");
>      } else if (st->codecpar->codec_id == AV_CODEC_ID_AAC) {
> -        /* TODO : For HE-AAC, HE-AACv2, the last digit needs to be set to 5 and 29 respectively */
> -        snprintf(attr, sizeof(attr), "mp4a.40.2");
> +        if (st->codecpar->profile != AV_PROFILE_UNKNOWN)
> +            snprintf(attr, sizeof(attr), "mp4a.40.%d", st->codecpar->profile+1);
> +        else
> +            goto fail;
>      } else if (st->codecpar->codec_id == AV_CODEC_ID_AC3) {
>          snprintf(attr, sizeof(attr), "ac-3");
>      } else if (st->codecpar->codec_id == AV_CODEC_ID_EAC3) {
> --
> 2.39.3 (Apple Git-145)
>

On Fri, Dec 22, 2023 at 8:10 AM Romain Beauxis <toots@rastageeks.org> wrote:

> This patch populates the third entry for HLS codec attribute using the
> AAC profile.
>
> The HLS specifications[1] require this digit to be the Object Type ID as
> referred to in table 1.3 of ISO/IEC 14496-3:2009[2].
>
> The numerical constants in the code refer to these OTIs minus one, as
> documeted in commit 372597e[3], confirmed by comparing the values in the
> code with the values in the table mentioned above.
>
> Links:
> 1: https://datatracker.ietf.org/doc/html/rfc6381#section-3.3
> 2: https://csclub.uwaterloo.ca/~ehashman/ISO14496-3-2009.pdf
> 3:
> https://github.com/FFmpeg/FFmpeg/commit/372597e5381c097455a7b73849254d56083eb056
>
> ---
>  libavformat/hlsenc.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 7049956dd7..2551bac6ae 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -418,8 +418,10 @@ static void write_codec_attr(AVStream *st,
> VariantStream *vs)
>      } else if (st->codecpar->codec_id == AV_CODEC_ID_MP3) {
>          snprintf(attr, sizeof(attr), "mp4a.40.34");
>      } else if (st->codecpar->codec_id == AV_CODEC_ID_AAC) {
> -        /* TODO : For HE-AAC, HE-AACv2, the last digit needs to be set to
> 5 and 29 respectively */
> -        snprintf(attr, sizeof(attr), "mp4a.40.2");
> +        if (st->codecpar->profile != AV_PROFILE_UNKNOWN)
> +            snprintf(attr, sizeof(attr), "mp4a.40.%d",
> st->codecpar->profile+1);
> +        else
> +            goto fail;
>      } else if (st->codecpar->codec_id == AV_CODEC_ID_AC3) {
>          snprintf(attr, sizeof(attr), "ac-3");
>      } else if (st->codecpar->codec_id == AV_CODEC_ID_EAC3) {
> --
> 2.39.3 (Apple Git-145)
>

I love this change, but it appears that st->codecpar->profile is always
AV_PROFILE_UNKNOWN when using libfdk_aac as the encoder. Any indications
where I should look for fix that so this can be used with that encoder?

Thanks,
Dave

>
> I love this change, but it appears that st->codecpar->profile is always
> AV_PROFILE_UNKNOWN when using libfdk_aac as the encoder. Any indications
> where I should look for fix that so this can be used with that encoder?
>

It appears that the issue is that profile doesn't default to what's being
used so `--profile:a` has to be set explicitly with libfdk_aac and then it
works. Not sure if that's an issue worth fixing, but if someone points me
to where it needs to be done, then I'd be glad to take a look at fixing it

Le jeu. 28 déc. 2023 à 17:26, David Johansen <davejohansen@gmail.com> a écrit :
>>
>> I love this change, but it appears that st->codecpar->profile is always AV_PROFILE_UNKNOWN when using libfdk_aac as the encoder. Any indications where I should look for fix that so this can be used with that encoder?
>
>
> It appears that the issue is that profile doesn't default to what's being used so `--profile:a` has to be set explicitly with libfdk_aac and then it works. Not sure if that's an issue worth fixing, but if someone points me to where it needs to be done, then I'd be glad to take a look at fixing it

This feels like a second, separate issue to me?

Maybe we could get these changes in first and then tackle it?

-- Romain

On Sat, Dec 30, 2023 at 8:23 AM Romain Beauxis <toots@rastageeks.org> wrote:

> Le jeu. 28 déc. 2023 à 17:26, David Johansen <davejohansen@gmail.com> a
> écrit :
> >>
> >> I love this change, but it appears that st->codecpar->profile is always
> AV_PROFILE_UNKNOWN when using libfdk_aac as the encoder. Any indications
> where I should look for fix that so this can be used with that encoder?
> >
> >
> > It appears that the issue is that profile doesn't default to what's
> being used so `--profile:a` has to be set explicitly with libfdk_aac and
> then it works. Not sure if that's an issue worth fixing, but if someone
> points me to where it needs to be done, then I'd be glad to take a look at
> fixing it
>
> This feels like a second, separate issue to me?
>
> Maybe we could get these changes in first and then tackle it?
>

But this is technically a breaking change, because it takes commands/uses
that currently work and changes them to no longer include CODECS since the
profile value is unknown by default

Le sam. 30 déc. 2023 à 16:25, David Johansen <davejohansen@gmail.com> a écrit :
>
> On Sat, Dec 30, 2023 at 8:23 AM Romain Beauxis <toots@rastageeks.org> wrote:
>>
>> Le jeu. 28 déc. 2023 à 17:26, David Johansen <davejohansen@gmail.com> a écrit :
>> >>
>> >> I love this change, but it appears that st->codecpar->profile is always AV_PROFILE_UNKNOWN when using libfdk_aac as the encoder. Any indications where I should look for fix that so this can be used with that encoder?
>> >
>> >
>> > It appears that the issue is that profile doesn't default to what's being used so `--profile:a` has to be set explicitly with libfdk_aac and then it works. Not sure if that's an issue worth fixing, but if someone points me to where it needs to be done, then I'd be glad to take a look at fixing it
>>
>> This feels like a second, separate issue to me?
>>
>> Maybe we could get these changes in first and then tackle it?
>
>
> But this is technically a breaking change, because it takes commands/uses that currently work and changes them to no longer include CODECS since the profile value is unknown by default

Ha gotcha.

Yes, in this case we do need to send the previous value as fallback
when the profile is unknown.

Just sent an updated patch!

-- Romain