[FFmpeg-devel] [PATCH] H.264fixinterlacedflag:bringback pic_struct

Haruhiko Yamagata h.yamagata
Tue Jun 2 13:59:10 CEST 2009


>On Sun, May 31, 2009 at 05:42:37PM +0900, Haruhiko Yamagata wrote:
>>> On Fri, May 29, 2009 at 07:18:22PM +0900, Haruhiko Yamagata wrote:
>>>>> On Thu, May 28, 2009 at 11:23:17PM +0900, Haruhiko Yamagata wrote:
>>>>>>> On Thu, May 28, 2009 at 10:59:09PM +0900, Haruhiko Yamagata wrote:
>>>>>>>>> On Thu, May 28, 2009 at 09:24:59PM +0900, Haruhiko Yamagata wrote:
>>>>>>>>>> On Tue, Mar 03, 2009, Michael Niedermayer wrote:
>>>>>>>>>>> ct_type should always be used if available, otherwise
>>>>>>>>>>> FIELD_OR_MBAFF_PICTURE should be.
>>>>>>>>>>> that at least is how i understand the spec, if this fails for some
>>>>>>>>>>> case iam interrested in the file
>>>>>>>>>>
>>>>>>>>>> Excuse me for late response, I found a file which fails.
>>>>>>>>>> http://x264.nl/h.264.samples/premiere-paff.ts
>>>>>>>>>>
>>>>>>>>>> Please note that ct_type is optional in the picture timing SEI.
>>>>>>>>>>
>>>>>>>>>>>            if(get_bits(&s->gb, 1)){                  /* 
>>>>>>>>>>> clock_timestamp_flag */
>>>>>>>>>>>                unsigned int full_timestamp_flag;
>>>>>>>>>>>                h->sei_ct_type |= 1<<get_bits(&s->gb, 2);
>>>>>>>>>>
>>>>>>>>>> That file does not have ct_type at all, but has correct pic_struct.
>>>>>>>>>> pic_struct is basically correct, only a few streams are badly 
>>>>>>>>>> encoded.
>>>>>>>>>> The attached patch fixes this issue and still honor ct_type if 
>>>>>>>>>> available.
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> Haruhiko Yamagata
>>>>>>>>>
>>>>>>>>>>  h264.c |   19 +++++++++++++------
>>>>>>>>>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>>>>>>>>> 832b6de132e2ccb691445f47a3ee3b56d0b1e65d  
>>>>>>>>>> check_presence_of_cy_type.patch
>>>>>>>>>> Index: libavcodec/h264.c
>>>>>>>>>> ===================================================================
>>>>>>>>>> --- libavcodec/h264.c (revision 18971)
>>>>>>>>>> +++ libavcodec/h264.c (working copy)
>>>>>>>>>> @@ -6866,7 +6866,7 @@
>>>>>>>>>>          for (i = 0 ; i < num_clock_ts ; i++){
>>>>>>>>>>              if(get_bits(&s->gb, 1)){                  /* 
>>>>>>>>>> clock_timestamp_flag */
>>>>>>>>>>                  unsigned int full_timestamp_flag;
>>>>>>>>>> -                h->sei_ct_type |= 1<<get_bits(&s->gb, 2);
>>>>>>>>>> +                h->sei_ct_type |= 1<<(get_bits(&s->gb, 2)+1);
>>>>>>>>>>                  skip_bits(&s->gb, 1);                 /* 
>>>>>>>>>> nuit_field_based_flag */
>>>>>>>>>>                  skip_bits(&s->gb, 5);                 /* 
>>>>>>>>>> counting_type */
>>>>>>>>>
>>>>>>>>> why?
>>>>>>>>
>>>>>>>> So that h->sei_ct_type will be none zero if ct_type exists, even if 
>>>>>>>> ct_type is zero.
>>>>>>>> In other words, h->sei_ct_type shall be zero if ct_type does not 
>>>>>>>> exit.
>>>>>>>
>>>>>>> so 1<<0 is 0 for you?
>>>>>>
>>>>>> My mistake.
>>>>>> A new patch attached.
>>>>> [...]
>>>>>> @@ -7819,6 +7823,9 @@
>>>>>>                  cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
>>>>>>              }
>>>>>>  +            if (h->sei_ct_type)
>>>>>> +                cur->interlaced_frame = (h->sei_ct_type & (1<<1)) != 
>>>>>> 0;
>>>>>
>>>>> this overrides the value set from SEI_PIC_STRUCT_FRAME_TRIPLING:
>>>>
>>>> Yes, I moved the two lines into if(h->sps.pic_struct_present_flag) block 
>>>> and
>>>> added check.
>>>>
>>>> Best regards,
>>>> Haruhiko Yamagata
>>>
>>>>  h264.c |   17 ++++++++++++-----
>>>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>>> 3d86c0ddddd340d6af20ef493f043900ed02cc43  
>>>> check_presence_of_ct_type_3.patch
>>>> Index: libavcodec/h264.c
>>>> ===================================================================
>>>> --- libavcodec/h264.c (revision 18980)
>>>> +++ libavcodec/h264.c (working copy)
>>>> @@ -7790,14 +7790,18 @@
>>>>               /* Signal interlacing information externally. */
>>>>              /* Prioritize picture timing SEI information over used 
>>>> decoding process if it exists. */
>>>> -            if (h->sei_ct_type)
>>>> -                cur->interlaced_frame = (h->sei_ct_type & (1<<1)) != 0;
>>>> -            else
>>>> -                cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
>>>> -
>>>>              if(h->sps.pic_struct_present_flag){
>>>>                  switch (h->sei_pic_struct)
>>>>                  {
>>>> +                case SEI_PIC_STRUCT_FRAME:
>>>> +                    cur->interlaced_frame = 0;
>>>> +                    break;
>>>> +                case SEI_PIC_STRUCT_TOP_FIELD:
>>>> +                case SEI_PIC_STRUCT_BOTTOM_FIELD:
>>>> +                case SEI_PIC_STRUCT_TOP_BOTTOM:
>>>> +                case SEI_PIC_STRUCT_BOTTOM_TOP:
>>>> +                    cur->interlaced_frame = 1;
>>>> +                    break;
>>>>                  case SEI_PIC_STRUCT_TOP_BOTTOM_TOP:
>>>>                  case SEI_PIC_STRUCT_BOTTOM_TOP_BOTTOM:
>>>>                      // Signal the possibility of telecined film 
>>>> externally (pic_struct 5,6)
>>>> @@ -7814,6 +7818,9 @@
>>>>                      cur->repeat_pict = 4;
>>>>                      break;
>>>>                  }
>>>> +
>>>> +                if (h->sei_ct_type && h->sei_pic_struct <= 
>>>> SEI_PIC_STRUCT_BOTTOM_TOP)
>>>> +                    cur->interlaced_frame = (h->sei_ct_type & (1<<1)) != 
>>>> 0;
>>>
>>> i think that leaves at least one case where interlaced_frame frame is not
>>> initialized
>>>
>>>
>>>>              }else{
>>>>                  /* Derive interlacing flag from used decoding process. 
>>>> */
>>>>                  cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
>>
>> In this case, neither pic_struct nor ct_type is available.
>> This is the best information that we can have here.
>> Both pic_struct and ct_type are under pic_struct_present_flag.
>
>ct_type is under clock_timestamp_flag, pic_struct is not
>also ct_type can be set to "unknown" the code should fallback to someting
>else in that case

The spec says 
> If CpbDpbDelaysPresentFlag is equal to 1 or pic_struct_present_flag is equal to 1,
> one picture timing SEI message shall be present in every access unit of the coded
> video sequence.

So if pic_struct_present_flag is equal to 1, decode_picture_timing shall be called
and h->sei_ct_type shall be flagged unknown (0) or set correct value.

In my patch,

> +                if (h->sei_ct_type && h->sei_pic_struct <= 

Presence of ct_type is checked here. This is only referenced 
if pic_struct_present_flag is equal to 1.

Best regards,
Haruhiko Yamagata




More information about the ffmpeg-devel mailing list