[FFmpeg-devel] [PATCH] H.264fixinterlacedflag:bringback pic_struct
Michael Niedermayer
michaelni
Tue Jun 2 22:12:43 CEST 2009
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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- 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/20090602/85bd6cd2/attachment.pgp>
More information about the ffmpeg-devel
mailing list