Attached is a patch that implements PAFF in H.264. I don't expect this to be accepted right away, due to size and complexity, but I want to get it out there for review and for people to try out and comment on. It is working stable for me on a number of samples, including ones that mix PAFF with progressive and/or MBAFF. Basic overview of how it works: 1. Fields are decoded and deinterlaced. 2. If non-paired fields are encountered, they are not output, but are used for reference if so marked. 3. Reference lists are kept as they were before, but now may also contain unmatched field pairs. Reference list construction splits field pairs out to fields as needed. There is a major shortcoming/known issue with this patch; it disables the error concealing calls for field pictures. This is because the error concealer needs slices to be contiguous regions of the picture buffers. I'd appreciate input on how best to augment the error concealer to handle interlaced slices (or if I am just missing something....) Also interested in hearing how this should be split into smaller patches (I know that at least the addition/changes to header documentation should come out). -Jeff
On Tue, Sep 18, 2007 at 02:30:39PM -0400, Jeff Downs wrote:
I don't expect this to be accepted right away, due to size and complexity, but I want to get it out there for review and for people to try out and comment on.
Just a quick note: You have trailing whitespace in the patch and some lines are awfully long. Diego
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
On Tue, 18 Sep 2007, Vitor Sessak wrote:
Hi,
I hope you don't mind some nitpicking...
No, that's why I posted it. Thanks for your comments.
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.
ok, sure. That is going to be a lot of re-indenting. The attached patch was produced using diff -b. So now the indenting is only changed on lines that are changed for substantial reasons. Hope this is sufficient, but if not, I will get around to un-indenting and then re-indenting. The attached also removes trailing whitespace re Diego's comment. I will try to shorten lines as per his comment in a future revision when/if I get more comments.
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.
Big block of mostly (only?) cosmetics
Ignored in attached patch, as per above. Can revert remaining indent modifications if necessary in a future patch.
@@ -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).
Sure. Same reasoning as above. Will work on split versions; here is the whitespace-ignoring version for anyone else to chime in on in the meantime. -Jeff
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
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
Hi On Thu, Sep 20, 2007 at 03:04:50PM -0400, Jeff Downs wrote:
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: [...]
[...]
+ /* + * 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
skipping rows should be a matter of setting *stride correctly / storing things with the stride which is stored in the context
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.
ok
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.
reference was intended to keep track of which fields are still needed as reference fields/frames, here 3 means both that is 1|2 any code doing something else with it is wrong and has to be changed duplicating it just because the code using it is buggy is absolutely not ok
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.
reference was intended to have the same values as PICT_XXX, i as well see that something went wrong with h264.c in that respect though ... maybe changing the current 1/"keep it around because I need to display it" to 4 to would solve the problem?
-Jeff
Content-Description: Cosmetics patch [...]
@@ -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)
iam against this hunk it doesnt do any good, the rest of patch 1 is ok though [...] Content-Description: MMCO variable rename patch ok [...] Content-Description: Substance of PAFF implementation [...] ill review this soon -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On Mon, 24 Sep 2007, Michael Niedermayer wrote:
Hi
On Thu, Sep 20, 2007 at 03:04:50PM -0400, Jeff Downs wrote:
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: [...]
[...]
+ /* + * 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
skipping rows should be a matter of setting *stride correctly / storing things with the stride which is stored in the context
Agree. I'll look at going that route.
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.
ok
Thanks.
reference was intended to keep track of which fields are still needed as reference fields/frames, here 3 means both that is 1|2 any code doing something else with it is wrong and has to be changed duplicating it just because the code using it is buggy is absolutely not ok
Understand and agree. Are you then proposing that I table the PAFF implementation pending a (separate) patch to fix this? I want to get the desired order right here...
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.
reference was intended to have the same values as PICT_XXX, i as well see that something went wrong with h264.c in that respect though ...
maybe changing the current 1/"keep it around because I need to display it" to 4 to would solve the problem?
Sure. Will look at common code in mpegvideo.c to see if that causes issues there.
Content-Description: Cosmetics patch [...]
@@ -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)
iam against this hunk it doesnt do any good, the rest of patch 1 is ok though
That's fine -- I put that there only to stress the relationship of the three is important and not just arbitrary number choices. I will remove. Please hold to apply - I have a slightly modified version I was about to post when your review came through which reflects the threads of discussion with Martin Z. re pic_num/pic_id.
[...]
Content-Description: MMCO variable rename patch
ok
Again, please hold application pending an additional documentation item for MMCO->short_pic_num, posted shortly in updated patch.
Content-Description: Substance of PAFF implementation [...]
ill review this soon
Likewise, please hold off for a short time while I post a new version reflecting some of Martin Z.'s contributions. -Jeff
Hi On Mon, Sep 24, 2007 at 05:43:05PM -0400, Jeff Downs wrote: [...]
reference was intended to keep track of which fields are still needed as reference fields/frames, here 3 means both that is 1|2 any code doing something else with it is wrong and has to be changed duplicating it just because the code using it is buggy is absolutely not ok
Understand and agree. Are you then proposing that I table the PAFF implementation pending a (separate) patch to fix this? I want to get the desired order right here...
IMHO the AVFrame.reference code should be fixed first unless this turns out to be very complex to fix [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin
On Mon, 24 Sep 2007, Michael Niedermayer wrote:
Hi
On Mon, Sep 24, 2007 at 05:43:05PM -0400, Jeff Downs wrote:
Understand and agree. Are you then proposing that I table the PAFF implementation pending a (separate) patch to fix this? I want to get the desired order right here...
IMHO the AVFrame.reference code should be fixed first unless this turns out to be very complex to fix
OK. I took a second look at how Picture.reference is used in mpegvideo.c and h264.c. For current SVN (no PAFF), its not too difficult to fix (replace use of reference = 1 with 4 as suggested). When PAFF is considered, two problems present: First, a place to store a second field's "is a reference" flag is needed, separate from the reference marking of the first field in a pair. Would using s->dropable for this be an acceptable solution? Or should I continue with current_picture.reference? This way: s->dropable == current field or frame needs reference marking when complete. current_picture.(_ptr->)reference == PICT_XXX, or 4, as dictated by and set in reference picture marking process and delayed picture holding. Second, MPV_frame_start forces a value of 3 onto reference for ref frames. This could be solved by replacing "3" with s->picture_structure. But that doesn't really make complete sense for mpeg2. Another option is to let MPV_frame_start still set 3, but override in h264 with picture_structure. Another option still is to override with zero, and then let the frame end processing (ref marking and pic output) take total control of the reference variable handling. This is good because it isolates reference handling to one area. If you have a preference, let me know. Otherwise I'll look at it a little more and see what makes sense. -Jeff
On Mon, 24 Sep 2007, Jeff Downs wrote:
On Mon, 24 Sep 2007, Michael Niedermayer wrote:
IMHO the AVFrame.reference code should be fixed first unless this turns out to be very complex to fix
OK. I took a second look at how Picture.reference is used in mpegvideo.c and h264.c. For current SVN (no PAFF), its not too difficult to fix (replace use of reference = 1 with 4 as suggested).
When PAFF is considered, two problems present:
First, a place to store a second field's "is a reference" flag is needed, separate from the reference marking of the first field in a pair.
Would using s->dropable for this be an acceptable solution? Or should I continue with current_picture.reference?
This way:
s->dropable == current field or frame needs reference marking when complete.
current_picture.(_ptr->)reference == PICT_XXX, or 4, as dictated by and set in reference picture marking process and delayed picture holding.
Second, MPV_frame_start forces a value of 3 onto reference for ref frames. This could be solved by replacing "3" with s->picture_structure. But that doesn't really make complete sense for mpeg2.
Another option is to let MPV_frame_start still set 3, but override in h264 with picture_structure.
Another option still is to override with zero, and then let the frame end processing (ref marking and pic output) take total control of the reference variable handling. This is good because it isolates reference handling to one area.
Attached is a patch that cleans up handling of Picture.reference in h264 using some of the above-described approach. I decided to change MPV_frame_start to use s->picture_structure for reference in h264 case because it was already testing for h264 there anyway. This prepares for PAFF implementation. This also makes current_picture.reference necessarily == current_picture_ptr->reference and updates any tests of either to have field references in mind. If ok, I will update PAFF implementation based on this. -Jeff
Hi On Tue, Sep 25, 2007 at 03:51:11PM -0400, Jeff Downs wrote:
On Mon, 24 Sep 2007, Jeff Downs wrote:
On Mon, 24 Sep 2007, Michael Niedermayer wrote:
IMHO the AVFrame.reference code should be fixed first unless this turns out to be very complex to fix
OK. I took a second look at how Picture.reference is used in mpegvideo.c and h264.c. For current SVN (no PAFF), its not too difficult to fix (replace use of reference = 1 with 4 as suggested).
When PAFF is considered, two problems present:
First, a place to store a second field's "is a reference" flag is needed, separate from the reference marking of the first field in a pair.
Would using s->dropable for this be an acceptable solution? Or should I continue with current_picture.reference?
This way:
s->dropable == current field or frame needs reference marking when complete.
current_picture.(_ptr->)reference == PICT_XXX, or 4, as dictated by and set in reference picture marking process and delayed picture holding.
Second, MPV_frame_start forces a value of 3 onto reference for ref frames. This could be solved by replacing "3" with s->picture_structure. But that doesn't really make complete sense for mpeg2.
Another option is to let MPV_frame_start still set 3, but override in h264 with picture_structure.
Another option still is to override with zero, and then let the frame end processing (ref marking and pic output) take total control of the reference variable handling. This is good because it isolates reference handling to one area.
Attached is a patch that cleans up handling of Picture.reference in h264 using some of the above-described approach.
I decided to change MPV_frame_start to use s->picture_structure for reference in h264 case because it was already testing for h264 there anyway. This prepares for PAFF implementation.
This also makes current_picture.reference necessarily == current_picture_ptr->reference and updates any tests of either to have field references in mind.
If ok, I will update PAFF implementation based on this.
ok [...]
+/* + * Value of Picture.reference when Picture is not a reference picture, but + * is held for delayed output. + */ +#define DELAYED_PIC_REF 4
not doxygen compatible [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates
Hi On Tue, Sep 25, 2007 at 09:54:47PM -0400, Jeff Downs wrote:
On Wed, 26 Sep 2007, Michael Niedermayer wrote:
[...]
+/* + * Value of Picture.reference when Picture is not a reference picture, but + * is held for delayed output. + */ +#define DELAYED_PIC_REF 4
not doxygen compatible
Fixed in attached.
looks ok if it works [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data
On Thu, Sep 20, 2007 at 03:04:50PM -0400, Jeff Downs wrote: [...]
+ if(h0->current_slice == 0){ + /* figure out if we have a complementary field pair */ + if (s->first_field) { + assert(s->current_picture_ptr && s->current_picture_ptr->data[0]); + + if (s->picture_structure == PICT_FRAME || + s->picture_structure == last_pic_structure) { + /* + * Unmatched field pair. Don't display it, but use + * for reference if so marked + */ + if (s->current_picture_ptr->reference == 1) + s->current_picture_ptr->reference = 0; + s->current_picture_ptr = NULL; + + s->first_field = FIELD_PICTURE; + + } else { + if (s->current_picture.reference && + s->current_picture_ptr->reference == 3 && + s->current_picture_ptr->frame_num != h->frame_num) { + /* + * This and previous field were reference, but had + * different frame_nums. Dump first field (keep in + * reference list) and consider this as first field. + */ + s->first_field = 1; + s->current_picture_ptr = NULL; + + } else { + /* Second field in complementary pair */ + if (s->current_picture.reference) + s->current_picture_ptr->reference = 3; + s->first_field = 0; + } + } + + } else { + /* Frame or first field in a potentially complementary pair */
+ assert(!s->current_picture_ptr);
This assert is false for FM1_BT_B.h264 and FM2_SVA_C.264, please fix this! [...] -- 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
On Mon, 21 Jul 2008, Michael Niedermayer wrote: [...]
+ /* Frame or first field in a potentially complementary pair */
+ assert(!s->current_picture_ptr);
This assert is false for FM1_BT_B.h264 and FM2_SVA_C.264, please fix this!
This assert was to indicate if we would be allocating a new picture (frame_start) without cleaning up the old one. This was important until recently because all pictures were marked reference when created, and not cleared unless they got to frame_end (where current_picture_ptr is cleared). Your recent change (sensibly!) sets references off unless marked as such. For at least FM1_BT_B.h264, the assert is tripped because processing of a frame is aborted (QP 4294967295 out of range) and the next frame processing begins w/out cleaning up current_picture_ptr. At this point, assuming you are ok with the current_picture_ptr NOT being cleared on frame processing errors, the assert could be changed to: assert(!s0->current_picture_ptr || !s0->current_picture_ptr->reference) -Jeff
On Wed, Jul 23, 2008 at 02:28:46PM -0400, Jeff Downs wrote:
On Mon, 21 Jul 2008, Michael Niedermayer wrote:
[...]
+ /* Frame or first field in a potentially complementary pair */
+ assert(!s->current_picture_ptr);
This assert is false for FM1_BT_B.h264 and FM2_SVA_C.264, please fix this!
This assert was to indicate if we would be allocating a new picture (frame_start) without cleaning up the old one. This was important until recently because all pictures were marked reference when created, and not cleared unless they got to frame_end (where current_picture_ptr is cleared).
Your recent change (sensibly!) sets references off unless marked as such.
For at least FM1_BT_B.h264, the assert is tripped because processing of a frame is aborted (QP 4294967295 out of range) and the next frame processing begins w/out cleaning up current_picture_ptr.
At this point, assuming you are ok with the current_picture_ptr NOT being cleared on frame processing errors, the assert could be changed to:
assert(!s0->current_picture_ptr || !s0->current_picture_ptr->reference)
ok, and thanks for the problem analysis [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On Wed, Jul 23, 2008 at 10:36:49PM +0200, Michael Niedermayer wrote:
On Wed, Jul 23, 2008 at 02:28:46PM -0400, Jeff Downs wrote:
On Mon, 21 Jul 2008, Michael Niedermayer wrote:
[...]
+ /* Frame or first field in a potentially complementary pair */
+ assert(!s->current_picture_ptr);
This assert is false for FM1_BT_B.h264 and FM2_SVA_C.264, please fix this!
This assert was to indicate if we would be allocating a new picture (frame_start) without cleaning up the old one. This was important until recently because all pictures were marked reference when created, and not cleared unless they got to frame_end (where current_picture_ptr is cleared).
Your recent change (sensibly!) sets references off unless marked as such.
For at least FM1_BT_B.h264, the assert is tripped because processing of a frame is aborted (QP 4294967295 out of range) and the next frame processing begins w/out cleaning up current_picture_ptr.
At this point, assuming you are ok with the current_picture_ptr NOT being cleared on frame processing errors, the assert could be changed to:
assert(!s0->current_picture_ptr || !s0->current_picture_ptr->reference)
ok, and thanks for the problem analysis
This change does not help at all, the assert stil triggers. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong.
On Thu, 24 Jul 2008, Michael Niedermayer wrote: [...]
At this point, assuming you are ok with the current_picture_ptr NOT being cleared on frame processing errors, the assert could be changed to:
assert(!s0->current_picture_ptr || !s0->current_picture_ptr->reference)
ok, and thanks for the problem analysis
This change does not help at all, the assert stil triggers.
Because of the new gaps in frame num code... it doesn't clear current_picture_ptr at the end. I didn't have that when I suggested the change... You could clear current_picture_ptr after synthesizing the missing frames to be consistent with the clearing of current_picture_ptr at the end of successful decode. Otherwise, the assert would be removed, or modified to check if the reference is legitimate (managed in one of the reference lists). If kept, it probably should be moved to frame_start(). -Jeff
On Thu, Sep 20, 2007 at 03:04:50PM -0400, Jeff Downs wrote:
On Wed, 19 Sep 2007, Michael Niedermayer wrote: [...] case MMCO_LONG: - pic= remove_long(h, mmco[i].long_arg); - if(pic) unreference_pic(h, pic); + j = 1; + if (FIELD_PICTURE && !s->first_field) { + if (h->long_ref[mmco[i].long_arg] == s->current_picture_ptr) { + /* Just mark second field as referenced */ + j = 0; + } else if (s->current_picture_ptr->valid_structure) { + /* First field in pair in short term list. + * This is not allowed; see 7.4.3, notes 2 and 3. + */ + av_log(h->s.avctx, AV_LOG_ERROR, + "illegal long term reference assignment for second " + "field in complementary field pair (first field is " + "short term)\n"); + j = 0; [...] - if(!current_is_long){ - pic= remove_short(h, s->current_picture_ptr->frame_num); + if (!current_ref_assigned && FIELD_PICTURE && + !s->first_field && s->current_picture_ptr->valid_structure) { + + /* Second field of complementary field pair; the first field of + * which is already referenced. If short referenced, it + * should be first entry in short_ref. If not, it must exist + * in long_ref; trying to put it on the short list here is an + * error in the encoded bit stream (ref: 7.4.3, NOTE 2 and 3). + * If on neither list, we have a logic problem elsewhere + */
the notes 2 & 3 in 7.4.3 do not say anything that would support that view. the notes 2 & 3 in 7.4.3.3 could be bent toward this given the assumtation that the stream does not contain non paired reference fields. What made you think such fields do not occur? No i dont have a stream that i know contains such things. iam just curious if theres something in the spec that supports this view. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
On Fri, 25 Jul 2008, Michael Niedermayer wrote:
On Thu, Sep 20, 2007 at 03:04:50PM -0400, Jeff Downs wrote:
On Wed, 19 Sep 2007, Michael Niedermayer wrote: [...] case MMCO_LONG: - pic= remove_long(h, mmco[i].long_arg); - if(pic) unreference_pic(h, pic); + j = 1; + if (FIELD_PICTURE && !s->first_field) { + if (h->long_ref[mmco[i].long_arg] == s->current_picture_ptr) { + /* Just mark second field as referenced */ + j = 0; + } else if (s->current_picture_ptr->valid_structure) { + /* First field in pair in short term list. + * This is not allowed; see 7.4.3, notes 2 and 3. + */ + av_log(h->s.avctx, AV_LOG_ERROR, + "illegal long term reference assignment for second " + "field in complementary field pair (first field is " + "short term)\n"); + j = 0; [...] - if(!current_is_long){ - pic= remove_short(h, s->current_picture_ptr->frame_num); + if (!current_ref_assigned && FIELD_PICTURE && + !s->first_field && s->current_picture_ptr->valid_structure) { + + /* Second field of complementary field pair; the first field of + * which is already referenced. If short referenced, it + * should be first entry in short_ref. If not, it must exist + * in long_ref; trying to put it on the short list here is an + * error in the encoded bit stream (ref: 7.4.3, NOTE 2 and 3). + * If on neither list, we have a logic problem elsewhere + */
the notes 2 & 3 in 7.4.3 do not say anything that would support that view.
Yes. Apologies - the citation as you noticed should be 7.4.3.3. I've changed this.
the notes 2 & 3 in 7.4.3.3 could be bent toward this given the assumtation that the stream does not contain non paired reference fields. What made you think such fields do not occur? No i dont have a stream that i know contains such things. iam just curious if theres something in the spec that supports this view.
The first hunk quoted up there is wrong. I misread spec to be that you cannot have second field in a complementary pair be marked long unless the first had already been marked long. That doesn't hold here. The only restriction is that if the first was made long, then the second must be too. It looks like that comment is the only thing surviving there, and can probably be removed if you agree. Second hunk... Both ways to assign long term to a field (IDR long term ref flag, MMCO current is long, and MMCO short to long) specify that if applied to the first, you have to apply the same to the second field in a pair. Unpaired fields aren't considered by that comment/code (first_field == 1 for unpaired fields); the restriction doesn't apply. All that said, maybe its better to just remove this check and put the field on the short list anyway. I was unsure of how to handle this case since it shouldn't happen. In light of the comments on the first hunk, I do not know if the code properly handles field pairs on mixed lists or not. I am still catching up on the recent work that's been done. -Jeff
On Sat, Jul 26, 2008 at 03:58:34PM -0400, Jeff Downs wrote:
On Fri, 25 Jul 2008, Michael Niedermayer wrote:
On Thu, Sep 20, 2007 at 03:04:50PM -0400, Jeff Downs wrote:
On Wed, 19 Sep 2007, Michael Niedermayer wrote: [...] case MMCO_LONG: - pic= remove_long(h, mmco[i].long_arg); - if(pic) unreference_pic(h, pic); + j = 1; + if (FIELD_PICTURE && !s->first_field) { + if (h->long_ref[mmco[i].long_arg] == s->current_picture_ptr) { + /* Just mark second field as referenced */ + j = 0; + } else if (s->current_picture_ptr->valid_structure) { + /* First field in pair in short term list. + * This is not allowed; see 7.4.3, notes 2 and 3. + */ + av_log(h->s.avctx, AV_LOG_ERROR, + "illegal long term reference assignment for second " + "field in complementary field pair (first field is " + "short term)\n"); + j = 0; [...] - if(!current_is_long){ - pic= remove_short(h, s->current_picture_ptr->frame_num); + if (!current_ref_assigned && FIELD_PICTURE && + !s->first_field && s->current_picture_ptr->valid_structure) { + + /* Second field of complementary field pair; the first field of + * which is already referenced. If short referenced, it + * should be first entry in short_ref. If not, it must exist + * in long_ref; trying to put it on the short list here is an + * error in the encoded bit stream (ref: 7.4.3, NOTE 2 and 3). + * If on neither list, we have a logic problem elsewhere + */
the notes 2 & 3 in 7.4.3 do not say anything that would support that view.
Yes. Apologies - the citation as you noticed should be 7.4.3.3. I've changed this.
the notes 2 & 3 in 7.4.3.3 could be bent toward this given the assumtation that the stream does not contain non paired reference fields. What made you think such fields do not occur? No i dont have a stream that i know contains such things. iam just curious if theres something in the spec that supports this view.
The first hunk quoted up there is wrong. I misread spec to be that you cannot have second field in a complementary pair be marked long unless the first had already been marked long. That doesn't hold here. The only restriction is that if the first was made long, then the second must be too.
hmmmmmm are you sure? I mean, what is the first field then? Its not a long ref because that would force the 2nd to be too, but if it is a short ref, I think IIRC the 2nd must be a short ref as well. Whats left is a non reference field, but i think non reference and reference may not be mixed in 1 pair again IIRC. The spec is very obfuscated and information is spread out nastily so i may be totally wrong, iam just writing this from memory without cross checking ...
It looks like that comment is the only thing surviving there, and can probably be removed if you agree.
Second hunk... Both ways to assign long term to a field (IDR long term ref flag, MMCO current is long, and MMCO short to long) specify that if applied to the first, you have to apply the same to the second field in a pair.
Unpaired fields aren't considered by that comment/code (first_field == 1 for unpaired fields); the restriction doesn't apply.
My problem with unpaired fields is we dont support them at all ... Think of Top Top Top Bottom Bottom Bottom and these get reordered before output ... What iam really curious about is, if the spec allows something like that ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
Hi, I was searching information regarding PicAFF/interlace features and stumbled on this post. My problem with unpaired fields is we dont support them at all ...
Think of Top Top Top Bottom Bottom Bottom and these get reordered before output ... What iam really curious about is, if the spec allows something like that ...
Is this supported by spec?? Best Regards Satendra On Sun, Jul 27, 2008 at 7:59 AM, Michael Niedermayer <michaelni at gmx.at>wrote:
On Fri, 25 Jul 2008, Michael Niedermayer wrote:
On Thu, Sep 20, 2007 at 03:04:50PM -0400, Jeff Downs wrote:
On Wed, 19 Sep 2007, Michael Niedermayer wrote: [...] case MMCO_LONG: - pic= remove_long(h, mmco[i].long_arg); - if(pic) unreference_pic(h, pic); + j = 1; + if (FIELD_PICTURE && !s->first_field) { + if (h->long_ref[mmco[i].long_arg] == s->current_picture_ptr) { + /* Just mark second field as referenced */ + j = 0; + } else if (s->current_picture_ptr->valid_structure) { + /* First field in pair in short term list. + * This is not allowed; see 7.4.3, notes 2 and
+ */ + av_log(h->s.avctx, AV_LOG_ERROR, + "illegal long term reference assignment for second " + "field in complementary field pair (first field is " + "short term)\n"); + j = 0; [...] - if(!current_is_long){ - pic= remove_short(h, s->current_picture_ptr->frame_num); + if (!current_ref_assigned && FIELD_PICTURE && + !s->first_field && s->current_picture_ptr->valid_structure) { + + /* Second field of complementary field pair; the first field of + * which is already referenced. If short referenced, it + * should be first entry in short_ref. If not, it must exist + * in long_ref; trying to put it on the short list here is an + * error in the encoded bit stream (ref: 7.4.3, NOTE 2 and 3). + * If on neither list, we have a logic problem elsewhere + */
the notes 2 & 3 in 7.4.3 do not say anything that would support that view.
Yes. Apologies - the citation as you noticed should be 7.4.3.3. I've changed this.
the notes 2 & 3 in 7.4.3.3 could be bent toward this given the assumtation that the stream does not contain non paired reference fields. What made you think such fields do not occur? No i dont have a stream
On Sat, Jul 26, 2008 at 03:58:34PM -0400, Jeff Downs wrote: 3. that
i know contains such things. iam just curious if theres something in the spec that supports this view.
The first hunk quoted up there is wrong. I misread spec to be that you cannot have second field in a complementary pair be marked long unless the first had already been marked long. That doesn't hold here. The only restriction is that if the first was made long, then the second must be too.
hmmmmmm are you sure? I mean, what is the first field then? Its not a long ref because that would force the 2nd to be too, but if it is a short ref, I think IIRC the 2nd must be a short ref as well. Whats left is a non reference field, but i think non reference and reference may not be mixed in 1 pair again IIRC. The spec is very obfuscated and information is spread out nastily so i may be totally wrong, iam just writing this from memory without cross checking ...
It looks like that comment is the only thing surviving there, and can probably be removed if you agree.
Second hunk... Both ways to assign long term to a field (IDR long term ref flag, MMCO current is long, and MMCO short to long) specify that if applied to the first, you have to apply the same to the second field in a pair.
Unpaired fields aren't considered by that comment/code (first_field == 1 for unpaired fields); the restriction doesn't apply.
My problem with unpaired fields is we dont support them at all ... Think of Top Top Top Bottom Bottom Bottom and these get reordered before output ... What iam really curious about is, if the spec allows something like that ...
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux)
iD8DBQFIi913YR7HhwQLD6sRApl4AJ9kiEA6kFKoYPlXXJLNsCbuxyoiFQCfYxKX BPO9BXolH90XL/I2Hyq1+9M= =8pFY -----END PGP SIGNATURE-----
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
-- ----------------------------------------------------------------------------------------------------------------------------------- "We all agree on the necessity of compromise. We just can't agree on when it's necessary to compromise." ------Larry Wall -----------------------------------------------------------------------------------------------------------------------------------
On Tue, Mar 23, 2010 at 06:04:01PM +0530, Satendra wrote:
Hi,
I was searching information regarding PicAFF/interlace features and stumbled on this post.
My problem with unpaired fields is we dont support them at all ...
Think of Top Top Top Bottom Bottom Bottom and these get reordered before output ... What iam really curious about is, if the spec allows something like that ...
Is this supported by spec??
i am not aware of something in th spec forbidding it, but if its important you should ask on the jvt-experts list unless this has been asked before [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato
Hello, better but not perfect results are given when the attached patch is applied over Jeff's patch. This patch fixes pic_id of reference frames/fields in "Initialisation process for reference picture lists". On Tue, 18 Sep 2007 20:30:39 +0200, Jeff Downs <heydowns at borg.com> wrote:
Attached is a patch that implements PAFF in H.264.
I don't expect this to be accepted right away, due to size and complexity, but I want to get it out there for review and for people to try out and comment on.
It is working stable for me on a number of samples, including ones that mix PAFF with progressive and/or MBAFF.
Basic overview of how it works: 1. Fields are decoded and deinterlaced. 2. If non-paired fields are encountered, they are not output, but are used for reference if so marked. 3. Reference lists are kept as they were before, but now may also contain unmatched field pairs. Reference list construction splits field pairs out to fields as needed.
There is a major shortcoming/known issue with this patch; it disables the error concealing calls for field pictures. This is because the error concealer needs slices to be contiguous regions of the picture buffers. I'd appreciate input on how best to augment the error concealer to handle interlaced slices (or if I am just missing something....)
Also interested in hearing how this should be split into smaller patches (I know that at least the addition/changes to header documentation should come out).
-Jeff
-- Martin Zlomek martin.zlomek at email.cz
On Wed, 19 Sep 2007, Martin Zlomek wrote:
Hello,
better but not perfect results are given when the attached patch is applied over Jeff's patch. This patch fixes pic_id of reference frames/fields in "Initialisation process for reference picture lists".
Thanks. I've merged this into my latest patch revision (see recent post). The first two hunks correct obvious typos in the original PAFF implementation. Thanks again for catching those. -Jeff
The attached patch fixes obvious typo (array length) in reference picture list initialization. PS: I am very sorry for wrong mime type of my previous mail attachment. On Wed, 19 Sep 2007 21:28:18 +0200, Martin Zlomek <martin.zlomek at email.cz> wrote:
Hello,
better but not perfect results are given when the attached patch is applied over Jeff's patch. This patch fixes pic_id of reference frames/fields in "Initialisation process for reference picture lists".
On Tue, 18 Sep 2007 20:30:39 +0200, Jeff Downs <heydowns at borg.com> wrote:
Attached is a patch that implements PAFF in H.264.
I don't expect this to be accepted right away, due to size and complexity, but I want to get it out there for review and for people to try out and comment on.
It is working stable for me on a number of samples, including ones that mix PAFF with progressive and/or MBAFF.
Basic overview of how it works: 1. Fields are decoded and deinterlaced. 2. If non-paired fields are encountered, they are not output, but are used for reference if so marked. 3. Reference lists are kept as they were before, but now may also contain unmatched field pairs. Reference list construction splits field pairs out to fields as needed.
There is a major shortcoming/known issue with this patch; it disables the error concealing calls for field pictures. This is because the error concealer needs slices to be contiguous regions of the picture buffers. I'd appreciate input on how best to augment the error concealer to handle interlaced slices (or if I am just missing something....)
Also interested in hearing how this should be split into smaller patches (I know that at least the addition/changes to header documentation should come out).
-Jeff
-- Martin Zlomek martin.zlomek at email.cz
On Thu, 20 Sep 2007, Martin Zlomek wrote:
The attached patch fixes obvious typo (array length) in reference picture list initialization.
Yes... good catch. Obviously, I don't have samples that use long term reference pictures. Thanks. Incorporated into my working tree; will post new full patch soon. -Jeff
The attached patch fixes CurrPicNum of short term reference fields/frames in reference picture list reordering. -- Martin Zlomek martin.zlomek at email.cz
On Thu, 20 Sep 2007, Martin Zlomek wrote:
The attached patch fixes CurrPicNum of short term reference fields/frames in reference picture list reordering.
--- h264.c.orig 2007-09-20 23:30:38.000000000 +0200 +++ h264.c 2007-09-20 23:30:53.000000000 +0200 @@ -3056,6 +3056,9 @@ add = 0; }
+ if (pred > h->curr_pic_num) + pred -= h->max_pic_num; + for(i= h->short_ref_count-1; i>=0; i--){ ref = h->short_ref[i]; assert(ref->reference == 3);
This doesn't look right -- shouldn't the wrap check come prior to the block at the start of the patch? At this point of insertion, frame number to look for has already been determined. Also, pred value can't be modified like this; it can be used in subsequent iterations of the loop and has to retain the unmodified value. This is actually not intimately tied to PAFF support; this was there all along so I'm guessing this should be kept distinct from the PAFF support patches. So, the attached patch is one more to add, to be applied after the PAFF patches. A different (smaller) patch is needed if we want to fix prior to PAFF. -Jeff
On Fri, 21 Sep 2007 18:06:32 +0200, Jeff Downs <heydowns at borg.com> wrote:
On Thu, 20 Sep 2007, Martin Zlomek wrote:
The attached patch fixes CurrPicNum of short term reference fields/frames in reference picture list reordering.
--- h264.c.orig 2007-09-20 23:30:38.000000000 +0200 +++ h264.c 2007-09-20 23:30:53.000000000 +0200 @@ -3056,6 +3056,9 @@ add = 0; }
+ if (pred > h->curr_pic_num) + pred -= h->max_pic_num; + for(i= h->short_ref_count-1; i>=0; i--){ ref = h->short_ref[i]; assert(ref->reference == 3);
This doesn't look right -- shouldn't the wrap check come prior to the block at the start of the patch? At this point of insertion, frame number to look for has already been determined. Also, pred value can't be modified like this; it can be used in subsequent iterations of the loop and has to retain the unmodified value.
This is actually not intimately tied to PAFF support; this was there all along so I'm guessing this should be kept distinct from the PAFF support patches.
So, the attached patch is one more to add, to be applied after the PAFF patches. A different (smaller) patch is needed if we want to fix prior to PAFF.
-Jeff
It seems that it is wrong, but it is not really true. You are right that I have applied formula 8-37 in "8.2.4.3.1 Reordering process of reference picture lists for short-term reference pictures" section of h.264 spec in a wrong way, but it is "fixed" in each following loop of the for-cycle by line pred &= h->max_pic_num - 1; Anyway, my previously posted patch should be replaced by the attached one; it has the same effect but looks better. For my tests, I use the following sample and the results are better with the patch: http://ftp3.itu.ch/av-arch/jvt-site/draft_conformance/MR6_BT_B.zip -- Martin Zlomek martin.zlomek at email.cz
On Sat, 22 Sep 2007, Martin Zlomek wrote:
It seems that it is wrong, but it is not really true. You are right that I have applied formula 8-37 in "8.2.4.3.1 Reordering process of reference picture lists for short-term reference pictures" section of h.264 spec in a wrong way, but it is "fixed" in each following loop of the for-cycle by line
pred &= h->max_pic_num - 1;
Yes, your new patch makes this much clearer. I've got my head wrapped around the whole PicNum thing now.
Anyway, my previously posted patch should be replaced by the attached one; it has the same effect but looks better.
Yes, I agree - and it is obviously better than the one I posted. I still want to keep this separate from the main PAFF patch since it is fixing something that was broken before (and after) the PAFF implementation. -Jeff
@@ -3064,10 +3065,14 @@ if(reordering_of_pic_nums_idc == 0) pred-= abs_diff_pic_num; else pred+= abs_diff_pic_num; pred &= h->max_pic_num - 1; + + pic_num = pred; + if (pic_num > h->curr_pic_num) + pic_num -= h->max_pic_num; if (FIELD_PICTURE) { - frame_num = pred >> 1; - if (pred & 1) { + frame_num = pic_num >> 1; + if (pic_num & 1) { pic_structure = s->picture_structure; add = (s->picture_structure == PICT_BOTTOM_FIELD); } else { Well, "pic_num -= h->max_pic_num;" should be applied after "frame_num = pic_num >> 1;" because frame_num should not have negative value if pic_num has... -- Martin Zlomek martin.zlomek at email.cz
The attached patch fixes picNumX in "Adaptive memory control decoded reference picture marking process". -- Martin Zlomek martin.zlomek at email.cz
On Thu, 20 Sep 2007, Martin Zlomek wrote:
The attached patch fixes picNumX in "Adaptive memory control decoded reference picture marking process".
--- h264.c.orig 2007-09-20 23:30:38.000000000 +0200 +++ h264.c 2007-09-20 23:31:42.000000000 +0200 @@ -3644,7 +3644,7 @@
h->mmco[i].opcode= opcode; if(opcode==MMCO_SHORT2UNUSED || opcode==MMCO_SHORT2LONG){ - h->mmco[i].short_pic_num= h->curr_pic_num - get_ue_golomb(gb) - 1; + h->mmco[i].short_pic_num= (h->curr_pic_num - get_ue_golomb(gb) - 1) & (h->max_pic_num - 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;
Why? I'm having trouble mapping this to anything in the spec. Compliant streams should contain data that produces a valid short_pic_num here. Non-compliant streams might produce a garbage pic num that is beyond maximum pic num, but that'll just cause the MMCO execution to skip this operation because it can't find a picture with that picture number. -Jeff
On Fri, 21 Sep 2007 18:36:32 +0200, Jeff Downs <heydowns at borg.com> wrote:
On Thu, 20 Sep 2007, Martin Zlomek wrote:
The attached patch fixes picNumX in "Adaptive memory control decoded reference picture marking process".
--- h264.c.orig 2007-09-20 23:30:38.000000000 +0200 +++ h264.c 2007-09-20 23:31:42.000000000 +0200 @@ -3644,7 +3644,7 @@
h->mmco[i].opcode= opcode; if(opcode==MMCO_SHORT2UNUSED || opcode==MMCO_SHORT2LONG){ - h->mmco[i].short_pic_num= h->curr_pic_num - get_ue_golomb(gb) - 1; + h->mmco[i].short_pic_num= (h->curr_pic_num - get_ue_golomb(gb) - 1) & (h->max_pic_num - 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;
Why? I'm having trouble mapping this to anything in the spec. Compliant streams should contain data that produces a valid short_pic_num here. Non-compliant streams might produce a garbage pic num that is beyond maximum pic num, but that'll just cause the MMCO execution to skip this operation because it can't find a picture with that picture number.
You are right that this is not in the spec. But using (short_)pic_num in execute_ref_pic_marking(), you are calculating frame_num in remove_field_short() in MMCO_SHORT2UNUSED case and comparing to frame_num in MMCO_SHORT2LONG case. pic_num can be negative, frame_num should not - pic_num / 2 does not match frame_num in such cases... PS: Be careful to that "x >> 1" and "x / 2" does not equal for negative odd values of x. -- Martin Zlomek martin.zlomek at email.cz
On Sat, 22 Sep 2007, Martin Zlomek wrote:
--- h264.c.orig 2007-09-20 23:30:38.000000000 +0200 +++ h264.c 2007-09-20 23:31:42.000000000 +0200 @@ -3644,7 +3644,7 @@
h->mmco[i].opcode= opcode; if(opcode==MMCO_SHORT2UNUSED || opcode==MMCO_SHORT2LONG){ - h->mmco[i].short_pic_num= h->curr_pic_num - get_ue_golomb(gb) - 1; + h->mmco[i].short_pic_num= (h->curr_pic_num - get_ue_golomb(gb) - 1) & (h->max_pic_num - 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;
Why? I'm having trouble mapping this to anything in the spec. Compliant streams should contain data that produces a valid short_pic_num here. Non-compliant streams might produce a garbage pic num that is beyond maximum pic num, but that'll just cause the MMCO execution to skip this operation because it can't find a picture with that picture number.
You are right that this is not in the spec. But using (short_)pic_num in execute_ref_pic_marking(), you are calculating frame_num in remove_field_short() in MMCO_SHORT2UNUSED case and comparing to frame_num in MMCO_SHORT2LONG case. pic_num can be negative, frame_num should not - pic_num / 2 does not match frame_num in such cases...
PS: Be careful to that "x >> 1" and "x / 2" does not equal for negative odd values of x.
Actually, I'm totally wrong here; sorry! It is in the spec in the pic id calculation and I overlooked it and the relationship to frame number. Your other patch thread made me realize whats going on. Seems like this is the only change (from original PAFF implementation) for pic_id-related stuff if we consider Picture::pic_id for short terms not as a true picture id, but rather the "no wrap" version (basically frame_num for frames, 2*frame_num + parity for fields, aka pic_id & max_pic_num). And this is how the original frame implementation treated pic_id everywhere but here, which is good to keep the changes limited. I'm going to throw this concept into the next iteration of the PAFF patch. Output of ffmpeg against the compliance file you cited (and others in the same group) match. Let me know if it doesn't address everything related to pic_id you posted about in this thread. -Jeff
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode". -- Martin Zlomek martin.zlomek at email.cz
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch. On Sun, 23 Sep 2007 19:02:56 +0200, Martin Zlomek <martin.zlomek at email.cz> wrote:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
-- Martin Zlomek martin.zlomek at email.cz
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested. Michael, could you comment? Thank you, Carl Eugen
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
That's quite an achievement for such a small patch :) Diego
Hello, I have tried the latest trunk on mac os x with a PAFF interlaced flow. It seems to hang after some seconds. I have a 20 megs PAFF interlaced .TS that failes for me. Does anyone want to test it ? 2007/10/11, Diego Biurrun <diego at biurrun.de>:
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
That's quite an achievement for such a small patch :)
Diego _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
On Thu, 11 Oct 2007 11:25:14 +0200, Diego Biurrun <diego at biurrun.de> wrote:
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
That's quite an achievement for such a small patch :)
Diego
Thanks. I have discovered that the PAFF implementation still produces different color values of several pixels in comparision with the reference decoder, even in I-frames. I use the following sample for testing: http://ftp3.itu.ch/av-arch/jvt-site/draft_conformance/FI1_Sony_E.zip and I am working on solution... Martin -- Martin Zlomek martin.zlomek at email.cz
On Thu, Oct 11, 2007 at 05:23:59PM +0200, Martin Zlomek wrote:
On Thu, 11 Oct 2007 11:25:14 +0200, Diego Biurrun <diego at biurrun.de> wrote:
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
That's quite an achievement for such a small patch :)
Thanks.
Hmm, I was just poking fun at Carl Eugen for forgetting to attach the patch ... That nonwithstanding your work on PAFF has been more than welcome and appreciated :) Diego
There is special case for deblocking top- and left-most macroblocks in the ffmpeg implementation. The problem is in the condition, which tests macroblock y-coordinate for equation to 0, and in y-coordinate of top-most macroblocks in bottom fields, where the coordinate is 1. The attached patch fixes this problem - the condition is fulfilled for top-most macroblocks in both top and bottom fields. Decoded I-frames (fields in fact) should be equivalent to those decoded by the reference decoder now. P-frames are still different... On Thu, 11 Oct 2007 17:23:59 +0200, Martin Zlomek <martin.zlomek at email.cz> wrote:
I have discovered that the PAFF implementation still produces different color values of several pixels in comparision with the reference decoder, even in I-frames. I use the following sample for testing:
http://ftp3.itu.ch/av-arch/jvt-site/draft_conformance/FI1_Sony_E.zip
and I am working on solution...
-- Martin Zlomek martin.zlomek at email.cz
I have discovered that h264_loop_filter_strength_mmx2(), used for 'bS' calculation in inter predicted macroblocks, gives wrong output when used in fields (compared with the reference decoder). Attached patch is temporary not-optimized solution giving correct output. I would like to ask Loren Merritt for help - could you look at this, please? Learning MMX instructions and understanding your code would not take me a while... On Sun, 14 Oct 2007 21:17:00 +0200, Martin Zlomek <martin.zlomek at email.cz> wrote:
There is special case for deblocking top- and left-most macroblocks in the ffmpeg implementation. The problem is in the condition, which tests macroblock y-coordinate for equation to 0, and in y-coordinate of top-most macroblocks in bottom fields, where the coordinate is 1. The attached patch fixes this problem - the condition is fulfilled for top-most macroblocks in both top and bottom fields.
Decoded I-frames (fields in fact) should be equivalent to those decoded by the reference decoder now. P-frames are still different...
On Thu, 11 Oct 2007 17:23:59 +0200, Martin Zlomek <martin.zlomek at email.cz> wrote:
I have discovered that the PAFF implementation still produces different color values of several pixels in comparision with the reference decoder, even in I-frames. I use the following sample for testing:
http://ftp3.itu.ch/av-arch/jvt-site/draft_conformance/FI1_Sony_E.zip
and I am working on solution...
-- Martin Zlomek martin.zlomek at email.cz
On Mon, 15 Oct 2007, Martin Zlomek wrote:
I have discovered that h264_loop_filter_strength_mmx2(), used for 'bS' calculation in inter predicted macroblocks, gives wrong output when used in fields (compared with the reference decoder). Attached patch is temporary not-optimized solution giving correct output.
I would like to ask Loren Merritt for help - could you look at this, please? Learning MMX instructions and understanding your code would not take me a while...
It's probably due to mvy_limit. I don't know how to fix it without sacrificing some speed (not that I benchmarked anything), so I would just leave filter_mb_fast for the common case that needs to be fast (i.e. progressive), and let PAFF uses the non-asm code. But if you really want: patch is untested and doesn't modify the prototype and caller to match. --Loren Merritt
On Mon, 15 Oct 2007 14:11:02 +0200, Loren Merritt <lorenm at u.washington.edu> wrote:
On Mon, 15 Oct 2007, Martin Zlomek wrote:
I have discovered that h264_loop_filter_strength_mmx2(), used for 'bS' calculation in inter predicted macroblocks, gives wrong output when used in fields (compared with the reference decoder). Attached patch is temporary not-optimized solution giving correct output.
I would like to ask Loren Merritt for help - could you look at this, please? Learning MMX instructions and understanding your code would not take me a while...
It's probably due to mvy_limit. I don't know how to fix it without sacrificing some speed (not that I benchmarked anything), so I would just leave filter_mb_fast for the common case that needs to be fast (i.e. progressive), and let PAFF uses the non-asm code. But if you really want: patch is untested and doesn't modify the prototype and caller to match.
--Loren Merritt
After deeper analysis, I think that the problem is not in the MMX routine. Different output is given on several test samples when using non-asm code as well. The problem is probably in indeces into h->non_zero_count_cache or in h->non_zero_count_cache as such - this array is used in the condition when assigning bS[i] value of 2: for( i = 0; i < 4; i++ ) { int x = dir == 0 ? edge : i; int y = dir == 0 ? i : edge; int b_idx= 8 + 4 + x + 8*y; int bn_idx= b_idx - (dir ? 8:1); if( h->non_zero_count_cache[b_idx] != 0 || h->non_zero_count_cache[bn_idx] != 0 ) { bS[i] = 2; } else if(!mv_done) { Working on exact problem identification and on patch... -- Martin Zlomek martin.zlomek at email.cz
On Sun, Oct 14, 2007 at 09:17:00PM +0200, Martin Zlomek wrote:
There is special case for deblocking top- and left-most macroblocks in the ffmpeg implementation. The problem is in the condition, which tests macroblock y-coordinate for equation to 0, and in y-coordinate of top-most macroblocks in bottom fields, where the coordinate is 1. The attached patch fixes this problem - the condition is fulfilled for top-most macroblocks in both top and bottom fields.
Decoded I-frames (fields in fact) should be equivalent to those decoded by the reference decoder now. P-frames are still different...
patching file libavcodec/h264.c Hunk #1 FAILED at 6372. Hunk #2 succeeded at 6532 (offset 35 lines). 1 out of 2 hunks FAILED -- saving rejects to file libavcodec/h264.c.rej [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If a bugfix only changes things apparently unrelated to the bug with no further explanation, that is a good sign that the bugfix is wrong.
Forget it, please. This problem has already been fixed in another thread (Hunk #1 at least). On Fri, 14 Dec 2007 11:02:52 +0100, Michael Niedermayer <michaelni at gmx.at> wrote:
On Sun, Oct 14, 2007 at 09:17:00PM +0200, Martin Zlomek wrote:
There is special case for deblocking top- and left-most macroblocks in the ffmpeg implementation. The problem is in the condition, which tests macroblock y-coordinate for equation to 0, and in y-coordinate of top-most macroblocks in bottom fields, where the coordinate is 1. The attached patch fixes this problem - the condition is fulfilled for top-most macroblocks in both top and bottom fields.
Decoded I-frames (fields in fact) should be equivalent to those decoded by the reference decoder now. P-frames are still different...
patching file libavcodec/h264.c Hunk #1 FAILED at 6372. Hunk #2 succeeded at 6532 (offset 35 lines). 1 out of 2 hunks FAILED -- saving rejects to file libavcodec/h264.c.rej
[...]
-- Martin Zlomek martin.zlomek at email.cz
On 11/10/2007, Diego Biurrun <diego at biurrun.de> wrote:
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
That's quite an achievement for such a small patch :)
Diego
I think Carl Eugen Hoyos meant: "The patch mentioned above fixes smearing issues in several files I tested. The patch was posted by Martin Zlomek on 3 Oct 2007 ( paff.5.patch )" Though paff.5.patch isn't very large either :) -V
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
Michael, could you comment?
yes, why does the original code not work? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When you are offended at any man's fault, turn to yourself and study your own failings. Then you will forget your anger. -- Epictetus
On Fri, 12 Oct 2007 02:32:48 +0200, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
Michael, could you comment?
yes, why does the original code not work?
[...]
The original code alters 'my' depending on mutual relation between the current macroblock parity and the picture index (in reference picture list) of the reference macroblock part. Well, I think it is wrong for MBAFF macroblocks anyway. Or can anybody explain to me how can I put into relation macroblock parity and picture index, not parity of the picture at the index? What have I missed? Anyway, if the reference picture is a frame (neither top field nor bottom field), no altering should be realized... -- Martin Zlomek martin.zlomek at email.cz
On Sat, Oct 13, 2007 at 11:52:00AM +0200, Martin Zlomek wrote:
On Fri, 12 Oct 2007 02:32:48 +0200, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
Michael, could you comment?
yes, why does the original code not work?
[...]
The original code alters 'my' depending on mutual relation between the current macroblock parity and the picture index (in reference picture list) of the reference macroblock part. Well, I think it is wrong for MBAFF macroblocks anyway. Or can anybody explain to me how can I put into relation macroblock parity and picture index, not parity of the picture at the index? What have I missed? Anyway, if the reference picture is a frame (neither top field nor bottom field), no altering should be realized...
patch rejected either i dont understand you or you dont understand the code also this needs a benchmark and some _clear_ explanation of why we cannot use the same code for MBAFF and PAFF but why we would need another special case in speed critical code and iam not saying the current MBAFF code should work for PAFF just that i see no need to fork it and calculate the stuff totally differently it seems only ref_cache is wrong for PAFF? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato
On Sat, 13 Oct 2007 21:48:52 +0200, Michael Niedermayer <michaelni at gmx.at> wrote:
On Sat, Oct 13, 2007 at 11:52:00AM +0200, Martin Zlomek wrote:
On Fri, 12 Oct 2007 02:32:48 +0200, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
The attached patch fixes chroma motion vectors as specified in the h.264 spec in section "8.4.1.4 Derivation process for chroma motion vectors", table "Table 8-10 ? Derivation of the vertical component of the chroma vector in field coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
Michael, could you comment?
yes, why does the original code not work?
[...]
The original code alters 'my' depending on mutual relation between the current macroblock parity and the picture index (in reference picture list) of the reference macroblock part. Well, I think it is wrong for MBAFF macroblocks anyway. Or can anybody explain to me how can I put into relation macroblock parity and picture index, not parity of the picture at the index? What have I missed? Anyway, if the reference picture is a frame (neither top field nor bottom field), no altering should be realized...
patch rejected either i dont understand you or you dont understand the code also this needs a benchmark and some _clear_ explanation of why we cannot use the same code for MBAFF and PAFF but why we would need another special case in speed critical code and iam not saying the current MBAFF code should work for PAFF just that i see no need to fork it and calculate the stuff totally differently it seems only ref_cache is wrong for PAFF?
[...]
No, I think that the original code is wrong for MBAFF as well. It uses reference picture index, not parity of that picture. -- Martin Zlomek martin.zlomek at email.cz
On Sun, Oct 14, 2007 at 08:19:01PM +0200, Martin Zlomek wrote:
On Sat, 13 Oct 2007 21:48:52 +0200, Michael Niedermayer <michaelni at gmx.at> wrote:
On Sat, Oct 13, 2007 at 11:52:00AM +0200, Martin Zlomek wrote:
On Fri, 12 Oct 2007 02:32:48 +0200, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Oct 10, 2007 at 08:09:11PM +0000, Carl Eugen Hoyos wrote:
Martin Zlomek <martin.zlomek <at> email.cz> writes:
> The attached patch fixes chroma motion vectors as specified > in the h.264 spec in section "8.4.1.4 Derivation process > for chroma motion vectors", table "Table 8-10 ? Derivation > of the vertical component of the chroma vector in field > coding mode".
Previous patch (paff.5.patch) should be replaced be the attached one. I am not sure if 'emu' flag is updated correctly, but if it is correct in surrounding code, it is correct in this patch as well. If anyone (e.g. svn user lorenm) deeply understands edge emulation, please check this patch.
This patch fixes smearing issues in several files I tested.
Michael, could you comment?
yes, why does the original code not work?
[...]
The original code alters 'my' depending on mutual relation between the current macroblock parity and the picture index (in reference picture list) of the reference macroblock part. Well, I think it is wrong for MBAFF macroblocks anyway. Or can anybody explain to me how can I put into relation macroblock parity and picture index, not parity of the picture at the index? What have I missed? Anyway, if the reference picture is a frame (neither top field nor bottom field), no altering should be realized...
patch rejected either i dont understand you or you dont understand the code also this needs a benchmark and some _clear_ explanation of why we cannot use the same code for MBAFF and PAFF but why we would need another special case in speed critical code and iam not saying the current MBAFF code should work for PAFF just that i see no need to fork it and calculate the stuff totally differently it seems only ref_cache is wrong for PAFF?
[...]
No, I think that the original code is wrong for MBAFF as well. It uses reference picture index, not parity of that picture.
and i disagree here, these are the same in MBAFF if iam correct then you dont understand the code and i wont apply a patch from someone who doesnt understand the code if OTOH you are correct and its wrong for MBAFF as well then your patch is not correct as it does not fix the problem [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker, user questions for the command line tools ffmpeg, ffplay, ... as well as questions about how to use libav* should be sent to the ffmpeg-user mailinglist.
Hi, Attached are new versions of the PAFF patches. As noted prior, there are three patches to be applied in order. Please disregard all prior versions. Order: 1. paff-cosmetics.patch 2. paff-mmcorename.patch 3. paff-noindent-substance.patch Summary: The first two are the cosmetic ones already reviewed by Michael, removing the hunk he was against (mods to PICT_TYPE), and with minor modifications to the documentation reflecting how current h264.c, as well as PAFF implementation, treats Picture.pic_id and MMCO.short_pic_num. Third is the substance of the PAFF implementation, not yet fully reviewed or approved. Changes from last posted version include fixing a typo and the handling of MMCO.short_pic_num as identified by Martin Zlomek. Thanks Martin for your help. This version does not include fixing the use of Picture.valid_structure/Picture.reference. As before, large scale indentation was not changed - I'll provide a patch to do so after acceptance as requested. -Jeff
Hi, Attached are new versions of the PAFF implementation patches. As in prior posts, the three patches are to be applied in order. paff-cosmetics.patch & paff-mmcorename.patch have not changed since last posting. Changed slightly since Michael approved. See previous post for details on what changed. See prior posts for overall description of paff-noindent-substance.patch. Changes since last post: 1. valid_structure member of Picture has been removed; now uses reference member as it was intended. 2. Fixed blocky effects near edges observed in some clips (manifested when second field in pair predicts against first; fixed by drawing edges for first field after decode). 3. Includes modified version of Neil Brown's fix for IDR fields. Known limitations (detailed in prior postings): 1. Error concealer is disabled pending augmentation to handle interlacing after base PAFF implementation is finished. 2. Many PAFF samples will trigger errors in ref pic reordering (abs_diff_pic_num overflow). This is not a PAFF problem. I have a fix for posting in separate thread. Knowing it will take time for complete review/accept, I'd appreciate short-term comments on any additional splitting/cleaning needed. -Jeff
Hi, Various comments... Jeff Downs wrote:
Hi,
[snip]
First two patches applied, (previously ok'ed by Michael) I fixed the doxygen incompat stuff and some vertical alignment.
- const int pic_height = 16*s->mb_height >> MB_MBAFF; + const int pic_height = 16*s->mb_height >> (MB_MBAFF || FIELD_PICTURE);
I believe you should use MB_FIELD here instead (and on all other places) Perhaps that should be a separate patch, i dunno really..
@@ -4835,19 +5209,13 @@ if( cbp_a == 2 ) ctx++; if( cbp_b == 2 ) ctx += 2; return 1 + get_cabac_noinline( &h->cabac, &h->cabac_state[77 + ctx] ); } static int decode_cabac_mb_dqp( H264Context *h) { - MpegEncContext * const s = &h->s; - int mbn_xy; int ctx = 0; int val = 0;
- if( s->mb_x > 0 ) - mbn_xy = s->mb_x + s->mb_y*s->mb_stride - 1; - else - mbn_xy = s->mb_width - 1 + (s->mb_y-1)*s->mb_stride;
if( h->last_qscale_diff != 0 ) ctx++;
while( get_cabac_noinline( &h->cabac, &h->cabac_state[60 + ctx] ) ) {
Unrelated removal of unused code, but deserves to be fixed either way.
On Tue, 2 Oct 2007, Andreas ?man wrote:
First two patches applied, (previously ok'ed by Michael)
I fixed the doxygen incompat stuff and some vertical alignment.
Thanks. Does multiple trailing documentation lines with ///< not work properly? The doxy manual implied that it did.
- const int pic_height = 16*s->mb_height >> MB_MBAFF; + const int pic_height = 16*s->mb_height >> (MB_MBAFF || FIELD_PICTURE);
I believe you should use MB_FIELD here instead (and on all other places) Perhaps that should be a separate patch, i dunno really..
Yes, that would work as well. I did it this way to be consistent with how it was currently being done. MB_FIELD would be a little faster at runtime. I can post a new patch with this change, or provide one to do it after, depending on the desired order of things. Stronger opinions anyone?
@@ -4835,19 +5209,13 @@ if( cbp_a == 2 ) ctx++; if( cbp_b == 2 ) ctx += 2; return 1 + get_cabac_noinline( &h->cabac, &h->cabac_state[77 + ctx] ); } static int decode_cabac_mb_dqp( H264Context *h) { - MpegEncContext * const s = &h->s; - int mbn_xy; int ctx = 0; int val = 0;
- if( s->mb_x > 0 ) - mbn_xy = s->mb_x + s->mb_y*s->mb_stride - 1; - else - mbn_xy = s->mb_width - 1 + (s->mb_y-1)*s->mb_stride;
if( h->last_qscale_diff != 0 ) ctx++;
while( get_cabac_noinline( &h->cabac, &h->cabac_state[60 + ctx] ) ) {
Unrelated removal of unused code, but deserves to be fixed either way.
Yes. My mistake - I meant to remove that hunk prior to posting. Patch w/out that hunk attached. Thanks for taking a look. -Jeff
Jeff Downs wrote:
On Tue, 2 Oct 2007, Andreas ?man wrote:
First two patches applied, (previously ok'ed by Michael)
I fixed the doxygen incompat stuff and some vertical alignment.
Thanks. Does multiple trailing documentation lines with ///< not work properly? The doxy manual implied that it did.
Yes, you are right. I was under the impression that // -style comments in doxygen only operated on single lines.
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
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
Hi On Wed, Oct 03, 2007 at 05:03:13PM -0400, Jeff Downs wrote: [...]
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.
very nicely split :) i dont remember that anyone splited a patch like that lately ...
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
Content-Description: Patch 1: mbdecode
looks ok [...]
Content-Description: Patch 2: unreference
ok [...]
Content-Description: Patch 3: unreference-indent
ok [...]
Content-Description: Patch 4: shortrefmgmt
ok [...]
Content-Description: Patch 5: longoprename
ok [...]
Content-Description: Patch 6: currpicnum
likely ok [...]
Content-Description: Patch 7: keyframe
ok [...]
Content-Description: Patch 8: longrefmgmt
ok [...]
+/** + * 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 int split_field_half_ref_list(Picture *dest, int dest_len, + Picture *src, int src_len, int parity){ + int same = 1; + int same_i = 0; + int opp_i = 0; + int out_i; + int i; + + for (out_i = 0; out_i < dest_len; out_i += i) { + 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; + } + }
i think the variables could be named better, especially "i" is not a good name here [...]
+static int pic_num_extract(H264Context *h, int pic_num, int *structure){ + MpegEncContext * const s = &h->s; + int ret; + + if (FIELD_PICTURE) { + ret = pic_num >> 1; + if (pic_num & 1) { + *structure = s->picture_structure; + } else { + *structure = PICT_FRAME - s->picture_structure; + } + } else { + ret = pic_num; + *structure = PICT_FRAME; + } + return ret; +}
*structure = s->picture_structure; if(FIELD_PICTURE){ if(!(pic_num & 1)) *structure ^= 3; pic_num >>=1; } return pic_num; remaining patch not reviewed (ill review it tomorrow unless you want to split it further) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data
Hi, Michael Niedermayer wrote:
Content-Description: Patch 1: mbdecode looks ok
Content-Description: Patch 2: unreference ok
Content-Description: Patch 3: unreference-indent ok
Content-Description: Patch 4: shortrefmgmt ok
Content-Description: Patch 5: longoprename ok
Content-Description: Patch 6: currpicnum likely ok
Content-Description: Patch 7: keyframe ok
Content-Description: Patch 8: longrefmgmt ok
All of these applied. I run the usual md5sum tests using both normal and MBAFF-content in between each patch to check for any regressions.
On Thu, 4 Oct 2007, Michael Niedermayer wrote: [...]
very nicely split :) i dont remember that anyone splited a patch like that lately ...
Thanks. I'm trying! [...]
Content-Description: Patch 6: currpicnum
likely ok
To further reinforce - Page 93 of the ITU (free) PDF version of the spec: The variable CurrPicNum is derived as follows. If field_pic_flag is equal to 0, CurrPicNum is set equal to frame_num. Otherwise (field_pic_flag is equal to 1), CurrPicNum is set equal to 2 * frame_num + 1. We only use curr_pic_num in picture reordering (and, in later paff patch, for mmco) and the calculations its based on are pretty much straight from spec. [...]
+static int split_field_half_ref_list(Picture *dest, int dest_len, + Picture *src, int src_len, int parity){ [...]
i think the variables could be named better, especially "i" is not a good name here
Sure. i -> field_output. same->same_parity. I kept xxx_i; similar is used elsewhere in the existing code. Revised paff-defreflist.patch (and indent fixer) attached.
+static int pic_num_extract(H264Context *h, int pic_num, int *structure){ [...]
*structure = s->picture_structure; if(FIELD_PICTURE){ if(!(pic_num & 1)) *structure ^= 3; pic_num >>=1; } return pic_num;
Indeed, thanks. Incorporated and revised paff-reorder.patch attached.
remaining patch not reviewed (ill review it tomorrow unless you want to split it further)
I'll continue to split as I have time, given how productive this round was. -Jeff
On Thu, Oct 04, 2007 at 10:44:30AM -0400, Jeff Downs wrote:
On Thu, 4 Oct 2007, Jeff Downs wrote:
Revised paff-defreflist.patch (and indent fixer) attached.
[...]
Indeed, thanks. Incorporated and revised paff-reorder.patch attached.
And.... I pulled the vanishing patch trick....
Content-Description: Patch 1: defreflist
looks ok [...]
Content-Description: Patch 2: defreflist-indent
ok if its just reindention (havnt checked)
Content-Description: Patch 3: Reordering
ok -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are too smart to engage in politics are punished by being governed by those who are dumber. -- Plato
Hi, Michael Niedermayer wrote:
Content-Description: Patch 1: defreflist
looks ok
Content-Description: Patch 2: defreflist-indent
ok if its just reindention (havnt checked)
It was.
Content-Description: Patch 3: Reordering
ok
Doesn't seem to break anything today either :-) Applied these three.
Jeff Downs wrote:
On Thu, 4 Oct 2007, Jeff Downs wrote:
Revised paff-defreflist.patch (and indent fixer) attached.
[...]
Indeed, thanks. Incorporated and revised paff-reorder.patch attached.
And.... I pulled the vanishing patch trick....
Jeff, could you also please provide a decent commit message for all patches (as you did with another set). Trying to figure out the exact purpose of all the patches is a bit time consuming and error prone.
On Fri, 5 Oct 2007, Andreas ?man wrote:
Jeff, could you also please provide a decent commit message for all patches (as you did with another set). Trying to figure out the exact purpose of all the patches is a bit time consuming and error prone.
These were revisions of previously posted ones with the suggested commit message, so I didn't repost the commit message. Sorry - patches from here out will be in their own e-mail as asked and I'll be sure to put commit messages in each time. -Jeff
On Thu, Oct 04, 2007 at 10:41:17AM -0400, Jeff Downs wrote:
On Thu, 4 Oct 2007, Michael Niedermayer wrote: [...]
very nicely split :) i dont remember that anyone splited a patch like that lately ...
Thanks. I'm trying!
I was going to compliment you on this as well, thanks for taking the time to lessen the load on the reviewers. Diego
On Thu, 4 Oct 2007, Jeff Downs wrote:
I'll continue to split as I have time, given how productive this round was.
OK, here is the next round of split patches. I took these diffs against a tree with the defreflist and reordering patches (posted short time ago) applied, but they will likely also apply clean to current svn. Again, in suggested application order with commit message. 1. paff-mbaddress.patch Modifies macroblock addressing and current macroblock y-position for field decoding. Part of PAFF implementation. 2. paff-poc.patch Set Picture.poc for fields and field pairs. Part of PAFF implementation. 3. paff-emuedge.patch Edge emulation for fields. Part of PAFF implementation 4. paff-mmcodecode.patch Augment mmcodecoding process to work properly with fields. Part of PAFF implementation 5. paff-mmcorefrename.patch Cosmetic renaming variable so that it makes more sense for forthcoming PAFF mmco patches. 6. paff-mmco.patch Augment MMCO execution to work with both fields and frames. Part of PAFF implementation 7. paff-mmco-indent.patch Re-indent after PAFF MMCO implementation patch. 8. paff-picmgmt.patch Manage Picture buffers for fields as well as frames. Pair complementary fields into one MPV Picture. Part of PAFF implementation. After these, there are two more -- one to indent after #8 and one to actually enable PAFF handling by properly defining FIELD_PICTURE. Since the indent one is quite large, I'll wait until these can be looked at to post the last two. -Jeff
Hi On Thu, Oct 04, 2007 at 03:07:09PM -0400, Jeff Downs wrote:
On Thu, 4 Oct 2007, Jeff Downs wrote:
I'll continue to split as I have time, given how productive this round was.
OK, here is the next round of split patches. I took these diffs against a tree with the defreflist and reordering patches (posted short time ago) applied, but they will likely also apply clean to current svn.
Again, in suggested application order with commit message.
1. paff-mbaddress.patch Modifies macroblock addressing and current macroblock y-position for field decoding. Part of PAFF implementation.
2. paff-poc.patch Set Picture.poc for fields and field pairs. Part of PAFF implementation.
3. paff-emuedge.patch Edge emulation for fields. Part of PAFF implementation
4. paff-mmcodecode.patch Augment mmcodecoding process to work properly with fields. Part of PAFF implementation
5. paff-mmcorefrename.patch Cosmetic renaming variable so that it makes more sense for forthcoming PAFF mmco patches.
6. paff-mmco.patch Augment MMCO execution to work with both fields and frames. Part of PAFF implementation
7. paff-mmco-indent.patch Re-indent after PAFF MMCO implementation patch.
8. paff-picmgmt.patch Manage Picture buffers for fields as well as frames. Pair complementary fields into one MPV Picture. Part of PAFF implementation.
After these, there are two more -- one to indent after #8 and one to actually enable PAFF handling by properly defining FIELD_PICTURE. Since the indent one is quite large, I'll wait until these can be looked at to post the last two.
-Jeff
Content-Description: Patch 1: mbaddress [...] @@ -3928,6 +3928,11 @@ } s->resync_mb_x = s->mb_x = first_mb_in_slice % s->mb_width; s->resync_mb_y = s->mb_y = (first_mb_in_slice / s->mb_width) << h->mb_aff_frame; + if (FIELD_PICTURE) { + s->resync_mb_y = s->mb_y = s->mb_y * 2; + if (s->picture_structure == PICT_BOTTOM_FIELD) + s->resync_mb_y = s->mb_y = s->mb_y + 1; + } assert(s->mb_y < s->mb_height);
i think this breaks the check above this hunk [...]
@@ -6666,7 +6673,7 @@ s->mb_x = 0; ff_draw_horiz_band(s, 16*s->mb_y, 16); ++s->mb_y; - if(FRAME_MBAFF) { + if(FRAME_MBAFF || FIELD_PICTURE) {
this really should be FRAME_MBAFF_OR_FIELD or something like that [...]
Content-Description: Patch 2: poc [...] ok
Content-Description: Patch 3: emuedge [...] ok
Content-Description: Patch 4: MMCO decode [...] ok
Content-Description: Patch 5: MMCOrefrename [...] ok
Content-Description: Patch 6: MMCO [...] case MMCO_LONG: + j = 1; + if (FIELD_PICTURE && !s->first_field) { + if (h->long_ref[mmco[i].long_arg] == s->current_picture_ptr) { + /* Just mark second field as referenced */ + j = 0; + } else if (s->current_picture_ptr->reference) { + /* First field in pair is in short term list or + * at a different long term index. + * This is not allowed; see 7.4.3, notes 2 and 3. + * Report the problem and keep the pair where it is, + * and mark this field valid. + */ + av_log(h->s.avctx, AV_LOG_ERROR, + "illegal long term reference assignment for second " + "field in complementary field pair (first field is " + "short term or has non-matching long index)\n"); + j = 0; + } + } + + if (j) {
please use a better name than j [...]
+ } else { + av_log(h->s.avctx, AV_LOG_ERROR, "problem in internal reference " + "list handling; marking second " + "field in pair finds first field " + "in reference, but not in any " + "ref list\n");
if this cannot happen unless our code is buggy te correct behavior is assert(0); [...]
Content-Description: Patch 7: MMCO indent
ok [...]
Content-Description: Patch 8: picmgmt
will review later also i think it would be better if you would post 1 patch per email instead of many as later can get quite confusing for everyone if some are approved some are not and some arent revied yet ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB While the State exists there can be no freedom; when there is freedom there will be no State. -- Vladimir Lenin
On Thu, 4 Oct 2007, Michael Niedermayer wrote:
also i think it would be better if you would post 1 patch per email instead of many as later can get quite confusing for everyone if some are approved some are not and some arent revied yet ...
Sure, no problem. Prefer new thread or same for each (related) patch?
On Thu, Oct 04, 2007 at 05:49:11PM -0400, Jeff Downs wrote:
On Thu, 4 Oct 2007, Michael Niedermayer wrote:
also i think it would be better if you would post 1 patch per email instead of many as later can get quite confusing for everyone if some are approved some are not and some arent revied yet ...
Sure, no problem. Prefer new thread or same for each (related) patch?
whichever way you prefer [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No human being will ever know the Truth, for even if they happen to say it by chance, they would not even known they had done so. -- Xenophanes
Hi, Michael Niedermayer wrote:
Hi
@@ -6666,7 +6673,7 @@ s->mb_x = 0; ff_draw_horiz_band(s, 16*s->mb_y, 16); ++s->mb_y; - if(FRAME_MBAFF) { + if(FRAME_MBAFF || FIELD_PICTURE) {
this really should be FRAME_MBAFF_OR_FIELD or something like that
Michael, correct me if i'm wrong here, but isn't this already in place? h264.h: #define MB_FIELD h->mb_field_decoding_flag And h->mb_field_decoding_flag is already correctly computed, both for MBAFF and PAFF, in the beginning of decode_mb_cabac() and decode_mb_cavlc().
also i think it would be better if you would post 1 patch per email instead of many as later can get quite confusing for everyone if some are approved some are not and some arent revied yet ...
Agree here...
On Fri, 5 Oct 2007, Andreas ?man wrote:
Michael Niedermayer wrote: [...]
@@ -6666,7 +6673,7 @@ s->mb_x = 0; ff_draw_horiz_band(s, 16*s->mb_y, 16); ++s->mb_y; - if(FRAME_MBAFF) { + if(FRAME_MBAFF || FIELD_PICTURE) {
this really should be FRAME_MBAFF_OR_FIELD or something like that
Michael, correct me if i'm wrong here, but isn't this already in place?
h264.h: #define MB_FIELD h->mb_field_decoding_flag
And h->mb_field_decoding_flag is already correctly computed, both for MBAFF and PAFF, in the beginning of decode_mb_cabac() and decode_mb_cavlc().
Not exactly. MB_FIELD == MB_MBAFF || FIELD_PICTURE (or, in english, the current macroblock is a field macroblock when its in an MBAFF frame and marked as an MBAFF macroblock or is part of a field picture). This is where we said prior it might be nice to replace the several occurances of MB_MBAFF || FIELD_PICTURE with MB_FIELD, and rightfully so. Contrast with the above, which simply says the current frame has some field macroblocks (FIELD_PICTURE || FRAME_MBAFF). -Jeff
On Thu, 4 Oct 2007, Michael Niedermayer wrote: [...]
Content-Description: Patch 6: MMCO [...] case MMCO_LONG: + j = 1; + if (FIELD_PICTURE && !s->first_field) { + if (h->long_ref[mmco[i].long_arg] == s->current_picture_ptr) { + /* Just mark second field as referenced */ + j = 0; + } else if (s->current_picture_ptr->reference) { + /* First field in pair is in short term list or + * at a different long term index. + * This is not allowed; see 7.4.3, notes 2 and 3. + * Report the problem and keep the pair where it is, + * and mark this field valid. + */ + av_log(h->s.avctx, AV_LOG_ERROR, + "illegal long term reference assignment for second " + "field in complementary field pair (first field is " + "short term or has non-matching long index)\n"); + j = 0; + } + } + + if (j) {
please use a better name than j
Used j because it was already defined and unused in this context. Added new var named unref_pic.
[...]
+ } else { + av_log(h->s.avctx, AV_LOG_ERROR, "problem in internal reference " + "list handling; marking second " + "field in pair finds first field " + "in reference, but not in any " + "ref list\n");
if this cannot happen unless our code is buggy te correct behavior is assert(0);
OK. Added the assert, changed the log message to a comment, removed the reference clearing. The block will be a no-op when NDEBUG is defined. Revised patch attached. If ok, I'll follow up with revised indent patch to match prior to application in svn. -Jeff
On Fri, Oct 05, 2007 at 02:23:48PM -0400, Jeff Downs wrote: [...]
[...]
+ } else { + av_log(h->s.avctx, AV_LOG_ERROR, "problem in internal reference " + "list handling; marking second " + "field in pair finds first field " + "in reference, but not in any " + "ref list\n");
if this cannot happen unless our code is buggy te correct behavior is assert(0);
OK. Added the assert, changed the log message to a comment, removed the reference clearing. The block will be a no-op when NDEBUG is defined.
Revised patch attached.
If ok, I'll follow up with revised indent patch to match prior to application in svn.
patch looks ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Complexity theory is the science of finding the exact solution to an approximation. Benchmarking OTOH is finding an approximation of the exact
On Thu, 4 Oct 2007, Michael Niedermayer wrote:
Content-Description: Patch 1: mbaddress [...] @@ -3928,6 +3928,11 @@ } s->resync_mb_x = s->mb_x = first_mb_in_slice % s->mb_width; s->resync_mb_y = s->mb_y = (first_mb_in_slice / s->mb_width) << h->mb_aff_frame; + if (FIELD_PICTURE) { + s->resync_mb_y = s->mb_y = s->mb_y * 2; + if (s->picture_structure == PICT_BOTTOM_FIELD) + s->resync_mb_y = s->mb_y = s->mb_y + 1; + } assert(s->mb_y < s->mb_height);
i think this breaks the check above this hunk
It does - I missed that. Added proper check, and simplified this part using the new FIELD_OR_MBAFF macro.
[...]
@@ -6666,7 +6673,7 @@ s->mb_x = 0; ff_draw_horiz_band(s, 16*s->mb_y, 16); ++s->mb_y; - if(FRAME_MBAFF) { + if(FRAME_MBAFF || FIELD_PICTURE) {
this really should be FRAME_MBAFF_OR_FIELD or something like that
Made a FIELD_OR_MBAFF_PICTURE, using picture to coincide with spec "coded picture" definition. Revised paff-mbaddress.patch attached. Suggested commit message is: Modifies macroblock addressing and current macroblock y-position for field decoding. Adds convenience definition for pictures that are field or mbaff based. Part of PAFF implementation.
Hi On Fri, Oct 05, 2007 at 04:26:17PM -0400, Jeff Downs wrote:
On Thu, 4 Oct 2007, Michael Niedermayer wrote:
Content-Description: Patch 1: mbaddress [...] @@ -3928,6 +3928,11 @@ } s->resync_mb_x = s->mb_x = first_mb_in_slice % s->mb_width; s->resync_mb_y = s->mb_y = (first_mb_in_slice / s->mb_width) << h->mb_aff_frame; + if (FIELD_PICTURE) { + s->resync_mb_y = s->mb_y = s->mb_y * 2; + if (s->picture_structure == PICT_BOTTOM_FIELD) + s->resync_mb_y = s->mb_y = s->mb_y + 1; + } assert(s->mb_y < s->mb_height);
i think this breaks the check above this hunk
It does - I missed that. Added proper check, and simplified this part using the new FIELD_OR_MBAFF macro.
[...]
@@ -6666,7 +6673,7 @@ s->mb_x = 0; ff_draw_horiz_band(s, 16*s->mb_y, 16); ++s->mb_y; - if(FRAME_MBAFF) { + if(FRAME_MBAFF || FIELD_PICTURE) {
this really should be FRAME_MBAFF_OR_FIELD or something like that
Made a FIELD_OR_MBAFF_PICTURE, using picture to coincide with spec "coded picture" definition.
Revised paff-mbaddress.patch attached. Suggested commit message is:
Modifies macroblock addressing and current macroblock y-position for field decoding. Adds convenience definition for pictures that are field or mbaff based. Part of PAFF implementation.
patch ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
Hi Michael Niedermayer wrote:
Hi
Revised paff-mbaddress.patch attached. Suggested commit message is:
Modifies macroblock addressing and current macroblock y-position for field decoding. Adds convenience definition for pictures that are field or mbaff based. Part of PAFF implementation.
patch ok
Applied
Dear Carl Eugen: The ffmpeg does not surport T-DMB, What are the means? Thank you! ?2007-10-08?"Carl Eugen Hoyos" <cehoyos at ag.or.at> ??? Hi! Michael Niedermayer <michaelni <at> gmx.at> writes: > > Content-Description: Patch 7: MMCO indent > > ok Patch applied. Carl Eugen _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
jiaxin_li <jiaxin_li at 126.com> writes:
Dear Carl Eugen: The ffmpeg does not surport T-DMB, What are the means? Thank you! ??2007-10-08??"Carl Eugen Hoyos" <cehoyos at ag.or.at> ?????? Hi! Michael Niedermayer <michaelni <at> gmx.at> writes: > > Content-Description: Patch 7: MMCO indent > > ok Patch applied. Carl Eugen _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
Don't hijack threads. Don't top-post. Don't write long lines. Don't wrap quoted text into one line. Set the correct character encoding. Don't use the wrong mailing list. -- M?ns Rullg?rd mans at mansr.com
On Thu, Oct 04, 2007 at 03:07:09PM -0400, Jeff Downs wrote:
On Thu, 4 Oct 2007, Jeff Downs wrote:
I'll continue to split as I have time, given how productive this round was.
OK, here is the next round of split patches. I took these diffs against a tree with the defreflist and reordering patches (posted short time ago) applied, but they will likely also apply clean to current svn.
Again, in suggested application order with commit message.
[...]
8. paff-picmgmt.patch Manage Picture buffers for fields as well as frames. Pair complementary fields into one MPV Picture. Part of PAFF implementation.
After these, there are two more -- one to indent after #8 and one to actually enable PAFF handling by properly defining FIELD_PICTURE. Since the indent one is quite large, I'll wait until these can be looked at to post the last two.
-Jeff
[...]
@@ -3991,8 +3998,50 @@ }
if(h0->current_slice == 0){ - if(frame_start(h) < 0) + /* See if we have a decoded first field looking for a pair... */ + if (s0->first_field) { + assert(s0->current_picture_ptr); + assert(s0->current_picture_ptr->data[0]); + assert(s0->current_picture_ptr->reference != DELAYED_PIC_REF); + + /* figure out if we have a complementary field pair */ + if (!FIELD_PICTURE || s->picture_structure == last_pic_structure) { + /* + * Previous field is unmatched. Don't display it, but let it + * remain for reference if marked as such. + */ + s0->current_picture_ptr = NULL; + s0->first_field = FIELD_PICTURE;
first field is set to 0, 1, FIELD_PICTURE and tested against 0/not 0 this is inconsistent also it should be here !=0 already [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle
On Mon, 8 Oct 2007, Michael Niedermayer wrote:
On Thu, Oct 04, 2007 at 03:07:09PM -0400, Jeff Downs wrote:
[...]
8. paff-picmgmt.patch Manage Picture buffers for fields as well as frames. Pair complementary fields into one MPV Picture. Part of PAFF implementation.
[...]
@@ -3991,8 +3998,50 @@ }
if(h0->current_slice == 0){ - if(frame_start(h) < 0) + /* See if we have a decoded first field looking for a pair... */ + if (s0->first_field) { + assert(s0->current_picture_ptr); + assert(s0->current_picture_ptr->data[0]); + assert(s0->current_picture_ptr->reference != DELAYED_PIC_REF); + + /* figure out if we have a complementary field pair */ + if (!FIELD_PICTURE || s->picture_structure == last_pic_structure) { + /* + * Previous field is unmatched. Don't display it, but let it + * remain for reference if marked as such. + */ + s0->current_picture_ptr = NULL; + s0->first_field = FIELD_PICTURE;
first field is set to 0, 1, FIELD_PICTURE and tested against 0/not 0 this is inconsistent also it should be here !=0 already
Hmm not sure I follow your comment. This block is dealing with any sort of unmatched field pair. That is, Had an initial (unmatched as of yet) field. Now decoding either a frame picture (!FIELD_PICTURE) or a field of same parity. FIELD_PICTURE is 0/1. If desired for clarity, it can easily be changed to: first_field = FIELD_PICTURE ? 1 : 0 or if (!FIELD_PICTURE) first_field = 0 -Jeff
On Mon, Oct 08, 2007 at 11:50:48AM -0400, Jeff Downs wrote:
On Mon, 8 Oct 2007, Michael Niedermayer wrote:
On Thu, Oct 04, 2007 at 03:07:09PM -0400, Jeff Downs wrote:
[...]
8. paff-picmgmt.patch Manage Picture buffers for fields as well as frames. Pair complementary fields into one MPV Picture. Part of PAFF implementation.
[...]
@@ -3991,8 +3998,50 @@ }
if(h0->current_slice == 0){ - if(frame_start(h) < 0) + /* See if we have a decoded first field looking for a pair... */ + if (s0->first_field) { + assert(s0->current_picture_ptr); + assert(s0->current_picture_ptr->data[0]); + assert(s0->current_picture_ptr->reference != DELAYED_PIC_REF); + + /* figure out if we have a complementary field pair */ + if (!FIELD_PICTURE || s->picture_structure == last_pic_structure) { + /* + * Previous field is unmatched. Don't display it, but let it + * remain for reference if marked as such. + */ + s0->current_picture_ptr = NULL; + s0->first_field = FIELD_PICTURE;
first field is set to 0, 1, FIELD_PICTURE and tested against 0/not 0 this is inconsistent also it should be here !=0 already
Hmm not sure I follow your comment. This block is dealing with any sort of unmatched field pair. That is,
Had an initial (unmatched as of yet) field. Now decoding either a frame picture (!FIELD_PICTURE) or a field of same parity.
FIELD_PICTURE is 0/1. If desired for clarity, it can easily be changed to:
first_field = FIELD_PICTURE ? 1 : 0
or
if (!FIELD_PICTURE) first_field = 0
forget my comment ... i have confused FIELD_PICTURE with a constant but this brings up another issue, i and probably other devels as well tend to think of upper case names as constants maybe it would be cleaner if we would use code like #define FRAME_MBAFF(h) h->mb_aff_frame instead of the current #define FRAME_MBAFF h->mb_aff_frame that of course is seperate from this patch ... ill retry reviewing it :) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable
On Mon, 8 Oct 2007, Michael Niedermayer wrote:
forget my comment ... i have confused FIELD_PICTURE with a constant but this brings up another issue, i and probably other devels as well tend to think of upper case names as constants
maybe it would be cleaner if we would use code like #define FRAME_MBAFF(h) h->mb_aff_frame instead of the current #define FRAME_MBAFF h->mb_aff_frame
that of course is seperate from this patch ... ill retry reviewing it :)
I would certainly not be opposed to that -- makes sense to me. I can probably contribute something to this effect after we get through this stuff. There's some other cleanup things that I'll be posting later anyway (previously mentioned in this thread). -Jeff
Hi On Thu, Oct 04, 2007 at 03:07:09PM -0400, Jeff Downs wrote: [...]
8. paff-picmgmt.patch Manage Picture buffers for fields as well as frames. Pair complementary fields into one MPV Picture. Part of PAFF implementation.
After these, there are two more -- one to indent after #8 and one to actually enable PAFF handling by properly defining FIELD_PICTURE. Since the indent one is quite large, I'll wait until these can be looked at to post the last two.
-Jeff
[...]
Content-Description: Patch 8: picmgmt
patch ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
On Mon, 8 Oct 2007, Michael Niedermayer wrote:
On Thu, Oct 04, 2007 at 03:07:09PM -0400, Jeff Downs wrote:
8. paff-picmgmt.patch
[...]
After these, there are two more -- one to indent after #8 and one to actually enable PAFF handling by properly defining FIELD_PICTURE. Since the indent one is quite large, I'll wait until these can be looked at to post the last two.
Content-Description: Patch 8: picmgmt
patch ok
And, as promised, here is the indentation patch for the PAFF picmgmt patch mentioned above. -Jeff
Hi On Tue, Oct 09, 2007 at 10:35:05AM -0400, Jeff Downs wrote:
On Mon, 8 Oct 2007, Michael Niedermayer wrote:
On Thu, Oct 04, 2007 at 03:07:09PM -0400, Jeff Downs wrote:
8. paff-picmgmt.patch
[...]
After these, there are two more -- one to indent after #8 and one to actually enable PAFF handling by properly defining FIELD_PICTURE. Since the indent one is quite large, I'll wait until these can be looked at to post the last two.
Content-Description: Patch 8: picmgmt
patch ok
And, as promised, here is the indentation patch for the PAFF picmgmt patch mentioned above.
patch ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you really think that XML is the answer, then you definitly missunderstood the question -- Attila Kinali
Jeff Downs wrote:
And, as promised, here is the indentation patch for the PAFF picmgmt patch mentioned above.
Applied
Hi, Jeff Downs wrote:
Patch w/out that hunk attached.
I was testing this patch yesterday with VDR recordings of channel Pro7 HD. From time to time the image gets distorted. Are you interested in such a stream for testing? Bye. -- Dipl.-Inform. (FH) Reinhard Nissl mailto:rnissl at gmx.de
Hi, Reinhard Nissl wrote:
Hi,
Jeff Downs wrote:
Patch w/out that hunk attached.
I was testing this patch yesterday with VDR recordings of channel Pro7 HD. From time to time the image gets distorted. Are you interested in such a stream for testing?
The answer is most probably yes, so please upload it to incoming ftp directory. Even if Jeff won't have any use for it, it'll be one more sample to the sample collection. Guillaume
Hi, Guillaume Poirier wrote:
Patch w/out that hunk attached.
I was testing this patch yesterday with VDR recordings of channel Pro7 HD. From time to time the image gets distorted. Are you interested in such a stream for testing?
The answer is most probably yes, so please upload it to incoming ftp directory. Even if Jeff won't have any use for it, it'll be one more sample to the sample collection.
Please have a look at directory /MPlayer/incoming/h264_paff, files paff_artefacts.es.h264.txt and paff_artefacts.es.h264. Bye. -- Dipl.-Inform. (FH) Reinhard Nissl mailto:rnissl at gmx.de
On Wed, 3 Oct 2007, Guillaume Poirier wrote:
Hi,
Reinhard Nissl wrote:
Hi,
Jeff Downs wrote:
Patch w/out that hunk attached.
I was testing this patch yesterday with VDR recordings of channel Pro7 HD. From time to time the image gets distorted. Are you interested in such a stream for testing?
The answer is most probably yes, so please upload it to incoming ftp directory. Even if Jeff won't have any use for it, it'll be one more sample to the sample collection.
I am happy to take a look at all problematic samples. I cannot promise any sort of timeline for doing so, however, as I remain committed at this point to getting the base implementation in place. I fully intend to support with bug fixes/enhancements from there. Priority will be given to those who have tested their samples with the reference decoder (and who state so) and those that describe the problem well (crashes, visual problems, error messages). Anyone having problems, please be sure I haven't mentioned the type of error you are getting in prior posts as a known issue. I would still like the sample, but won't take time to form detailed responses to all submissions. In brief, known issues include: 1. Errors about abs_diff_pic_num overflow 2. Errors about get_buffer failing (happens with non-PAFF files/pre-PAFF patches, too) -Jeff
participants (15)
-
andreas＠olebyn.nu -
cehoyos＠ag.or.at -
diego＠biurrun.de -
gpoirier＠mplayerhq.hu -
H.O.W.aka.V+ffmpeg＠gmail.com -
heydowns＠borg.com -
jiaxin_li＠126.com -
lorenm＠u.washington.edu -
mans＠mansr.com -
martin.zlomek＠email.cz -
michaelni＠gmx.at -
rnissl＠gmx.de -
spsatendra＠gmail.com -
trollito＠gmail.com -
vitor1001＠gmail.com