[FFmpeg-devel] Re: [RFC] C++

["Tomas Härdin via ffmpeg-devel" <ffmpeg-devel@ffmpeg.org>]

[-- Attachment #1: Type: text/plain, Size: 1629 bytes --]

tis 2025-10-21 klockan 04:24 +0200 skrev Lynne via ffmpeg-devel:
> On 20/10/2025 19:50, Tomas Härdin via ffmpeg-devel wrote:
> > Hi
> > 
> > I'm writing this email to get a feel for how everyone feels about
> > making more use of C++ in the codebase. I am only proposing using
> > C++
> > *internally*, and only where it makes sense. I am not suggesting a
> > "move" to C++, merely using features already present in the
> > compilers
> > we target: gcc, clang and cl. The impedance mismatch should
> > therefore
> > be small, and any missing compiler features should be caught by
> > FATE.
> 
> Definitely not.
> The patch you posted hardly changes anything.

Here's a more illustrative example. What it means for a given offset to
be contained "within" a partition is made explicit. This also allows us
to reject files where partitions are overlapping, which wasn't obvious
with the previous code

The codebase is actually littered with binary searches like the one the
attached patchset removes. That's a major code stink imo

KLV keys could be given a similar treatment. Most importantly, the
entire index code could be made far more readable and robust. That's a
rather large task however, which I'm not going to undertake unless I
know I won't face major opposition

A continuation of the partition stuff attached is to remove
MXFContext::partitions and instead use an std::map in MXFCppContext for
the partitions themselves, not just their offsets. This would address
some performance issues with the present code for files with a large
number of partitions, such as mxfenc.c

/Tomas

[-- Attachment #2: 0001-lavf-mxfdec-Add-C-context.patch --]
[-- Type: text/x-patch, Size: 1880 bytes --]

From bda56e7d1259047e65c02cc40ab53728eedbfc6f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Wed, 22 Oct 2025 11:01:21 +0200
Subject: [PATCH 1/2] lavf/mxfdec: Add C++ context

---
 libavformat/mxfdec.cpp | 5 +++++
 libavformat/mxfdec.h   | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/libavformat/mxfdec.cpp b/libavformat/mxfdec.cpp
index 1b3c00c07c..522d870981 100644
--- a/libavformat/mxfdec.cpp
+++ b/libavformat/mxfdec.cpp
@@ -333,6 +333,9 @@ typedef struct MXFIndexTable {
     int8_t *offsets;            /* temporal offsets for display order to stored order conversion */
 } MXFIndexTable;
 
+struct MXFCppContext {
+};
+
 /* NOTE: klv_offset is not set (-1) for local keys */
 typedef int MXFMetadataReadFunc(void *arg, AVIOContext *pb, int tag, int size, UID uid, int64_t klv_offset);
 
@@ -3786,6 +3789,7 @@ int mxf_read_header(AVFormatContext *s)
     int ret;
     int64_t run_in;
 
+    mxf->cpp_context = new MXFCppContext();
     mxf->last_forward_tell = INT64_MAX;
 
     if (!mxf_read_sync(s->pb, mxf_header_partition_pack_key, 14)) {
@@ -4206,6 +4210,7 @@ int mxf_read_close(AVFormatContext *s)
         }
     }
     av_freep(&mxf->index_tables);
+    delete mxf->cpp_context;
 
     return 0;
 }
