diff mbox series

[FFmpeg-devel,1/3] avformat/mxfdec: SMPTE RDD 48:2018 support

Message ID 20220711214417.12286-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/3] avformat/mxfdec: SMPTE RDD 48:2018 support | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Michael Niedermayer July 11, 2022, 9:44 p.m. UTC
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mxf.c    |  3 +++
 libavformat/mxf.h    |  1 +
 libavformat/mxfdec.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

Comments

Dave Rice July 13, 2022, 1:58 p.m. UTC | #1
> On Jul 11, 2022, at 5:44 PM, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavformat/mxf.c    |  3 +++
> libavformat/mxf.h    |  1 +
> libavformat/mxfdec.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 52 insertions(+)
> 
> diff --git a/libavformat/mxf.c b/libavformat/mxf.c
> index 36d662b58c..8ef928b8fc 100644
> --- a/libavformat/mxf.c
> +++ b/libavformat/mxf.c
> @@ -66,6 +66,9 @@ const MXFCodecUL ff_mxf_codec_uls[] = {
>     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x01,0x01,0x02,0x02,0x01 }, 16,       AV_CODEC_ID_V210 }, /* V210 */
>     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0E,0x04,0x02,0x01,0x02,0x11,0x00,0x00 }, 14,     AV_CODEC_ID_PRORES }, /* Avid MC7 ProRes */
>     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x06,0x00,0x00 }, 14,     AV_CODEC_ID_PRORES }, /* Apple ProRes */
> +    { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> +    { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> +    { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x04,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V3 */
>     /* SoundEssenceCompression */
>     { { 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,0x01,0x04,0x02,0x02,0x01,0x00,0x00,0x00,0x00 }, 13,  AV_CODEC_ID_PCM_S16LE }, /* uncompressed */
> diff --git a/libavformat/mxf.h b/libavformat/mxf.h
> index 4d9f5119a3..2561605ce5 100644
> --- a/libavformat/mxf.h
> +++ b/libavformat/mxf.h
> @@ -54,6 +54,7 @@ enum MXFMetadataSetType {
>     AudioChannelLabelSubDescriptor,
>     SoundfieldGroupLabelSubDescriptor,
>     GroupOfSoundfieldGroupsLabelSubDescriptor,
> +    FFV1SubDescriptor,
> };
> 
> enum MXFFrameLayout {
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 400941c348..c4d9d6ed93 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -237,6 +237,12 @@ typedef struct MXFMCASubDescriptor {
>     char *language;
> } MXFMCASubDescriptor;
> 
> +typedef struct MXFFFV1SubDescriptor {
> +    MXFMetadataSet meta;
> +    uint8_t *extradata;
> +    int extradata_size;
> +} MXFFFV1SubDescriptor;
> +
> typedef struct MXFIndexTableSegment {
>     MXFMetadataSet meta;
>     int edit_unit_byte_count;
> @@ -337,6 +343,7 @@ static const uint8_t mxf_crypto_source_container_ul[]      = { 0x06,0x0e,0x2b,0x
> static const uint8_t mxf_encrypted_triplet_key[]           = { 0x06,0x0e,0x2b,0x34,0x02,0x04,0x01,0x07,0x0d,0x01,0x03,0x01,0x02,0x7e,0x01,0x00 };
> static const uint8_t mxf_encrypted_essence_container[]     = { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x07,0x0d,0x01,0x03,0x01,0x02,0x0b,0x01,0x00 };
> static const uint8_t mxf_sony_mpeg4_extradata[]            = { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0e,0x06,0x06,0x02,0x02,0x01,0x00,0x00 };
> +static const uint8_t mxf_ffv1_extradata[]                  = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x01,0x06,0x0c,0x01,0x00,0x00,0x00 };
> static const uint8_t mxf_avid_project_name[]               = { 0xa5,0xfb,0x7b,0x25,0xf6,0x15,0x94,0xb9,0x62,0xfc,0x37,0x17,0x49,0x2d,0x42,0xbf };
> static const uint8_t mxf_jp2k_rsiz[]                       = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x01,0x00,0x00,0x00 };
> static const uint8_t mxf_indirect_value_utf16le[]          = { 0x4c,0x00,0x02,0x10,0x01,0x00,0x00,0x00,0x00,0x06,0x0e,0x2b,0x34,0x01,0x04,0x01,0x01 };
> @@ -377,6 +384,9 @@ static void mxf_free_metadataset(MXFMetadataSet **ctx, int freectx)
>         av_freep(&((MXFDescriptor *)*ctx)->file_descriptors_refs);
>         av_freep(&((MXFDescriptor *)*ctx)->sub_descriptors_refs);
>         break;
> +    case FFV1SubDescriptor:
> +        av_freep(&((MXFFFV1SubDescriptor *)*ctx)->extradata);
> +        break;
>     case AudioChannelLabelSubDescriptor:
>     case SoundfieldGroupLabelSubDescriptor:
>     case GroupOfSoundfieldGroupsLabelSubDescriptor:
> @@ -1473,6 +1483,25 @@ static int mxf_read_mca_sub_descriptor(void *arg, AVIOContext *pb, int tag, int
>     return 0;
> }
> 
> +static int mxf_read_ffv1_sub_descriptor(void *arg, AVIOContext *pb, int tag, int size, UID uid, int64_t klv_offset)
> +{
> +    MXFFFV1SubDescriptor *ffv1_sub_descriptor = arg;
> +
> +    if (IS_KLV_KEY(uid, mxf_ffv1_extradata)) {
> +        if (ffv1_sub_descriptor->extradata)
> +            av_log(NULL, AV_LOG_WARNING, "Duplicate ffv1_extradata\n");
> +        av_free(ffv1_sub_descriptor->extradata);
> +        ffv1_sub_descriptor->extradata_size = 0;
> +        ffv1_sub_descriptor->extradata = av_malloc(size);
> +        if (!ffv1_sub_descriptor->extradata)
> +            return AVERROR(ENOMEM);
> +        ffv1_sub_descriptor->extradata_size = size;
> +        avio_read(pb, ffv1_sub_descriptor->extradata, size);
> +    }
> +
> +    return 0;
> +}
> +
> static int mxf_read_indirect_value(void *arg, AVIOContext *pb, int size)
> {
>     MXFTaggedValue *tagged_value = arg;
> @@ -1554,6 +1583,7 @@ static const MXFCodecUL mxf_picture_essence_container_uls[] = {
>     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x02,0x0d,0x01,0x03,0x01,0x02,0x1c,0x01,0x00 }, 14,     AV_CODEC_ID_PRORES, NULL, 14 }, /* ProRes */
>     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x02,0x0d,0x01,0x03,0x01,0x02,0x04,0x60,0x01 }, 14, AV_CODEC_ID_MPEG2VIDEO, NULL, 15 }, /* MPEG-ES */
>     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0d,0x01,0x03,0x01,0x02,0x01,0x04,0x01 }, 14, AV_CODEC_ID_MPEG2VIDEO, NULL, 15, D10D11Wrap }, /* SMPTE D-10 mapping */
> +    { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x23,0x01,0x00 }, 14,       AV_CODEC_ID_FFV1, NULL, 14 },
>     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0d,0x01,0x03,0x01,0x02,0x02,0x41,0x01 }, 14,    AV_CODEC_ID_DVVIDEO, NULL, 15 }, /* DV 625 25mbps */
>     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0d,0x01,0x03,0x01,0x02,0x05,0x00,0x00 }, 14,   AV_CODEC_ID_RAWVIDEO, NULL, 15, RawVWrap }, /* uncompressed picture */
>     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0a,0x0e,0x0f,0x03,0x01,0x02,0x20,0x01,0x01 }, 15,     AV_CODEC_ID_HQ_HQA },
> @@ -2444,6 +2474,21 @@ static MXFMCASubDescriptor *find_mca_link_id(MXFContext *mxf, enum MXFMetadataSe
>     return NULL;
> }
> 
> +static void parse_ffv1_sub_descriptor(MXFContext *mxf, MXFTrack *source_track, MXFDescriptor *descriptor, AVStream *st)
> +{
> +    for (int i = 0; i < descriptor->sub_descriptors_count; i++) {
> +        MXFFFV1SubDescriptor *ffv1_sub_descriptor = mxf_resolve_strong_ref(mxf, &descriptor->sub_descriptors_refs[i], FFV1SubDescriptor);
> +        if (ffv1_sub_descriptor == NULL)
> +            continue;
> +
> +        descriptor->extradata      = ffv1_sub_descriptor->extradata;
> +        descriptor->extradata_size = ffv1_sub_descriptor->extradata_size;
> +        ffv1_sub_descriptor->extradata = NULL;
> +        ffv1_sub_descriptor->extradata_size = 0;
> +        break;
> +    }
> +}
> +
> static int parse_mca_labels(MXFContext *mxf, MXFTrack *source_track, MXFDescriptor *descriptor, AVStream *st)
> {
>     uint64_t routing[FF_SANE_NB_CHANNELS] = {0};
> @@ -2972,6 +3017,8 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>                 st->codecpar->codec_id = AV_CODEC_ID_EIA_608;
>             }
>         }
> +        if (!descriptor->extradata)
> +            parse_ffv1_sub_descriptor(mxf, source_track, descriptor, st);
>         if (descriptor->extradata) {
>             if (!ff_alloc_extradata(st->codecpar, descriptor->extradata_size)) {
>                 memcpy(st->codecpar->extradata, descriptor->extradata, descriptor->extradata_size);
> @@ -3159,6 +3206,7 @@ static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
>     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6b,0x00 }, mxf_read_mca_sub_descriptor, sizeof(MXFMCASubDescriptor), AudioChannelLabelSubDescriptor },
>     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6c,0x00 }, mxf_read_mca_sub_descriptor, sizeof(MXFMCASubDescriptor), SoundfieldGroupLabelSubDescriptor },
>     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6d,0x00 }, mxf_read_mca_sub_descriptor, sizeof(MXFMCASubDescriptor), GroupOfSoundfieldGroupsLabelSubDescriptor },
> +    { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x81,0x03 }, mxf_read_ffv1_sub_descriptor, sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },
>     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x3A,0x00 }, mxf_read_track, sizeof(MXFTrack), Track }, /* Static Track */
>     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x3B,0x00 }, mxf_read_track, sizeof(MXFTrack), Track }, /* Generic Track */
>     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x14,0x00 }, mxf_read_timecode_component, sizeof(MXFTimecodeComponent), TimecodeComponent },
> -- 
> 2.17.1

For those interested, this patch supports SMPTE RDD48 which is at https://www.digitizationguidelines.gov/guidelines/rdd48-2018_published.pdf <https://www.digitizationguidelines.gov/guidelines/rdd48-2018_published.pdf> and is one of SMPTE’s first documents published with a Creative Commons license. 
Dave Rice
Tomas Härdin July 18, 2022, 6:35 p.m. UTC | #2
mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mxf.c    |  3 +++
>  libavformat/mxf.h    |  1 +
>  libavformat/mxfdec.c | 48
> ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+)
> 
> diff --git a/libavformat/mxf.c b/libavformat/mxf.c
> index 36d662b58c..8ef928b8fc 100644
> --- a/libavformat/mxf.c
> +++ b/libavformat/mxf.c
> @@ -66,6 +66,9 @@ const MXFCodecUL ff_mxf_codec_uls[] = {
>      { {
> 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x01,0x01,0x02
> ,0x02,0x01 }, 16,       AV_CODEC_ID_V210 }, /* V210 */
>      { {
> 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0E,0x04,0x02,0x01,0x02,0x11
> ,0x00,0x00 }, 14,     AV_CODEC_ID_PRORES }, /* Avid MC7 ProRes */
>      { {
> 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x06
> ,0x00,0x00 }, 14,     AV_CODEC_ID_PRORES }, /* Apple ProRes */
> +    { {
> 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> +    { {
> 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> +    { {
> 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> ,0x04,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V3 */

I do not see these ULs listed in the spec. Are they in some appropriate
RP?

> +static const uint8_t mxf_ffv1_extradata[]                  = {
> 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x01,0x06,0x0c,0x01,0x00
> ,0x00,0x00 };

Nor do I see this UL. In fact the spec doesn't seem to mention FFV1 at
all unless my eyes deceive me.

/Tomas
Michael Niedermayer July 19, 2022, 11:54 a.m. UTC | #3
On Mon, Jul 18, 2022 at 08:35:00PM +0200, Tomas Härdin wrote:
> mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/mxf.c    |  3 +++
> >  libavformat/mxf.h    |  1 +
> >  libavformat/mxfdec.c | 48
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 52 insertions(+)
> > 
> > diff --git a/libavformat/mxf.c b/libavformat/mxf.c
> > index 36d662b58c..8ef928b8fc 100644
> > --- a/libavformat/mxf.c
> > +++ b/libavformat/mxf.c
> > @@ -66,6 +66,9 @@ const MXFCodecUL ff_mxf_codec_uls[] = {
> >      { {
> > 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x01,0x01,0x02
> > ,0x02,0x01 }, 16,       AV_CODEC_ID_V210 }, /* V210 */
> >      { {
> > 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0E,0x04,0x02,0x01,0x02,0x11
> > ,0x00,0x00 }, 14,     AV_CODEC_ID_PRORES }, /* Avid MC7 ProRes */
> >      { {
> > 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x06
> > ,0x00,0x00 }, 14,     AV_CODEC_ID_PRORES }, /* Apple ProRes */
> > +    { {
> > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> > ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> > +    { {
> > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> > ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> > +    { {
> > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> > ,0x04,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V3 */
> 
> I do not see these ULs listed in the spec. Are they in some appropriate
> RP?
> 
> > +static const uint8_t mxf_ffv1_extradata[]                  = {
> > 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x01,0x06,0x0c,0x01,0x00
> > ,0x00,0x00 };
> 
> Nor do I see this UL. In fact the spec doesn't seem to mention FFV1 at
> all unless my eyes deceive me.

you need this:
SMPTE RDD 48:2018
Amendment 1:2022

i will update the patch to list this correctly

[...]
Tomas Härdin July 19, 2022, 1:48 p.m. UTC | #4
mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> 
> +    { {
> 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> +    { {
> 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> +    { {

Double-checked, these are correct

> +typedef struct MXFFFV1SubDescriptor {
> +    MXFMetadataSet meta;
> +    uint8_t *extradata;
> +    int extradata_size;

Is FFV1 extradata size bounded? It so we could avoid an allocation.
Either way the local set syntax limits this to 64k, see below.

> +static const uint8_t mxf_ffv1_extradata[]                  = {
> 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x01,0x06,0x0c,0x01,0x00
> ,0x00,0x00 };

Maybe add a comment // FFV1InitializationMetadata

> +static int mxf_read_ffv1_sub_descriptor(void *arg, AVIOContext *pb,
> int tag, int size, UID uid, int64_t klv_offset)
> +{
> +    MXFFFV1SubDescriptor *ffv1_sub_descriptor = arg;
> +
> +    if (IS_KLV_KEY(uid, mxf_ffv1_extradata)) {
> +        if (ffv1_sub_descriptor->extradata)
> +            av_log(NULL, AV_LOG_WARNING, "Duplicate
> ffv1_extradata\n");

Perhaps we should pass AVFormatContext* along with these functions to
enable proper logging. Doesn't need to hold up this patch though.

> +        av_free(ffv1_sub_descriptor->extradata);
> +        ffv1_sub_descriptor->extradata_size = 0;
> +        ffv1_sub_descriptor->extradata = av_malloc(size);
> +        if (!ffv1_sub_descriptor->extradata)
> +            return AVERROR(ENOMEM);
> +        ffv1_sub_descriptor->extradata_size = size;

This could be simplified with av_fast_malloc(), or no allocation at all
if we use a static sized array.

> +        avio_read(pb, ffv1_sub_descriptor->extradata, size);
> +    }

I presume the other items (GOP size, version number etc) are of no
interest and can be probed by the decoder.

> +    { {
> 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x23
> ,0x01,0x00 }, 14,       AV_CODEC_ID_FFV1, NULL, 14 },

matching_len and wrapping_indicator_pos appear to be correct.
mxf_get_wrapping_kind() should report the correct wrapping type AFAICT.

> +static void parse_ffv1_sub_descriptor(MXFContext *mxf, MXFTrack
> *source_track, MXFDescriptor *descriptor, AVStream *st)
> +{
> +    for (int i = 0; i < descriptor->sub_descriptors_count; i++) {
> +        MXFFFV1SubDescriptor *ffv1_sub_descriptor =
> mxf_resolve_strong_ref(mxf, &descriptor->sub_descriptors_refs[i],
> FFV1SubDescriptor);
> +        if (ffv1_sub_descriptor == NULL)
> +            continue;
> +
> +        descriptor->extradata      = ffv1_sub_descriptor->extradata;
> +        descriptor->extradata_size = ffv1_sub_descriptor-
> >extradata_size;
> +        ffv1_sub_descriptor->extradata = NULL;
> +        ffv1_sub_descriptor->extradata_size = 0;

I guess this will require an allocation either way

> +    { {
> 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01
> ,0x81,0x03 }, mxf_read_ffv1_sub_descriptor,
> sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },

The spec says 0x7F not 0x53. 0x53 is used in groups with 2-byte tags
rather than full KLVs. The intent here seems to be to use local tags,
which fortuitously limits extradata_size to 64k. This makes me think
Amendment 1:2022 is wrong or that 0x7F is just to signal private use
until it gets rolled into the next version of RDD 48.

Tables 18 and 23 in S377m-1-2009 say that 0x7F corresponds to "Abstract
Groups" which are "never encoded as Metadata Sets".

Reading S336m-2007 it seems one can actually use various lengths and
tag sizes. 0x53 corresponds to 2 byte length and 2 byte tag. S377m says
that in addition to this, 0x13 is allowed in MXF which uses ASN.1 BER
encoded lengths. Don't know if any files in the wild use that. Probably
not.

/Tomas
Michael Niedermayer July 28, 2022, 11:18 p.m. UTC | #5
On Tue, Jul 19, 2022 at 03:48:59PM +0200, Tomas Härdin wrote:
> mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> > 
> > +    { {
> > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> > ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> > +    { {
> > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> > ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> > +    { {
> 
> Double-checked, these are correct
> 
> > +typedef struct MXFFFV1SubDescriptor {
> > +    MXFMetadataSet meta;
> > +    uint8_t *extradata;
> > +    int extradata_size;
> 
> Is FFV1 extradata size bounded? It so we could avoid an allocation.
> Either way the local set syntax limits this to 64k, see below.

the extradata is extensible so future versions can be bigger.
For the current version there should be a maximum. As the extradata uses
an adaptive range coder it is not trivial to give a tight limit. It would
be easy to give some non tight limit. But iam not sure this has any point
as future versions can be bigger


> 
> > +static const uint8_t mxf_ffv1_extradata[]                  = {
> > 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x01,0x06,0x0c,0x01,0x00
> > ,0x00,0x00 };
> 
> Maybe add a comment // FFV1InitializationMetadata

done


> 
> > +static int mxf_read_ffv1_sub_descriptor(void *arg, AVIOContext *pb,
> > int tag, int size, UID uid, int64_t klv_offset)
> > +{
> > +    MXFFFV1SubDescriptor *ffv1_sub_descriptor = arg;
> > +
> > +    if (IS_KLV_KEY(uid, mxf_ffv1_extradata)) {
> > +        if (ffv1_sub_descriptor->extradata)
> > +            av_log(NULL, AV_LOG_WARNING, "Duplicate
> > ffv1_extradata\n");
> 
> Perhaps we should pass AVFormatContext* along with these functions to
> enable proper logging. Doesn't need to hold up this patch though.
> 
> > +        av_free(ffv1_sub_descriptor->extradata);
> > +        ffv1_sub_descriptor->extradata_size = 0;
> > +        ffv1_sub_descriptor->extradata = av_malloc(size);
> > +        if (!ffv1_sub_descriptor->extradata)
> > +            return AVERROR(ENOMEM);
> > +        ffv1_sub_descriptor->extradata_size = size;
> 
> This could be simplified with av_fast_malloc(), or no allocation at all
> if we use a static sized array.

this was missing AV_INPUT_BUFFER_PADDING_SIZE, added that.
I dont think av_fast_malloc() is a good idea, its a once per stream
allocation, i also dont think a static array is a good idea, there is
no size limit unless you want to limit to a specific version and
compute a worst case bound on a adaptive coder. And then
that worst case would be orders of magnitude bigger than real extradata
because real extradata compresses quite well. While the worst case would
be the case that is biggest and compresses worst. So a static array
would waste space


> 
> > +        avio_read(pb, ffv1_sub_descriptor->extradata, size);
> > +    }
> 
> I presume the other items (GOP size, version number etc) are of no
> interest and can be probed by the decoder.

They are of no interrest to me ATM. Maybe there is some use case for
parsing some mroe values, i do not really know. I would say we add them
when we understand what to do with them.
I presume theres at least one person who saw a use in them being stored
there so i presume theres someone in some standard committee who saw some
use in it. It seems that usecase did not make it into writing in the
part of the specification that i did read


[...]

> 
> > +    { {
> > 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01
> > ,0x81,0x03 }, mxf_read_ffv1_sub_descriptor,
> > sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },
> 
> The spec says 0x7F not 0x53. 0x53 is used in groups with 2-byte tags

If i put 0x7F with no other change there, it will break demuxing the files i have
I guess i must have copied this from the files without noticing it mismatches
the spec


> rather than full KLVs. The intent here seems to be to use local tags,
> which fortuitously limits extradata_size to 64k. This makes me think
> Amendment 1:2022 is wrong or that 0x7F is just to signal private use
> until it gets rolled into the next version of RDD 48.
> 
> Tables 18 and 23 in S377m-1-2009 say that 0x7F corresponds to "Abstract
> Groups" which are "never encoded as Metadata Sets".
> 
> Reading S336m-2007 it seems one can actually use various lengths and
> tag sizes. 0x53 corresponds to 2 byte length and 2 byte tag. S377m says
> that in addition to this, 0x13 is allowed in MXF which uses ASN.1 BER
> encoded lengths. Don't know if any files in the wild use that. Probably
> not.

couldnt they make this more complex and bizarr?

thx

[...]
Tomas Härdin July 29, 2022, 4:15 a.m. UTC | #6
fre 2022-07-29 klockan 01:18 +0200 skrev Michael Niedermayer:
> On Tue, Jul 19, 2022 at 03:48:59PM +0200, Tomas Härdin wrote:
> > mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> > > 
> > > +    { {
> > > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,
> > > 0x09
> > > ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> > > +    { {
> > > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,
> > > 0x09
> > > ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> > > +    { {
> > 
> > Double-checked, these are correct
> > 
> > > +typedef struct MXFFFV1SubDescriptor {
> > > +    MXFMetadataSet meta;
> > > +    uint8_t *extradata;
> > > +    int extradata_size;
> > 
> > Is FFV1 extradata size bounded? It so we could avoid an allocation.
> > Either way the local set syntax limits this to 64k, see below.
> 
> the extradata is extensible so future versions can be bigger.
> For the current version there should be a maximum. As the extradata
> uses
> an adaptive range coder it is not trivial to give a tight limit. It
> would
> be easy to give some non tight limit. But iam not sure this has any
> point
> as future versions can be bigger
> [...]
> i also dont think a static array is a good idea, there is
> no size limit unless you want to limit to a specific version and
> compute a worst case bound on a adaptive coder. And then
> that worst case would be orders of magnitude bigger than real
> extradata
> because real extradata compresses quite well. While the worst case
> would
> be the case that is biggest and compresses worst. So a static array
> would waste space

Values in (0x53) local sets are limited to 64k, so it should be fine in
this context


> > 
> > > +    { {
> > > 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,
> > > 0x01
> > > ,0x81,0x03 }, mxf_read_ffv1_sub_descriptor,
> > > sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },
> > 
> > The spec says 0x7F not 0x53. 0x53 is used in groups with 2-byte
> > tags
> 
> If i put 0x7F with no other change there, it will break demuxing the
> files i have
> I guess i must have copied this from the files without noticing it
> mismatches
> the spec

Yeah I would expect it to break with 0x7F. Perhaps this will change
when the spec becomes official. If you have contact with the people
involved in this then I suggest asking them about this. It could also
be a typo in the spec.

> 
> 
> > rather than full KLVs. The intent here seems to be to use local
> > tags,
> > which fortuitously limits extradata_size to 64k. This makes me
> > think
> > Amendment 1:2022 is wrong or that 0x7F is just to signal private
> > use
> > until it gets rolled into the next version of RDD 48.
> > 
> > Tables 18 and 23 in S377m-1-2009 say that 0x7F corresponds to
> > "Abstract
> > Groups" which are "never encoded as Metadata Sets".
> > 
> > Reading S336m-2007 it seems one can actually use various lengths
> > and
> > tag sizes. 0x53 corresponds to 2 byte length and 2 byte tag. S377m
> > says
> > that in addition to this, 0x13 is allowed in MXF which uses ASN.1
> > BER
> > encoded lengths. Don't know if any files in the wild use that.
> > Probably
> > not.
> 
> couldnt they make this more complex and bizarr?

Welcome to the world of MXF.

The flip side here is that extradata over 64k *can* be encoded legally,
but I have never seen it in the wild and mxfdec doesn't support it
(yet). In that light it makes sense to keep the extradata allocation
dynamic

/Tomas
Pierre-Anthony Lemieux July 29, 2022, 12:14 p.m. UTC | #7
On Fri, Jul 29, 2022 at 6:15 AM Tomas Härdin <tjoppen@acc.umu.se> wrote:
>
> fre 2022-07-29 klockan 01:18 +0200 skrev Michael Niedermayer:
> > On Tue, Jul 19, 2022 at 03:48:59PM +0200, Tomas Härdin wrote:
> > > mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> > > >
> > > > +    { {
> > > > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,
> > > > 0x09
> > > > ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> > > > +    { {
> > > > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,
> > > > 0x09
> > > > ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> > > > +    { {
> > >
> > > Double-checked, these are correct

I recommend the draft SMPTE metadata registry at the following as the
reference for ULs:

https://registry.smpte-ra.org/apps/pages/

The registry is kept up-to-date, machine readable and free to access.

> > >
> > > > +typedef struct MXFFFV1SubDescriptor {
> > > > +    MXFMetadataSet meta;
> > > > +    uint8_t *extradata;
> > > > +    int extradata_size;
> > >
> > > Is FFV1 extradata size bounded? It so we could avoid an allocation.
> > > Either way the local set syntax limits this to 64k, see below.
> >
> > the extradata is extensible so future versions can be bigger.
> > For the current version there should be a maximum. As the extradata
> > uses
> > an adaptive range coder it is not trivial to give a tight limit. It
> > would
> > be easy to give some non tight limit. But iam not sure this has any
> > point
> > as future versions can be bigger
> > [...]
> > i also dont think a static array is a good idea, there is
> > no size limit unless you want to limit to a specific version and
> > compute a worst case bound on a adaptive coder. And then
> > that worst case would be orders of magnitude bigger than real
> > extradata
> > because real extradata compresses quite well. While the worst case
> > would
> > be the case that is biggest and compresses worst. So a static array
> > would waste space
>
> Values in (0x53) local sets are limited to 64k, so it should be fine in
> this context
>
>
> > >
> > > > +    { {
> > > > 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,
> > > > 0x01
> > > > ,0x81,0x03 }, mxf_read_ffv1_sub_descriptor,
> > > > sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },
> > >
> > > The spec says 0x7F not 0x53. 0x53 is used in groups with 2-byte
> > > tags
> >
> > If i put 0x7F with no other change there, it will break demuxing the
> > files i have
> > I guess i must have copied this from the files without noticing it
> > mismatches
> > the spec
>
> Yeah I would expect it to break with 0x7F. Perhaps this will change
> when the spec becomes official. If you have contact with the people
> involved in this then I suggest asking them about this. It could also
> be a typo in the spec.

Byte 6 of Group ULs is set by convention to the wildcard value 0x7F to
indicate that the encoding of the Group is not limited to 0x53 (local
set with 2-byte local tags and length field). See the following:

https://registry.smpte-ra.org/view/draft/docs/Submissions%20Overview/Document-Editors-Information--Style-Guide/#groups-ul

MXF restricts header metadata to local sets with 2-byte local tags and
2-byte or BER lengths, so byte 6 can be 0x13 or 0x53.

>
> >
> >
> > > rather than full KLVs. The intent here seems to be to use local
> > > tags,
> > > which fortuitously limits extradata_size to 64k. This makes me
> > > think
> > > Amendment 1:2022 is wrong or that 0x7F is just to signal private
> > > use
> > > until it gets rolled into the next version of RDD 48.
> > >
> > > Tables 18 and 23 in S377m-1-2009 say that 0x7F corresponds to
> > > "Abstract
> > > Groups" which are "never encoded as Metadata Sets".
> > >
> > > Reading S336m-2007 it seems one can actually use various lengths
> > > and
> > > tag sizes. 0x53 corresponds to 2 byte length and 2 byte tag. S377m
> > > says
> > > that in addition to this, 0x13 is allowed in MXF which uses ASN.1
> > > BER
> > > encoded lengths. Don't know if any files in the wild use that.
> > > Probably
> > > not.
> >
> > couldnt they make this more complex and bizarr?
>
> Welcome to the world of MXF.
>
> The flip side here is that extradata over 64k *can* be encoded legally,
> but I have never seen it in the wild and mxfdec doesn't support it
> (yet). In that light it makes sense to keep the extradata allocation
> dynamic
>
> /Tomas
>
> _______________________________________________
> 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".
Tomas Härdin July 29, 2022, 2:19 p.m. UTC | #8
fre 2022-07-29 klockan 14:14 +0200 skrev Pierre-Anthony Lemieux:
> On Fri, Jul 29, 2022 at 6:15 AM Tomas Härdin <tjoppen@acc.umu.se>
> wrote:
> > 
> > fre 2022-07-29 klockan 01:18 +0200 skrev Michael Niedermayer:
> > > On Tue, Jul 19, 2022 at 03:48:59PM +0200, Tomas Härdin wrote:
> > > > mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> > > > > 
> > > > > +    { {
> > > > > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0
> > > > > x03,
> > > > > 0x09
> > > > > ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> > > > > +    { {
> > > > > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0
> > > > > x03,
> > > > > 0x09
> > > > > ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> > > > > +    { {
> > > > 
> > > > Double-checked, these are correct
> 
> I recommend the draft SMPTE metadata registry at the following as the
> reference for ULs:
> 
> https://registry.smpte-ra.org/apps/pages/
> 
> The registry is kept up-to-date, machine readable and free to access.

Neato. I actually have a tool for parsing ULs that I call wtful. For
now it parses relevant RP spreadsheets, but it's kinda shitty. Maybe I
can improve and publish it.
> 

> > > > 
> > > > > +    { {
> > > > > 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0
> > > > > x01,
> > > > > 0x01
> > > > > ,0x81,0x03 }, mxf_read_ffv1_sub_descriptor,
> > > > > sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },
> > > > 
> > > > The spec says 0x7F not 0x53. 0x53 is used in groups with 2-byte
> > > > tags
> > > 
> > > If i put 0x7F with no other change there, it will break demuxing
> > > the
> > > files i have
> > > I guess i must have copied this from the files without noticing
> > > it
> > > mismatches
> > > the spec
> > 
> > Yeah I would expect it to break with 0x7F. Perhaps this will change
> > when the spec becomes official. If you have contact with the people
> > involved in this then I suggest asking them about this. It could
> > also
> > be a typo in the spec.
> 
> Byte 6 of Group ULs is set by convention to the wildcard value 0x7F
> to
> indicate that the encoding of the Group is not limited to 0x53 (local
> set with 2-byte local tags and length field). See the following:
> 
>   
> https://registry.smpte-ra.org/view/draft/docs/Submissions%20Overview/Document-Editors-Information--Style-Guide/#groups-ul
> 
> MXF restricts header metadata to local sets with 2-byte local tags
> and
> 2-byte or BER lengths, so byte 6 can be 0x13 or 0x53.

Alright, then it's fine. Maybe at some point we'll need to implement
BER lengths in local sets, but not now

/Tomas
Pierre-Anthony Lemieux July 29, 2022, 2:24 p.m. UTC | #9
On Fri, Jul 29, 2022 at 4:19 PM Tomas Härdin <tjoppen@acc.umu.se> wrote:
>
> fre 2022-07-29 klockan 14:14 +0200 skrev Pierre-Anthony Lemieux:
> > On Fri, Jul 29, 2022 at 6:15 AM Tomas Härdin <tjoppen@acc.umu.se>
> > wrote:
> > >
> > > fre 2022-07-29 klockan 01:18 +0200 skrev Michael Niedermayer:
> > > > On Tue, Jul 19, 2022 at 03:48:59PM +0200, Tomas Härdin wrote:
> > > > > mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> > > > > >
> > > > > > +    { {
> > > > > > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0
> > > > > > x03,
> > > > > > 0x09
> > > > > > ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> > > > > > +    { {
> > > > > > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0
> > > > > > x03,
> > > > > > 0x09
> > > > > > ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> > > > > > +    { {
> > > > >
> > > > > Double-checked, these are correct
> >
> > I recommend the draft SMPTE metadata registry at the following as the
> > reference for ULs:
> >
> > https://registry.smpte-ra.org/apps/pages/
> >
> > The registry is kept up-to-date, machine readable and free to access.
>
> Neato. I actually have a tool for parsing ULs that I call wtful. For
> now it parses relevant RP spreadsheets, but it's kinda shitty. Maybe I
> can improve and publish it.

The canonical format for the SMPTE registers is XML. The data (and
corresponding XSDs) can also be found at:

https://registry.smpte-ra.org/apps/pages/draft/

("draft" registries are 99% correct. "published" are 99.9% correct but
lag by about 6 months.)

The following are Java bindings for folks that are into that :)

https://github.com/sandflow/regxmllib/tree/master/src/main/java/com/sandflow/smpte/register

> >
>
> > > > >
> > > > > > +    { {
> > > > > > 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0
> > > > > > x01,
> > > > > > 0x01
> > > > > > ,0x81,0x03 }, mxf_read_ffv1_sub_descriptor,
> > > > > > sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },
> > > > >
> > > > > The spec says 0x7F not 0x53. 0x53 is used in groups with 2-byte
> > > > > tags
> > > >
> > > > If i put 0x7F with no other change there, it will break demuxing
> > > > the
> > > > files i have
> > > > I guess i must have copied this from the files without noticing
> > > > it
> > > > mismatches
> > > > the spec
> > >
> > > Yeah I would expect it to break with 0x7F. Perhaps this will change
> > > when the spec becomes official. If you have contact with the people
> > > involved in this then I suggest asking them about this. It could
> > > also
> > > be a typo in the spec.
> >
> > Byte 6 of Group ULs is set by convention to the wildcard value 0x7F
> > to
> > indicate that the encoding of the Group is not limited to 0x53 (local
> > set with 2-byte local tags and length field). See the following:
> >
> >
> > https://registry.smpte-ra.org/view/draft/docs/Submissions%20Overview/Document-Editors-Information--Style-Guide/#groups-ul
> >
> > MXF restricts header metadata to local sets with 2-byte local tags
> > and
> > 2-byte or BER lengths, so byte 6 can be 0x13 or 0x53.
>
> Alright, then it's fine. Maybe at some point we'll need to implement
> BER lengths in local sets, but not now
>
> /Tomas
>
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavformat/mxf.c b/libavformat/mxf.c
index 36d662b58c..8ef928b8fc 100644
--- a/libavformat/mxf.c
+++ b/libavformat/mxf.c
@@ -66,6 +66,9 @@  const MXFCodecUL ff_mxf_codec_uls[] = {
     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x01,0x01,0x02,0x02,0x01 }, 16,       AV_CODEC_ID_V210 }, /* V210 */
     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0E,0x04,0x02,0x01,0x02,0x11,0x00,0x00 }, 14,     AV_CODEC_ID_PRORES }, /* Avid MC7 ProRes */
     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x06,0x00,0x00 }, 14,     AV_CODEC_ID_PRORES }, /* Apple ProRes */
+    { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
+    { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
+    { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x04,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V3 */
     /* SoundEssenceCompression */
     { { 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,0x01,0x04,0x02,0x02,0x01,0x00,0x00,0x00,0x00 }, 13,  AV_CODEC_ID_PCM_S16LE }, /* uncompressed */
diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index 4d9f5119a3..2561605ce5 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -54,6 +54,7 @@  enum MXFMetadataSetType {
     AudioChannelLabelSubDescriptor,
     SoundfieldGroupLabelSubDescriptor,
     GroupOfSoundfieldGroupsLabelSubDescriptor,
+    FFV1SubDescriptor,
 };
 
 enum MXFFrameLayout {
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 400941c348..c4d9d6ed93 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -237,6 +237,12 @@  typedef struct MXFMCASubDescriptor {
     char *language;
 } MXFMCASubDescriptor;
 
+typedef struct MXFFFV1SubDescriptor {
+    MXFMetadataSet meta;
+    uint8_t *extradata;
+    int extradata_size;
+} MXFFFV1SubDescriptor;
+
 typedef struct MXFIndexTableSegment {
     MXFMetadataSet meta;
     int edit_unit_byte_count;
@@ -337,6 +343,7 @@  static const uint8_t mxf_crypto_source_container_ul[]      = { 0x06,0x0e,0x2b,0x
 static const uint8_t mxf_encrypted_triplet_key[]           = { 0x06,0x0e,0x2b,0x34,0x02,0x04,0x01,0x07,0x0d,0x01,0x03,0x01,0x02,0x7e,0x01,0x00 };
 static const uint8_t mxf_encrypted_essence_container[]     = { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x07,0x0d,0x01,0x03,0x01,0x02,0x0b,0x01,0x00 };
 static const uint8_t mxf_sony_mpeg4_extradata[]            = { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0e,0x06,0x06,0x02,0x02,0x01,0x00,0x00 };
+static const uint8_t mxf_ffv1_extradata[]                  = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x01,0x06,0x0c,0x01,0x00,0x00,0x00 };
 static const uint8_t mxf_avid_project_name[]               = { 0xa5,0xfb,0x7b,0x25,0xf6,0x15,0x94,0xb9,0x62,0xfc,0x37,0x17,0x49,0x2d,0x42,0xbf };
 static const uint8_t mxf_jp2k_rsiz[]                       = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x01,0x00,0x00,0x00 };
 static const uint8_t mxf_indirect_value_utf16le[]          = { 0x4c,0x00,0x02,0x10,0x01,0x00,0x00,0x00,0x00,0x06,0x0e,0x2b,0x34,0x01,0x04,0x01,0x01 };
@@ -377,6 +384,9 @@  static void mxf_free_metadataset(MXFMetadataSet **ctx, int freectx)
         av_freep(&((MXFDescriptor *)*ctx)->file_descriptors_refs);
         av_freep(&((MXFDescriptor *)*ctx)->sub_descriptors_refs);
         break;
+    case FFV1SubDescriptor:
+        av_freep(&((MXFFFV1SubDescriptor *)*ctx)->extradata);
+        break;
     case AudioChannelLabelSubDescriptor:
     case SoundfieldGroupLabelSubDescriptor:
     case GroupOfSoundfieldGroupsLabelSubDescriptor:
@@ -1473,6 +1483,25 @@  static int mxf_read_mca_sub_descriptor(void *arg, AVIOContext *pb, int tag, int
     return 0;
 }
 
+static int mxf_read_ffv1_sub_descriptor(void *arg, AVIOContext *pb, int tag, int size, UID uid, int64_t klv_offset)
+{
+    MXFFFV1SubDescriptor *ffv1_sub_descriptor = arg;
+
+    if (IS_KLV_KEY(uid, mxf_ffv1_extradata)) {
+        if (ffv1_sub_descriptor->extradata)
+            av_log(NULL, AV_LOG_WARNING, "Duplicate ffv1_extradata\n");
+        av_free(ffv1_sub_descriptor->extradata);
+        ffv1_sub_descriptor->extradata_size = 0;
+        ffv1_sub_descriptor->extradata = av_malloc(size);
+        if (!ffv1_sub_descriptor->extradata)
+            return AVERROR(ENOMEM);
+        ffv1_sub_descriptor->extradata_size = size;
+        avio_read(pb, ffv1_sub_descriptor->extradata, size);
+    }
+
+    return 0;
+}
+
 static int mxf_read_indirect_value(void *arg, AVIOContext *pb, int size)
 {
     MXFTaggedValue *tagged_value = arg;
@@ -1554,6 +1583,7 @@  static const MXFCodecUL mxf_picture_essence_container_uls[] = {
     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x02,0x0d,0x01,0x03,0x01,0x02,0x1c,0x01,0x00 }, 14,     AV_CODEC_ID_PRORES, NULL, 14 }, /* ProRes */
     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x02,0x0d,0x01,0x03,0x01,0x02,0x04,0x60,0x01 }, 14, AV_CODEC_ID_MPEG2VIDEO, NULL, 15 }, /* MPEG-ES */
     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0d,0x01,0x03,0x01,0x02,0x01,0x04,0x01 }, 14, AV_CODEC_ID_MPEG2VIDEO, NULL, 15, D10D11Wrap }, /* SMPTE D-10 mapping */
+    { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x23,0x01,0x00 }, 14,       AV_CODEC_ID_FFV1, NULL, 14 },
     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0d,0x01,0x03,0x01,0x02,0x02,0x41,0x01 }, 14,    AV_CODEC_ID_DVVIDEO, NULL, 15 }, /* DV 625 25mbps */
     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0d,0x01,0x03,0x01,0x02,0x05,0x00,0x00 }, 14,   AV_CODEC_ID_RAWVIDEO, NULL, 15, RawVWrap }, /* uncompressed picture */
     { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0a,0x0e,0x0f,0x03,0x01,0x02,0x20,0x01,0x01 }, 15,     AV_CODEC_ID_HQ_HQA },
@@ -2444,6 +2474,21 @@  static MXFMCASubDescriptor *find_mca_link_id(MXFContext *mxf, enum MXFMetadataSe
     return NULL;
 }
 
+static void parse_ffv1_sub_descriptor(MXFContext *mxf, MXFTrack *source_track, MXFDescriptor *descriptor, AVStream *st)
+{
+    for (int i = 0; i < descriptor->sub_descriptors_count; i++) {
+        MXFFFV1SubDescriptor *ffv1_sub_descriptor = mxf_resolve_strong_ref(mxf, &descriptor->sub_descriptors_refs[i], FFV1SubDescriptor);
+        if (ffv1_sub_descriptor == NULL)
+            continue;
+
+        descriptor->extradata      = ffv1_sub_descriptor->extradata;
+        descriptor->extradata_size = ffv1_sub_descriptor->extradata_size;
+        ffv1_sub_descriptor->extradata = NULL;
+        ffv1_sub_descriptor->extradata_size = 0;
+        break;
+    }
+}
+
 static int parse_mca_labels(MXFContext *mxf, MXFTrack *source_track, MXFDescriptor *descriptor, AVStream *st)
 {
     uint64_t routing[FF_SANE_NB_CHANNELS] = {0};
@@ -2972,6 +3017,8 @@  static int mxf_parse_structural_metadata(MXFContext *mxf)
                 st->codecpar->codec_id = AV_CODEC_ID_EIA_608;
             }
         }
+        if (!descriptor->extradata)
+            parse_ffv1_sub_descriptor(mxf, source_track, descriptor, st);
         if (descriptor->extradata) {
             if (!ff_alloc_extradata(st->codecpar, descriptor->extradata_size)) {
                 memcpy(st->codecpar->extradata, descriptor->extradata, descriptor->extradata_size);
@@ -3159,6 +3206,7 @@  static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6b,0x00 }, mxf_read_mca_sub_descriptor, sizeof(MXFMCASubDescriptor), AudioChannelLabelSubDescriptor },
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6c,0x00 }, mxf_read_mca_sub_descriptor, sizeof(MXFMCASubDescriptor), SoundfieldGroupLabelSubDescriptor },
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6d,0x00 }, mxf_read_mca_sub_descriptor, sizeof(MXFMCASubDescriptor), GroupOfSoundfieldGroupsLabelSubDescriptor },
+    { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x81,0x03 }, mxf_read_ffv1_sub_descriptor, sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x3A,0x00 }, mxf_read_track, sizeof(MXFTrack), Track }, /* Static Track */
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x3B,0x00 }, mxf_read_track, sizeof(MXFTrack), Track }, /* Generic Track */
     { { 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x14,0x00 }, mxf_read_timecode_component, sizeof(MXFTimecodeComponent), TimecodeComponent },