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

[Michael Niedermayer]

On Sat, Feb 21, 2009 at 01:31:48PM +0100, Ivan Schreter wrote:
> 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.

hmm i thought it was signed anyway as its unsigned ...


>
> I take #2 is OK now.

yes #2 looks ok


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

yes ok too


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

yes, though i would not mind if some of it is factored
with h264.c if possible


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

#6 should document the field in the .h file not (just) its use

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

I hate to see young programmers poisoned by the kind of thinking
Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
-------------- 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/20090221/8afad203/attachment.pgp>