On Wed, 19 Sep 2007, Michael Niedermayer wrote:
just a quick review below, ill wait with a full review until there are clean and well split patches ...
Thanks. I knew it had to be split and I'm happy to be receiving guidance as to where. I've attached 3 patches, which are intended to be examined/applied in order: 1. paff-cosmetics.patch: documentation clarifications and change PICT_* to reinforce that the forthcoming PAFF code depends on PICT_FRAME == PICT_TOP_FIELD | PICT_BOTTOM_FIELD 2. paff-mmcorename.patch: Renaming for mmco structure fields; new names make more sense than old when considering their operations on field pictures (in addition to frame pictures). 3. paff-noindent-substance.patch: The actual substance of the PAFF implementation. This is the remainder of my original patch, with all large blocks of reindentation removed at Vitor's suggestion, intending to be re-indented after this is accepted. It also contains several fixes and changes from the last patch to address some of Michael's concerns (below), Diego's comments about long lines (all substantially changed or added lines are <= 80 chars), and incorporates Martin Zlomek's posted contributions. Comments to detailed concerns below.
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
Done via paff-mmcorename.patch.
i dont think this should be inline, it doesnt appear speed critical
No longer inline.
+ 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
Fixed by pulling common pieces to a function.
[...]
/** + * 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()
Fixed by folding functionality into 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
This is different for the second field in a pair, a by product of not calling MPV_frame_start to get a new picture for the second field in a pair. I implemented it that way partly because that is how the mpeg2 decoder works. mpeg2 decoder uses current_picture to represent the current field parameters (data and linesize, mostly) which differ from *current_picture_ptr parameters (represents full frame). In this case, current_picture_ptr->reference is combined reference status of top and bottom fields, while current_picture.reference is reference status of currently decoding field (as in data/linesize above). This area needs the reference marking of the currently decoding field, regardless of any prior field in a pair. If it cannot be done using current_picture.reference, then should a field be added to H264Context to represent this?
[...]
+ /* + * 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?
The error concealer doesn't seem to have any notion of being able to skip rows, both when marking errors and doing the concealing calculations. Calls to ff_er_add_slice end result in error_count going through the roof for perfectly ok frames. The frame_end call then falsely corrects the errors. It was easiest and arguably more efficient to abort the frame_end call instead of all the different add_slice calls. I would be willing to work to fix this after we get through this already behemoth patch, at which time this could be removed. There was a comment explaining this near an add_slice call in my previous patch. I've moved that here in the current patch.
--- 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
Yes. From what I've been able to tell by reading the existing code, Picture.reference == 1 means keep it around because I need to display it still. Picture.reference == 3 means keep it around because it is a reference frame. This new variable is used to track reference marking of the top and bottom fields. Most of the new reference marking code uses this to keep track of which field in a pair (or both) is used for reference. I created a new variable to avoid modifying the behavior of the reference variable and because it was very nice to be able to set it to PICT_XXX values, something I couldn't do with the existing variable because PICT_TOP_FIELD == 1 == existing semantic meaning above. I'm open to suggestions on changing this if needed; if the replacement doesn't have it taking on values of PICT_XXX, then it could be a substantial rework. -Jeff