[FFmpeg-devel] [PATCH 2/2] avformat/mxf: stop parsing if index table is coherent

[Tomas Härdin]

ons 2024-08-14 klockan 09:59 +0200 skrev Marc-Antoine Arnaud:
> sponsored by nxtedition
> ---
>  libavformat/mxfdec.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index df958819300..83f9e5fc9e0 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -2063,6 +2063,52 @@ static int mxf_compute_ptses_fake_index(MXFContext *mxf, MXFIndexTable *index_ta
>      return 0;
>  }
>  
> +static int mxf_validate_coherent_index_tables(MXFContext *mxf, int *is_coherent) {
> +      int i, j, ret, nb_sorted_segments = 0;
> +      MXFIndexTable *index_tables;
> +      int nb_index_tables = 0;
> +      int coherent_index_tables = 1;
> +      MXFIndexTableSegment **sorted_segments = NULL;
> +
> +      ret = mxf_get_sorted_table_segments(mxf, &nb_sorted_segments, &sorted_segments);

Should probably bail out here if ret != 0. It seems the intent is to
allow the caller to detect that there has been zero index segments so
far

> +
> +      index_tables = av_calloc(nb_index_tables, sizeof(MXFIndexTable));
> +      if (!index_tables) {
> +          av_log(mxf->fc, AV_LOG_ERROR, "failed to allocate index tables\n");
> +          av_free(sorted_segments);
> +          return AVERROR(ENOMEM);
> +      }
> +
> +      /* distribute sorted segments to index tables */
> +      for (i = j = 0; i < nb_sorted_segments; i++) {
> +          if (i != 0 && sorted_segments[i-1]->index_sid != sorted_segments[i]->index_sid) {
> +              /* next IndexSID */
> +              j++;
> +          }
> +
> +          index_tables[j].nb_segments++;
> +      }

This is copy-pasted from mxf_compute_index_tables. Consider maybe
splitting this part out into a common function.

> +
> +      for (i = j = 0; j < nb_index_tables; i += index_tables[j++].nb_segments) {
> +          if (sorted_segments[i]->index_start_position) {
> +              av_log(mxf->fc, AV_LOG_WARNING, "IndexSID %i starts at EditUnit %"PRId64" - seeking may not work as expected\n",
> +                     sorted_segments[i]->index_sid, sorted_segments[i]->index_start_position);
> +              coherent_index_tables = 0;
> +          }
> +      }

This looks like it just checks for index tables that have just one
segment. From the name of the function I'd have guessed that it looks
for contiguous segments that cover the entire duration of the track.
Doing so would be a good idea - then we know to stop even if we're
parsing the file backwards

Also you could break as soon as you set coherent_index_tables = 0

/Tomas