[FFmpeg-devel] [PATCH] avoid infinite loop when seeking flv and non seekable input

[Michael Niedermayer]

On Sat, Jul 26, 2008 at 09:56:17PM -0700, Baptiste Coudurier wrote:
> Michael Niedermayer wrote:
> > On Wed, Jul 23, 2008 at 08:35:49PM -0700, Baptiste Coudurier wrote:
> >> Hi Michael,
> >>
> >> Michael Niedermayer wrote:
> >>> On Tue, Jul 22, 2008 at 07:41:51PM -0700, Baptiste Coudurier wrote:
> >>>> Hi,
> >>>>
> >>>> $subject,
> >>>>
> >>>> Reproduceable when trying to seek and input is non seekable (like http
> >>>> through ffserver).
> >>>>
> >>>> [flv @ 0x8e27eb0]skipping flv packet: type 32, size 2705408, flags 0
> >>>> [flv @ 0x8e27eb0]skipping flv packet: type 32, size 2763520, flags 0
> >>>> [flv @ 0x8e27eb0]skipping flv packet: type 32, size 2818048, flags 0
> >>>> [flv @ 0x8e27eb0]skipping flv packet: type 32, size 2856192, flags 0
> >>>> [flv @ 0x8e27eb0]skipping flv packet: type 32, size 2889728, flags 0
> >>>> [flv @ 0x8e27eb0]skipping flv packet: type 0, size 11469089, flags 0
> >>>> [flv @ 0x8e27eb0]skipping flv packet: type 32, size 2949120, flags 0
> >>>> [flv @ 0x8e27eb0]skipping flv packet: type 32, size 2987264, flags 0
> >>>>
> >>>> With patch resync happens faster:
> >>> I dont understand the problem, "trying to seek and input is non seekable"
> >>> makes no sense to me. If the input is unseekable, seeking wont succeed
> >>> anyway.
> >> It will it when seeking forward, aviobuf.c:
> >> } else if(s->is_streamed && !s->write_flag &&
> >>        offset1 >= 0 && offset1 < (s->buf_end - s->buffer) + (1<<16)){
> > 
> > indeed
> > this could be fixed by removing  "offset1 < (s->buf_end - s->buffer) + (1<<16)"
> > or increasing (1<<16)
> > feel free to change that.
> > 
> 
> I checked more deeply and here comes patches.
> 
> av_read_frame_flush needs to be done after seek in case of failure, this
> hunk will be applied before and separately.
> 
> url_fseek is checked for error, and when is_streamed is set but seeking
> forward is not possible, return error.

[...]

> Index: libavformat/utils.c
> ===================================================================
> --- libavformat/utils.c	(revision 14275)
> +++ libavformat/utils.c	(working copy)
> @@ -1430,7 +1430,7 @@
>  static int av_seek_frame_generic(AVFormatContext *s,
>                                   int stream_index, int64_t timestamp, int flags)
>  {
> -    int index;
> +    int index, ret;
>      AVStream *st;
>      AVIndexEntry *ie;
>  
> @@ -1445,7 +1445,8 @@
>          if(st->nb_index_entries){
>              assert(st->index_entries);
>              ie= &st->index_entries[st->nb_index_entries-1];
> -            url_fseek(s->pb, ie->pos, SEEK_SET);
> +            if ((ret = url_fseek(s->pb, ie->pos, SEEK_SET)) < 0)
> +                return ret;
>              av_update_cur_dts(s, st, ie->timestamp);
>          }else
>              url_fseek(s->pb, 0, SEEK_SET);

ok


> @@ -1465,14 +1466,14 @@
>              return 0;
>      }
>      ie = &st->index_entries[index];
> -    url_fseek(s->pb, ie->pos, SEEK_SET);
> -
> +    if ((ret = url_fseek(s->pb, ie->pos, SEEK_SET)) < 0)
> +        return ret;
>      av_update_cur_dts(s, st, ie->timestamp);
>  
>      return 0;

ok


> @@ -1465,14 +1466,14 @@
>      if (index < 0)
>          return -1;
>  
> -    av_read_frame_flush(s);
>      if (s->iformat->read_seek){
>          if(s->iformat->read_seek(s, stream_index, timestamp, flags) >= 0)
>              return 0;
>      }
>      ie = &st->index_entries[index];
>      if ((ret = url_fseek(s->pb, ie->pos, SEEK_SET)) < 0)
>          return ret;
> +    av_read_frame_flush(s);
>      av_update_cur_dts(s, st, ie->timestamp);
>  
>      return 0;

Does this pass the regression tests?
What concerns me is that av_read_frame_flush() is not done anymore when
s->iformat->read_seek is set and it succeeds



> Index: libavformat/aviobuf.c
> ===================================================================
> --- libavformat/aviobuf.c	(revision 14275)
> +++ libavformat/aviobuf.c	(working copy)
> @@ -149,13 +149,17 @@
>          offset1 >= 0 && offset1 < (s->buf_end - s->buffer)) {
>          /* can do the seek inside the buffer */
>          s->buf_ptr = s->buffer + offset1;
> -    } else if(s->is_streamed && !s->write_flag &&
> +    } else if(s->is_streamed) {
> +        if (!s->write_flag &&
>                offset1 >= 0 && offset1 < (s->buf_end - s->buffer) + (1<<16)){
>          while(s->pos < offset && !s->eof_reached)
>              fill_buffer(s);
>          if (s->eof_reached)
>              return AVERROR(EPIPE);
>          s->buf_ptr = s->buf_end + offset - s->pos;
> +        } else {
> +            return AVERROR(EPIPE);
> +        }
>      } else {
>          offset_t res = AVERROR(EPIPE);

This is not the correct solution i think.
I think the code in the else below may be buggy when seek() fails, it can
fail for non streamed things as well (network problems, old scratched cd...)
I think moving 
            s->buf_end = s->buffer;
        }
        s->buf_ptr = s->buffer;

after
if (!s->seek || (res = s->seek(s->opaque, offset, SEEK_SET)) < 0)
            return res;

might help ?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080727/a49a3cdc/attachment.pgp>