[FFmpeg-devel] [PATCH] mxfdec: Parse IndexTableSegments and convert them into AVIndexEntry arrays

[Tomas Härdin]

On Sat, 2011-10-08 at 22:33 +0200, Georg Lippitsch wrote:
> Am 06.10.2011, 17:20 Uhr, schrieb Tomas Härdin <tomas.hardin at codemill.se>:
> 
> > I've split this patch in two parts per Baptiste's request.
> > The first part reads and converts the IndexTableSegments into
> > AVIndexEntry arrays while the second part implements the clip wrapping.
> 
>  From my perspective, your patches look fine!
> 
> > * handling audio duration % 8192 != 0
> 
> Actually I didn't like this kind of audio handling very much, so I wrote  
> another patch for this.
> After all, it avoids parsing the index table and furthermore generating  
> st->index_entries if there is only one table-segment, and if this segments  
> has no entries but only EditUnitByteCount set. It is meant to handle the  
> audio-files with small EditUnitByteCount correctly, and certainly also  
> saves much memory in this case.

Sounds reasonable I suppose. While I'm not the biggest fan of having
different code paths here, the proposed way is at least easy to
understand.

> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 1069167..06dda3f 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -187,6 +187,7 @@ typedef struct {
>      int64_t first_essence_length;
>      KLVPacket current_klv_data;
>      int current_klv_index;
> +    int single_eubc;
>  } MXFContext;
>  
>  enum MXFWrappingScheme {
> @@ -368,8 +369,11 @@ static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
>              klv = mxf->current_klv_data;
>              index = mxf->current_klv_index;
>              tr = s->streams[index]->priv_data;
> -            klv.length = FFMIN(s->streams[index]->index_entries[tr->current_sample_index].size,
> -                               mxf->current_klv_data.length);
> +            if (mxf->single_eubc)
> +                klv.length = FFMAX(mxf->single_eubc, 8192);

You should probably make sure klv.length % samples_per_byte == 0. 24-bit
could have a problem with this. For instance, PAL mono 24-bit 48 kHz ->
5760 B, meaning it'd pick 8192 which isn't divisible by 3.

Shouldn't this also do FFMIN() with current_klv_data.length just like
below?

> +            else
> +                klv.length = FFMIN(s->streams[index]->index_entries[tr->current_sample_index].size,
> +                                   mxf->current_klv_data.length);
>              mxf->current_klv_data.offset += klv.length;
>              mxf->current_klv_data.length -= klv.length;
>              av_dlog(s, "Clip-wrapped reading. klv.length=%"PRId64" current_sample_index=%d\n",
> @@ -398,9 +402,10 @@ static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
>              }
>              if (s->streams[index]->discard == AVDISCARD_ALL)
>                  goto skip;
> -            if (tr->current_sample_index < s->streams[index]->nb_index_entries &&
> +            if ((tr->current_sample_index < s->streams[index]->nb_index_entries &&
>                  klv.length > s->streams[index]->index_entries[tr->current_sample_index].size &&
> -                s->streams[index]->index_entries[tr->current_sample_index].size) {
> +                 s->streams[index]->index_entries[tr->current_sample_index].size) ||
> +                mxf->single_eubc) {
>                  mxf->current_klv_data = klv;
>                  mxf->current_klv_index = index;
>                  return mxf_read_packet(s, pkt);
> @@ -952,6 +957,14 @@ static int mxf_parse_index(MXFContext *mxf, int i, AVStream *st)
>      if ((ret = mxf_get_sorted_table_segments(mxf, &nb_sorted_segments, &sorted_segments)))
>          return ret;
>  
> +    if (nb_sorted_segments == 1 && !sorted_segments[0]->index_duration &&
> +        sorted_segments[0]->edit_unit_byte_count > 0)
> +    {
> +        mxf->single_eubc = sorted_segments[0]->edit_unit_byte_count;
> +        av_free(sorted_segments);
> +        return 0;
> +    }
> +
>      for (j = 0; j < nb_sorted_segments; j++) {
>          int n_delta = i;
>          int duration, sample_duration = 1, last_sample_size = 0;
> @@ -1447,13 +1460,23 @@ static int mxf_read_seek(AVFormatContext *s, int stream_index, int64_t sample_ti
>              index = 0;
>          if (index < 0)
>              return -1;
> -        seekpos =  st->index_entries[index].pos;
> +        seekpos = st->index_entries[index].pos;

Not intended?

>          av_update_cur_dts(s, st, st->index_entries[index].timestamp);
>          tr->current_sample_index = index;
>          if (!index) {
>              mxf->current_klv_data.length = 0;
>              mxf->current_klv_index = 0;
>          }
> +    } else if (mxf->single_eubc) {
> +        seekpos = mxf->single_eubc * sample_time + mxf->essence_offset;
> +        if (sample_time > 0 && seekpos)
> +            seekpos += mxf->first_essence_kl_length;
> +        av_update_cur_dts(s, st, sample_time);
> +        tr->current_sample_index = sample_time;
> +        if (!sample_time) {
> +            mxf->current_klv_data.length = 0;
> +            mxf->current_klv_index = 0;
> +        }

Looks reasonable.

/Tomas