Hi On Tue, Oct 02, 2007 at 11:57:15AM -0400, Jeff Downs wrote:
On Tue, 2 Oct 2007, Andreas ?an wrote: [...] Patch w/out that hunk attached.
Thanks for taking a look.
-Jeff
--- ../ffmpeg-cosmetics/libavcodec/h264.c 2007-10-01 14:02:06.000000000 -0400 +++ libavcodec/h264.c 2007-10-01 15:56:22.000000000 -0400 @@ -175,11 +175,11 @@ if(for_deblock && (h->slice_num == 1 || h->slice_table[mb_xy] == h->slice_table[mb_xy-s->mb_stride]) && !FRAME_MBAFF) return;
//wow what a mess, why didn't they simplify the interlacing&intra stuff, i can't imagine that these complex rules are worth it
- top_xy = mb_xy - s->mb_stride; + top_xy = mb_xy - (s->mb_stride << FIELD_PICTURE);
could mb_stride itself be <<=1 ? or are there many uses which need it like it is even for field pics?
topleft_xy = top_xy - 1; topright_xy= top_xy + 1; left_xy[1] = left_xy[0] = mb_xy-1; left_block[0]= 0; left_block[1]= 1; @@ -1701,11 +1701,11 @@ int extra_height= h->emu_edge_height; int emu=0; const int full_mx= mx>>2; const int full_my= my>>2; const int pic_width = 16*s->mb_width; - const int pic_height = 16*s->mb_height >> MB_MBAFF; + const int pic_height = 16*s->mb_height >> (MB_MBAFF || FIELD_PICTURE);
if(!pic->data[0]) //FIXME this is unacceptable, some senseable error concealment must be done for missing reference frames return;
if(mx&7) extra_width -= 3; @@ -1725,11 +1725,11 @@ qpix_op[luma_xy](dest_y + delta, src_y + delta, h->mb_linesize); }
if(ENABLE_GRAY && s->flags&CODEC_FLAG_GRAY) return;
- if(MB_MBAFF){ + if(MB_MBAFF || FIELD_PICTURE){ // chroma offset when predicting from a field of opposite parity my += 2 * ((s->mb_y & 1) - (h->ref_cache[list][scan8[n]] & 1)); emu |= (my>>3) < 0 || (my>>3) + 8 >= (pic_height>>1); } src_cb= pic->data[1] + (mx>>3) + (my>>3)*h->mb_uvlinesize; @@ -1760,11 +1760,11 @@
dest_y += 2*x_offset + 2*y_offset*h-> mb_linesize; dest_cb += x_offset + y_offset*h->mb_uvlinesize; dest_cr += x_offset + y_offset*h->mb_uvlinesize; x_offset += 8*s->mb_x; - y_offset += 8*(s->mb_y >> MB_MBAFF); + y_offset += 8*(s->mb_y >> (MB_MBAFF || FIELD_PICTURE));
if(list0){ Picture *ref= &h->ref_list[0][ h->ref_cache[0][ scan8[n] ] ]; mc_dir_part(h, ref, n, square, chroma_height, delta, 0, dest_y, dest_cb, dest_cr, x_offset, y_offset, @@ -1793,11 +1793,11 @@
dest_y += 2*x_offset + 2*y_offset*h-> mb_linesize; dest_cb += x_offset + y_offset*h->mb_uvlinesize; dest_cr += x_offset + y_offset*h->mb_uvlinesize; x_offset += 8*s->mb_x; - y_offset += 8*(s->mb_y >> MB_MBAFF); + y_offset += 8*(s->mb_y >> (MB_MBAFF || FIELD_PICTURE));
if(list0 && list1){ /* don't optimize for luma-only case, since B-frames usually * use implicit weights => chroma too. */ uint8_t *tmp_cb = s->obmc_scratchpad;
these can be commited if we have a FIELD_PICTURE=0
@@ -2247,10 +2247,11 @@ int i;
if(MPV_frame_start(s, s->avctx) < 0) return -1; ff_er_frame_start(s); + s->current_picture_ptr->key_frame= 0;
this line could benefit from a comment explaining how it is/has to be initalized for field_pics [...]
+static int split_field_copy(Picture *dest, Picture *src, + int parity, int id_add){ + int match = (src->reference & parity) != 0;
!!(src->reference & parity); [...]
+ int same_i, opp_i; + int i; + int same; + int out_i; + + same_i = 0; + opp_i = 0; + same = 1; + out_i = 0;
declaration and initalization can be merged
+ + while (out_i < dest_len) { + if (same && same_i < src_len) { + i = split_field_copy(dest + out_i, src + same_i, parity, 1); + same = !i; + same_i++; + + } else if (opp_i < src_len) { + i = split_field_copy(dest + out_i, src + opp_i, + PICT_FRAME - parity, 0); + same = i; + opp_i++; + + } else { + break; + } + out_i += i; + }
for(out_i = 0; out_i < dest_len; out_i += i) [...]
+ if (!FIELD_PICTURE) { + structure_sel = 0; + frame_list[0] = h->default_ref_list[0]; + frame_list[1] = h->default_ref_list[1]; + } else { + structure_sel = PICT_FRAME; + frame_list[0] = field_entry_list[0]; + frame_list[1] = field_entry_list[1]; + }
if(FIELD_PICTURE){ }else{ } is a "not" less and IMHO a tiny bit easier to understand [...]
+ if (FIELD_PICTURE) { + frame_num = pred >> 1; + if (pred & 1) { + pic_structure = s->picture_structure; + bot = (s->picture_structure == PICT_BOTTOM_FIELD); + } else { + pic_structure = PICT_FRAME - s->picture_structure; + bot = (s->picture_structure == PICT_TOP_FIELD); + } + } else { + frame_num = pred; + pic_structure = PICT_FRAME; + bot = 0; + } + for(i= h->short_ref_count-1; i>=0; i--){ ref = h->short_ref[i]; - assert(ref->reference == 3); + assert(ref->reference); assert(!ref->long_ref); - if(ref->data[0] != NULL && ref->frame_num == pred && ref->long_ref == 0) // ignore non existing pictures by testing data[0] pointer + if(ref->data[0] != NULL && + ref->frame_num == frame_num && + (ref->reference & pic_structure) && + ref->long_ref == 0) // ignore non existing pictures by testing data[0] pointer break; } if(i>=0) - ref->pic_id= ref->frame_num; + ref->pic_id= pred; }else{ + int long_idx; pic_id= get_ue_golomb(&s->gb); //long_term_pic_idx - if(pic_id>31){ + + if (FIELD_PICTURE) { + long_idx = pic_id >> 1; + if (pic_id & 1) { + pic_structure = s->picture_structure; + bot = (s->picture_structure == PICT_BOTTOM_FIELD); + } else { + pic_structure = PICT_FRAME - s->picture_structure; + bot = (s->picture_structure == PICT_TOP_FIELD); + } + } else { + long_idx = pic_id; + pic_structure = PICT_FRAME; + bot = 0; + }
this looks duplicated [...]
-static inline void unreference_pic(H264Context *h, Picture *pic){ +/** + * Mark a picture as no longer needed for reference. The refmask + * argument allows unreferencing of individual fields or the whole frame. + * If the picture becomes entirely unreferenced, but is being held for + * display purposes, it is marked as such. + * @param refmask mask of fields to unreference; the mask is bitwise + * anded with the reference marking of pic + * @return non-zero if pic becomes entirely unreferenced (except possibly + * for display purposes) zero if one of the fields remains in + * reference + */ +static inline int unreference_pic(H264Context *h, Picture *pic, int refmask){ int i; - pic->reference=0; + if (pic->reference &= refmask) { + return 0; + } else { if(pic == h->delayed_output_pic) pic->reference=DELAYED_PIC_REF; else{ for(i = 0; h->delayed_pic[i]; i++) if(pic == h->delayed_pic[i]){ pic->reference=DELAYED_PIC_REF; break; } } + return 1; + } }
this looks ok, if it where in a seperate patch with the related unreference_pic() call changes that could be commited [...]
/** - * - * @return the removed picture or NULL if an error occurs + * Find a Picture in the short term reference list by frame number. + * @param frame_num frame number to search for + * @param idx the index into h->short_ref where returned picture is found + * undefined if no picture found. + * @return pointer to the found picture, or NULL if no pic with the provided + * frame number is found */ -static Picture * remove_short(H264Context *h, int frame_num){ +static Picture * find_short(H264Context *h, int frame_num, int *idx){ MpegEncContext * const s = &h->s; int i;
- if(s->avctx->debug&FF_DEBUG_MMCO) - av_log(h->s.avctx, AV_LOG_DEBUG, "remove short %d count %d\n", frame_num, h->short_ref_count); - for(i=0; i<h->short_ref_count; i++){ Picture *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){ - 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*)); + if(pic->frame_num == frame_num) { + *idx = i; return pic; } } return NULL; }
this split out could also be in its own patch [...]
- unsigned int long_index= get_ue_golomb(gb); - if(/*h->mmco[i].long_arg >= h->long_ref_count || h->long_ref[ h->mmco[i].long_arg ] == 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_arg= long_index; + h->mmco[i].long_arg= long_arg;
this renaming should be in a seperate patch [...]
} + if(h != h0)
cosmetic [...]
- //FIXME do something with unavailable reference frames [...] + //FIXME do something with unavailable reference frames
cosmetic [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire