PR #25192 opened by thardin URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25192 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25192.patch
This patchset fixes some issues with mxfdec and also makes it more strict in its parsing. The last two patches are likely the most controversial. Passes FATE. >From 93c71e0f7d67522108159cb23dc425b2a42a338b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <[email protected]> Date: Sun, 18 Oct 2026 15:06:27 +0200 Subject: [PATCH 1/5] avformat/mxfdec: Only permit 9 or 10 byte ProductVersion and ToolkitVersion This fixes demuxer desynchronization between ToolkitVersion and ModificationDate in opatom_essencegroup_alpha_raw.mxf. S377m says this field is 10 bytes, but Avid support is important, so permit this slight deviation from the spec explicitly. --- libavformat/mxfdec.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index 280a20aad3..acce55b17a 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -3201,7 +3201,16 @@ static int64_t mxf_timestamp_to_int64(uint64_t timestamp) minor = avio_rb16(pb); \ tertiary = avio_rb16(pb); \ patch = avio_rb16(pb); \ - release = avio_rb16(pb); \ + if (size == 10) { \ + release = avio_rb16(pb); \ + } else if (size == 9) { \ + /* opatom_essencegroup_alpha_raw.mxf */ \ + release = avio_r8(pb); \ + } else { \ + /* we accept Avid's brokenness but no others */ \ + av_log(((MXFContext*)arg)->fc, AV_LOG_ERROR, "Product/ToolkitVersion must be 9 or 10 bytes, not %i\n", size); \ + return AVERROR_INVALIDDATA; \ + } \ if ((ret = mxf_version_to_str(major, minor, tertiary, patch, release, &str)) < 0) \ return ret; \ av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL); \ -- 2.54.0 >From d02b0111ac8d7bb35c2232f2acb74fd70c443c5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <[email protected]> Date: Sun, 18 Oct 2026 15:15:54 +0200 Subject: [PATCH 2/5] avformat/mxfdec: Bail out if UID is truncated --- libavformat/mxfdec.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index acce55b17a..4580072dcd 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -3395,7 +3395,10 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF } } if (meta && tag == 0x3C0A) { - avio_read(pb, meta->uid, 16); + if (avio_read(pb, meta->uid, 16) != 16) { + av_log(mxf->fc, AV_LOG_ERROR, "truncated UID\n"); + return AVERROR_INVALIDDATA; + } } else if ((ret = read_child(ctx, pb, tag, size, uid, -1)) < 0) { if (meta) { mxf_free_metadataset(&meta, type); -- 2.54.0 >From 68762b90d0505ec86f5923b4c680e8daf45a5f93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <[email protected]> Date: Sun, 18 Oct 2026 16:00:15 +0200 Subject: [PATCH 3/5] avformat/mxfdec: Parse ECs in partition packs We skip them for now, but this validates that the batch element size is as the spec says. --- libavformat/mxfdec.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index 4580072dcd..30d04f4c4b 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -759,6 +759,7 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size UID op; uint64_t footer_partition; uint32_t nb_essence_containers; + uint32_t sz_essence_containers; uint64_t this_partition; int ret; @@ -832,6 +833,14 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size return ret; } nb_essence_containers = avio_rb32(pb); + sz_essence_containers = avio_rb32(pb); + + if (sz_essence_containers != 16) { + av_log(mxf->fc, AV_LOG_ERROR, "EssenceContainers batch element size is %"PRIu32", not 16\n", sz_essence_containers); + return AVERROR_INVALIDDATA; + } + + avio_skip(pb, (int64_t)nb_essence_containers*sz_essence_containers); if (partition->type == Header) { char str[36]; -- 2.54.0 >From ce12b6caea8e8fbdee23f9be358e6d56f6d3835c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <[email protected]> Date: Sun, 18 Oct 2026 16:00:24 +0200 Subject: [PATCH 4/5] avformat/mxfdec: Require that all KLVs are parsed fully This makes mxfdec loud about any data it misses. We still skip KLV fill and dark metadata. --- libavformat/mxfdec.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index 30d04f4c4b..c1c1805d6a 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -3458,22 +3458,18 @@ static int mxf_parse_klv(MXFContext *mxf, KLVPacket klv, MXFMetadataReadFunc *re if (klv.key[5] == 0x53) { res = mxf_read_local_tags(mxf, &klv, read, ctx_size, type); } else { - uint64_t next = avio_tell(s->pb) + klv.length; res = read(mxf, s->pb, 0, klv.length, klv.key, klv.offset); - - /* only seek forward, else this can loop for a long time */ - if (avio_tell(s->pb) > next) { - av_log(s, AV_LOG_ERROR, "read past end of KLV @ %#"PRIx64"\n", - klv.offset); - return AVERROR_INVALIDDATA; - } - - avio_seek(s->pb, next, SEEK_SET); } if (res < 0) { av_log(s, AV_LOG_ERROR, "error reading header metadata\n"); return res; } + + if (avio_tell(mxf->fc->pb) != klv.next_klv) { + av_log(mxf->fc, AV_LOG_ERROR, "KLV @ %"PRIx64" should have ended at %"PRIx64" but ended at %"PRIx64" instead. truncated file?\n", klv.offset, klv.next_klv, avio_tell(mxf->fc->pb)); + return AVERROR_INVALIDDATA; + } + return 0; } -- 2.54.0 >From f21df29375d36dc8bb1ec049de45d842ea3689cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <[email protected]> Date: Sun, 18 Oct 2026 15:57:32 +0200 Subject: [PATCH 5/5] avformat/mxfdec: Bail out instead of silently overriding SP EditRate with MP EditRate An example would be a file containing 50 Hz material played back at 25 Hz or vice versa, both of which are wrong. Interlacing makes the situation even worse. --- libavformat/mxfdec.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index c1c1805d6a..50f15f83bd 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -2861,9 +2861,18 @@ static int mxf_parse_structural_metadata(MXFContext *mxf) } avpriv_set_pts_info(st, 64, material_track->edit_rate.den, material_track->edit_rate.num); - /* ensure SourceTrack EditRate == MaterialTrack EditRate since only - * the former is accessible via st->priv_data */ - source_track->edit_rate = material_track->edit_rate; + // Files with different SP and MP EditRates are not something we can easily support + // Such files are better handled using NLE tools like kdenlive + // MXF has its basis in AAF after all + if (source_track->edit_rate.num != material_track->edit_rate.num || + source_track->edit_rate.den != material_track->edit_rate.den) { + // We need samples before we can make a proper decision about this + avpriv_request_sample(mxf->fc, + "SourcePackage/MaterialPackage track EditRate mismatch: %i/%i vs %i/%i\n", + source_track->edit_rate.num, source_track->edit_rate.den, + material_track->edit_rate.num, material_track->edit_rate.den); + return AVERROR_PATCHWELCOME; + } PRINT_KEY(mxf->fc, "data definition ul", source_track->sequence->data_definition_ul); codec_ul = mxf_get_codec_ul(ff_mxf_data_definition_uls, &source_track->sequence->data_definition_ul); -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
