[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set

Michael Niedermayer michaelni
Mon Feb 23 23:33:06 CET 2009


On Mon, Feb 23, 2009 at 09:00:03PM +0100, Ivan Schreter wrote:
> Michael Niedermayer wrote:
> > On Sun, Feb 22, 2009 at 10:41:27PM +0100, Ivan Schreter wrote:
> >   
> >> Michael Niedermayer wrote:
> >>     
> >>> [...]
> >>>       
> >>> exporting
> >>> dts_last_dts_delta (cpb_removial_delay)
> >>> pts_dts_delta      (dpb_output_delay)
> >>> from the parser
> >>> seems nicer
> >>>   
> >>>       
> >> I named the fields a bit differently, but otherwise agreed. Also, buffering 
> >> period start needed to be exported.
> >>     
> >
> > i would prefer my suggested names but at least
> > "timestamp" without mentioning of dts vs pts is ambigous
> >
> >   
> OK, I'll rework them.
> 
> > [...]
> >   
> >> Index: libavcodec/options.c
> >> ===================================================================
> >> --- libavcodec/options.c	(revision 17520)
> >> +++ libavcodec/options.c	(working copy)
> >> @@ -431,6 +431,7 @@
> >>  
> >>      s->palctrl = NULL;
> >>      s->reget_buffer= avcodec_default_reget_buffer;
> >> +    s->ticks_per_frame = 1;
> >>  }
> >>  
> >>  AVCodecContext *avcodec_alloc_context2(enum CodecType codec_type){
> >> Index: libavcodec/avcodec.h
> >> ===================================================================
> >> --- libavcodec/avcodec.h	(revision 17520)
> >> +++ libavcodec/avcodec.h	(working copy)
> >> @@ -30,7 +30,7 @@
> >>  #include "libavutil/avutil.h"
> >>  
> >>  #define LIBAVCODEC_VERSION_MAJOR 52
> >> -#define LIBAVCODEC_VERSION_MINOR 18
> >> +#define LIBAVCODEC_VERSION_MINOR 19
> >>  #define LIBAVCODEC_VERSION_MICRO  0
> >>  
> >>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> >> @@ -2315,6 +2315,14 @@
> >>       * - decoding: unused.
> >>       */
> >>      float rc_min_vbv_overflow_use;
> >> +
> >> +    /**
> >> +     * For some codecs, time base is not equal to frame time, but an integer fraction thereof.
> >> +     * Most notably, H.264 specifies time_base as half of frame duration.
> >> +     *
> >> +     * Set to time_base ticks per frame. Default 1, e.g., H.264 sets it to 2.
> >> +     */
> >> +    int ticks_per_frame;
> >>  } AVCodecContext;
> >>     
> >
> > i cannot accept this, it is just wrong
> > fps is not 1/tb nor a multiple of it fps and the duration of frames can
> > change tb can not, all frame times are specified in tb units but these dont
> > have to be constant.
> > if you know that each frame makes 2 tb steps then you could change tb
> > fact that you cannot shows this isnt true, h264 could have frames 
> > with 3 "tick" durations.
> >   
> Yes. See below.
> 
> >
> > [...]
> >   
> >> Index: libavcodec/h264_parser.c
> >> ===================================================================
> >> --- libavcodec/h264_parser.c	(revision 17520)
> >> +++ libavcodec/h264_parser.c	(working copy)
> >> @@ -196,29 +206,29 @@
> >>                  switch (h->sei_pic_struct) {
> >>                      case SEI_PIC_STRUCT_TOP_FIELD:
> >>                      case SEI_PIC_STRUCT_BOTTOM_FIELD:
> >> -                        s->repeat_pict = -1;
> >> +                        s->repeat_pict = 0;
> >>                          break;
> >>                      case SEI_PIC_STRUCT_FRAME:
> >>                      case SEI_PIC_STRUCT_TOP_BOTTOM:
> >>                      case SEI_PIC_STRUCT_BOTTOM_TOP:
> >> -                        s->repeat_pict = 0;
> >> +                        s->repeat_pict = 2;
> >>                          break;
> >>                      case SEI_PIC_STRUCT_TOP_BOTTOM_TOP:
> >>                      case SEI_PIC_STRUCT_BOTTOM_TOP_BOTTOM:
> >> -                        s->repeat_pict = 1;
> >> +                        s->repeat_pict = 4;
> >>                          break;
> >>                      case SEI_PIC_STRUCT_FRAME_DOUBLING:
> >> -                        s->repeat_pict = 2;
> >> +                        s->repeat_pict = 6;
> >>                          break;
> >>                      case SEI_PIC_STRUCT_FRAME_TRIPLING:
> >> -                        s->repeat_pict = 4;
> >> +                        s->repeat_pict = 10;
> >>                          break;
> >>                      default:
> >> -                        s->repeat_pict = h->s.picture_structure == PICT_FRAME ? 0 : -1;
> >> +                        s->repeat_pict = h->s.picture_structure == PICT_FRAME ? 2 : 0;
> >>                          break;
> >>                  }
> >>              } else {
> >> -                s->repeat_pict = h->s.picture_structure == PICT_FRAME ? 0 : -1;
> >> +                s->repeat_pict = h->s.picture_structure == PICT_FRAME ? 2 : 0;
> >>              }
> >>  
> >>              return 0; /* no need to evaluate the rest */
> >>     
> >
> > i understand why you change repeat_pict but this is majorly ugly
> > repeat_pict should be specified in tb units not tb/2 units
> >
> >   
> Hm, yes, why not. Provided the rest works.
> 
> > also ive just tried to change tb & repeat_pict in mpeg2 and the regression
> > tests passed so iam not sure what problems you speak of (mpeg2 patch below,
> > its just a quick thing not well tested ...)
> >
> > [...]
> Doesn't work for H.264. Frame rate is computed directly from time_base, 
> in contrast to MPEG-2, where it's found out by looking at durations of 
> single frames (tb_unreliable).

