[FFmpeg-cvslog] r13061 - trunk/libavformat/avio.c

[Corey Hickey]

Michael Niedermayer wrote:
> On Wed, May 07, 2008 at 12:00:01PM -0700, Corey Hickey wrote:
>> michael wrote:
>>> Author: michael
>>> Date: Mon May  5 11:17:56 2008
>>> New Revision: 13061
>>>
>>> Log:
>>> Check url_seek() in url_open().
>>>
>>>
>>> Modified:
>>>    trunk/libavformat/avio.c
>>>
>>> Modified: trunk/libavformat/avio.c
>>> ==============================================================================
>>> --- trunk/libavformat/avio.c	(original)
>>> +++ trunk/libavformat/avio.c	Mon May  5 11:17:56 2008
>>> @@ -113,6 +113,12 @@ int url_open(URLContext **puc, const cha
>>>          *puc = NULL;
>>>          return err;
>>>      }
>>> +
>>> +    //We must be carefull here as url_seek() could be slow, for example for http
>>> +    if(   (flags & (URL_WRONLY | URL_RDWR))
>>> +       || !strcmp(proto_str, "file"))
>>> +        if(!uc->is_streamed && url_seek(uc, 0, SEEK_SET) < 0)
>>> +            uc->is_streamed= 1;
>>>      *puc = uc;
>>>      return 0;
>>>   fail:
>> This patch makes mencoder segfault with lavf muxing. I don't know if
>> mencoder or lavf should be changed, so I'm reporting it here as a start.
>>
>> Steps to reproduce:
>> $ mencoder -nosound -ovc copy -of lavf input.avi -o output.avi
>>
>> Here's what appears to be happening:
>> 1. The new code in avio.c calls url_seek()
>> 2. url_seek() calls a pointer to mp_seek()  (in muxer_lavf.c)
>> 3. mp_seek hits a null pointer dereference because muxer is null
>> ----however----
>> 4. muxer is set in muxer_lavf.c immediately below the code that calls
>>    libavformat and does what I described above.
>>
>> I can't just move that line that sets muxer because it's actually
>> storing muxer in a URLContext struct member, and the pointer to that
>> struct isn't set until after the code that now segfaults. Here's the
>> code in muxer_lavf.c I'm talking about:
>>
>> -------------------------------------------------------------------
>> if(url_fopen(&priv->oc->pb, mp_filename, URL_WRONLY))
>> {
>> 	mp_msg(MSGT_MUXER, MSGL_FATAL, "Could not open outfile\n");
>>         goto fail;
>> }
> 
>> ((URLContext*)(priv->oc->pb->opaque))->priv_data= muxer;
> 
> mp_open() should set this IMHO.
> (for example by using mp_filename "menc://%p" or a static var)

Thanks, but I don't understand what you mean. I've looked at the
relevant code and I think I can tell what it's doing, but I keep coming
back to what looks like a circular dependency.

If it's not much trouble for you, feel free to explain your solution by
committing a patch and letting me read -cvslog. ;)  Otherwise, the only
thing I see so far is to make mp_seek() return 0 if muxer is null,
assuming that will only happen in this case. I doubt that's a good
solution, though.

-Corey