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

[Michael Niedermayer]

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;

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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/20090530/d11a7d42/attachment.pgp>