[FFmpeg-devel] [PATCH] fix av_url_read_fseek

[Howard Chu]

Michael Niedermayer wrote:
> On Sat, Apr 10, 2010 at 04:37:36PM -0700, Howard Chu wrote:
>> Michael Niedermayer wrote:
>>> On Sat, Apr 10, 2010 at 09:26:31AM -0700, Howard Chu wrote:
>>>> Index: libavformat/aviobuf.c
>>>> ===================================================================
>>>> --- libavformat/aviobuf.c	(revision 22813)
>>>> +++ libavformat/aviobuf.c	(working copy)
>>>> @@ -698,8 +698,11 @@
>>>>            return AVERROR(ENOSYS);
>>>>        ret = s->read_seek(h, stream_index, timestamp, flags);
>>>>        if(ret>= 0) {
>>>> +        int64_t pos;
>>>>            s->buf_ptr = s->buf_end; // Flush buffer
>>>> -        s->pos = s->seek(h, 0, SEEK_CUR);
>>>> +        pos = s->seek(h, 0, SEEK_CUR);
>>>> +        if (pos != AVERROR(ENOSYS))
>>>> +            s->pos = pos;
>>>>        }
>>>>        return ret;
>>>>    }
>>>
>>> this should be a seperate patch
>>
>> Protocols that implement read_seek are unlikely to also implement seek, so
>> this patch is needed to prevent the pos from getting set to a negative
>> value. Possibly it should just check for pos<= 0 instead of checking for
>> any specific error code. It's not clear that there's any real point to
>> calling seek here anyway, any stream that knows the value of SEEK_CUR would
>> have already updated itself while executing the read_seek.

> should that check not be<0 ?

Probably. As I already noted above, I have a lot of doubts about this chunk of 
code. Maybe better would be something like:

   if (pos >= 0)
      s->pos = pos
   else if (pos != AVERROR(ENOSYS))
      ret = pos;

I.e., if seek is actually implemented and fails, that probably should be 
reported to the caller.

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/