[FFmpeg-devel] [PATCH] Implement PAFF in H.264

Vitor Sessak vitor1001
Tue Sep 18 21:10:18 CEST 2007


Hi,

I hope you don't mind some nitpicking...

Jeff Downs wrote:
> Attached is a patch that implements PAFF in H.264.

[...]

>  
> -            // find the largest poc
> -            for(list=0; list<2; list++){
> -                int index = 0;
> -                int j= -99;
> -                int step= list ? -1 : 1;
>  
> -                for(i=0; i<h->short_ref_count && index < h->ref_count[list]; i++, j+=step) {
> -                    while(j<0 || j>= h->short_ref_count){
> -                        if(j != -99 && step == (list ? -1 : 1))
> -                            return -1;
> -                        step = -step;
> -                        j= smallest_poc_greater_than_current + (step>>1);
> -                    }
> -                    if(sorted_short_ref[j].reference != 3) continue;
> -                    h->default_ref_list[list][index  ]= sorted_short_ref[j];
> -                    h->default_ref_list[list][index++].pic_id= sorted_short_ref[j].frame_num;
> -                }
> +        // find the largest poc
> +        for(list=0; list<2; list++){
> +            int index = 0;
> +            int j= -99;
> +            int step= list ? -1 : 1;

Cosmetical. The idea is to reindent in another patch, after the first 
one is accepted. If you keep the number of modified lines low, your 
patch will be easier to read and will get reviewed faster.

[...]

> -            h->long_ref[ mmco[i].long_index ]= remove_short(h, mmco[i].short_frame_num);
> -            if (h->long_ref[ mmco[i].long_index ]){
> -                h->long_ref[ mmco[i].long_index ]->long_ref=1;
> -                h->long_ref_count++;
> +                pic= remove_long(h, mmco[i].long_arg);
> +                if(pic) unreference_pic(h, pic);
> +
> +                h->long_ref[ mmco[i].long_arg ]= remove_short(h, frame_num);
> +                if (h->long_ref[ mmco[i].long_arg ]){
> +                    h->long_ref[ mmco[i].long_arg ]->long_ref=1;
> +                    h->long_ref_count++;
> +                }
>              }

More cosmetics

>  
>      if(h->nal_unit_type == NAL_IDR_SLICE){ //FIXME fields
>          s->broken_link= get_bits1(gb) -1;
> -        h->mmco[0].long_index= get_bits1(gb) - 1; // current_long_term_idx
> -        if(h->mmco[0].long_index == -1)
> +        h->mmco[0].long_arg= get_bits1(gb) - 1; // current_long_term_idx
> +        if(h->mmco[0].long_arg == -1)
>              h->mmco_index= 0;
>          else{
>              h->mmco[0].opcode= MMCO_LONG;
> @@ -3321,19 +3637,19 @@
>  
>                  h->mmco[i].opcode= opcode;
>                  if(opcode==MMCO_SHORT2UNUSED || opcode==MMCO_SHORT2LONG){
> -                    h->mmco[i].short_frame_num= (h->frame_num - get_ue_golomb(gb) - 1) & ((1<<h->sps.log2_max_frame_num)-1); //FIXME fields
> +                    h->mmco[i].short_pic_num= h->curr_pic_num - get_ue_golomb(gb) - 1;
>  /*                    if(h->mmco[i].short_frame_num >= h->short_ref_count || h->short_ref[ h->mmco[i].short_frame_num ] == NULL){
>                          av_log(s->avctx, AV_LOG_ERROR, "illegal short ref in memory management control operation %d\n", mmco);
>                          return -1;
>                      }*/
>                  }
>                  if(opcode==MMCO_SHORT2LONG || opcode==MMCO_LONG2UNUSED || opcode==MMCO_LONG || opcode==MMCO_SET_MAX_LONG){
> -                    unsigned int long_index= get_ue_golomb(gb);
> -                    if(/*h->mmco[i].long_index >= h->long_ref_count || h->long_ref[ h->mmco[i].long_index ] == NULL*/ long_index >= 16){
> +                    unsigned int long_arg= get_ue_golomb(gb);
> +                    if(long_arg >= 32 || (long_arg >= 16 && !(opcode == MMCO_LONG2UNUSED && FIELD_PICTURE))){
>                          av_log(h->s.avctx, AV_LOG_ERROR, "illegal long ref in memory management control operation %d\n", opcode);
>                          return -1;
>                      }
> -                    h->mmco[i].long_index= long_index;
> +                    h->mmco[i].long_arg= long_arg;
>                  }

Maybe it would be better to do the renaming of that var before or after 
the bulk of your patch is applied.


>  
> -        MPV_frame_end(s);
> +            MPV_frame_end(s);
>  
> -    //FIXME do something with unavailable reference frames
> +        //FIXME do something with unavailable reference frames
>  
>  #if 0 //decode order
> -        *data_size = sizeof(AVFrame);
> +            *data_size = sizeof(AVFrame);
>  #else
> -        /* Sort B-frames into display order */
> +            /* Sort B-frames into display order */
>  
> -        if(h->sps.bitstream_restriction_flag
> -           && s->avctx->has_b_frames < h->sps.num_reorder_frames){
> -            s->avctx->has_b_frames = h->sps.num_reorder_frames;
> -            s->low_delay = 0;
> -        }
> +            if(h->sps.bitstream_restriction_flag
> +               && s->avctx->has_b_frames < h->sps.num_reorder_frames){
> +                s->avctx->has_b_frames = h->sps.num_reorder_frames;
> +                s->low_delay = 0;
> +            }
>  
> -        pics = 0;
> -        while(h->delayed_pic[pics]) pics++;
> +            pics = 0;
> +            while(h->delayed_pic[pics]) pics++;
>  
> -        assert(pics+1 < sizeof(h->delayed_pic) / sizeof(h->delayed_pic[0]));
> +            assert(pics+1 < sizeof(h->delayed_pic) / sizeof(h->delayed_pic[0]));
>  
> -        h->delayed_pic[pics++] = cur;
> -        if(cur->reference == 0)
> -            cur->reference = 1;
> +            h->delayed_pic[pics++] = cur;
> +            if(cur->reference == 0)
> +                cur->reference = 1;
>  
> -        cross_idr = 0;
> -        for(i=0; h->delayed_pic[i]; i++)
> -            if(h->delayed_pic[i]->key_frame || h->delayed_pic[i]->poc==0)
> -                cross_idr = 1;
> +            cross_idr = 0;
> +            for(i=0; h->delayed_pic[i]; i++)
> +                if(h->delayed_pic[i]->key_frame || h->delayed_pic[i]->poc==0)
> +                    cross_idr = 1;
>  
> -        out = h->delayed_pic[0];
> -        out_idx = 0;
> -        for(i=1; h->delayed_pic[i] && !h->delayed_pic[i]->key_frame; i++)
> -            if(h->delayed_pic[i]->poc < out->poc){
> -                out = h->delayed_pic[i];
> -                out_idx = i;
> +            out = h->delayed_pic[0];
> +            out_idx = 0;
> +            for(i=1; h->delayed_pic[i] && !h->delayed_pic[i]->key_frame; i++)
> +                if(h->delayed_pic[i]->poc < out->poc){
> +                    out = h->delayed_pic[i];
> +                    out_idx = i;
> +                }
> +
> +            out_of_order = !cross_idr && prev && out->poc < prev->poc;
> +            if(h->sps.bitstream_restriction_flag && s->avctx->has_b_frames >= h->sps.num_reorder_frames)
> +                { }
> +            else if(prev && pics <= s->avctx->has_b_frames)
> +                out = prev;
> +            else if((out_of_order && pics-1 == s->avctx->has_b_frames && pics < 15)
> +               || (s->low_delay &&
> +                ((!cross_idr && prev && out->poc > prev->poc + 2)
> +                 || cur->pict_type == B_TYPE)))
> +            {
> +                s->low_delay = 0;
> +                s->avctx->has_b_frames++;
> +                out = prev;
>              }
> +            else if(out_of_order)
> +                out = prev;
>  
> -        out_of_order = !cross_idr && prev && out->poc < prev->poc;
> -        if(h->sps.bitstream_restriction_flag && s->avctx->has_b_frames >= h->sps.num_reorder_frames)
> -            { }
> -        else if(prev && pics <= s->avctx->has_b_frames)
> -            out = prev;
> -        else if((out_of_order && pics-1 == s->avctx->has_b_frames && pics < 15)
> -           || (s->low_delay &&
> -            ((!cross_idr && prev && out->poc > prev->poc + 2)
> -             || cur->pict_type == B_TYPE)))
> -        {
> -            s->low_delay = 0;
> -            s->avctx->has_b_frames++;
> -            out = prev;
> -        }
> -        else if(out_of_order)
> -            out = prev;
> +            if(out_of_order || pics > s->avctx->has_b_frames){
> +                for(i=out_idx; h->delayed_pic[i]; i++)
> +                    h->delayed_pic[i] = h->delayed_pic[i+1];
> +            }
>  
> -        if(out_of_order || pics > s->avctx->has_b_frames){
> -            for(i=out_idx; h->delayed_pic[i]; i++)
> -                h->delayed_pic[i] = h->delayed_pic[i+1];
> -        }
> -
> -        if(prev == out)
> -            *data_size = 0;
> -        else
> -            *data_size = sizeof(AVFrame);
> -        if(prev && prev != out && prev->reference == 1)
> -            prev->reference = 0;
> -        h->delayed_output_pic = out;
> +            if(prev == out)
> +                *data_size = 0;
> +            else
> +                *data_size = sizeof(AVFrame);
> +            if(prev && prev != out && prev->reference == 1)
> +                prev->reference = 0;
> +            h->delayed_output_pic = out;
>  #endif
>  
> -        if(out)
> -            *pict= *(AVFrame*)out;
> -        else
> -            av_log(avctx, AV_LOG_DEBUG, "no picture\n");
> +            if(out)
> +                *pict= *(AVFrame*)out;
> +            else
> +                av_log(avctx, AV_LOG_DEBUG, "no picture\n");
> +        }
>      }
>  

Big block of mostly (only?) cosmetics

> @@ -84,8 +86,8 @@
>      int poc_cycle_length;              ///< num_ref_frames_in_pic_order_cnt_cycle
>      int ref_frame_count;               ///< num_ref_frames
>      int gaps_in_frame_num_allowed_flag;
> -    int mb_width;                      ///< frame_width_in_mbs_minus1 + 1
> -    int mb_height;                     ///< frame_height_in_mbs_minus1 + 1
> +    int mb_width;                      ///< pic_width_in_mbs_minus1 + 1
> +    int mb_height;                     ///< pic_height_in_map_units_minus1 + 1
>      int frame_mbs_only_flag;
>      int mb_aff;                        ///<mb_adaptive_frame_field_flag
>      int direct_8x8_inference_flag;
> @@ -151,8 +153,8 @@
>   */
>  typedef struct MMCO{
>      MMCOOpcode opcode;
> -    int short_frame_num;
> -    int long_index;
> +    int short_pic_num;
> +    int long_arg;  ///< index, pic_num, or num long refs depending on opcode
>  } MMCO;
>  
>  /**
> @@ -283,7 +285,7 @@
>      int prev_frame_num;           ///< frame_num of the last pic for POC type 1/2
>  
>      /**
> -     * frame_num for frames or 2*frame_num for field pics.
> +     * frame_num for frames or 2*frame_num+1 for field pics.
>       */
>      int curr_pic_num;
>  
> @@ -323,8 +325,9 @@
>      unsigned int list_count;
>      Picture *short_ref[32];
>      Picture *long_ref[32];
> -    Picture default_ref_list[2][32];
> +    Picture default_ref_list[2][32]; ///< base reference list for all slices of a coded picture
>      Picture ref_list[2][48];     ///< 0..15: frame refs, 16..47: mbaff field refs
> +                                 ///< Reordered version of default_ref_list according to picture reordering in slice header
>      Picture *delayed_pic[18]; //FIXME size?
>      Picture *delayed_output_pic;
>  
> Index: libavcodec/mpegvideo.c
> ===================================================================
> --- libavcodec/mpegvideo.c	(revision 10526)
> +++ libavcodec/mpegvideo.c	(working copy)
> @@ -949,7 +949,7 @@
>  
>      assert(s->pict_type == I_TYPE || (s->last_picture_ptr && s->last_picture_ptr->data[0]));
>  
> -    if(s->picture_structure!=PICT_FRAME){
> +    if(s->picture_structure!=PICT_FRAME && s->out_format != FMT_H264){
>          int i;
>          for(i=0; i<4; i++){
>              if(s->picture_structure == PICT_BOTTOM_FIELD){
> Index: libavcodec/mpegvideo.h
> ===================================================================
> --- libavcodec/mpegvideo.h	(revision 10526)
> +++ libavcodec/mpegvideo.h	(working copy)
> @@ -138,8 +138,9 @@
>  
>      int field_poc[2];           ///< h264 top/bottom POC
>      int poc;                    ///< h264 frame POC
> -    int frame_num;              ///< h264 frame_num
> -    int pic_id;                 ///< h264 pic_num or long_term_pic_idx
> +    int valid_structure;        ///< h264 one of PICT_XXXX, stating which fields are referenced
> +    int frame_num;              ///< h264 frame_num (raw frame_num from slice header)
> +    int pic_id;                 ///< h264 pic_num (short or long term)
>      int long_ref;               ///< 1->long term reference 0->short term reference
>      int ref_poc[2][16];         ///< h264 POCs of the frames used as reference
>      int ref_count[2];           ///< number of entries in ref_poc
> @@ -622,9 +623,9 @@
>      int mpeg_f_code[2][2];
>      int picture_structure;
>  /* picture type */
> -#define PICT_TOP_FIELD     1
> -#define PICT_BOTTOM_FIELD  2
> -#define PICT_FRAME         3
> +#define PICT_TOP_FIELD     0x1
> +#define PICT_BOTTOM_FIELD  0x2
> +#define PICT_FRAME         (PICT_TOP_FIELD | PICT_BOTTOM_FIELD)

Maybe some of those changes could be in another patch (applied before or 
after the main one).

-Vitor





More information about the ffmpeg-devel mailing list