[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set
Ivan Schreter
schreter
Mon Feb 23 21:00:03 CET 2009
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).
If time_base is changed to 1/2, frame rate is 2x original frame rate
(i.e., 50fps instead of 25fps). Therefore I introduced ticks_per_frame,
so we have correct frame rate.
OTOH, as you write, effectively we can have various frame durations for
different frames (1-6 ticks)...
With just correcting time_base and timestamps, we have effectively 50fps
movie with each 2nd frame missing (well, not really, as each frame
duration is 2*time_base), which will get duplicated & encoded by ffmpeg
as separate picture. Resulting converted movie has 50fps, instead of 25.
Somehow we need to get the source to 25fps, which _is_ the correct frame
rate.
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), 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 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.
[...]
Regards,
Ivan
More information about the ffmpeg-devel
mailing list