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