[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