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

[Baptiste Coudurier]

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,
personnally I would not try generic seeking if native failed, and I even
sent a patch for this.

Also I also wondered what was "timestamp" supposed to be, dts or pts ?
Because mpeg ps returns dts for read_timestamps.

>> 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.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Smartjog USA Inc.                                http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: url_fseek_fail.patch
Type: text/x-diff
Size: 788 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080727/3046b06b/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: seek_regression.patch
Type: text/x-diff
Size: 28938 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080727/3046b06b/attachment-0001.patch>