[Ffmpeg-devel] [PATCH] Add True Audio Seeking Support

[Måns Rullgård]

"Masahiro Kiyota" <hiro3726 at gmail.com> writes:

> Hi,
>
> The attached patch enables applications using libavc/libavf as backend
> (e.g. mplayer) to seek True Audio streams.
>
> In order to enable seeking true audio stream in mplayer, I made some
> changes to tta_read_header() and tta_read_packet() in addition to
> implementing tta_read_seek().
>
> Tested on mplayer and ffplay.
>
> With friendly regards,
> Masahiro
>
> --- libavformat/tta.c.orig      2007-03-30 22:41:59.000000000 +0900
> +++ libavformat/tta.c   2007-03-30 22:54:45.000000000 +0900
> @@ -21,8 +21,10 @@
>  #include "avformat.h"
>  #include "bitstream.h"
>  
> +#define FRAME_TIME 1.04489795918367346939

WTF is this?  I think it deserves a comment at the least.  I also
suspect it ought really be a fraction.

>  typedef struct {
> -    int totalframes, currentframe;
> +    int totalframes, currentframe, firstpos;
>      uint32_t *seektable;
>  } TTAContext;
>  
> @@ -36,6 +38,16 @@ static int tta_probe(AVProbeData *p)
>      return 0;
>  }
>  
> +static uint32_t get_pos(uint32_t *seek_table, int frm)
> +{
> +    uint32_t *st, ret = 0;
> +    

Trailing whitespace.

> +    for (st = seek_table; st < (seek_table + frm); st += 1) {

Useless parens.  Use st++ instead.

> +        ret += *st;
> +    }
> +    return ret;
> +}

This is inefficient.  It could be calculated once along with the
seektable array instead?  Alternatively, to save some memory, store
only the offsets and calculate the sizes when needed.

>  static int tta_read_header(AVFormatContext *s, AVFormatParameters *ap)
>  {
>      TTAContext *c = s->priv_data;
> @@ -64,7 +76,7 @@ static int tta_read_header(AVFormatConte
>  
>      url_fskip(&s->pb, 4); // header crc
>  
> -    framelen = 1.04489795918367346939 * samplerate;
> +    framelen = FRAME_TIME * samplerate;

Things like this should really be done as separate patches.

>      c->totalframes = datalen / framelen + ((datalen % framelen) ? 1 : 0);
>      c->currentframe = 0;
>  
> @@ -99,7 +111,10 @@ static int tta_read_header(AVFormatConte
>      st->codec->extradata = av_mallocz(st->codec->extradata_size+FF_INPUT_BUFFER_PADDING_SIZE);
>      url_fseek(&s->pb, start, SEEK_SET); // or SEEK_CUR and -size ? :)
>      get_buffer(&s->pb, st->codec->extradata, st->codec->extradata_size);
> -
> +    

Trailing whitespace.

> +    c->firstpos = start + st->codec->extradata_size;
> +    s->start_time = 0;
> +    s->duration = FRAME_TIME * c->totalframes * AV_TIME_BASE; // not precise?

If you made FRAME_TIME a fraction this could probably be made
precise.  In fact, the stream timebase should be set to whatever that
fraction is.  Then duration will simply be the frame count.

>      return 0;
>  }
>  
> @@ -109,18 +124,18 @@ static int tta_read_packet(AVFormatConte
>      int ret, size;
>  
>      // FIXME!
> -    if (c->currentframe > c->totalframes)
> +    if (c->currentframe >= c->totalframes)
>          size = 0;
>      else
>          size = c->seektable[c->currentframe];
>  
> -    c->currentframe++;
> -
>      if (av_new_packet(pkt, size) < 0)
>          return AVERROR_IO;
>  
>      pkt->pos = url_ftell(&s->pb);
>      pkt->stream_index = 0;
> +    pkt->pts = (c->currentframe > 0) ? (FRAME_TIME * (c->currentframe) / av_q2d(s->streams[0]->time_base)) : 1;

This looks convoluted.  It could be simplified if the proper timebase
was set instead.

> +    

Trailing whitespace.

>      ret = get_buffer(&s->pb, pkt->data, size);
>      if (ret <= 0) {
>          av_free_packet(pkt);
> @@ -129,6 +144,8 @@ static int tta_read_packet(AVFormatConte
>      pkt->size = ret;
>  //    av_log(s, AV_LOG_INFO, "TTA packet #%d desired size: %d read size: %d at pos %d\n",
>  //        c->currentframe, size, ret, pkt->pos);
> +    c->currentframe++;
> +
>      return 0; //ret;
>  }
>  
> @@ -140,6 +157,25 @@ static int tta_read_close(AVFormatContex
>      return 0;
>  }
>  
> +static int tta_read_seek(AVFormatContext *s, int stream_index, int64_t pts, int flags) 

Trailing whitespace.

> +{
> +    TTAContext *c = s->priv_data;
> +    uint32_t to_frm, seek_pos;
> +    

Trailing whitespace.

> +    if (c->seektable) {
> +        pts = (pts > 0) ? pts : 0;

PTS is usually unsigned, no?

> +        to_frm = (uint32_t)(pts * av_q2d(s->streams[0]->time_base) / FRAME_TIME);

Yuck.  Just set the timebase and be done with it.

> +        if (to_frm >= c->totalframes) return -1; // beyond the end of stream

Put the return -1 on a new line.

> +        

Trailing whitespace.

> +        seek_pos = get_pos(c->seektable, to_frm) + c->firstpos;

See above.

> +//        av_log(s, AV_LOG_INFO, "TTA read_seek time: %lf, to_frame: %d (pos: %x)\n", pts * av_q2d(s->streams[0]->time_base), to_frm, seek_pos);
> +        

Trailing whitespace.

> +        url_fseek(&s->pb, seek_pos, SEEK_SET);
> +        c->currentframe = to_frm;
> +    }
> +    return 0;
> +}
> +
>  AVInputFormat tta_demuxer = {
>      "tta",
>      "true-audio",
> @@ -148,5 +184,6 @@ AVInputFormat tta_demuxer = {
>      tta_read_header,
>      tta_read_packet,
>      tta_read_close,
> +    tta_read_seek,
>      .extensions = "tta",
>  };

I suggest first cleaning up the timebase mess in patch of its own
before adding seeking.

-- 
M?ns Rullg?rd
mans at mansr.com