[FFmpeg-devel] [PATCH] avoid infinite loop when seeking flv and non seekable input
Baptiste Coudurier
baptiste.coudurier
Wed Jul 30 23:34:37 CEST 2008
Michael Niedermayer wrote:
> 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
Yes, [Ffmpeg-devel] [RFC] av_seek_frame behaviour, 21/04/07, indeed I
let this rotting for some time....
My last enumeration of seeking behaviour is still valid, and indeed your
remove flv seek function on 14/04/08, sorry for that.
>> 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!
I'll try to code something, but Im not sure if for now mpeg seeking is
really problematic, is it ?
>>>> 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.
>
Ok, I'll dig further.
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
Smartjog USA Inc. http://www.smartjog.com
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
More information about the ffmpeg-devel
mailing list