[FFmpeg-devel] [PATCH] H.264: decode picture timing SEI message and get field order
Michael Niedermayer
michaelni
Sat Nov 1 23:31:51 CET 2008
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()
> + 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
> + }
> + 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
> + 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?
> +
> + // 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())
> + unsigned int full_timestamp_flag;
> + get_bits(&s->gb, 2); /* ct_type */
> + get_bits(&s->gb, 1); /* nuit_field_based_flag */
> + get_bits(&s->gb, 5); /* counting_type */
> + full_timestamp_flag = get_bits(&s->gb, 1);
> + get_bits(&s->gb, 1); /* discontinuity_flag */
> + get_bits(&s->gb, 1); /* cnt_dropped_flag */
> + get_bits(&s->gb, 8); /* n_frames */
> + if(full_timestamp_flag){
> + get_bits(&s->gb, 6); /* seconds_value 0..59 */
> + get_bits(&s->gb, 6); /* minutes_value 0..59 */
> + get_bits(&s->gb, 5); /* hours_value 0..23 */
> + }else{
> + 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 */
}
}
}
[...]
> + }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
> + // FIXME do something to signal frame doubling/tripling externally.
> + cur->interlaced_frame = 0;
repeat_pict= 2 or 4 should be here
> + }
> + }else{
> + /* Derive interlacing flag from used decoding process. */
> + cur->interlaced_frame = FIELD_OR_MBAFF_PICTURE;
> + }
> +
> + if (cur->field_poc[0] != cur->field_poc[1]){
> + /* Derive top_field_first from field pocs. */
> + cur->top_field_first = cur->field_poc[0] < cur->field_poc[1];
> + }else{
> + if(cur->interlaced_frame || h->sei_pic_struct_valid_flag){
> + /* Use picture timing SEI information. Even if it is a information of a past frame, better than nothing. */
> + if(h->sei_pic_struct == SEI_PIC_STRUCT_TOP_BOTTOM
> + || h->sei_pic_struct == SEI_PIC_STRUCT_TOP_BOTTOM_TOP)
> + cur->top_field_first = 1;
> + else
> + cur->top_field_first = 0;
> + }else{
> + /* Most likely progressive */
> + cur->top_field_first = 0;
> + }
> + }
> + // av_log(avctx, AV_LOG_DEBUG, "interlaced_frame %d, top_field_first %d, repeat_pict %d\n",
> + // cur->interlaced_frame, cur->top_field_first, cur->repeat_pict);
> +
this shouldnt be in the patch
> //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
> +
> /**
> * 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
> }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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- 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/20081101/f94575e2/attachment.pgp>
More information about the ffmpeg-devel
mailing list