[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