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

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Wed, Jun 25, 2008 at 02:23:00PM +0100, M?ns Rullg?rd wrote:
>> 
>> 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.
>
> After various bugfixes, i now get timestamps which are consistant in
> relation to the number of samples output by the decoder with oggdec.c
> svn and vorbis. The theora timestamp code does break vorbis timestamps.
>
> So i think my patch is correct. And ill apply it in a few days unless
> you object or i stumble accross an issue with it.

I object as long as I don't understand it, and I can't see how the
specs are really saying anything differently.  If there is a
difference in behaviour, the bug must be elsewhere.

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