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

I don't understand your patch.  Yes, I know the meaning of
"granulepos" depends on the codec.  I have read the specs for vorbis
and theora, and I can't see the difference you claim is there
(assuming you're talking about the new theora way; the old one was off
by one).

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