[...]
> What do you suggest?
> 
> Setting tb_unreliable for H.264 works for progressive (correct 
> framerate), frame doubling (halving framerate) and frame tripling (1/3 
> of framerate),

yes, i suggest setting tb_unreliable for H.264


> but not for interlaced and only partially for TBT and BTB 
> pictures. Reason: interlaced frame has time_base duration and we don't 
> communicate "field flag" to lavf. If we define duration of interlaced 
> frame == duration of frame (instead of 1/2 frame), then it will work. Is 
> this in your opinion OK?

my oppinion is that having r_frame_rate = 50 for a 25fps interlaced or
TBT/BTB video is correct.

also frame duplication as ffmpeg does depends on the value of -vsync
change it to 2 or 0 and the frame dups are gone. This should work fine
with all common container formats, and i wanted to change the default
already a few times.
Maybe a simple AVFMT_VARIABLE_FPS could be added and then vsync
default changed to 2 for containers having that flag set.
If this would help i can add that flag and change ffmpeg.c.


> 
> My opinion: The movie has frame rate of 2*time_base. Single frames can 
> have various durations, but the base frame rate _is_ 2*time_base, so 
> ticks_per_frame approach is probably the best and cleanest (of course, 
> I'd change repeat_pict to use tb instead of tb/2, so only minimal 
> adjustments are necessary in repeat_pict computation).
> 
> > Index: libavcodec/mpeg12.c
> > ===================================================================
> > --- libavcodec/mpeg12.c	(revision 17533)
> > +++ libavcodec/mpeg12.c	(working copy)
> > @@ -1274,7 +1274,7 @@
> >              av_reduce(
> >                  &s->avctx->time_base.den,
> >                  &s->avctx->time_base.num,
> > -                ff_frame_rate_tab[s->frame_rate_index].num * s1->frame_rate_ext.num,
> > +                ff_frame_rate_tab[s->frame_rate_index].num * s1->frame_rate_ext.num*2,
> >                  ff_frame_rate_tab[s->frame_rate_index].den * s1->frame_rate_ext.den,
> >                  1<<30);
> >          //MPEG-2 aspect
> > Index: libavcodec/mpegvideo_parser.c
> > ===================================================================
> > --- libavcodec/mpegvideo_parser.c	(revision 17533)
> > +++ libavcodec/mpegvideo_parser.c	(working copy)
> > @@ -81,7 +81,7 @@
> >                          pc->height |=( vert_size_ext << 12);
> >                          avctx->bit_rate += (bit_rate_ext << 18) * 400;
> >                          avcodec_set_dimensions(avctx, pc->width, pc->height);
> > -                        avctx->time_base.den = pc->frame_rate.den * (frame_rate_ext_n + 1);
> > +                        avctx->time_base.den = pc->frame_rate.den * (frame_rate_ext_n + 1) * 2;
> >                          avctx->time_base.num = pc->frame_rate.num * (frame_rate_ext_d + 1);
> >                          avctx->codec_id = CODEC_ID_MPEG2VIDEO;
> >                          avctx->sub_id = 2; /* forces MPEG2 */
> >   
> r_frame_rate is computed for MPEG-2 from durations, so change in 
> time_base is neutral. I suppose, it won't work for MPEG-1, though, 
> except if tb_unreliable returns by chance 1 for it as well.

my changes in this patch only change mpeg2 because only mpeg2 allows
interlaced stuff ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20090223/e2231954/attachment.pgp>



More information about the ffmpeg-devel mailing list