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

[Martin Storsjö]

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);

> 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.

And yes, avoiding to do a seek when it isn't strictly needed could fix 
this particular case, but would just make the problem appear less often. 
This if statement either needs an else branch, or needs to use 
c->first_pts completely.

// Martin