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

Haruhiko Yamagata h.yamagata
Fri Jun 5 13:28:40 CEST 2009


>On Thu, Jun 04, 2009 at 06:57:29PM +0900, Haruhiko Yamagata wrote:
>>> 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.
>
>hmpf, why cant people just mark their video material correctly ...
>
>anyway, lets see whats left
>1-2 fields &&  FIELD_OR_MBAFF_PICTURE -> interlaced
>1-2 fields && !FIELD_OR_MBAFF_PICTURE -> leave as previous
>3   fields -> progressive
>*   frames -> progressive
>no pic_struct -> set depending on FIELD_OR_MBAFF_PICTURE
>
>would that work?
>is it failing for some files?

Yes, it flags soft telecine progressive.

I added a new variable prev_interlaced_frame in H264Context.
Which value to initialize prev_interlaced_frame is a big problem.

SEI_PIC_STRUCT_TOP_BOTTOM and SEI_PIC_STRUCT_BOTTOM_TOP 
are most annoying because there are progressive videos that use them only.
But of course there are correct interlaced videos that use them only and do not use
MBAFF / PAFF at all (not very nice though).
I confirmed prev_interlaced_frame must be initialized by 1,
otherwise luxe.hd.ts which has correct flags are broken.

After all, it's the encoder's responsibility to flag their video correctly.

Best regards,
Haruhiko Yamagata
-------------- next part --------------
A non-text attachment was scrubbed...
Name: check_presence_of_ct_type_5.patch
Type: application/octet-stream
Size: 3830 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090605/d3fbc710/attachment.obj>



More information about the ffmpeg-devel mailing list