[FFmpeg-devel] [PATCH] fix av_url_read_fseek

[Howard Chu]

Michael Niedermayer wrote:
> On Sun, Apr 11, 2010 at 04:17:41PM -0700, Howard Chu wrote:
>> Michael Niedermayer wrote:
>>> On Sat, Apr 10, 2010 at 06:57:09PM -0700, Howard Chu wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Sat, Apr 10, 2010 at 04:37:36PM -0700, Howard Chu wrote:
>>>>>> 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.
>>>
>>> what you say sounds reasonable at first glance
>>
>> --
>>    -- Howard Chu
>>    CTO, Symas Corp.           http://www.symas.com
>>    Director, Highland Sun     http://highlandsun.com/hyc/
>>    Chief Architect, OpenLDAP  http://www.openldap.org/project/
>
>> Index: libavformat/aviobuf.c
>> ===================================================================
>> --- libavformat/aviobuf.c	(revision 22813)
>> +++ libavformat/aviobuf.c	(working copy)
>> @@ -698,8 +698,13 @@
>>           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>= 0)
>> +            s->pos = pos;
>> +        else if (pos != AVERROR(ENOSYS))
>> +            ret = pos;
>>       }
>
> looks ok if it works&  doesnt break any of the reg/fate tests

make test succeeds, make fate fails on nsv-demux. But it fails identically 
without this patch too, so that appears to be unrelated.

--- /home/software/ffmpeg/tests/ref/fate/nsv-demux	2010-03-18 
14:16:23.000000000 -0700
+++ tests/data/fate/nsv-demux	2010-04-12 08:03:53.000000000 -0700
@@ -44,7 +44,7 @@
  1, 120668, 105, 0x6b9c2ed5
  1, 125370, 104, 0xce6c3b04
  0, 126126, 532, 0xd5650f54
-1, 130073, 105, 0x31db399e
+1, 130072, 105, 0x31db399e
  1, 134775, 104, 0xd50b347a
  0, 138138, 476, 0x77f1f3a7
  1, 139477, 105, 0xe87734d6
@@ -69,7 +69,7 @@
  0, 204204, 196, 0x1ff16161
  1, 206844, 105, 0xedc4374a
  0, 210210, 176, 0x9b455230
-1, 211547, 104, 0x0b3938d2
+1, 211546, 104, 0x0b3938d2
  0, 216216, 156, 0xbbbf4bf3
  1, 216249, 105, 0xb2653246
  1, 220951, 104, 0x76333479
@@ -105,10 +105,10 @@
  1, 314991, 105, 0xd9233422
  0, 318318, 340, 0x7229a1b7
  1, 319693, 104, 0x5c603350
-1, 324396, 105, 0x76e631bc
-1, 329098, 104, 0x657e3b35
+1, 324395, 105, 0x76e631bc
+1, 329097, 104, 0x657e3b35
  0, 330330, 280, 0x48948b60
-1, 333800, 105, 0x9d283226
+1, 333799, 105, 0x9d283226
  1, 338502, 104, 0x574936ef
  0, 342342, 304, 0x3ae68dcf
  1, 343204, 105, 0x1b923555
make: *** [fate-nsv-demux] Error 1

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