[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set

[Michael Niedermayer]

On Wed, Feb 18, 2009 at 12:33:57PM +0100, Ivan Schreter wrote:
> 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?

yes "#4" is ok now


[...]
>> [...]
>>
>>   
>>> @@ -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.

fixed doc, and you need to make the check unsigned


>
>> [...]
>> 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.

no initial_cpb_removal_delay_offset is still there


>
>> [...]
>>> @@ -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?

see above


>
>>
>> [...]
>>   
>>> 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?

you increase minor by 1 and rest micro to 0


>
> 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)?

lets assume we now have
lavc 1.2.3
lavf 2.3.4
then when keyframe is added
lavc 1.3.0
lavf 2.3.5 would depend on lavc >= 1.3.0
the old 2.3.4 would work with 1.3.0 as well


>
>>
>> [...]
>>   
>>> 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?

not at the moment but maybe i come up with something later

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- 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/f2c996d8/attachment.pgp>