[FFmpeg-devel] [PATCH] increase AVStream pts_buffer size

[Michael Niedermayer]

On Sat, Aug 09, 2008 at 05:34:51PM -0700, Baptiste Coudurier wrote:
> Baptiste Coudurier wrote:
> > M?ns Rullg?rd wrote:
> >> Baptiste Coudurier <baptiste.coudurier at smartjog.com> writes:
> >>
> >>> Hi,
> >>>
> >>> H264 decoder can (now?) set has_b_frames to MAX_DELAYED_PIC_COUNT which
> >>> is 16, need to port this to pts_buffer in AVStream struct.
> >>>
> >>> compute_pkt_files in utils.c:
> >>> int delay = FFMAX(st->codec->has_b_frames, !!st->codec->max_b_frames);
> >>>
> >>> [...]
> >>>
> >>> //calculate dts from pts
> >>> if(pkt->pts != AV_NOPTS_VALUE && pkt->dts == AV_NOPTS_VALUE){
> >>>     st->pts_buffer[0]= pkt->pts;
> >>>     for(i=1; i<delay+1 && st->pts_buffer[i] == AV_NOPTS_VALUE; i++)
> >>>         st->pts_buffer[i]= (i-delay-1) * pkt->duration;
> >>>     for(i=0; i<delay && st->pts_buffer[i] > st->pts_buffer[i+1]; i++)
> >>>         FFSWAP(int64_t, st->pts_buffer[i], st->pts_buffer[i+1]);
> >>>      pkt->dts= st->pts_buffer[0];
> >>> }
> >>>
> >>> Patch attached.
> >>>
> >>> Index: libavformat/avformat.h
> >>> ===================================================================
> >>> --- libavformat/avformat.h	(revision 14513)
> >>> +++ libavformat/avformat.h	(working copy)
> >>> @@ -385,7 +385,7 @@
> >>>  
> >>>      int64_t nb_frames;                 ///< number of frames in this stream if known or 0
> >>>  
> >>> -#define MAX_REORDER_DELAY 4
> >>> +#define MAX_REORDER_DELAY 16
> >>>      int64_t pts_buffer[MAX_REORDER_DELAY+1];
> >>>  
> >>>      char *filename; /**< source filename of the stream */
> >> This breaks ABI, so it needs a version bump.  Others can have an
> >> opinion on how bad that is.
> >>
> > 
> > Right, we can avoid going too far in the mean time to avoid breaking
> > ABI, it is not the right solution I believe but it should fix the bug,
> > patch attached.
> > 
> > 
> > 
> > ------------------------------------------------------------------------
> > 
> > Index: libavformat/utils.c
> > ===================================================================
> > --- libavformat/utils.c	(revision 14513)
> > +++ libavformat/utils.c	(working copy)
> > @@ -2472,7 +2472,7 @@
> >      //calculate dts from pts
> >      if(pkt->pts != AV_NOPTS_VALUE && pkt->dts == AV_NOPTS_VALUE){
> >          st->pts_buffer[0]= pkt->pts;
> > -        for(i=1; i<delay+1 && st->pts_buffer[i] == AV_NOPTS_VALUE; i++)
> > +        for(i=1; i<delay+1 && i < MAX_REORDER_DELAY+1 && st->pts_buffer[i] == AV_NOPTS_VALUE; i++)
> >              st->pts_buffer[i]= (i-delay-1) * pkt->duration;
> >          for(i=0; i<delay && st->pts_buffer[i] > st->pts_buffer[i+1]; i++)
> >              FFSWAP(int64_t, st->pts_buffer[i], st->pts_buffer[i+1]);
> > 
> 
> Any comments ? This fixes a bug :>

too fe hours each day ... :)

i think there ae 2 issues here.
first is that MAX_REORDER_DELAY is too small, that can be fixed adding a
new pts_buffer at the end of AVStream that is bigger and putting the old
one under #if VERSION<...

the second is that the code is missing a check for delay >= MAX_REORDER_DELAY
i think your patch is correct but not enough as the next loop looks like it
could still go over the end.

maybe the following would be enoughm but ive not tested it
    }

-    if(pkt->pts != AV_NOPTS_VALUE){
+    if(pkt->pts != AV_NOPTS_VALUE && delay <= MAX_REORDER_DELAY){
        st->pts_buffer[0]= pkt->pts;
        for(i=1; i<delay+1 && st->pts_buffer[i] == AV


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20080810/8ec37518/attachment.pgp>