[FFmpeg-devel] [PATCH]H.264fixinterlacedflag:bringback pic_struct
Michael Niedermayer
michaelni
Thu Jun 4 14:18:54 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?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Republics decline into democracies and democracies degenerate into
despotisms. -- 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/20090604/66426283/attachment.pgp>
More information about the ffmpeg-devel
mailing list