[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: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

// Martin