[FFmpeg-devel] avformat/mxfdec: Remove this_partition

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

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

On Fri, 23 Dec 2022, Michael Niedermayer wrote:

> Suggested-by: Tomas Härdin <git@haerdin.se>
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavformat/mxfdec.c | 37 +++++++++++++++++++++++--------------
> 1 file changed, 23 insertions(+), 14 deletions(-)

this_partition should be == pack_ofs - run_in, not pack_ofs + run_in.

>
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index e6118e141d..cbacd03d1e 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 < 0 || klv_offset > INT64_MAX - RUN_IN_MAX)
> +        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);
> 
> @@ -887,7 +896,7 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size
>     return 0;
> }
> 
> -static uint64_t partition_score(MXFPartition *p)
> +static uint64_t partition_score(MXFContext *mxf, MXFPartition *p)
> {
>     uint64_t score;
>     if (!p)
> @@ -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 + mxf->run_in >> 4);
> }

You can use pack_ofs here directly instead of this_partition, so the 
function prototype does not have to be changed.

Regards,
Marton

> 
> static int mxf_add_metadata_set(MXFContext *mxf, MXFMetadataSet **metadata_set)
> @@ -3244,10 +3253,10 @@ static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
>     { { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 }, NULL, 0, AnyType },
> };
> 
> -static int mxf_metadataset_init(MXFMetadataSet *ctx, enum MXFMetadataSetType type, MXFPartition *partition)
> +static int mxf_metadataset_init(MXFContext *mxf, MXFMetadataSet *ctx, enum MXFMetadataSetType type, MXFPartition *partition)
> {
>     ctx->type = type;
> -    ctx->partition_score = partition_score(partition);
> +    ctx->partition_score = partition_score(mxf, partition);
>     switch (type){
>     case MultipleDescriptor:
>     case Descriptor:
> @@ -3272,7 +3281,7 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF
>         if (!meta)
>             return AVERROR(ENOMEM);
>         ctx  = meta;
> -        mxf_metadataset_init(meta, type, mxf->current_partition);
> +        mxf_metadataset_init(mxf, meta, type, mxf->current_partition);
>     } else {
>         meta = NULL;
>         ctx  = mxf;
> @@ -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".

On Fri, Dec 23, 2022 at 11:01:55PM +0100, Marton Balint wrote:
> 
> 
> On Fri, 23 Dec 2022, Michael Niedermayer wrote:
> 
> > Suggested-by: Tomas Härdin <git@haerdin.se>
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> > libavformat/mxfdec.c | 37 +++++++++++++++++++++++--------------
> > 1 file changed, 23 insertions(+), 14 deletions(-)
> 
> this_partition should be == pack_ofs - run_in, not pack_ofs + run_in.
> 
[...]
> > @@ -887,7 +896,7 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size
> >     return 0;
> > }
> > 
> > -static uint64_t partition_score(MXFPartition *p)
> > +static uint64_t partition_score(MXFContext *mxf, MXFPartition *p)
> > {
> >     uint64_t score;
> >     if (!p)
> > @@ -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 + mxf->run_in >> 4);
> > }
> 
> You can use pack_ofs here directly instead of this_partition, so the
> function prototype does not have to be changed.

ok will repost with these 2 changes

thx

[...]