Hi,
On Wed, Dec 21, 2011 at 11:25 AM, доналд овчаров <[email protected]> wrote:
[..]
> - if(ref >= h->ref_count[0]) //FIXME it is posible albeit uncommon
> that slice references differ between slices, we take the easy approuch and
> ignore it for now. If this turns out to have any relevance in practice then
> correct remapping should be added
> - ref=0;
> - fill_rectangle(&s->current_picture.f.ref_index[0][4*h->mb_xy], 2, 2,
> 2, ref, 1);
> + assert(ref >= 0);
> + if (ref >= h->ref_count[0])
> + //FIXME it is posible albeit uncommon that slice references differ
> + //between slices, we take the easy approuch and ignore it for now.
> + //If this turns out to have any relevance in practice then correct
> + //remapping should be added
> + ref = 0;
The comments should go on the line before, i.e.
if (..) // comment
bla();
becomes
// comment
if (..)
bla();
> - fill_rectangle(h->mv_cache[0][ scan8[0] ], 4, 4, 8,
> pack16to32(s->mv[0][0][0],s->mv[0][0][1]), 4);
> + fill_rectangle(h->mv_cache[0][scan8[0]], 4, 4, 8,
> pack16to32(s->mv[0][0][0],
> +
> s->mv[0][0][1]), 4);
2 too many spaces in the second line, the s->mv should vertically align.
> -static void set_mv_strides(MpegEncContext *s, int *mv_step, int *stride){
> - if(s->codec_id == CODEC_ID_H264){
> - H264Context *h= (void*)s;
> +static void set_mv_strides(MpegEncContext *s, int *mv_step, int *stride) {
{ goes on new line for a function opening.
> - dc= s->dc_val[0][mb_x*2 + (i&1) + (mb_y*2 + (i>>1))*s->b8_stride];
> - if(dc<0) dc=0;
> - else if(dc>2040) dc=2040;
> - for(y=0; y<8; y++){
> + for (i = 0; i < 4; i++) {
> + dc = s->dc_val[0][mb_x * 2 + (i & 1) +
> + (mb_y * 2 + (i >> 1)) * s->b8_stride];
The vertical alignment is good, but now the "[" and the "(" vertically
align, which isn't correct. The solution is to add one more space in
the first line, i.e.:
dc = s->dc_val[0][ mv_x ...
(mb_y ...];
Otherwise good job.
> + for (y = 0; y < 8; y++){
Space between ) and {.
> - for(x=0; x<8; x++){
> - dest_y[x + (i&1)*8 + (y + (i>>1)*8)*s->linesize]= dc/8;
> + for (x = 0; x < 8; x++) {
> + dest_y[x + (i & 1) * 8 +
> + (y + (i >> 1) * 8) * s->linesize] = dc / 8;
See comments earlier, second line needs an extra space so [ and (
don't vertically align.
> - dc= - prev_dc
> - + data[x + y*stride]*8
> - - data[x + 1 + y*stride];
> - dc= (dc*10923 + 32768)>>16;
> - prev_dc= data[x + y*stride];
> - data[x + y*stride]= dc;
> + dc = - prev_dc
> + + data[x + y * stride] * 8
> + - data[x + 1 + y * stride];
+ and - of last 2 lines should go one line above. Also, the - prev_dc
in the first line needs no space, it's a negator.
> - dc= - prev_dc
> - + data[x + y *stride]*8
> - - data[x + (y+1)*stride];
> - dc= (dc*10923 + 32768)>>16;
> - prev_dc= data[x + y*stride];
> - data[x + y*stride]= dc;
> + dc = - prev_dc
> + + data[x + y * stride] * 8
> + - data[x + (y+1) * stride];
See comment above.
> +static void guess_dc(MpegEncContext *s, int16_t *dc, int w,
> + int h, int stride, int is_luma)
> +{
[..]
> - for(b_y=0; b_y<h; b_y++){
> - for(b_x=0; b_x<w; b_x++){
> - int color[4]={1024,1024,1024,1024};
> - int distance[4]={9999,9999,9999,9999};
> + for (b_y = 0; b_y < h; b_y++) {
> + for (b_x = 0; b_x < w; b_x++) {
> + int color[4] = {1024, 1024, 1024, 1024};
> + int distance[4] = { 9999, 9999, 9999, 9999 };
Spaces after { and before } in "int color[4]" line.
> - if(!(error&ER_DC_ERROR)) continue; //dc-ok
> + if (IS_INTER(s->current_picture.f.mb_type[mb_index]))
> + continue; //inter
> + if (!(error&ER_DC_ERROR))
> + continue; //dc-ok
Spaces around &.
> /* bottom block */
> - for(j=b_y+1; j<h; j++){
> + for (j=b_y+1; j<h; j++){
> int mb_index_j= (b_x>>is_luma) + (j>>is_luma)*s->mb_stride;
var= val -> var = val.
> int error_j= s->error_status_table[mb_index_j];
Same.
> + for (j = 0; j < 4; j++) {
> + int64_t weight = 256 * 256 * 256 * 16 / distance[j];
> + guess += weight * (int64_t)color[j];
Space between cast and value, i.e. "(int64_t) color[j]".
> +static void h_block_filter(MpegEncContext *s, uint8_t *dst, int w,
> + int h, int stride, int is_luma)
> +{
[..]
> + int left_intra = IS_INTRA(s->current_picture.f.mb_type[( b_x
> >> is_luma) + ( b_y >> is_luma) * s->mb_stride]);
> + int right_intra = IS_INTRA(s->current_picture.f.mb_type[((b_x +
> 1) >> is_luma) + (b_y >> is_luma) * s->mb_stride]);
Please remove the space between "(" and "b_y", I'm pretty sure that
was unintentional.
> + int16_t * left_mv =
> s->current_picture.f.motion_val[0][mvy_stride * b_y +
> +
> mvx_stride * b_x];
> + int16_t * right_mv =
> s->current_picture.f.motion_val[0][mvy_stride * b_y +
> +
> mvx_stride * (b_x + 1)];
int16_t *left_mv, i.e. remove space between * and left_mv. Same for right_mv.
> + if ((!left_intra) && (!right_intra)
> + && FFABS(left_mv[0] - right_mv[0]) +
&& goes on previous line.
> +static void v_block_filter(MpegEncContext *s, uint8_t *dst, int w, int h,
> + int stride, int is_luma)
> +{
[..]
> + int top_damage = top_status&ER_MB_ERROR;
> + int bottom_damage = bottom_status&ER_MB_ERROR;
Spaces around &, and why all the spaces between = and top_status?
> + if ((!top_intra) && (!bottom_intra)
> + && FFABS(top_mv[0]-bottom_mv[0]) +
&& goes on previous line.
> + if (bottom_damage) {
> + dst[offset + x + 8 * stride] = cm[dst[offset + x + 8 *
> stride] -
> + ((d * 7) >> 4)];
> + dst[offset + x + 9 * stride] = cm[dst[offset + x + 9 *
> stride] -
> + ((d * 5) >> 4)];
> + dst[offset + x + 10 * stride] = cm[dst[offset + x + 10 *
> stride] -
> + ((d * 3) >> 4)];
> + dst[offset + x + 11 * stride] = cm[dst[offset + x + 11 *
> stride] -
> + ((d * 1) >> 4)];
> }
Please vertically align by context, i.e. "((d * N) >> 4)" should
vertically align with "dst[offset + ...", not with "cm[...".
> -static void guess_mv(MpegEncContext *s){
> +static void guess_mv(MpegEncContext *s) {
{ goes on its own line.
> - if(IS_INTRA(s->current_picture.f.mb_type[mb_xy])) f=MV_FROZEN;
> //intra //FIXME check
> - if(!(error&ER_MV_ERROR)) f=MV_FROZEN; //inter with
> undamaged MV
> + if (IS_INTRA(s->current_picture.f.mb_type[mb_xy])) f = MV_FROZEN;
> //intra //FIXME check
> + if (!(error&ER_MV_ERROR)) f = MV_FROZEN; //inter with undamaged MV
Spaces around &. Also please split the part after the if (..) on its
own line, i.e.:
if (..)
f = ..;
> + if ((!(s->avctx->error_concealment&FF_EC_GUESS_MVS)) ||
> + num_avail <= mb_width / 2) {
One space too many, i.e. "num_avail" should vertically align with the
second "(", not the "!".
> + if (!(s->error_status_table[mb_xy]&ER_MV_ERROR))
> + continue;
Spaces around &.
> + s->mv_dir = s->last_picture.f.data[0] ? MV_DIR_FORWARD :
> + MV_DIR_BACKWARD;
Vertical alignment, i.e. 4 more spaces for the second line.
> - if((mb_x^mb_y^pass)&1) continue;
> + if ((mb_x^mb_y^pass) & 1)
> + continue;
Spaces around ^.
I'm still reviewing the rest (line 537 and onwards).
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
