[FFmpeg-devel] [PATCH] H.264 fix interlaced flag

[Michael Niedermayer]

On Tue, Mar 03, 2009 at 07:49:53PM +0100, Ivan Schreter wrote:
> Michael Niedermayer wrote:
>> On Fri, Feb 20, 2009 at 05:16:13PM +0100, Ivan Schreter wrote:
>>   
>>> Michael Niedermayer wrote:
>>>     
>>>> On Fri, Feb 20, 2009 at 04:04:37PM +0100, Ivan Schreter wrote:
>>>>         
>>>>> Michael Niedermayer wrote:
>>>>>             
>>>>>> [...]                the patch is wrong, there seems no relation 
>>>>>> between the pic_struct and
>>>>>> the interlacing vs progressive.
>>>>>> Maybe the ct_type field could be used and the whole 
>>>>>> pic_struct->interlaced
>>>>>> code removed
>>>>>>                   
>>>>> Yes, ct_type could be used. But IMHO, it would be sufficient to say
>>>>>
>>>>> cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
>>>>>
>>>>> outside of the switch and not set interlaced_frame at all in the switch 
>>>>> (i.e., same as for missing picture structure). If you are OK with that, 
>>>>> I'll prepare a patch.
>>>>>             
>>>> cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
>>>> is not correct
>>>>
>>>> progressive frames can be coded as fields and interlaced fields
>>>> can be coded as frames
>>>>
>>>>         
>>> Then only ct_type remains...
>>>     
>>
>> yes, let us hope it actually contains a sane value in practice otherwise
>> we can throw a bit of code away and just use cur->interlaced_frame = 
>> FIELD_OR_MBAFF_PICTURE;
>>
>>   
> I added ct_type handling, as recommended by H.264 standard (basically, look 
> at all ct_type fields of a frame and if any of them is interlaced, then 
> it's interlaced frame).
>
> If ct_type is missing, then take field pictures as interlaced, other 
> picture types as non-interlaced by default.
>
> If picture structure is missing, then look at FIELD_OR_MBAFF_PICTURE flag 
> (same as today).
>
> Seems to work OK with my samples (and is consistent with only looking at 
> FIELD_OR_MBAFF_PICTURE flag, BTW)...
>
> Patch for review attached.
>
> Since I now have svn access, I can commit it by myself, so I can try it on 
> this one, whether it works :-)
>
> Regards,
>
> Ivan
>

> Index: libavcodec/h264.c
> ===================================================================
> --- libavcodec/h264.c	(revision 17723)
> +++ libavcodec/h264.c	(working copy)
> @@ -6805,7 +6805,7 @@
>          for (i = 0 ; i < num_clock_ts ; i++){
>              if(get_bits(&s->gb, 1)){                  /* clock_timestamp_flag */
>                  unsigned int full_timestamp_flag;
> -                skip_bits(&s->gb, 2);                 /* ct_type */
> +                h->sei_ct_type[i] = get_bits(&s->gb, 2);

h->sei_ct_type |= 1<<get_bits(&s->gb, 2);
stores things in a, for us easier to use form


[...]

> @@ -7767,20 +7784,22 @@
>                  switch (h->sei_pic_struct)
>                  {
>                  case SEI_PIC_STRUCT_FRAME:
> -                    cur->interlaced_frame = 0;
> +                    cur->interlaced_frame = h264_is_interlaced(h);
>                      break;
>                  case SEI_PIC_STRUCT_TOP_FIELD:
>                  case SEI_PIC_STRUCT_BOTTOM_FIELD:
> +                    cur->interlaced_frame = 1;
> +                    break;
>                  case SEI_PIC_STRUCT_TOP_BOTTOM:
>                  case SEI_PIC_STRUCT_BOTTOM_TOP:
> -                    cur->interlaced_frame = 1;
> +                    cur->interlaced_frame = h264_is_interlaced(h);
>                      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)
>                      // From these hints, let the applications decide if they apply deinterlacing.
>                      cur->repeat_pict = 1;
> -                    cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
> +                    cur->interlaced_frame = h264_is_interlaced(h);
>                      break;
>                  case SEI_PIC_STRUCT_FRAME_DOUBLING:
>                      // Force progressive here, as doubling interlaced frame is a bad idea.

this is incorrect
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


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

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20090303/9414bb1c/attachment.pgp>