Hi,
On Sun, Dec 25, 2011 at 2:09 PM, Ronald S. Bultje <[email protected]>wrote:
> Until line 537. Rest will come in a separate review.
>
> @@ -537,150 +614,167 @@ skip_mean_and_median:
[..]
> + prev_x =
s->last_picture.f.motion_val[0][mot_index][0];
> + prev_y =
s->last_picture.f.motion_val[0][mot_index][1];
> + prev_ref = s->last_picture.f.ref_index[0][4 *
mb_xy];
[..]
> + prev_x =
s->current_picture.f.motion_val[0][mot_index][0];
> + prev_y =
s->current_picture.f.motion_val[0][mot_index][1];
> + prev_ref = s->current_picture.f.ref_index[0][4 *
mb_xy];
80 characters. Same as what I said before, you can fix this by making the
motion_val[0] and ref_index[0] local variables so the lines become shorter.
> + uint8_t *src = s->current_picture.f.data[0] +
mb_x * 16 +
> + mb_y * 16 * s->linesize;
Break at the first + (instead of the second) so it fits on 80 characters.
> - if(ref[j]<0) //predictor intra or otherwise not
available
> + if (ref[j] < 0) // predictor intra or otherwise
not available
> continue;
80 characters. This you can fix by putting the comment on its own line:
// comment
if (..)
continue;
> + for (k = 0; k < 16; k++)
> + score += FFABS(src[k * s->linesize - 1]
- src[k * s->linesize]);
80 characters. fix by e.g. breaking at the -:
score += FFABS(a -
b);
> + for (j = 0; j < mot_step; j++) {
> + s->current_picture.f.motion_val[0][mot_index
+ i + j *
> +
mot_stride][0] = s->mv[0][0][0];
> + s->current_picture.f.motion_val[0][mot_index
+ i + j *
> +
mot_stride][1] = s->mv[0][0][1];
> }
80 characters, probably best solved by making
s->current_picture.f.motion_val[0] a local variable.
> +static int is_intra_more_likely(MpegEncContext *s)
[..]
> if (h->list_count <= 0 || h->ref_count[0] <= 0 ||
!h->ref_list[0][0].f.data[0])
80 characters.
> @@ -688,9 +782,11 @@ static int is_intra_more_likely(MpegEncContext *s){
[..]
> + is_intra_likely -= s->dsp.sad[0](NULL, last_mb_ptr,
last_mb_ptr +
> + s->linesize *
16,s->linesize, 16);
80 characters, plus space after each , (see the missing one on the last
line).
> @@ -698,15 +794,17 @@ static int is_intra_more_likely(MpegEncContext *s){
[..]
> -//printf("is_intra_likely: %d type:%d\n", is_intra_likely, s->pict_type);
> +// printf("is_intra_likely: %d type:%d\n", is_intra_likely,
s->pict_type);
> return is_intra_likely > 0;
> }
Please indent the commented line correctly, i.e.:
// printf(..);
return ..;
}
> +void ff_er_frame_start(MpegEncContext *s) {
{ goes on its own line.
> @@ -716,99 +814,111 @@ void ff_er_frame_start(MpegEncContext *s){
[..]
> * @param status the status at the end (ER_MV_END, ER_AC_ERROR, ...), it
is assumed that no earlier end or
80 characters.
> +void ff_er_add_slice(MpegEncContext *s, int startx, int starty,
> + int endx, int endy, int status)
[..]
> + const int start_i = av_clip(startx + starty * s->mb_width, 0,
s->mb_num - 1);
> + const int end_i = av_clip(endx + endy * s->mb_width, 0,
s->mb_num);
80 characters.
> + if (start_i > end_i || start_xy > end_xy) {
> av_log(s->avctx, AV_LOG_ERROR, "internal error, slice end before
start\n");
> return;
> }
Same.
> + if (status & (ER_AC_ERROR | ER_AC_END)) {
> mask &= ~(ER_AC_ERROR|ER_AC_END);
Spaces around |.
> + if (status & (ER_DC_ERROR | ER_DC_END)) {
> mask &= ~(ER_DC_ERROR|ER_DC_END);
Same.
> + if (status & (ER_MV_ERROR | ER_MV_END)) {
> mask &= ~(ER_MV_ERROR|ER_MV_END);
Same.
> + for (i = start_xy; i < end_xy; i++) {
> s->error_status_table[ i ] &= mask;
> }
No spaces after [ or before ].
> +void ff_er_frame_end(MpegEncContext *s)
[..]
> + if (!s->err_recognition || s->error_count == 0 || s->avctx->lowres ||
> s->avctx->hwaccel ||
> s->avctx->codec->capabilities&CODEC_CAP_HWACCEL_VDPAU ||
> + s->picture_structure != PICT_FRAME ||
> + // we do not support ER of field pictures yet,
> + // though it should not crash if enabled
> + s->error_count == 3 * s->mb_width * (s->avctx->skip_top +
s->avctx->skip_bottom)) {
> + return;
> + };
Wrong indenting. Everything in the if() should vertically align with the
character after the opening bracket, not the opening bracket itself:
if (a ||
b ||
..) {
return;
}
Also the last line is >80 characters, and the field-comment should go on
the line above.
> + for (i = 0; i < 2; i++) {
> + pic->f.ref_index[i] = av_mallocz(s->mb_stride *
> + s->mb_height * 4 *
sizeof(uint8_t));
> + pic->motion_val_base[i] = av_mallocz((size + 4) * 2 *
sizeof(uint16_t));
> + pic->f.motion_val[i] = pic->motion_val_base[i] + 4;
> }
2 lines are >80 characters.
> + if (s->avctx->debug&FF_DEBUG_ER) {
Spaces around &.
> @@ -817,140 +927,142 @@ void ff_er_frame_end(MpegEncContext *s){
[..]
> + for (i = s->mb_num - 1; i >= 0; i--) {
> + const int mb_xy = s->mb_index2xy[i];
> + int error = s->error_status_table[mb_xy];
"int error =", not "int error =".
> + if ((error & ER_MV_END) || (error & ER_DC_END) || (error &
ER_AC_ERROR))
> + end_ok = 1;
>80 characters.
> + if (error&VP_START)
> + end_ok = 0;
Spaces around &.
> + for (i = s->mb_num - 2; i >= s->mb_width + 100; i--) { // FIXME
+ 100 hack
>80 characters; fix it by putting the comment on the line before.
> + if (error2 == (VP_START | ER_MB_ERROR | ER_MB_END) &&
> + error1 != (VP_START | ER_MB_ERROR | ER_MB_END) &&
> + ((error1 & ER_AC_END) || (error1 & ER_DC_END) || (error1
& ER_MV_END))) {
> + // end & uninit
> + end_ok = 0;
> }
Last line in the if() is >80 characters.
> + for (i = 0; i < s->mb_num; i++) {
> + const int mb_xy = s->mb_index2xy[i];
> + int old_error = s->error_status_table[mb_xy];
"int old_error =", not "int old_error =".
> + if (old_error&VP_START)
> + error = old_error & ER_MB_ERROR;
Spaces around &.
> @@ -958,164 +1070,186 @@ void ff_er_frame_end(MpegEncContext *s){
[..]
> + for (mb_y = 0; mb_y < s->mb_height; mb_y++) {
> + for (mb_x = 0; mb_x < s->mb_width; mb_x++) {
> + const int mb_xy = mb_x + mb_y * s->mb_stride;
> + const int mb_type = s->current_picture.f.mb_type[mb_xy];
> + int dir = !s->last_picture.f.data[0];
"int dir =", not "int dir =".
> + if (IS_INTRA(mb_type))
> + continue; // intra
> + if (error & ER_MV_ERROR)
> + continue; // inter with damaged MV
> + if (!(error & ER_AC_ERROR))
> + continue; // undamaged inter
One space between ; and // is enough.
> + for (j = 0; j < 4; j++) {
> + s->mv[0][j][0] =
> + s->current_picture.f.motion_val[dir][mb_index +
(j & 1) +
> + (j >> 1) *
s->b8_stride][0];
> + s->mv[0][j][1] =
> + s->current_picture.f.motion_val[dir][mb_index +
(j & 1) +
> + (j >> 1) *
s->b8_stride][1];
> }
>80 characters for all of them; solve by making
s->current_picture.f.motion_val[dir] a local variable.
> + } else {
> + s->mv_type = MV_TYPE_16X16;
> + s->mv[0][0][0] =
s->current_picture.f.motion_val[dir][mb_x * 2 + mb_y * 2 *
> +
s->b8_stride][0];
> + s->mv[0][0][1] =
s->current_picture.f.motion_val[dir][mb_x * 2 + mb_y * 2 *
> +
s->b8_stride][1];
> }
Same.
> /* guess MVs */
[..]
> + if (!(error & ER_MV_ERROR))
> + continue; // inter with undamaged MV
> + if (!(error & ER_AC_ERROR))
> + continue; // undamaged inter
1 space between ; and // is enough.
> + if (!s->last_picture.f.data[0]) s->mv_dir &=
~MV_DIR_FORWARD;
> + if (!s->next_picture.f.data[0]) s->mv_dir &=
~MV_DIR_BACKWARD;
Please split if() code into:
if (..)
code;
> /* the filters below are not XvMC compatible, skip them */
[..]
> + for (mb_y = 0; mb_y < s->mb_height; mb_y++) {
> + for (mb_x = 0; mb_x < s->mb_width; mb_x++) {
> + int dc, dcu, dcv, y, n;
> + int16_t *dc_ptr;
> + uint8_t *dest_y, *dest_cb, *dest_cr;
You can remove the spaces between variable type and variable name.
> dest_y = s->current_picture.f.data[0] + mb_x * 16 + mb_y *
16 * s->linesize;
> dest_cb = s->current_picture.f.data[1] + mb_x * 8 + mb_y *
8 * s->uvlinesize;
> dest_cr = s->current_picture.f.data[2] + mb_x * 8 + mb_y *
8 * s->uvlinesize;
>80 characters.
> + for (x = 0; x < 8; x++) {
> + dc += dest_y[x + (n & 1) * 8 + (y + (n >> 1) * 8)
* s->linesize];
> }
>80 characters.
> /* guess DC for damaged blocks */
> + guess_dc(s, s->dc_val[0], s->mb_width * 2, s->mb_height * 2,
s->b8_stride, 1);
> + guess_dc(s, s->dc_val[1], s->mb_width, s->mb_height,
s->mb_stride, 0);
> + guess_dc(s, s->dc_val[2], s->mb_width, s->mb_height,
s->mb_stride, 0);
>80 characters.
> /* render DC only intra */
[..]
> + if (!(error & ER_AC_ERROR))
> + continue; // undamaged
One space between ; and // is enough.
> dest_y = s->current_picture.f.data[0] + mb_x * 16 + mb_y *
16 * s->linesize;
> dest_cb = s->current_picture.f.data[1] + mb_x * 8 + mb_y *
8 * s->uvlinesize;
>80 characters.
> @@ -1125,27 +1259,34 @@ void ff_er_frame_end(MpegEncContext *s){
[..]
> + h_block_filter(s, s->current_picture.f.data[0], s->mb_width * 2,
> + s->mb_height * 2, s->linesize, 1);
> + h_block_filter(s, s->current_picture.f.data[1], s->mb_width ,
> + s->mb_height , s->uvlinesize, 0);
> + h_block_filter(s, s->current_picture.f.data[2], s->mb_width ,
> + s->mb_height , s->uvlinesize, 0);
Remove the spaces before the trailing comma on each line.
> /* filter vertical block boundaries */
[..]
> + v_block_filter(s, s->current_picture.f.data[0], s->mb_width * 2,
> + s->mb_height * 2, s->linesize, 1);
> + v_block_filter(s, s->current_picture.f.data[1], s->mb_width ,
> + s->mb_height , s->uvlinesize, 0);
> + v_block_filter(s, s->current_picture.f.data[2], s->mb_width ,
> + s->mb_height , s->uvlinesize, 0);
Same.
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
