[FFmpeg-devel] [PATCH]H.264fixinterlacedflag:bringback pic_struct
Michael Niedermayer
michaelni
Tue Jun 9 15:08:38 CEST 2009
On Fri, Jun 05, 2009 at 08:28:40PM +0900, Haruhiko Yamagata wrote:
>> 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.
patch probably ok
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/20090609/0aa759d8/attachment.pgp>
More information about the ffmpeg-devel
mailing list