[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set
Ivan Schreter
schreter
Wed Feb 18 12:33:57 CET 2009
Michael Niedermayer wrote:
> On Tue, Feb 17, 2009 at 09:57:32PM +0100, Ivan Schreter wrote:
>
> [...]
>
>> 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 ?
>
>
You are right, it should go to SPS. Moved there, updated patch #4
attached. OK now?
> [...]
>
> this is redundant,
> h->field_pic_flag == (s->picture_structure != PICT_FRAME)
> h->bottom_field_flag == (s->picture_structure != PICT_TOP_FIELD)
>
OK, I'll update parser later to use this instead.
> [...]
>
>
>> @@ -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
>
According to docs of get_ue_golomb_31(), it can only return value in
range 0..31. SPS ID can be in range 0..31 as well, so no check required.
However, looking at get_ue_golomb_31() code, the lookup table contains
also return value of 32! So either doc is wrong or the lookup table is
wrong. I've added the check to be on the safe side.
> [...]
> also initial_cpb_removal_delay_offset is unused in your patchset thus
> there seems no need to store it
>
Yes. I stored it for completeness. Removed again.
> [...]
>> @@ -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)
>
Renamed.
Attached updated patch #6. OK now?
>
> [...]
>
>> 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
>
I can do that easily (also wanted originally), but what about binary
compatibility? If I add a field, minor version must be bumped up, right?
How do I do that?
Furthermore, lavf must check version of lavc whether the field exists at
all.
Again, no problem to go this way, but I simply don't know how to do it
correctly. Please explain in more detail (and maybe a howto on FFmpeg
website would be also a good investition :-).
Or do we expect to have same SVN revision of lavf and lavc on the system
(at least under same major/minor)?
>
> [...]
>
>> 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
>
Yes. I was thinking about creating additional function like
ff_h264_decode_slice_header_0(), which would contain the first part of
decode_slice_header(), which is relevant for parser. Do you have a
better name suggestion?
Regards,
Ivan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_timing_4_cpb_cnt.patch
Type: text/x-patch
Size: 1031 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090218/2c071e84/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_timing_6_buffering_period.patch
Type: text/x-patch
Size: 3579 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090218/2c071e84/attachment-0001.bin>
More information about the ffmpeg-devel
mailing list