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

[Ivan Schreter]

Hi Michael,

so here the next round:

Michael Niedermayer wrote:
> On Fri, Feb 20, 2009 at 01:24:30PM +0100, Ivan Schreter wrote:
>   
> [...]
>   
>> +        } else {
>> +            while(ptr[dst_length - 1] == 0 && dst_length > 0)
>> +                dst_length--;
>> +            bit_length= !dst_length ? 0 : (8*dst_length - ff_h264_decode_rbsp_trailing(h, ptr + dst_length - 1));
>> +        }
>>     
>
> still seems useless
>   
Not strictly necessary, so I removed it from #2.

>> +            slice_type = get_ue_golomb_31(&h->s.gb);
>> +            s->pict_type = golomb_to_pict_type[slice_type % 5];
>>     
>
> missing validity checks
>   
None needed. slice_type is unsigned int and unsigned int % 5 is always 
in 0..4 range.

I take #2 is OK now.

I also take #3 is OK, as you didn't have comments (and it's pretty trivial).

I left out #4 (convergence duration), since this needs to be addressed 
probably in more complex way, as we already discussed.

> [...]
>   
>> Index: libavcodec/h264_parser.c
>> ===================================================================
>> --- libavcodec/h264_parser.c    (revision 17392)
>> +++ libavcodec/h264_parser.c    (working copy)
>> @@ -112,6 +112,7 @@
>>  {
>>      H264Context *h = s->priv_data;
>>      const uint8_t *buf_end = buf + buf_size;
>> +    int pps_id;
>>      unsigned int slice_type;
>>      int state;
>>      const uint8_t *ptr;
>> @@ -176,6 +180,33 @@
>>              } else {
>>                  s->convergence_duration = AV_NOPTS_VALUE;
>>              }
>> +            pps_id= get_ue_golomb(&h->s.gb);
>> +            if(pps_id>=MAX_PPS_COUNT) {
>>     
>
> this check is insufficient, let me repeat, these checks are critical for
> security please make tripple certain that you do not forget checking a
> array index taken from the stream
>
>   
Sorry. Changed pps_id to unsigned, this fixes the problem.

So #5 should be OK now as well.

I also implemented repeat_pict handling for H.264 parser (see attached 
patch #6), since we came to the agreement it is missing for H.264. OK so?

Timestamping patch has to be re-worked (discussion in another 
sub-thread), so I don't include it here.

Regards,

Ivan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_parser_1_funcs.patch
Type: text/x-patch
Size: 4962 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090221/3c47041f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_parser_2_parser.patch
Type: text/x-patch
Size: 2945 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090221/3c47041f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_parser_3_keyframe.patch
Type: text/x-patch
Size: 1092 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090221/3c47041f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_parser_5_field_flags.patch
Type: text/x-patch
Size: 1760 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090221/3c47041f/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_parser_6_repeat_pic.patch
Type: text/x-patch
Size: 2192 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090221/3c47041f/attachment-0004.bin>