Hi just a quick review below, ill wait with a full review until there are clean and well split patches ... On Tue, Sep 18, 2007 at 04:24:54PM -0400, Jeff Downs wrote: [...]
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.
I can do that if necessary; the renaming was done to have the variable make sense in both frame and field contexts.
yes, please do, all cosmetics must be in a seperate patch [...]
/** + * Split one reference list into field parts, interleaving by parity + * as per H.264 spec section 8.2.4.2.5. Output fields have their data pointers + * set to look at the actual start of data for that field. + * + * @param dest output list + * @param dest_len maximum number of fields to put in dest + * @param src the source reference list containing fields and/or field pairs + * (aka short_ref/long_ref, or + * refFrameListXShortTerm/refFrameListLongTerm in spec-speak) + * @param src_len number of Picture's in source (pairs and unmatched fields) + * @param parity the parity of the picture being decoded/needing + * these ref pics (PICT_{TOP,BOTTOM}_FIELD) + * @return number of fields placed in dest + */ +static inline int split_field_half_ref_list(Picture *dest, int dest_len, Picture *src, int src_len, int parity) +{
i dont think this should be inline, it doesnt appear speed critical
+ int same_i, opp_i; + int i; + int same; + int out_i; + + same_i = 0; + opp_i = 0; + same = 1; + out_i = 0; + + while (out_i < dest_len) {
+ if (same && same_i < src_len) { + if ((src[same_i].valid_structure & parity)) { + same = 0; + dest[out_i] = src[same_i]; + for (i = 0; i < 4; ++i) { + if (parity == PICT_BOTTOM_FIELD) + dest[out_i].data[i] += dest[out_i].linesize[i]; + dest[out_i].linesize[i] *= 2; + dest[out_i].pic_id *= 2; + } + out_i++; + } + same_i++; + + } else if (opp_i < src_len) { + if ((src[opp_i].valid_structure & (PICT_FRAME - parity))) { + same = 1; + dest[out_i] = src[opp_i]; + for (i = 0; i < 4; ++i) { + if (parity == PICT_TOP_FIELD) + dest[out_i].data[i] += dest[out_i].linesize[i]; + dest[out_i].linesize[i] *= 2; + dest[out_i].pic_id *= 2; + dest[out_i].pic_id++; + } + out_i++; + } + opp_i++; +
near duplicate [...]
/** + * Removes reference marking from a field or field pair by picture number. + * If an unmatched field pair, or both fields in a pair become unreferenced, + * the field (pair) is removed from the short term reference list and list + * state is updated accordingly. + * @param picret set to the unreferenced and removed picture, or NULL if the + * picture still has fields in reference or was not found. + * @return -1 if picture with pic_num not found. 0 otherwise + */ +static int remove_field_short(H264Context *h, int pic_num, Picture **picret){ + MpegEncContext * const s = &h->s; + int i; + Picture *pic = NULL; + int frame_num = pic_num >> 1; + + if(s->avctx->debug&FF_DEBUG_MMCO) + av_log(h->s.avctx, AV_LOG_DEBUG, "remove field short %d count %d\n", pic_num, h->short_ref_count); + + for(i=0; i<h->short_ref_count; i++){ + pic= h->short_ref[i]; + + if(s->avctx->debug&FF_DEBUG_MMCO) + av_log(h->s.avctx, AV_LOG_DEBUG, "%d %d %p\n", i, pic->frame_num, pic); + + if (pic->frame_num == frame_num) + break; + } + + *picret = NULL; + if (pic) { + int mask; + + mask = (pic_num & 1) ? ~s->picture_structure : s->picture_structure; + pic->valid_structure &= mask; + + if (pic->valid_structure == 0) { + h->short_ref[i]= NULL; + if (--h->short_ref_count) + memmove(&h->short_ref[i], &h->short_ref[i+1], (h->short_ref_count - i)*sizeof(Picture*)); + *picret = pic; + } + return 0; + } + + return -1; +}
this is largly a duplicate of remove_short() [...]
@@ -7402,13 +7788,26 @@
h->prev_frame_num_offset= h->frame_num_offset; h->prev_frame_num= h->frame_num; - if(s->current_picture_ptr->reference){ + if(s->current_picture.reference){
this is wrong, current_picture is a copy of current_picture_ptr if current_picture is correct while _ptr is not theres a bug [...]
+ /* + * FIXME: Error handling code does not seem to support interlaced + * when slices span multiple rows + */ + if (!FIELD_PICTURE) ff_er_frame_end(s);
why doesnt this work? [...]
@@ -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;
/** [...] @@ -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;
cosmetics
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 valid_structure; ///< h264 one of PICT_XXXX, stating which fields are referenced
if my memory doesnt fail my then we already have this variable, its called reference
- int frame_num; ///< h264 frame_num - int pic_id; ///< h264 pic_num or long_term_pic_idx + int frame_num; ///< h264 frame_num (raw frame_num from slice header) + int pic_id; ///< h264 pic_num (short or long term)
cosmetic
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)
cosmetic [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have never wished to cater to the crowd; for what I know they do not approve, and what they approve I do not know. -- Epicurus