[FFmpeg-devel] [PATCH]H.264fixinterlacedflag:bringback pic_struct
Haruhiko Yamagata
h.yamagata
Tue Jun 9 13:56:06 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.
What about adding this patch?
There are a few videos that are wrongly encoded, but it's not the majority.
This patch just flags correct videos correctly. Current svn does not.
It is not desirable correct videos are sacrificed to flag wrong videos correctly.
I guess "wrong videos" that use SEI_PIC_STRUCT_TOP_BOTTOM or
SEI_PIC_STRUCT_BOTTOM_TOP for progressive frames assume interlaced
display.
And the spec says,
> When another display type or display rate is used by the decoder, then pic_struct does not
> indicate the display method, but may aid in processing the decoded video for the alternative display.
This means even if our display type is different from the assume one, pic_struct can be used to
process the decoded picture.
Best regards,
Haruhiko Yamagata
More information about the ffmpeg-devel
mailing list