Hi Marton,

Thanks for your feedback.

On Mon, May 8, 2023 at 3:58 PM Marton Balint <cus@passwd.hu> wrote:
> On Thu, 27 Apr 2023, Ammon Riley wrote:
> > This patch simply recognizes the AAC audio tracks during
> > decode -- it does not add functionality to encode AAC in
> > MXF.
> >
> > A sample file (st381-4-sample.mxf) has been uploaded to
> > https://streams.videolan.org/upload/, and is also available
> > at https://harmonicinc.box.com/v/st381-4-sample.  Audio
> > and video are both licensed as CC0.
>
> > From 9765ec18f65b8ae147660e0d71bfa80293e57f56 Mon Sep 17 00:00:00 2001
> > From: Ammon Riley <ammon.riley@harmonicinc.com>
> > Date: Wed, 26 Apr 2023 18:26:35 -0700
> > Subject: [PATCH] avcodec/mxfdec: Recognize AAC per SMPTE ST 381-4
> >
> > This patch simply recognizes the AAC audio track during
> > decode -- it does not add functionality to encode AAC in
> > MXF.
> >
> > Signed-off-by: Ammon Riley <ammon.riley@gmail.com>
> > ---
> >  Changelog            | 1 +
> >  libavformat/mxf.c    | 1 +
> >  libavformat/mxfdec.c | 5 +++++
> >  3 files changed, 7 insertions(+)
> >
> > diff --git a/Changelog b/Changelog
> > index a40f32c23f..e68ee0f2c9 100644
> > --- a/Changelog
> > +++ b/Changelog
> > @@ -3,6 +3,7 @@ releases are sorted from youngest to oldest.
> >
> >  version <next>:
> >  - libaribcaption decoder
> > +- recognize AAC in MXF (SMPTE ST 381-4)
>
> I think such small change does not warrant a changelog entry.

Removed.

> > diff --git a/libavformat/mxf.c b/libavformat/mxf.c
> > index 8ef928b8fc..deb6091003 100644
> > --- a/libavformat/mxf.c
> > +++ b/libavformat/mxf.c
> > @@ -78,6 +78,7 @@ const MXFCodecUL ff_mxf_codec_uls[] = {
> >      { {
0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x02,0x02,0x02,0x03,0x02,0x01,0x00
}, 15,        AV_CODEC_ID_AC3 },
> >      { {
0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x02,0x02,0x02,0x03,0x02,0x05,0x00
}, 15,        AV_CODEC_ID_MP2 }, /* MP2 or MP3 */
> >    //{ {
0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x02,0x02,0x02,0x03,0x02,0x1C,0x00
}, 15,    AV_CODEC_ID_DOLBY_E }, /* Dolby-E */
> > +    { {
0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x02,0x02,0x02,0x04,0x00,0x00,0x00
}, 13,        AV_CODEC_ID_AAC }, /* AAC SMPTE 381-4 */
>
> If I read the SMPTE registry correctly, AAC is only ... 0x04 0x03, and
... 0x04 0x04.

Correct.  I wasn't sure whether adding both of those more specific ULs
would be considered polluting the table unnecessarily.  In retrospect,
the UL above merely "Identifies MPEG Audio Compression" (per Table 5),
which could also include non-AAC MPEG audio.  Though Table 6 doesn't
currently define anything other than AAC audio, that's not a guarantee some
future update to the spec won't add it.

> >      { {
0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00
},  0,       AV_CODEC_ID_NONE },
> >  };
> >
> > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > index 8a7008b298..c5960ecf0c 100644
> > --- a/libavformat/mxfdec.c
> > +++ b/libavformat/mxfdec.c
> > @@ -1628,6 +1628,9 @@ static const MXFCodecUL
mxf_sound_essence_container_uls[] = {
> >      { {
0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0d,0x01,0x03,0x01,0x02,0x01,0x01,0x01
}, 14, AV_CODEC_ID_PCM_S16LE, NULL, 13 }, /* D-10 Mapping 50Mbps PAL
Extended Template */
> >      { {
0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0xff,0x4b,0x46,0x41,0x41,0x00,0x0d,0x4d,0x4F
}, 14, AV_CODEC_ID_PCM_S16LE }, /* 0001GL00.MXF.A1.mxf_opatom.mxf */
> >      { {
0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x03,0x04,0x02,0x02,0x02,0x03,0x03,0x01,0x00
}, 14,       AV_CODEC_ID_AAC }, /* MPEG-2 AAC ADTS (legacy) */
> > +    { {
0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x16,0x00,0x00
}, 14,       AV_CODEC_ID_AAC, "aac_adif_smpte_381_4", 14 }, /* AAC SMPTE
381-4 */
> > +    { {
0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x17,0x00,0x00
}, 14,       AV_CODEC_ID_AAC, "aac_adts_smpte_381_4", 14 }, /* AAC SMPTE
381-4 */
> > +    { {
0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x18,0x00,0x00
}, 14,       AV_CODEC_ID_AAC, "aac_latm_loas_smpte_381_4", 14 }, /* AAC
SMPTE 381-4 */
>
> description fields are only used for data, so you can simply use NULL for
them for AAC.

Changed.

Updated patch attached.

Cheers,
Ammon