On Wed, 3 Oct 2007, Michael Niedermayer wrote:
Hi
On Tue, Oct 02, 2007 at 11:57:15AM -0400, Jeff Downs wrote:
--- ../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?
Most uses need it as-is; most times it is used to compute current macroblock location (mb_x + mb_y * mb_stride). y is in frame units all the time, currently, which is really convenient for supporting mbaff. mb_stride is setup in MPV_common_init (essentially) once. We'd have to move it to a per-frame setting to make it work right with frame & field intermingling, and change how mb_y, height are incremented/tested.
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
Assume you mean with a #define FIELD_PICTURE 0 until rest of the overall patch is incorportated. If so, patch doing so for application attached (paff-mbdecode.patch).
@@ -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
Added.
[...]
+static int split_field_copy(Picture *dest, Picture *src, + int parity, int id_add){ + int match = (src->reference & parity) != 0;
!!(src->reference & parity);
Changed.
[...]
+ 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
Changed.
+ + 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)
Changed.
[...]
+ 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
Sure. Changed.
[...]
+ 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
Assuming you mean derivation of pic_structure, long_idx/frame_num, then its now pulled out to a function.
[...]
-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
Separated. paff-unreference.patch
[...]
/** - * - * @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
With associated changes to remove_short I assume. Separated into paff-shortrefmgmt.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
paff-longoprename.patch
[...]
} + if(h != h0)
cosmetic
Byproduct of diff; with the new block above it, thought the extra line helped readability. Anyway, removed.
[...]
- //FIXME do something with unavailable reference frames [...] + //FIXME do something with unavailable reference frames
cosmetic
Didn't belong in the first place. Removed - sorry. Separated patches to be applied in order, w/ suggested commit messages: 1. paff-mbdecode.patch Partial PAFF implementation at macroblock level. PAFF support disabled until implementation complete. 2. paff-unreference.patch Modify unreference_pic implementation with PAFF in mind. 3. paff-unreference-indent.patch Re-indent unreference_pic. 4. paff-shortrefmgmt.patch Further modularize short reference list management for upcoming PAFF implementation. 5. paff-longoprename.patch Rename variable to make sense in both field and frame contexts (support of PAFF implementation). Then there is the remainder that you didn't specifically ask to be split, but that I've tried to split further with the intention of making it easier to review. 6. paff-currpicnum.patch Fix h->curr_pic_num for field pictures. Necessary for proper PAFF support. 7. paff-keyframe.patch Fix Picture.key_frame setting to be compatible with frame and field contexts. Part of PAFF implementation. Contributed in part by Neil Brown. 8. paff-longrefmgmt.patch Reorganize long reference management to minimize code duplication in upcoming PAFF implementation. 9. paff-defreflist.patch Support functions and changes to default reference list creation for PAFF. 10. paff-defreflist-indent.patch Reindent fill_default_ref_list after changes for PAFF 11. paff-reordering.patch Support function and changes to reference picture reordering for PAFF. The remainder of the original patch is in paff-noident.patch. Bulk is MMCO for fields and interleaving of fields into one AVPicture. Figure we can try to get through the above, then separate even further still if desired. -Jeff