[FFmpeg-devel] [PATCH 05/28] support seeking in nuv files

[Reimar Döffinger]

On Wed, Jun 30, 2010 at 10:09:33AM +0100, Mans Rullgard wrote:
> From: Cory Fields <theuni-nospam- at xbmc.org>
> 
> ---
>  libavformat/nuv.c |   84 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 84 insertions(+), 0 deletions(-)
> 
> diff --git a/libavformat/nuv.c b/libavformat/nuv.c
> index f0eacd5..2aa0bb4 100644
> --- a/libavformat/nuv.c
> +++ b/libavformat/nuv.c
> @@ -218,12 +218,18 @@ static int nuv_packet(AVFormatContext *s, AVPacket *pkt) {
>                  ret = av_new_packet(pkt, copyhdrsize + size);
>                  if (ret < 0)
>                      return ret;
> +
>                  // HACK: we have no idea if it is a keyframe,
>                  // but if we mark none seeking will not work at all.
>                  pkt->flags |= AV_PKT_FLAG_KEY;
>                  pkt->pos = pos;
>                  pkt->pts = AV_RL32(&hdr[4]);
>                  pkt->stream_index = ctx->v_id;
> +                if(hdr[2] == 0) {
> +                    AVStream *st = s->streams[pkt->stream_index];
> +                    pkt->flags |= PKT_FLAG_KEY;
> +                    av_add_index_entry(st, pkt->pos, pkt->pts, size + HDRSIZE, 0, AVINDEX_KEYFRAME);
> +                }

Setting PKT_FLAG_KEY _twice_ sure won't make it set any more.
Also, the generic code is supposed to handle adding index entries in this way.
And indeed we have
>      .flags = AVFMT_GENERIC_INDEX,
So this means we end having index entries double.
Now, using hdr[2] to indicate keyframes looks like a good idea, but I
tried that originally and there are just too many files around where it
is not set or not set correctly.

> @@ -257,6 +263,83 @@ static int nuv_packet(AVFormatContext *s, AVPacket *pkt) {
>      return AVERROR(EIO);
>  }
>  
> +/**
> + * \brief looks for the string RTjjjjjjjjjj in the stream too resync reading
> + * \return TRUE if the syncword is found.

Uh, no, it returns 1, not TRUE

> +static int nuv_resync(AVFormatContext *s, int64_t pos_limit) {
> +    ByteIOContext *pb = s->pb;
> +    uint32_t tag;
> +
> +    tag = get_be32(pb);
> +    while(!url_feof(pb) && url_ftell(pb) < pos_limit) {
> +        if(tag != MKBETAG('R','T','j','j')) {
> +            tag = (tag << 8) | get_byte(pb);
> +            continue;
> +        }
> +        tag = get_be32(pb);
> +        if(tag != MKBETAG('j','j','j','j'))
> +          continue;
> +
> +        tag = get_be32(pb);
> +        if(tag != MKBETAG('j','j','j','j'))
> +          continue;
> +
> +        return 1;
> +    }

This calls url_ftell a bit often.
I'd suggest something like
uint64_t tag = get_be64(pb);
while (!url_feof(pb) && url_ftell(pb) < pos_limit) {
    int i = 0;
    if ((tag & 0xffffff) != AV_RB24("jjj")) {
        tag = (tag << 32) | get_be32(pb);
        continue;
    }
    while (i++ < 3 && (tag >> 32) != AV_RB32("RTjj"))
        tag = (tag << 8) | get_byte(pb);
    if (tag != AV_RB64("RTjjjjjj"))
        continue;
    tag = (tag << 32) | get_be32(pb);
    if ((uint32_t)tag != AV_RB32("jjjj"))
        continue;
    return 1;
}
return 0;

> +/**
> + * \brief attempts to read a timestamp from stream at the given stream position
> + * \return timestamp if successfull and AV_NOPTS_VALUE if failure
> + */
> +static int64_t nuv_read_dts(AVFormatContext *s, int stream_index,
> +                               int64_t *ppos, int64_t pos_limit)

If the description and other places use the term "read timestamp" it makes
sense to call the function "nuv_read_timestamp".

> +    NUVContext *ctx = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    uint8_t hdr[HDRSIZE];
> +    nuv_frametype frametype;
> +    int size;
> +    int64_t pos, dts;
> +
> +    if (url_fseek(pb, *ppos, SEEK_SET) < 0)
> +        return AV_NOPTS_VALUE;

Is there any reason we can't do this in common code?
Like this we can't even pass the error value up...

> +    while (!url_feof(pb) && url_ftell(pb) < pos_limit) {
> +        if (get_buffer(pb, hdr, HDRSIZE) <= 0)

I think anything < HDRSIZE is a problem...

> +                // TODO - add general support in av_gen_search, so it adds positions after reading timestamps
> +                av_add_index_entry(s->streams[frametype == NUV_VIDEO ? ctx->v_id : ctx->a_id]
> +                                 , pos, dts, size + HDRSIZE, 0, hdr[2] == 0 ? AVINDEX_KEYFRAME : 0);

Unless this makes a large speed difference I am against this.