[FFmpeg-devel] [PATCH] Make sure AVFormatContext->start_time is initialized after the first patch

[Baptiste Coudurier]

On 06/02/2010 12:43 PM, Martin Storsj? wrote:
> On Wed, 2 Jun 2010, Baptiste Coudurier wrote:
>
>> On 06/02/2010 12:30 PM, Martin Storsj? wrote:
>>> On Wed, 2 Jun 2010, Baptiste Coudurier wrote:
>>>
>>>> On 06/02/2010 12:33 AM, Martin Storsj? wrote:
>>>>> On Fri, 28 May 2010, Martin Storsj? wrote:
>>>>>
>>>>>> On Fri, 28 May 2010, Baptiste Coudurier wrote:
>>>>>>
>>>>>>> On 05/25/2010 11:57 AM, Michael Niedermayer wrote:
>>>>>>>> On Tue, May 25, 2010 at 12:54:50AM +0300, Martin Storsj? wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> When discussing a ffserver patch with Baptiste, he insisted that
>>>>>>>>> if
>>>>>>>>> AVFormatContext->start_time isn't initialized after the first
>>>>>>>>> packet, it's
>>>>>>>>> a bug that should be fixed within libavformat, instead of
>>>>>>>>> handled in
>>>>>>>>> ffserver.
>>>>>>>>>
>>>>>>>>> This attached patch is one way of solving it, although I'm not
>>>>>>>>> sure
>>>>>>>>> if
>>>>>>>>> this is the correct way.
>>>>>>>>
>>>>>>>> The problem might be that ffserver calls av_seek_frame()
>>>>>>>> with that the first packet demuxed might no longer correspond to
>>>>>>>> the
>>>>>>>> first packet.
>>>>>>>> thus i think its not unreasonable if lavf doesnt set start_time to
>>>>>>>> the
>>>>>>>> first dts/pts once seeking has happened
>>>>>>>
>>>>>>> Hummm, av_find_stream_info should set the start_time however.
>>>>>>> Isn't av_find_stream_info called ?
>>>>>>
>>>>>> As far as I can see in open_input_stream, we first call
>>>>>> av_find_stream_info, then a few lines later do av_seek_frame.
>>>>>
>>>>> Baptiste, ping?
>>>>>
>>>>> With the current code:
>>>>>
>>>>>        if (ist->start_time != AV_NOPTS_VALUE)
>>>>>            c->cur_pts -= av_rescale_q(ist->start_time, ist->time_base,
>>>>> AV_TIME_BASE_Q);
>>>>>
>>>>> Either we assume that start_time shouldn't ever have the value
>>>>> AV_NOPTS_VALUE - in that case we should just remove the if and change it
>>>>> into an assert instead.
>>>>>
>>>>> If not, if there's even a remote chance that start_time could be
>>>>> AV_NOPTS_VALUE in some cases, this if statement absolutely needs an else
>>>>> branch. If not, the same problem (an absolute timestamp where a relative
>>>>> one was expected) will appear in the rest of ffserver somewhere.
>>>>
>>>> Then you need to set to set a similar field to ist->start_time in the
>>>> contact
>>>> and set it to the first pts you get with av_read_frame.
>>>
>>> Yes, there is such a field already, c->first_pts.
>>>
>>> Would the attached be ok, or should we skip using ist->start_time and only
>>> use c->first_pts?
>>
>> Humm is c->first_pts in ist->time_base ?
>
> No, it's in AV_TIME_BASE (as is c->cur_pts), it's initialized this way:
>
>      c->first_pts = av_rescale_q(pkt.dts, c->fmt_in->streams[pkt.stream_index]->time_base, AV_TIME_BASE_Q);
 >

So, it should be ok, and I think I agree that c->first_pts could be 
always used.

>> I'd like to know also why start_time isn't set in this case :)
>
> We've been through this a few times already.
>
> ffserver.c does a seek when it opens the input stream, which according to
> Michael may be the reason for why ist->start_time isn't initialized.

Hummm, quoting the discussion:
 >>>>>>> Hummm, av_find_stream_info should set the start_time however.
 >>>>>>> Isn't av_find_stream_info called ?
 >>>>>>
 >>>>>> As far as I can see in open_input_stream, we first call
 >>>>>> av_find_stream_info, then a few lines later do av_seek_frame.

av_find_stream_info _should_ set start_time, so you tell me 
av_seek_frame unset it ? It shouldn't.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org