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

Michael Niedermayer michaelni
Wed Jun 3 21:27:26 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

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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/20090603/bf6ccf37/attachment.pgp>



More information about the ffmpeg-devel mailing list