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

[Michael Niedermayer]

On Wed, Jun 25, 2008 at 07:25:46PM +0100, M?ns Rullg?rd wrote:
> 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, 

Ogg says granulepos is defined by the codec spec, and thats exactly what they
do


> 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.

What kind of logic is that? You dont understand it but you "know" they match.
That even in the light that the ogg spec explicitly says:
     Its meaning is
      dependent on the codec for that logical bitstream and specified in
      a specific media mapping.


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080625/949febd7/attachment.pgp>