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

Haruhiko Yamagata h.yamagata
Thu Jun 4 11:57:29 CEST 2009


>On Wed, Jun 03, 2009 at 06:58:02PM +0900, Haruhiko Yamagata wrote:
>>> On Tue, Jun 02, 2009 at 08:59:10PM +0900, Haruhiko Yamagata wrote:
>>>>> 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.
>>>
>>> 0 is not the only value that means unknown
>>>
>>>
>>>>
>>>> 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.
>>>
>>> and if sei_ct_type==0 interlaced_frame is left at whichever value it was
>>> previously it was guessed based on field/frame coding
>>
>> Thank you for your patience. I finally understand.
>> A new patch attached. Soft telecine is flagged progressive.
>
>no, i dont think it is
>the pic_struct used by this file is probably also used by soft telecine

You are right. I have a soft telecine sample.
In that file, there are 4 types of pic_struct.
  SEI_PIC_STRUCT_TOP_BOTTOM
  SEI_PIC_STRUCT_BOTTOM_TOP
    interlaced_frame = 1;
    repeat_pict = 0;

  SEI_PIC_STRUCT_TOP_BOTTOM_TOP
  SEI_PIC_STRUCT_BOTTOM_TOP_BOTTOM
    interlaced_frame = 0;
    repeat_pict = 1;

All frames are progressive, ie no combing artifact.
So it is annoying that interlaced_frame is set to 1
in SEI_PIC_STRUCT_TOP_BOTTOM and SEI_PIC_STRUCT_BOTTOM_TOP cases,
but premiere-paff.ts uses those values for interlaced frames.

Client applications can handle soft telecine by analyzing the pattern
of sequence. It is possible to implement such algorithm in libavcodec,
but I guess you dislike it.

Best regards,
Haruhiko Yamagata



More information about the ffmpeg-devel mailing list