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

[Haruhiko Yamagata]

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

Best regards,
Haruhiko Yamagata