[FFmpeg-devel] [PATCH v2] avformat/mxfdec: Remove this_partition

[FFmpeg-devel] [PATCH v2] avformat/mxfdec: Remove this_partition

[Michael Niedermayer]

Suggested-by: Tomas Härdin <git@haerdin.se>

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mxfdec.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index e6118e141d..0553728253 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -100,7 +100,6 @@ typedef struct MXFPartition {
     uint64_t previous_partition;
     int index_sid;
     int body_sid;
-    int64_t this_partition;
     int64_t essence_offset;         ///< absolute offset of essence
     int64_t essence_length;
     int32_t kag_size;
@@ -725,10 +724,14 @@ 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;
+    uint64_t this_partition;
 
     if (mxf->partitions_count >= INT_MAX / 2)
         return AVERROR_INVALIDDATA;
 
+    if (klv_offset < mxf->run_in)
+        return AVERROR_INVALIDDATA;
+
     tmp_part = av_realloc_array(mxf->partitions, mxf->partitions_count + 1, sizeof(*mxf->partitions));
     if (!tmp_part)
         return AVERROR(ENOMEM);
@@ -771,7 +774,13 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size
     partition->complete = uid[14] > 2;
     avio_skip(pb, 4);
     partition->kag_size = avio_rb32(pb);
-    partition->this_partition = avio_rb64(pb);
+    this_partition = avio_rb64(pb);
+    if (this_partition != klv_offset - mxf->run_in) {
+        av_log(mxf->fc, AV_LOG_WARNING,
+               "this_partition %"PRId64" mismatches %"PRId64"\n",
+               this_partition, klv_offset - mxf->run_in);
+        this_partition = klv_offset - mxf->run_in;
+    }
     partition->previous_partition = avio_rb64(pb);
     footer_partition = avio_rb64(pb);
     partition->header_byte_count = avio_rb64(pb);
@@ -791,8 +800,8 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size
         av_dict_set(&s->metadata, "operational_pattern_ul", str, 0);
     }
 
-    if (partition->this_partition &&
-        partition->previous_partition == partition->this_partition) {
+    if (this_partition &&
+        partition->previous_partition == this_partition) {
         av_log(mxf->fc, AV_LOG_ERROR,
                "PreviousPartition equal to ThisPartition %"PRIx64"\n",
                partition->previous_partition);
@@ -800,11 +809,11 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size
         if (!mxf->parsing_backward && mxf->last_forward_partition > 1) {
             MXFPartition *prev =
                 mxf->partitions + mxf->last_forward_partition - 2;
-            partition->previous_partition = prev->this_partition;
+            partition->previous_partition = prev->pack_ofs - mxf->run_in;
         }
         /* if no previous body partition are found point to the header
          * partition */
-        if (partition->previous_partition == partition->this_partition)
+        if (partition->previous_partition == this_partition)
             partition->previous_partition = 0;
         av_log(mxf->fc, AV_LOG_ERROR,
                "Overriding PreviousPartition with %"PRIx64"\n",
@@ -826,7 +835,7 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size
             "PartitionPack: ThisPartition = 0x%"PRIX64
             ", PreviousPartition = 0x%"PRIX64", "
             "FooterPartition = 0x%"PRIX64", IndexSID = %i, BodySID = %i\n",
-            partition->this_partition,
+            this_partition,
             partition->previous_partition, footer_partition,
             partition->index_sid, partition->body_sid);
 
@@ -900,7 +909,7 @@ static uint64_t partition_score(MXFPartition *p)
         score = 3;
     else
         score = 1;
-    return (score << 60) | ((uint64_t)p->this_partition >> 4);
+    return (score << 60) | ((uint64_t)p->pack_ofs >> 4);
 }
 
 static int mxf_add_metadata_set(MXFContext *mxf, MXFMetadataSet **metadata_set)
@@ -3520,14 +3529,14 @@ static void mxf_compute_essence_containers(AVFormatContext *s)
 
             /* essence container spans to the next partition */
             if (x < mxf->partitions_count - 1)
-                p->essence_length = mxf->partitions[x+1].this_partition - p->essence_offset;
+                p->essence_length = mxf->partitions[x+1].pack_ofs - mxf->run_in - p->essence_offset;
 
             if (p->essence_length < 0) {
                 /* next ThisPartition < essence_offset */
                 p->essence_length = 0;
                 av_log(mxf->fc, AV_LOG_ERROR,
                        "partition %i: bad ThisPartition = %"PRIX64"\n",
-                       x+1, mxf->partitions[x+1].this_partition);
+                       x+1, mxf->partitions[x+1].pack_ofs - mxf->run_in);
             }
         }
     }
-- 
2.17.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/mxfdec: Remove this_partition

[Tomas Härdin]

lör 2022-12-24 klockan 20:36 +0100 skrev Michael Niedermayer:
> @@ -725,10 +724,14 @@ 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;
> +    uint64_t this_partition;
>  
>      if (mxf->partitions_count >= INT_MAX / 2)
>          return AVERROR_INVALIDDATA;
>  
> +    if (klv_offset < mxf->run_in)
> +        return AVERROR_INVALIDDATA;

av_assert0() might be more appropriate since this should never happen
regardless of input

> @@ -771,7 +774,13 @@ static int mxf_read_partition_pack(void *arg,
> AVIOContext *pb, int tag, int size
>      partition->complete = uid[14] > 2;
>      avio_skip(pb, 4);
>      partition->kag_size = avio_rb32(pb);
> -    partition->this_partition = avio_rb64(pb);
> +    this_partition = avio_rb64(pb);
> +    if (this_partition != klv_offset - mxf->run_in) {
> +        av_log(mxf->fc, AV_LOG_WARNING,
> +               "this_partition %"PRId64" mismatches %"PRId64"\n",
> +               this_partition, klv_offset - mxf->run_in);
> +        this_partition = klv_offset - mxf->run_in;

This could even be outside the if. It turns into a nop if the file is
already good. Not a huge issue either way.

> @@ -900,7 +909,7 @@ static uint64_t partition_score(MXFPartition *p)
>          score = 3;
>      else
>          score = 1;
> -    return (score << 60) | ((uint64_t)p->this_partition >> 4);
> +    return (score << 60) | ((uint64_t)p->pack_ofs >> 4);

Noting that while this changes the scores computed, it doesn't change
the intended behavior of the code. In fact it should make it more
correct in cases where ThisPartition are bogus values.

>  }
>  
>  static int mxf_add_metadata_set(MXFContext *mxf, MXFMetadataSet
> **metadata_set)
> @@ -3520,14 +3529,14 @@ static void
> mxf_compute_essence_containers(AVFormatContext *s)
>  
>              /* essence container spans to the next partition */
>              if (x < mxf->partitions_count - 1)
> -                p->essence_length = mxf-
> >partitions[x+1].this_partition - p->essence_offset;
> +                p->essence_length = mxf->partitions[x+1].pack_ofs -
> mxf->run_in - p->essence_offset;

I wonder if there's perhaps an underflow issue lurking here. Not that
this patch changes that

/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".