[FFmpeg-devel] [PATCH] avoid infinite loop when seeking flv and non seekable input
Michael Niedermayer
michaelni
Wed Jul 30 16:10:13 CEST 2008
On Sun, Jul 27, 2008 at 05:08:47PM -0700, Baptiste Coudurier wrote:
> Michael Niedermayer wrote:
> > 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.
> >
> > [...]
> >
> >> @@ -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?
>
> Yes.
>
> > What concerns me is that av_read_frame_flush() is not done anymore when
> > s->iformat->read_seek is set and it succeeds
>
> Yes, but I don't understand the code either, iformat->read_seek will be
> called before
> calling av_seek_frame_generic anyway, and it would have failed.
>
> It seems to me that seeking internals needs to be cleaned up,
yes
> personnally I would not try generic seeking if native failed, and I even
> sent a patch for this.
hmm, my memory is rusty, do you remember the subject or date?
Maybe the fallback should be removed but id like to first check up the
past discussions
>
> Also I also wondered what was "timestamp" supposed to be, dts or pts ?
> Because mpeg ps returns dts for read_timestamps.
This is all rather tricky
read_timestamps() is used for binary searching, which requires the file
position - timestamp to be monotone, pts are not monotone due to B frames.
thus as it is it has to return dts or it will behave somewhat random.
But then it doesnt behave very well for mpeg anyway because of timestamp
discontinuities.
So for mpeg we really should do some sanity checks about monotonicity
and the byte vs timestamp distance the final seek achived, and if it looks
odd fall back to seeking based on strict CBR assumption.
or
we could try to first seek based on strict CBR assumption, that is
current_pos + (wanted_time - curren_time)*bitrate
then fetch a dts from the new position and apply above formular again
and if the new seek would be over a shorter distance than the previos
repeat. If not assume that we went over a timestamp discontinuity and
just accept the current position.
patch very welcome for this!
>
> >> 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 ?
> >
>
> I see, it works but seek regression tests fail:
> Patch and regression diff attached.
hmm, i guess there are more bugs in the code that need to be fixed first.
I would guess they are seeks that dont check the return value and previously
failed due to the messed up internal byteio state.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- 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/20080730/875a5b20/attachment.pgp>
More information about the ffmpeg-devel
mailing list