Hi,
On Sun, Dec 25, 2011 at 3:19 AM, доналд овчаров <[email protected]> wrote:
> I hope I got this right (about the top-posting)
>
[..]
> @@ -40,26 +40,38 @@
[..]
> + s->dest[0] = s->current_picture.f.data[0] + (s->mb_y * 16 *
s->linesize) +
> + s->mb_x * 16;
> + s->dest[1] = s->current_picture.f.data[1] + (s->mb_y * (16 >>
s->chroma_y_shift) *
> + s->uvlinesize) +
s->mb_x *
> + (16 >>
s->chroma_x_shift);
> + s->dest[2] = s->current_picture.f.data[2] + (s->mb_y * (16 >>
s->chroma_y_shift) *
> + s->uvlinesize) +
s->mb_x *
> + (16 >>
s->chroma_x_shift);
Try to indent this so you can break at outermost contexts. What I mean is
the following. Take the second statement. It looks like this:
a = b + c + d;
Where d is e * f, where f is g >> h, and e if i * j * k, where j is l >> m.
These are the contexts of each of the variables. I want you to try and
break this at the outermost context where-ever possible. So a possible
breaking would look like this:
a = b +
c +
d;
Which would look roughly like this:
s->dest[1] = s->current_picture.f.data[1] +
(s->mb_y * (16 >> s->chroma_y_shift) * s->uvlinesize) +
s->mb_x * (16 >> s->chroma_x_shift);
You can vertically align this a little nicer by aligning some parts of the
second and third line, which you've done in other places.
> + /* 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 */
For new comments, please make sure that the first character after the
comment indicator (/*) aligns vertically. We usually add a * at the start
of each new line, like this:
/* Comment line 1,
* comment line 2,
* comment line 3,
* comment line 4. */
> + fill_rectangle(&s->current_picture.f.ref_index[0][4 * h->mb_xy],
2, 2, 2, ref, 1);
[..]
> + fill_rectangle(h->mv_cache[0][scan8[0]], 4, 4, 8,
pack16to32(s->mv[0][0][0],
> +
s->mv[0][0][1]), 4);
Please limit lines to 80 characters max. In this case, you can break the
line before pack16to32 for the second case, and at the first 2 in the first
case.
> +static void guess_dc(MpegEncContext *s, int16_t *dc, int w,
> + int h, int stride, int is_luma)
[..]
> + for (j = b_x + 1; j < w; j++) {
> + int mb_index_j = (j >> is_luma) + (b_y >> is_luma) *
s->mb_stride;
> + int error_j = s->error_status_table[mb_index_j];
> + int intra_j =
IS_INTRA(s->current_picture.f.mb_type[mb_index_j]);
> + if (intra_j == 0 || !(error_j & ER_DC_ERROR)) {
> + color[0] = dc[j + b_y * stride];
> + distance[0] = j - b_x;
> break;
> }
> }
The two longest lines here don't fit in 80 characters. The first cab be
split at the +, the second at the =:
int mb_index_j = .. +
..;
int intra_j =
..;
> + for (j = b_x - 1; j >= 0; j--) {
> + int mb_index_j = (j>>is_luma) + (b_y>>is_luma) *
s->mb_stride;
> + int error_j = s->error_status_table[mb_index_j];
> + int intra_j =
IS_INTRA(s->current_picture.f.mb_type[mb_index_j]);
> + if (intra_j == 0 || !(error_j & ER_DC_ERROR)) {
> + color[1] = dc[j + b_y * stride];
> + distance[1] = b_x - j;
> break;
> }
> }
Same here. Add spaces around >> please.
> + for (j = b_y + 1; j < h; j++) {
> + int mb_index_j = (b_x>>is_luma) + (j>>is_luma) *
s->mb_stride;
> + int error_j = s->error_status_table[mb_index_j];
> + int intra_j =
IS_INTRA(s->current_picture.f.mb_type[mb_index_j]);
> +
> + if (intra_j == 0 || !(error_j & ER_DC_ERROR)) {
> + color[2] = dc[b_x + j * stride];
> + distance[2] = j - b_y;
> break;
> }
> }
And here, plus spaces around >>.
> + for (j = b_y - 1; j >= 0; j--) {
> + int mb_index_j = (b_x >> is_luma) + (j >> is_luma) *
s->mb_stride;
> + int error_j = s->error_status_table[mb_index_j];
> + int intra_j =
IS_INTRA(s->current_picture.f.mb_type[mb_index_j]);
> + if (intra_j == 0 || !(error_j & ER_DC_ERROR)) {
> + color[3] = dc[b_x + j * stride];
> + distance[3] = b_y - j;
> break;
> }
> }
Same here.
> +static void h_block_filter(MpegEncContext *s, uint8_t *dst, int w,
> + int h, int stride, int is_luma)
[..]
> + int left_status = s->error_status_table[( b_x >>
is_luma) + (b_y >> is_luma) * s->mb_stride];
> + int right_status = s->error_status_table[((b_x + 1) >>
is_luma) + (b_y >> is_luma) * s->mb_stride];
> + 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]);
80 characters. That may be difficult to fix without refactoring that a
little, e.g. you could make s->current_picture.f.mb_type a local variable
so it becomes shorter, and then break at the + inside the []. The top two
lines can be split at the + inside the [], but you'll have to indent the
second line a few spaces less than normal (i.e. don't vertically align all
the way so it starts at the character after "[") for the second line to
still fit on 80 characters.
> + 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)];
Same here, you can fix it by making s->current_picture.f.motion_val[0] a
local variable. You can ask on IRC for help in doing this.
> + if (left_damage) {
> + dst[offset + 7 + y * stride] = cm[dst[offset + 7 + y
* stride] + ((d * 7) >> 4)];
> + dst[offset + 6 + y * stride] = cm[dst[offset + 6 + y
* stride] + ((d * 5) >> 4)];
> + dst[offset + 5 + y * stride] = cm[dst[offset + 5 + y
* stride] + ((d * 3) >> 4)];
> + dst[offset + 4 + y * stride] = cm[dst[offset + 4 + y
* stride] + ((d * 1) >> 4)];
> }
80 characters again. Possible solutions are breaking at the =:
dst[..] =
cm[.. + ..];
> + if (right_damage) {
> + dst[offset + 8 + y * stride] = cm[dst[offset + 8 +
y * stride] - ((d * 7) >> 4)];
> + dst[offset + 9 + y * stride] = cm[dst[offset + 9 +
y * stride] - ((d * 5) >> 4)];
> + dst[offset + 10+ y * stride] = cm[dst[offset + 10 +
y * stride] - ((d * 3) >> 4)];
> + dst[offset + 11+ y * stride] = cm[dst[offset + 11 +
y * stride] - ((d * 1) >> 4)];
> }
Same.
> +static void v_block_filter(MpegEncContext *s, uint8_t *dst, int w, int h,
> + int stride, int is_luma)
[..]
> + int top_status = s->error_status_table[(b_x>>is_luma) + (
b_y >> is_luma) * s->mb_stride];
> + int bottom_status = s->error_status_table[(b_x>>is_luma) +
((b_y + 1 ) >> is_luma) * s->mb_stride];
> + int top_intra =
IS_INTRA(s->current_picture.f.mb_type[(b_x >> is_luma) + ( b_y >>
is_luma) * s->mb_stride]);
> + int bottom_intra =
IS_INTRA(s->current_picture.f.mb_type[(b_x >> is_luma) + ((b_y + 1) >>
is_luma) * s->mb_stride]);
[..]
> + int16_t *top_mv =
s->current_picture.f.motion_val[0][mvy_stride * b_y + mvx_stride * b_x];
> + int16_t *bottom_mv =
s->current_picture.f.motion_val[0][mvy_stride * (b_y + 1) + mvx_stride *
b_x];
See comments earlier.
> + if (top_damage) {
> + dst[offset + x + 7 * stride] = cm[dst[offset + x +
7 * stride] +
> + ((d * 7) >> 4)];
> + dst[offset + x + 6 * stride] = cm[dst[offset + x +
6 * stride] +
> + ((d * 5) >> 4)];
> + dst[offset + x + 5 * stride] = cm[dst[offset + x +
5 * stride] +
> + ((d * 3) >> 4)];
> + dst[offset + x + 4 * stride] = cm[dst[offset + x +
4 * stride] +
> + ((d * 1) >> 4)];
> }
This isn't enough. It still doesn't fit on 80 characters, and in addition
your vrtical alignment is under the "cm..", it should be under the
character behind the first "[", i.e. "dst". My suggestion is to use the
same I recommended earlier:
dst[..] =
cm[.. + ..];
> + 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)];
> }
Same here.
> + if ((!(s->avctx->error_concealment&FF_EC_GUESS_MVS)) ||
Spaces around &.
> + const int mot_index = (mb_x + mb_y * mot_stride)
* mot_step;
80 characters.
> assert(s->last_picture_ptr &&
s->last_picture_ptr->f.data[0]);
Same.
> + if (mb_x > 0 && fixed[mb_xy - 1]
== MV_FROZEN)
> + j = 1;
> + if (mb_x + 1 < mb_width && fixed[mb_xy + 1]
== MV_FROZEN)
> + j = 1;
> + if (mb_y > 0 && fixed[mb_xy - mb_stride]
== MV_FROZEN)
> + j = 1;
> + if (mb_y + 1 < mb_height && fixed[mb_xy + mb_stride]
== MV_FROZEN)
> + j = 1;
[..]
> + if (mb_x > 0 && fixed[mb_xy - 1 ]
== MV_CHANGED)
> + j = 1;
> + if (mb_x + 1 < mb_width && fixed[mb_xy + 1 ]
== MV_CHANGED)
> + j = 1;
> + if (mb_y > 0 && fixed[mb_xy - mb_stride]
== MV_CHANGED)
> + j = 1;
> + if (mb_y + 1 < mb_height && fixed[mb_xy + mb_stride]
== MV_CHANGED)
> + j = 1;
This does look really nice, but again it doesn't fit on 80 characters...
> + if (mb_x > 0 && fixed[mb_xy - 1]) {
> + mv_predictor[pred_count][0] =
> + s->current_picture.f.motion_val[0][mot_index
- mot_step][0];
> + mv_predictor[pred_count][1] =
> + s->current_picture.f.motion_val[0][mot_index
- mot_step][1];
> + ref [pred_count] =
> + s->current_picture.f.ref_index[0][4 * (mb_xy
- 1)];
> pred_count++;
> }
Same. You could fix this particular one by having a local variable for
s->current_picture.f.motion_val[0] or so.
> + if (mb_x + 1 < mb_width && fixed[mb_xy + 1]) {
> + mv_predictor[pred_count][0] =
> + s->current_picture.f.motion_val[0][mot_index
+ mot_step][0];
> + mv_predictor[pred_count][1] =
> + s->current_picture.f.motion_val[0][mot_index
+ mot_step][1];
> + ref [pred_count] =
> + s->current_picture.f.ref_index[0][4 * (mb_xy
+ 1)];
> pred_count++;
> }
Sane.
> + if (mb_y > 0 && fixed[mb_xy - mb_stride]) {
> + mv_predictor[pred_count][0] =
> + s->current_picture.f.motion_val[0][mot_index
- mot_stride * mot_step][0];
> + mv_predictor[pred_count][1] =
> + s->current_picture.f.motion_val[0][mot_index
- mot_stride * mot_step][1];
> + ref [pred_count] =
> + s->current_picture.f.ref_index[0][4 *
(mb_xy-s->mb_stride)];
> pred_count++;
> }
Sane.
> + if (mb_y + 1<mb_height && fixed[mb_xy + mb_stride]) {
> + mv_predictor[pred_count][0] =
> + s->current_picture.f.motion_val[0][mot_index
+ mot_stride * mot_step][0];
> + mv_predictor[pred_count][1] =
> + s->current_picture.f.motion_val[0][mot_index
+ mot_stride * mot_step][1];
> + ref [pred_count] =
> + s->current_picture.f.ref_index[0][4 * (mb_xy
+ s->mb_stride)];
> pred_count++;
> }
Same.
Until line 537. Rest will come in a separate review.
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
