[FFmpeg-devel] [PATCH] issue251, xvid within .ogm will not remux to .avi try2

[Måns Rullgård]

Michael Niedermayer wrote:
> On Wed, Jun 25, 2008 at 12:44:59PM +0100, M?ns Rullg?rd wrote:
>>
>> Michael Niedermayer wrote:
>> > Hi
>> >
>> > This patch fixes "according to spec" timestamp association for theora
>> > it also as a sideeffect fixes the file from issue 251
>> >
>> > quote of the theora spec: (A.2.2)
>> >    Frame data pages MUST be marked with a granule index corresponding to
>> > the display time of the last frame/packet that finishes in that page.
>> >
>> > Note ive not tested this extensively, i thought our users can do this
>> better
>> >
>> > Ill apply this in a few days if i hear no objections
>> >
>> >
>> > Index: libavformat/oggdec.c
>> > ===================================================================
>> > --- libavformat/oggdec.c	(revision 13799)
>> > +++ libavformat/oggdec.c	(working copy)
>> > @@ -523,11 +523,25 @@
>> >          return AVERROR(EIO);
>> >      pkt->stream_index = idx;
>> >      memcpy (pkt->data, os->buf + pstart, psize);
>> > +    if (s->streams[idx]->codec->codec_id == CODEC_ID_VORBIS){
>> >      if (os->lastgp != -1LL){
>> >          pkt->pts = ogg_gptopts (s, idx, os->lastgp);
>> >          os->lastgp = -1;
>> >      }
>> > +    }else{
>> > +        int segp= os->segp;
>> > +        int nsegs=os->nsegs;
>> > +        while (segp < nsegs){
>> > +            if (os->segments[segp] < 255)
>> > +                break;
>> > +            segp++;
>> > +        }
>> >
>> > +        if (segp == nsegs && os->granule != -1LL){
>> > +            pkt->pts = ogg_gptopts (s, idx, os->granule);
>> > +        }
>> > +    }
>> > +
>> >      pkt->flags = os->pflags;
>>
>> Can you please explain a little more why this change is needed, and
>> why it doesn't apply to Vorbis?
>
> As already quoted theora says:
>> > (A.2.2)
>> >    Frame data pages MUST be marked with a granule index corresponding to
>> > the display time of the last frame/packet that finishes in that page.
>
> vorbis (http://www.xiph.org/vorbis/doc/framing.html) says:
> --------
> absolute granule position
>
> (This is packed in the same way the rest of Ogg data is packed; LSb of LSB
> first. Note that the 'position' data specifies a 'sample' number (eg, in a
> CD quality sample is four octets, 16 bits for left and 16 bits for right;
> in video it would likely be the frame number. It is up to the specific codec
> in use to define the semantic meaning of the granule position value). The
> position specified is the total samples encoded after including all packets
> finished on this page (packets begun on this page but continuing on to the
> next page do not count). The rationale here is that the position specified
> in the frame header of the last page tells how long the data coded by the
> bitstream is. A truncated stream will still return the proper number of
> samples that can be decoded fully.
>
> A special value of '-1' (in two's complement) indicates that no packets
> finish on this page.
> --------
>
> Thus with theora
>
> page0       page1
> AAABB       BBCCC
>
> with theora the last frame finishing on page0 is A thus the granule pos from
> page0 applies to A and for page1 its B
>
> Iam not 100% certain what is correct for vorbis (iam still working on getting
> timestamps consistant with the number of samples returned by the decoder)
>
> But the vorbis spec sounds like one should associate B with page0 and
> C or D (when C is completed on page1) with page1

The way I read it, both "specs" are saying the same thing: that the
timestamp on a page refers to the last packet ending on that page.

It is quite possible that the code is wrong, but I don't think we need
special cases for these two codecs.

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