diff --git a/libavformat/mxfdec.h b/libavformat/mxfdec.h
index 3680e0a8ac..f4ad85fb46 100644
--- a/libavformat/mxfdec.h
+++ b/libavformat/mxfdec.h
@@ -41,6 +41,7 @@ typedef enum {
 struct MXFIndexTable;
 struct MXFMetadataSet;
 struct MXFPartition;
+struct MXFCppContext;
 
 typedef struct MXFMetadataSetGroup {
     struct MXFMetadataSet **metadata_sets;
@@ -71,6 +72,7 @@ typedef struct MXFContext {
     int nb_index_tables;
     struct MXFIndexTable *index_tables;
     int eia608_extract;
+    struct MXFCppContext *cpp_context; //< C++ context
 } MXFContext;
 
 int mxf_probe(const AVProbeData *p);
-- 
2.47.2


[-- Attachment #3: 0002-lavf-mxfdec-Switch-mxf_absolute_bodysid_offset-to-st.patch --]
[-- Type: text/x-patch, Size: 5104 bytes --]

From 682a647df0af169a673175eab13f99c7b58fffbd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Wed, 22 Oct 2025 12:28:32 +0200
Subject: [PATCH 2/2] lavf/mxfdec: Switch mxf_absolute_bodysid_offset() to
 std::map

This allows us to detect and reject evil files which have been constructed to have overlapping partitions
---
 libavformat/mxfdec.cpp | 90 +++++++++++++++++++++++++++++-------------
 1 file changed, 62 insertions(+), 28 deletions(-)

diff --git a/libavformat/mxfdec.cpp b/libavformat/mxfdec.cpp
index 522d870981..7a7b824e26 100644
--- a/libavformat/mxfdec.cpp
+++ b/libavformat/mxfdec.cpp
@@ -47,6 +47,9 @@
 #include <inttypes.h>
 #include <time.h>
 
+#include <algorithm>
+#include <map>
+
 extern "C" {
 #include "libavutil/aes.h"
 #include "libavutil/avstring.h"
@@ -334,6 +337,26 @@ typedef struct MXFIndexTable {
 } MXFIndexTable;
 
 struct MXFCppContext {
+    typedef std::map<int64_t, MXFPartition*> OffsetMap;      //< maps BodyOffset -> MXFPartition*
+    std::map<int, OffsetMap> bodysid_offset_partition_map;   //< maps (BodySID, BodyOffset) -> MXFPartition*
+
+    // comparator for whether a given offset is contained within [body_offset, body_offset + essence_length)
+    struct OffsetInPartitionComp {
+        bool operator() (const std::pair<int64_t, MXFPartition*> &a, int64_t b) const {
+            if (a.second->essence_length) {
+                // <= because the end of the range is exclusive
+                return a.second->body_offset + a.second->essence_length <= b;
+            } else {
+                // if essence_length == 0 then this partition spans the rest of the file
+                // this should only happen for the last partition
+                return false; // we can never be "less than" any given offset
+            }
+        }
+
+        bool operator() (int64_t a, const std::pair<int64_t, MXFPartition*> &b) const {
+            return a < b.second->body_offset;
+        }
+    };
 };
 
 /* NOTE: klv_offset is not set (-1) for local keys */
@@ -1880,35 +1903,40 @@ static int mxf_get_sorted_table_segments(MXFContext *mxf, int *nb_sorted_segment
  */
 static int mxf_absolute_bodysid_offset(MXFContext *mxf, int body_sid, int64_t offset, int64_t *offset_out, MXFPartition **partition_out)
 {
-    MXFPartition *last_p = NULL;
-    int a, b, m, m0;
-
-    if (offset < 0)
-        return AVERROR(EINVAL);
-
-    a = -1;
-    b = mxf->partitions_count;
-
-    while (b - a > 1) {
-        m0 = m = (a + b) >> 1;
-
-        while (m < b && mxf->partitions[m].body_sid != body_sid)
-            m++;
-
-        if (m < b && mxf->partitions[m].body_offset <= offset)
-            a = m;
-        else
-            b = m0;
-    }
-
-    if (a >= 0)
-        last_p = &mxf->partitions[a];
+    auto &partition_map = mxf->cpp_context->bodysid_offset_partition_map;
+    auto it = partition_map.find(body_sid);
+
+    if (it != partition_map.end()) {
+        // find range of partitions within which offset is contained
+        // there should be exactly one
+        auto [start, end] = std::equal_range(
+            it->second.begin(),
+            it->second.end(),
+            offset,
+            MXFCppContext::OffsetInPartitionComp()
+        );
+        auto d = std::distance(start, end);
+
+        if (d > 1) {
+            // this could happen for evil files - reject them
+            av_log(mxf->fc, AV_LOG_ERROR, "absolute offset %" PRIX64 " contained in more than one partition\n", offset);
+
+            // log offending partitions
+            for (; start != end; start++) {
+                MXFPartition *p = start->second;
+                av_log(mxf->fc, AV_LOG_ERROR, "BodyOffset %" PRIX64 " BodyOffset+essence_length %" PRIX64 "\n", p->body_offset, p->body_offset + p->essence_length);
+            }
 
-    if (last_p && (!last_p->essence_length || last_p->essence_length > (offset - last_p->body_offset))) {
-        *offset_out = last_p->essence_offset + (offset - last_p->body_offset);
-        if (partition_out)
-            *partition_out = last_p;
-        return 0;
+            return AVERROR_INVALIDDATA;
+        } else if (d == 1) {
+            MXFPartition *last_p = start->second;
+            if ((!last_p->essence_length || last_p->essence_length > (offset - last_p->body_offset))) {
+                *offset_out = last_p->essence_offset + (offset - last_p->body_offset);
+                if (partition_out)
+                    *partition_out = last_p;
+                return 0;
+            }
+        }
     }
 
     av_log(mxf->fc, AV_LOG_ERROR,
@@ -3899,6 +3927,12 @@ int mxf_read_header(AVFormatContext *s)
 
     mxf_compute_essence_containers(s);
 
+    // set up bodysid_offset_partition_map
+    for (int x = 0; x < mxf->partitions_count; x++) {
+        MXFPartition *partition = &mxf->partitions[x];
+        mxf->cpp_context->bodysid_offset_partition_map[partition->body_sid][partition->body_offset] = partition;
+    }
+
     for (int i = 0; i < s->nb_streams; i++)
         mxf_compute_edit_units_per_packet(mxf, s->streams[i]);
 
-- 
2.47.2


[-- Attachment #4: Type: text/plain, Size: 163 bytes --]

_______________________________________________
ffmpeg-devel mailing list -- ffmpeg-devel@ffmpeg.org
To unsubscribe send an email to ffmpeg-devel-leave@ffmpeg.org