[FFmpeg-devel] [PATCH] H.264: decode picture timing SEI messageand get field order
Haruhiko Yamagata
h.yamagata
Sun Nov 2 02:51:28 CET 2008
On Sat, Nov 02, 2008 at 04:43:06, Michael Niedermayer wrote:
>On Sat, Nov 01, 2008 at 06:43:06PM +0900, Haruhiko Yamagata wrote:
>> Hi,
>>
>> I wrote a patch which implements decoding of picture timing SEI.
>>
>> [Current problems]
>> libavcodec derives field order from POCs.
>>
>> cur->top_field_first = cur->field_poc[0] < cur->field_poc[1];
>>
>> It assumes BFF if two POSs are equal. This is not always correct.
>>
>> And it derives AVFrame::interlaced_frame from used decoding process.
>>
>> cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
>>
>> Sometimes this is not correct. Interlaced/progressive mode of post-decoding
>> processes such as deinterlacing and color space conversion is not always
>> same as the mode used during decoding. H.264 for example allows to encode
>> an interlaced frame using a progressive algorithm. Sample: premiere-paff.ts
>> (http://x264.nl/h.264.samples/premiere-paff.ts).
>>
>> [Solutions]
>> Picture timing SEI message contains pic_struct which defines
>> interlace/progressive
>> mode of post decoding processes and field order. Most streams have the SEI
>> for every frame or field. My patch implements decoding of the SEI and use
>> of pic_struct. Now the field order is always correct. Interlaced picture is
>> judged interlaced. Field 1 repeat flags are also set.
>>
>> [Limitation]
>> As usual, there are exceptions: badly encoded streams. Some progressive
>> streams from television have pic_struct 3, which indicates interlacing.
>> Fortunately such streams are rare. This is not my fault, other decoder show
>> the same behavior.
>>
>> Please review.
>>
>> ----------------------------- for svn log -----------------------
>> H.264: Implement decoding of picture timing SEI message
>> Now correct values are set to interlaced_frame, top_field_first
>> and repeat_pict in AVFrame structure.
>> patch by ffdshow tryouts
>> --------------------------------------------------------------
>>
>> Best regards,
>> Haruhiko Yamagata
>
>> Index: libavcodec/h264.c
>> ===================================================================
>> --- libavcodec/h264.c (revision 15762)
>> +++ libavcodec/h264.c (working copy)
>> @@ -6846,6 +6846,61 @@
>> }while(get_bits(&s->gb, 8) == 255);
>>
>> switch(type){
>> + case 1: // Picture timing SEI
>> + {
>> + if(h->sps.nal_hrd_parameters_present_flag || h->sps.vcl_hrd_parameters_present_flag){
>
>I think this and the code below would be cleaner in its own function like
>decode_unregistered_user_data()
done.
>
>
>> + get_bits(&s->gb, h->sps.cpb_removal_delay_length); /* cpb_removal_delay */
>> + get_bits(&s->gb, h->sps.dpb_output_delay_length); /* dpb_output_delay */
>
>as the value isnt used, skip_bits() would be enough
done.
>> + }
>> + if(h->sps.pic_struct_present_flag){
>
>> + unsigned int i, NumClockTS;
>
>num_clock_ts would be more consistent relative to the other variables in
>h264.c
done.
>> + h->sei_pic_struct = get_bits(&s->gb, 4);
>> +
>> + if (h->sei_pic_struct > SEI_PIC_STRUCT_FRAME_TRIPLING)
>> + return -1;
>> +
>
>> + h->sei_pic_struct_valid_flag = 1;
>
>why is this needed?
I removed sei_pic_struct_valid_flag.
Checking the range of h->sei_pic_struct is necessary for broken streams or future
extension of the SEI.
>> +
>> + // Just to skip bits, maybe better to skip bytes using SEI payloadSize.
>> + NumClockTS = sei_NumClockTS_table[h->sei_pic_struct];
>> +
>> + for (i = 0 ; i < NumClockTS ; i++){
>
>> + unsigned int clock_timestamp_flag = get_bits(&s->gb, 1);
>> + if(clock_timestamp_flag){
>
>if(get_bits1())
done.
>> + unsigned int seconds_flag = get_bits(&s->gb, 1);
>> + if(seconds_flag){
>> + unsigned int minutes_flag;
>> + get_bits(&s->gb, 6); /* seconds_value range 0..59 */
>> + minutes_flag = get_bits(&s->gb, 1);
>> + if(minutes_flag){
>> + unsigned int hours_flag;
>> + get_bits(&s->gb, 6); /* minutes_value 0..59 */
>> + hours_flag = get_bits(&s->gb, 1);
>> + if(hours_flag)
>> + get_bits(&s->gb, 5); /* hours_value 0..23 */
>> + }
>> + }
>
>if(get_bits1(&s->gb)){
> get_bits(&s->gb, 6); /* seconds_value range 0..59 */
> if(get_bits1(&s->gb)){
> get_bits(&s->gb, 6); /* minutes_value 0..59 */
> if(get_bits1(&s->gb)){
> get_bits(&s->gb, 5); /* hours_value 0..23 */
> }
> }
>}
done.
>[...]
>> + }else if(h->sei_pic_struct == SEI_PIC_STRUCT_TOP_BOTTOM_TOP){
>> + // Signal the possibility of telecined film externally (pic_struct 5,6)
>> + // From these hints, let the applications decide if they apply deinterlacing.
>> + cur->repeat_pict = 4;
>> + cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
>> + }
>> + else if(h->sei_pic_struct == SEI_PIC_STRUCT_BOTTOM_TOP_BOTTOM){
>> + cur->repeat_pict = 2;
>> + cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
>> + }else{
>
>both above should be repeat_pict=1 i think
done.
>> + // FIXME do something to signal frame doubling/tripling externally.
>> + cur->interlaced_frame = 0;
>
>repeat_pict= 2 or 4 should be here
done.
>> //FIXME do something with unavailable reference frames
>>
>> /* Sort B-frames into display order */
>> Index: libavcodec/h264.h
>> ===================================================================
>> --- libavcodec/h264.h (revision 15762)
>> +++ libavcodec/h264.h (working copy)
>> @@ -110,6 +110,18 @@
>> NAL_AUXILIARY_SLICE=19
>> };
>
>>
>> +enum {
>> + SEI_PIC_STRUCT_FRAME = 0, // * 0: frame
>> + SEI_PIC_STRUCT_TOP_FIELD = 1, // * 1: top field
>> + SEI_PIC_STRUCT_BOTTOM_FIELD = 2, // * 2: bottom field
>> + SEI_PIC_STRUCT_TOP_BOTTOM = 3, // * 3: top field, bottom field, in that order
>> + SEI_PIC_STRUCT_BOTTOM_TOP = 4, // * 4: bottom field, top field, in that order
>> + SEI_PIC_STRUCT_TOP_BOTTOM_TOP = 5, // * 5: top field, bottom field, top field repeated, in that order
>> + SEI_PIC_STRUCT_BOTTOM_TOP_BOTTOM = 6, // * 6: bottom field, top field, bottom field repeated, in that order
>> + SEI_PIC_STRUCT_FRAME_DOUBLING = 7, // * 7: frame doubling
>> + SEI_PIC_STRUCT_FRAME_TRIPLING = 8 // * 8: frame tripling
>> +};
>
>the enum should have a name and that should be used instead of int
>where appropriate
I named SEI_PicStructType.
Because sei_pic_struct is unsigned int, this may not be the ideal
situation for using enum.
>> +
>> /**
>> * Sequence parameter set
>> */
>> @@ -150,6 +162,12 @@
>> int scaling_matrix_present;
>> uint8_t scaling_matrix4[6][16];
>> uint8_t scaling_matrix8[2][64];
>> + int nal_hrd_parameters_present_flag;
>> + int vcl_hrd_parameters_present_flag;
>> + int pic_struct_present_flag;
>> + int time_offset_length;
>> + int cpb_removal_delay_length; ///< cpb_removal_delay_length_minus1 + 1
>> + int dpb_output_delay_length; ///< dpb_output_delay_length_minus1 + 1
>> }SPS;
>>
>> /**
>> @@ -460,6 +478,12 @@
>> int mb_xy;
>>
>> uint32_t svq3_watermark_key;
>
>> +
>> + /**
>> + * pic_struct in picture timing SEI message
>> + */
>> + unsigned int sei_pic_struct; // The formal name is pic_struct.
>> + int sei_pic_struct_valid_flag; // For each frame
>
>these comments look a little messy
fixed.
>> }H264Context;
>>
>> #endif /* AVCODEC_H264_H */
>> Index: libavcodec/h264data.h
>> ===================================================================
>> --- libavcodec/h264data.h (revision 15762)
>> +++ libavcodec/h264data.h (working copy)
>> @@ -1296,4 +1296,8 @@
>> }
>> };
>>
>> +static const unsigned int sei_NumClockTS_table[9]={
>> + 1, 1, 1, 2, 2, 3, 3, 2, 3
>> +};
>
>this one fits in uint8_t
done.
Thank you very much for reviewing.
Haruhiko Yamagata
-------------- next part --------------
A non-text attachment was scrubbed...
Name: decode_picture_timing_SEI_2.diff
Type: application/octet-stream
Size: 9989 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081102/eda5bbdb/attachment.obj>
More information about the ffmpeg-devel
mailing list