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

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