[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