[FFmpeg-devel] [PATCH] issue251, xvid within .ogm will not remux to .avi try2
Michael Niedermayer
michaelni
Thu Jun 26 13:50:47 CEST 2008
On Thu, Jun 26, 2008 at 02:11:21AM +0100, M?ns Rullg?rd wrote:
> 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).
without my patch:
[ogg @ 0x8507064]av_read_packet stream=0, pts=-9223372036854775808, dts=-9223372036854775808, size=21398, flags=0
[ogg @ 0x8507064]av_read_frame_internal stream=0, pts=610483, dts=610483, size=21398, flags=1
[ogg @ 0x8507064]av_read_packet stream=0, pts=610484, dts=-9223372036854775808, size=1087, flags=1
[ogg @ 0x8507064]av_read_frame_internal stream=0, pts=610484, dts=610484, size=1087, flags=0
with my patch:
[ogg @ 0x8507104]av_read_frame_internal stream=0, pts=610483, dts=610483, size=2831, flags=0
[ogg @ 0x8507104]av_read_packet stream=0, pts=610484, dts=-9223372036854775808, size=21398, flags=1
[ogg @ 0x8507104]av_read_frame_internal stream=0, pts=610484, dts=610484, size=21398, flags=1
[ogg @ 0x8507104]av_read_packet stream=0, pts=-9223372036854775808, dts=-9223372036854775808, size=1087, flags=0
svn marks the wrong frames as keyframes. This might also be related to why
reimars seek patch does not work.
So if you dislike my patch, could you please tell us how you want this
fixed?
And about things being off by 1, i do not think this is true. It does depend
on how packets are split over pages.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- 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/20080626/4909706b/attachment.pgp>
More information about the ffmpeg-devel
mailing list