[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