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

[Måns Rullgård]

Henrik Gulbrandsen <henrik at gulbra.net> writes:

> This problem showed up when I was getting ready to deliver my next patch
> yesterday. In effect, the duration calculation for Ogg streams seems to
> drop the first and last frame. This would be true for all Ogg data, but
> it's only noticeable for video at a low frame rate.
>
> The attached examples show the result of
>
>     ffmpeg -i video.mp4 -vcodec libtheora -f ogg video.ogv
>     ffplay -stats video.ogv > report.txt 2>&1
>
> before and after the patch. Here, video.mp4 is exactly 24 seconds long,
> at 10 fps. Notice the "Duration: 00:00:23.80, start: 0.100000" part in
> the stats given with current code!
>
> /Henrik
>
> P.S. The "granule + 1" part of the patch is not technically correct, but
> should work for Theora, Vorbis, FLAC, and Speex, since none of them use
> overly complex semantics for the granule position. A more correct way of
> handling it would require extra code in each of the supported codecs...
>
>
>
>
> Index: libavformat/oggdec.c
> ===================================================================
> --- libavformat/oggdec.c	(revision 12884)
> +++ libavformat/oggdec.c	(working copy)
> @@ -307,6 +307,7 @@ ogg_packet (AVFormatContext * s, int *st
>      ogg_stream_t *os;
>      int complete = 0;
>      int segp = 0, psize = 0;
> +    uint64_t start = url_ftell(s->pb);
>  
>  #if 0
>      av_log (s, AV_LOG_DEBUG, "ogg_packet: curidx=%i\n", ogg->curidx);
> @@ -368,6 +369,7 @@ ogg_packet (AVFormatContext * s, int *st
>      if (os->header < 0){
>          int hdr = os->codec->header (s, idx);
>          if (!hdr){
> +          url_fseek (s->pb, start, SEEK_SET);
>            os->header = os->seq;
>            os->segp = segp;
>            os->psize = psize;

This looks very wrong.

> @@ -463,14 +465,14 @@ ogg_get_length (AVFormatContext * s)
>  
>      if (idx != -1){
>          s->streams[idx]->duration =
> -            ogg_gptopts (s, idx, ogg->streams[idx].granule);
> +            ogg_gptopts (s, idx, ogg->streams[idx].granule + 1);
>      }

Baptiste is right, the last value gives the length of the stream.

>      ogg->size = size;
>      ogg_restore (s, 0);
>      ogg_save (s);
>      while (!ogg_read_page (s, &i)) {
> -        if (i == idx && ogg->streams[i].granule != -1 && ogg->streams[i].granule != 0)
> +        if (i == idx && ogg->streams[i].granule != -1)
>              break;
>      }
>      if (i == idx) {

Why?

-- 
M?ns Rullg?rd
mans at mansr.com