[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set
Michael Niedermayer
michaelni
Wed Feb 18 00:08:40 CET 2009
On Tue, Feb 17, 2009 at 09:57:32PM +0100, Ivan Schreter wrote:
> Hi *,
>
> so, now it's complete. Proper H.264 timestamp support can be added to
> FFmpeg by attached set of patches.
>
> First some trivial patches as prerequisites:
>
> #1 (h264_timing_1_init_cpb_del_len): Parse initial_cpb_removal_delay_length
> (needed for SEI buffering period).
ok
> #2 (h264_timing_2_output_delay): Parse dpb_output_delay in SEI picture
> structure (needed for timestamp calculation).
ok
> #3 (h264_timing_3_cpb_removal_delay): Parse cbp_removal_delay in SEI
> picture structure (needed for timestamp calculation; depends on #2).
ok
> #4 (h264_timing_4_cpb_cnt): Store CPB count in the context (needed for SEI
> buffering period).
see comments below
> #5 (h264_timing_5_field_flags): Parse field_pic_flag and bottom_field_flag
> (needed for timestamp calculation).
see comments below
> #6 (h264_default_sps): Parsing SPS doesn't assign the just-parsed SPS as
> current SPS on the context. Thus, decoding of SEIs (especially SEI
> buffering period) depending on current SPS wouldn't work. Assign
> just-parsed SPS as current SPS for the context.
this is wrong, buffering period contains a sps_id selecting the sps it
doesnt depend on the "current sps"
>
> To properly generate H.264 timestamps, we need to get some information from
> SEI buffering period. Until now, it was not parsed. Nontrivial patch to add
> it:
>
> #7 (h264_timing_6_buffering_period): Parse SEI buffering period (depends on
> #1-#4 and #6).
see comments below
>
> The actual timing code is implemented in the parser. First of all, we need
> to have the parser to actually parse frame header to get relevant
> information. Following patches add parsing of header NAL units to the
> parser:
>
> #8 (h264_parser_1_funcs): Make some relevant h264.c functions visible to
> h264_parser.c. Already OK-ed by Michael as prereq for the rest of parsing
> code (after it is OK-ed).
as you say already ok
> #9 (h264_parser_2_parser): Add parsing code to parse basic information
> about the frame (depends on #8). Already in review in another thread.
as you say, its in a seperate thread, so no review here
>
> Now, key frames are handled differently in H.264 than in MPEG-2. An I-frame
> is not necessarily a key frame. To address that, an additional flag must be
> passed to lavf so it knows about key frames:
>
> #10 (avformat_1_keyframe): Add two new flags on the parser context. One to
> tell lavf to handle key frames properly (i.e., independent of pict_type)
> and one to actually signalize key frame. If the first flag is not present,
> old (workaround) method of detecting key frames via I-frames will be used.
> When all parsers are migrated to the new flag, key frame detection via
> I-frames can be dropped completely. Nice thing is, this change is forward
> and backward binary compatible.
see comment below
> #11 (h264_parser_3_keyframe): Actually set the key frame flag for H.264 for
> IDR frames and for frames having SEI recovery point (initialization routine
> to set key frame flag presence flag must be added here as well; depends on
> #8-#10).
>
> H.264 timestamp computation also needs field_pic_flag:
>
> #12 (h264_parser_4_field_flags): Add parsing of field flags (depends on
> #8-#11).
comments below
>
> And finally:
>
> #13 (h264_parser_5_timestamps): Correctly compute H.264 timestamps based on
> cpb_removal_delay, dpb_output_delay and various other bits from #1-#12.
> Unfortunately, H.264 doesn't have notion about "timestamps" as such, only
> about delays. So the timestamps from container format (e.g., MPEG-TS) are
> taken as the basis to compute initial timestamp for first frame in
> buffering period. Only if DTS is not available from the format, initial
> delay from H.264 is used to start the timestamp generator as a fallback (of
> course, in this case, after a seek the timestamps will diverge, as they
> will start from arbitrary small timestamp).
comments below
!!!also testing of the whole patchset is very welcome!!!
if some volunteer could test this against the h264 from roundup that have
timestamp issues this wouldbe great.
(its easier to review code that i know works compared to code that i do not
know if it works with more than files from a single cammera)
[...]
> Index: libavcodec/h264.c
> ===================================================================
> --- libavcodec/h264.c (revision 17130)
> +++ libavcodec/h264.c (working copy)
> @@ -6920,6 +6959,7 @@
> sps->cpb_removal_delay_length = get_bits(&s->gb, 5) + 1;
> sps->dpb_output_delay_length = get_bits(&s->gb, 5) + 1;
> sps->time_offset_length = get_bits(&s->gb, 5);
> + h->cpb_cnt = cpb_count;
> return 0;
> }
are you sure this belongs in h instead of sps ?
[...]
> Index: libavcodec/h264.h
> ===================================================================
> --- libavcodec/h264.h (revision 17130)
> +++ libavcodec/h264.h (working copy)
> @@ -355,6 +355,8 @@
> int delta_poc_bottom;
> int delta_poc[2];
> int frame_num;
> + int field_pic_flag; ///< if 1 found a field picture
> + int bottom_field_flag; ///< if 1 found bottom field, 0 top field
> int prev_poc_msb; ///< poc_msb of the last reference pic for POC type 0
> int prev_poc_lsb; ///< poc_lsb of the last reference pic for POC type 0
> int frame_num_offset; ///< for POC type 2
this is redundant,
h->field_pic_flag == (s->picture_structure != PICT_FRAME)
h->bottom_field_flag == (s->picture_structure != PICT_TOP_FIELD)
[...]
> @@ -6859,6 +6860,37 @@
> return 0;
> }
>
> +static int decode_buffering_period(H264Context *h){
> + MpegEncContext * const s = &h->s;
> + int sps_id;
> + int sched_sel_idx;
> + SPS *sps;
> +
> + sps_id = get_ue_golomb_31(&s->gb);
this is missing a validity check (<32 i suspect but didnt check)
also as this would have been possibly exploitable, please be carefull not to
miss such checks
> + if(!h->sps_buffers[sps_id]) {
> + av_log(h->s.avctx, AV_LOG_ERROR, "non-existing SPS %d referenced in buffering period\n", sps_id);
> + return -1;
> + }
> + sps = h->sps_buffers[sps_id];
> +
> + // NOTE: This is really so duplicated in the standard... See H.264, D.1.1
> + if (sps->nal_hrd_parameters_present_flag) {
> + for (sched_sel_idx = 0; sched_sel_idx < h->cpb_cnt; sched_sel_idx++) {
> + h->initial_cpb_removal_delay[sched_sel_idx] = get_bits(&s->gb, sps->initial_cpb_removal_delay_length);
> + h->initial_cpb_removal_delay_offset[sched_sel_idx] = get_bits(&s->gb, sps->initial_cpb_removal_delay_length);
vertical align
> + }
> + }
> + if (sps->vcl_hrd_parameters_present_flag) {
> + for (sched_sel_idx = 0; sched_sel_idx < h->cpb_cnt; sched_sel_idx++) {
> + h->initial_cpb_removal_delay[sched_sel_idx] = get_bits(&s->gb, sps->initial_cpb_removal_delay_length);
> + h->initial_cpb_removal_delay_offset[sched_sel_idx] = get_bits(&s->gb, sps->initial_cpb_removal_delay_length);
same
also initial_cpb_removal_delay_offset is unused in your patchset thus
there seems no need to store it
[...]
> Index: libavcodec/h264.h
> ===================================================================
> --- libavcodec/h264.h (revision 17130)
> +++ libavcodec/h264.h (working copy)
> @@ -115,6 +115,7 @@
> * SEI message types
> */
> typedef enum {
> + SEI_BUFFERING_PERIOD = 0, ///< buffering period (H.264, D.1.1)
> SEI_TYPE_PIC_TIMING = 1, ///< picture timing
> SEI_TYPE_USER_DATA_UNREGISTERED = 5, ///< unregistered user data
> SEI_TYPE_RECOVERY_POINT = 6 ///< recovery point (frame # to decoder sync)
> @@ -516,5 +520,8 @@
>
> // Timestamp stuff
> int cpb_cnt; ///< See H.264 E.1.2
> + int sei_buffering_period_present_flag; ///< Buffering period SEI flag
i think sei_buffering_period_present is a better name but that can be
done after all patches are in svn (together with renaming the other _flag
fields)
[...]
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h (revision 17392)
> +++ libavcodec/avcodec.h (working copy)
> @@ -3022,6 +3022,15 @@
>
> int flags;
> #define PARSER_FLAG_COMPLETE_FRAMES 0x0001
> +/*!
> + * Set by parser initialization routine to indicate presence of key frame flag.
> + * By default, I-frames are considered key frames. E.g., for H.264, this is not
> + * always the case. If this flag is set, lavf will use PARSER_FLAG_KEY_FRAME
> + * flag to detect if the current frame is a key frame or not.
> + */
> +#define PARSER_FLAG_KEY_FRAME_FLG_PRESENT 0x0002
> +/// Key frame flag
> +#define PARSER_FLAG_KEY_FRAME 0x0004
>
> int64_t offset; ///< byte offset from starting packet start
> int64_t cur_frame_end[AV_PARSER_PTS_NB];
> Index: libavformat/utils.c
> ===================================================================
> --- libavformat/utils.c (revision 17392)
> +++ libavformat/utils.c (working copy)
> @@ -904,8 +904,15 @@
> else if (pc) {
> pkt->flags = 0;
> /* keyframe computation */
> + if (pc->flags & PARSER_FLAG_KEY_FRAME_FLG_PRESENT) {
> + // use key flag
> + if (pc->flags & PARSER_FLAG_KEY_FRAME)
> + pkt->flags |= PKT_FLAG_KEY;
> + } else {
> + // use I-type picture as key flag
> if (pc->pict_type == FF_I_TYPE)
> pkt->flags |= PKT_FLAG_KEY;
> + }
> }
> }
>
I do not like this implementation
its not hard to add a keyframe field and set that to -1 by default
and treat that default as I_TYPE == keyframe
[...]
> Index: libavcodec/h264_parser.c
> ===================================================================
> --- libavcodec/h264_parser.c (revision 17392)
> +++ libavcodec/h264_parser.c (working copy)
> @@ -171,6 +171,33 @@
> /* key frame, since recovery_frame_cnt is set */
> s->flags |= PARSER_FLAG_KEY_FRAME;
> }
> + pps_id= get_ue_golomb(&h->s.gb);
> + if(pps_id>=MAX_PPS_COUNT) {
> + av_log(h->s.avctx, AV_LOG_ERROR, "pps_id out of range\n");
> + return -1;
> + }
> + if(!h->pps_buffers[pps_id]) {
> + av_log(h->s.avctx, AV_LOG_ERROR, "non-existing PPS referenced\n");
> + return -1;
> + }
> + h->pps= *h->pps_buffers[pps_id];
> + if(!h->sps_buffers[h->pps.sps_id]) {
> + av_log(h->s.avctx, AV_LOG_ERROR, "non-existing SPS referenced\n");
> + return -1;
> + }
> + h->sps = *h->sps_buffers[h->pps.sps_id];
> + h->frame_num = get_bits(&h->s.gb, h->sps.log2_max_frame_num);
> +
> + if (h->sps.frame_mbs_only_flag) {
> + /* single picture for a frame */
> + h->field_pic_flag = 0;
> + } else {
> + h->field_pic_flag = get_bits1(&h->s.gb);
> + if (h->field_pic_flag) {
> + h->bottom_field_flag = get_bits1(&h->s.gb);
> + }
> + }
> +
> return 0; /* no need to evaluate the rest */
> }
> buf += consumed;
this maybe can be factored with h264.c
[...]
> Index: libavcodec/h264.h
> ===================================================================
> --- libavcodec/h264.h (revision 17392)
> +++ libavcodec/h264.h (working copy)
> @@ -533,6 +564,12 @@
> int sei_buffering_period_present_flag; ///< Buffering period SEI flag
> int initial_cpb_removal_delay[32]; ///< Initial timestamps for CPBs
> int initial_cpb_removal_delay_offset[32]; ///< Initial TS offsets for CPBs
> + int sched_sel_idx; ///< Current CPB
> + AVRational ts_tc; ///< Current Tc
> + int64_t ts_trn_nb; ///< Initial TS for current buffering period from H.264
> + int64_t ts_trn_nb_format; ///< Initial TS for current buffering period from format
> + int64_t cur_dts; ///< Current frame decoding timestamp
> + int64_t cur_duration; ///< Current frame duration
> }H264Context;
vertical alignment
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- 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/20090218/967476b9/attachment.pgp>
More information about the ffmpeg-devel
mailing list