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

[Baptiste Coudurier]

On 06/02/2010 01:17 PM, Martin Storsj? wrote:
> On Wed, 2 Jun 2010, Baptiste Coudurier wrote:
>
>> 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.
>
> Ok, so like in the attached, then.
>
> Howard, can you test run this and see if it seems to work fine for you,
> too? I'll apply it if you don't find any issues with it.
>
>>>> 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.
>
> Quoting Michael:
>
>> 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

_doesn't set_ does _not_ mean _unset_

start_time should be _set_ by av_find_stream_info _before_ the seek.

Did you check the _code_ ?
You have to know what's happening before modifying the current code.

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