[FFmpeg-devel] [PATCH] Incorrect Ogg Theora duration

[Henrik Gulbrandsen]

On Sat, 2008-04-26 at 12:52 +0100, M?ns Rullg?rd wrote:
> Henrik Gulbrandsen <henrik at gulbra.net> writes:

[...]

> > Index: libavformat/oggdec.c
> > ===================================================================
> > --- libavformat/oggdec.c	(revision 12969)
> > +++ libavformat/oggdec.c	(working copy)
> > @@ -468,15 +468,34 @@ ogg_get_length (AVFormatContext * s)
> >  
> >      ogg->size = size;
> >      ogg_restore (s, 0);
> > +    if (idx == -1)
> > +        return 0;
> > +
> >      ogg_save (s);
> > +
> > +    // Read the first stream packet if not already done
> > +    if (idx != ogg->curidx) {
> > +        while (!ogg_read_page (s, &i)) {
> > +            if (i == idx && ogg->streams[i].granule != -1)
> > +                break;
> > +        }
> > +    }
> > +
> > +    // Read the second stream packet
> >      while (!ogg_read_page (s, &i)) {
> >          if (i == idx && ogg->streams[i].granule != -1 && ogg->streams[i].granule != 0)
> >              break;
> >      }
> > +
> >      if (i == idx) {
> > -        s->streams[idx]->start_time = ogg_gptopts (s, idx, ogg->streams[idx].granule);
> > +        // Calulate start time assuming same duration for the two packets
> 
> That's one hell of an assumption.

...but the best we can do without decoding the packet, if we're not
allowed to rely on start_time being 0 in the first place...

> > +        ogg_stream_t *os = ogg->streams + idx;
> > +        int64_t pkt2_pts = ogg_gptopts(s, idx, os->lastgp);
> > +        int64_t duration = ogg_gptopts(s, idx, os->granule) - pkt2_pts;
> > +        s->streams[idx]->start_time = FFMAX(0, pkt2_pts - duration);
> >          s->streams[idx]->duration -= s->streams[idx]->start_time;
> >      }
> > +
> >      ogg_restore (s, 0);
> >  
> >      return 0;
> 
> The Ogg spec actually mandates a start time of zero for all streams:

As far as I'm concerned, feel free to rip out this entire part of the
code and set the start time to zero! That would solve all my problems.
However, somebody went through a lot of effort to handle a case where
it's apparently not guaranteed to be zero. I assume it involves audio
extracted from somewhere in the middle of a streamed recording, but that
is not explicitly stated anywhere. As long as this code is needed, I
want it to also work correctly for the common case where start_time
actually is zero, instead of always assuming non-zero as it does now.

/Henrik