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

Michael Niedermayer michaelni
Tue Jun 2 22:12:43 CEST 2009


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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- 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/20090602/85bd6cd2/attachment.pgp>



More information about the ffmpeg-devel mailing list