[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set
Michael Niedermayer
michaelni
Mon Feb 23 04:02:16 CET 2009
On Sun, Feb 22, 2009 at 10:41:27PM +0100, Ivan Schreter wrote:
> Michael Niedermayer wrote:
>> On Sat, Feb 21, 2009 at 01:15:46PM +0100, Ivan Schreter wrote:
>>
>>> Michael Niedermayer wrote:
>>>
>>>> [...] fps != timebase and if a video specifes 1/50 thats what
>>>> should be stored in
>>>> time_base.
>>>> That might break something but its that something that is broken not
>>>> setting
>>>> tb correctly.
>>>> And i dont think we can fix h264 if we keep a fundamental field like the
>>>> timebase set incorrectly.
>>>> also lavf / ffmpeg has and uses r_frame_rate for the real "base" frame
>>>> rate
>>>> not AVCodecContext.time_base
>>>>
>>> ... which is computed from AVCodecContext.time_base.
>>>
>>
>> the code in av_find_stream_info analyzes frame durations to set
>> r_frame_rate
>> of course there may be small adjustments needed but it should not really
>> require special handling of h264 ...
>>
> Well, no. It computes it from time_base directly, at least from formats
> with reliable TB.
>
> I added tick_per_frame field, which allows the codec to specify how many
> time_base ticks per frame are done, so frame rate can be computed
> correctly. Even ffmpeg.c checks, whether frame rate matches that of the
> codec, so I used the new field there as well.
>
> Patch avcodec_tick_per_frame attached.
>
> However, H.264 MOV doesn't work with it (gets 1/2 frame rate), so for now
> just pre-review. I'm afraid, we'll have to set H.264 time_base actually
> "wrong" and use ticks_per_frame for timestamp computation instead as
> additional divisor. This makes the code also simpler... What do you think?
>
>>> So we'd have to special-case H.264 to compute r_frame_rate properly. OK
>>> with you or any better idea?
>>>
>>>
> With aforementioned patch generalized, so no problem anymore.
>
>> [...]
>>>
>>>> Besides i belive it is better to export information to let the outer
>>>> part use it the way it likes instead of sucking things in and changing
>>>> them.
>>>> Also fields like repeat_pict are exported and used outside lavc so this
>>>> is not really different from existing code.
>>>>
>>> Would something like this be acceptable?
>>>
>>> 1) add parser_dts and parser_pts in AVCodecParserContext, defaulting to
>>> AV_NOPTS_VALUE (alternatively, instead of parser_pts we could use
>>> parser_delay relative to parser_dts)
>>>
>>
>> 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
[...]
> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c (revision 17520)
> +++ ffmpeg.c (working copy)
> @@ -2876,11 +2878,11 @@
> if(me_threshold)
> enc->debug |= FF_DEBUG_MV;
>
> - if (enc->time_base.den != rfps || enc->time_base.num != rfps_base) {
> + if (enc->time_base.den != rfps || enc->time_base.num * enc->ticks_per_frame != rfps_base) {
>
> if (verbose >= 0)
> fprintf(stderr,"\nSeems stream %d codec frame rate differs from container frame rate: %2.2f (%d/%d) -> %2.2f (%d/%d)\n",
> - i, (float)enc->time_base.den / enc->time_base.num, enc->time_base.den, enc->time_base.num,
> + i, (float)enc->time_base.den / (enc->time_base.num * enc->ticks_per_frame), enc->time_base.den, enc->time_base.num * enc->ticks_per_frame,
>
> (float)rfps / rfps_base, rfps, rfps_base);
> }
this change has no point, its just affecting a printf()
> 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.
[...]
> 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
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 ...)
Index: libavcodec/avcodec.h
===================================================================
--- libavcodec/avcodec.h (revision 17533)
+++ libavcodec/avcodec.h (working copy)
@@ -3010,12 +3010,9 @@
* It signals, how much longer the frame duration of the current frame
* is compared to normal frame duration.
*
- * frame_duration = (2 + repeat_pict) / (2*fps)
+ * frame_duration = (1 + repeat_pict) * tb
*
* It is used by codecs like H.264 to display telecined material.
- *
- * @note This field can also be set to -1 for half-frame duration in case
- * of field pictures.
*/
int repeat_pict; /* XXX: Put it back in AVCodecContext. */
int64_t pts; /* pts of the current frame */
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 */
@@ -94,15 +94,16 @@
repeat_first_field = buf[3] & (1 << 1);
progressive_frame = buf[4] & (1 << 7);
+ s->repeat_pict = 1;
/* check if we must repeat the frame */
if (repeat_first_field) {
if (pc->progressive_sequence) {
if (top_field_first)
- s->repeat_pict = 4;
+ s->repeat_pict = 5;
else
- s->repeat_pict = 2;
+ s->repeat_pict = 3;
} else if (progressive_frame) {
- s->repeat_pict = 1;
+ s->repeat_pict = 2;
}
}
}
Index: libavformat/utils.c
===================================================================
--- libavformat/utils.c (revision 17538)
+++ libavformat/utils.c (working copy)
@@ -680,10 +680,7 @@
*pnum = st->codec->time_base.num;
*pden = st->codec->time_base.den;
if (pc && pc->repeat_pict) {
- // NOTE: repeat_pict can be also -1 for half-frame durations,
- // e.g., in H.264 interlaced field picture stream
- *pden *= 2;
- *pnum = (*pnum) * (2 + pc->repeat_pict);
+ *pnum = (*pnum) * (1 + pc->repeat_pict);
}
}
break;
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/66fae51a/attachment.pgp>
More information about the ffmpeg-devel
mailing list