Hi,
On Sat, Dec 24, 2011 at 4:07 PM, Ronald S. Bultje <[email protected]>wrote:
> Until line 1512.
>
> @@ -1512,101 +1548,104 @@ static av_always_inline void
encode_mb_internal(MpegEncContext *s, int motion_x,
[..]
> - if(!(s->flags&CODEC_FLAG_QP_RD)){
> + if (!(s->flags&CODEC_FLAG_QP_RD)) {
Spaces around &.
> + ptr_y = s->new_picture.f.data[0] + (mb_y * 16 * wrap_y)
+ mb_x * 16;
> ptr_cb = s->new_picture.f.data[1] + (mb_y * mb_block_height *
wrap_c) + mb_x * 8;
> ptr_cr = s->new_picture.f.data[2] + (mb_y * mb_block_height *
wrap_c) + mb_x * 8;
>80 characters for each of these lines.
> + if (mb_x * 16 + 16 > s->width || mb_y * 16 + 16 > s->height) {
> uint8_t *ebuf= s->edge_emu_buffer + 32;
> + s->dsp.emulated_edge_mc(ebuf, ptr_y, wrap_y, 16, 16, mb_x * 16,
mb_y * 16, s->width, s->height);
> + ptr_y = ebuf;
> + s->dsp.emulated_edge_mc(ebuf + 18 * wrap_y, ptr_cb, wrap_c, 8,
mb_block_height,
> + mb_x * 8, mb_y * 8, s->width >> 1,
s->height >> 1);
> + ptr_cb = ebuf + 18 * wrap_y;
> + s->dsp.emulated_edge_mc(ebuf + 18 * wrap_y + 8, ptr_cr, wrap_c,
8, mb_block_height,
> + mb_x * 8, mb_y * 8, s->width >> 1,
s->height >> 1);
> + ptr_cr = ebuf + 18 * wrap_y + 8;
> }
>80 characters for most of these lines.
> + s->interlaced_dct = 0;
> + progressive_score = s->dsp.ildct_cmp[4](s, ptr_y,
NULL, wrap_y, 8) +
> + s->dsp.ildct_cmp[4](s, ptr_y + wrap_y *
8, NULL, wrap_y, 8) - 400;
[..]
> + if (progressive_score > 0) {
> + interlaced_score = s->dsp.ildct_cmp[4](s, ptr_y,
NULL, wrap_y * 2, 8) +
> + s->dsp.ildct_cmp[4](s, ptr_y +
wrap_y, NULL, wrap_y * 2, 8);
>80 characters for all of this.
> s->dsp.get_pixels(s->block[0], ptr_y , wrap_y);
> - s->dsp.get_pixels(s->block[1], ptr_y + 8, wrap_y);
> + s->dsp.get_pixels(s->block[1], ptr_y + 8 , wrap_y);
> s->dsp.get_pixels(s->block[2], ptr_y + dct_offset , wrap_y);
> s->dsp.get_pixels(s->block[3], ptr_y + dct_offset + 8, wrap_y);
I just want to make sure you're doing this intentionally, you see how the
+8 was originally indented, right?
> - if(s->flags&CODEC_FLAG_GRAY){
> + if (s->flags&CODEC_FLAG_GRAY) {
Spaces around &.
> + if (!s->chroma_y_shift) { /* 422 */
> + s->dsp.get_pixels(s->block[6], ptr_cb + (dct_offset >>
1), wrap_c);
> + s->dsp.get_pixels(s->block[7], ptr_cr + (dct_offset >>
1), wrap_c);
> }
>80 characters.
> @@ -1615,43 +1654,43 @@ static av_always_inline void
encode_mb_internal(MpegEncContext *s, int motion_x,
[..]
> if (s->mv_dir & MV_DIR_FORWARD) {
> MPV_motion(s, dest_y, dest_cb, dest_cr, 0,
s->last_picture.f.data, op_pix, op_qpix);
> + op_pix = s->dsp.avg_pixels_tab;
> + op_qpix = s->dsp.avg_qpel_pixels_tab;
> }
> if (s->mv_dir & MV_DIR_BACKWARD) {
> MPV_motion(s, dest_y, dest_cb, dest_cr, 1,
s->next_picture.f.data, op_pix, op_qpix);
> }
>80 characters x2.
> - if(s->flags&CODEC_FLAG_INTERLACED_DCT){
> + if (s->flags&CODEC_FLAG_INTERLACED_DCT) {
Spaces around &.
> + s->interlaced_dct = 0;
> + progressive_score = s->dsp.ildct_cmp[0](s, dest_y,
ptr_y, wrap_y, 8) +
> + s->dsp.ildct_cmp[0](s, dest_y + wrap_y *
8, ptr_y + wrap_y * 8, wrap_y, 8) - 400;
>80 characters.
> - if(s->avctx->ildct_cmp == FF_CMP_VSSE) progressive_score -=
400;
> + if (s->avctx->ildct_cmp == FF_CMP_VSSE) progressive_score -=
400;
Please break if() code like this:
if (..)
code;
> + if (progressive_score > 0) {
> + interlaced_score = s->dsp.ildct_cmp[0](s, dest_y,
ptr_y, wrap_y * 2, 8) +
> + s->dsp.ildct_cmp[0](s, dest_y +
wrap_y, ptr_y + wrap_y, wrap_y * 2, 8);
>80 characters.
> @@ -1661,106 +1700,106 @@ static av_always_inline void
encode_mb_internal(MpegEncContext *s, int motion_x,
> s->dsp.diff_pixels(s->block[2], ptr_y + dct_offset , dest_y +
dct_offset , wrap_y);
> s->dsp.diff_pixels(s->block[3], ptr_y + dct_offset + 8, dest_y +
dct_offset + 8, wrap_y);
>80 characters.
> + if (s->flags&CODEC_FLAG_GRAY) {
Spaces around &.
> + if (!s->chroma_y_shift) { /* 422 */
> + s->dsp.diff_pixels(s->block[6], ptr_cb + (dct_offset >>
1), dest_cb + (dct_offset >> 1), wrap_c);
> + s->dsp.diff_pixels(s->block[7], ptr_cr + (dct_offset >>
1), dest_cr + (dct_offset >> 1), wrap_c);
> }
>80 characters.
> + if (s->current_picture.mc_mb_var[s->mb_stride * mb_y + mb_x] < 2
* s->qscale * s->qscale) {
>80 characters.
> //FIXME optimize
Space between "//" and actual comment text.
> + if (s->dsp.sad[1](NULL, ptr_y , dest_y
, wrap_y, 8) < 20 * s->qscale) skip_dct[0] = 1;
> + if (s->dsp.sad[1](NULL, ptr_y + 8, dest_y
+ 8, wrap_y, 8) < 20 * s->qscale) skip_dct[1] = 1;
> + if (s->dsp.sad[1](NULL, ptr_y +dct_offset , dest_y
+dct_offset , wrap_y, 8) < 20 * s->qscale) skip_dct[2] = 1;
> + if (s->dsp.sad[1](NULL, ptr_y +dct_offset+ 8, dest_y
+dct_offset+ 8, wrap_y, 8) < 20 * s->qscale) skip_dct[3] = 1;
> + if (s->dsp.sad[1](NULL, ptr_cb , dest_cb
, wrap_c, 8) < 20 * s->qscale) skip_dct[4] = 1;
> + if (s->dsp.sad[1](NULL, ptr_cr , dest_cr
, wrap_c, 8) < 20 * s->qscale) skip_dct[5] = 1;
> + if (!s->chroma_y_shift) { /* 422 */
> + if (s->dsp.sad[1](NULL, ptr_cb + (dct_offset >> 1),
dest_cb + (dct_offset >> 1), wrap_c, 8) < 20 * s->qscale) skip_dct[6]= 1;
> + if (s->dsp.sad[1](NULL, ptr_cr + (dct_offset >> 1),
dest_cr + (dct_offset >> 1), wrap_c, 8) < 20 * s->qscale) skip_dct[7]= 1;
> }
All of these are >80 characters and should probably be split in several
lines. Don't forget to split the if(..) code into separate lines also, i.e.
what I want is:
if (s->dsp.sad[1](NULL, ...,
...) < 20 * s->qscale)
skip_dct[x] = 1;
> + if (s->avctx->quantizer_noise_shaping) {
> + if (!skip_dct[0]) get_visual_weight(weight[0], ptr_y
, wrap_y);
> + if (!skip_dct[1]) get_visual_weight(weight[1], ptr_y
+ 8, wrap_y);
> + if (!skip_dct[2]) get_visual_weight(weight[2], ptr_y +
dct_offset , wrap_y);
> + if (!skip_dct[3]) get_visual_weight(weight[3], ptr_y +
dct_offset + 8, wrap_y);
> + if (!skip_dct[4]) get_visual_weight(weight[4], ptr_cb
, wrap_c);
> + if (!skip_dct[5]) get_visual_weight(weight[5], ptr_cr
, wrap_c);
> + if (!s->chroma_y_shift) { /* 422 */
> + if (!skip_dct[6]) get_visual_weight(weight[6], ptr_cb +
(dct_offset >> 1), wrap_c);
> + if (!skip_dct[7]) get_visual_weight(weight[7], ptr_cr +
(dct_offset >> 1), wrap_c);
> }
All are >80 characters and you can fix that by splitting the if(..) code
into:
if (..)
code;
> + for (i = 0; i < mb_block_count; i++) {
> + if (!skip_dct[i]) {
> int overflow;
> s->block_last_index[i] = s->dct_quantize(s, s->block[i],
i, s->qscale, &overflow);
> // FIXME we could decide to change to quantizer instead of
clipping
> // JS: I don't think that would be a good idea it could
lower quality instead
> // of improve it. Just INTRADC clipping deserves changes
in quantizer
> if (overflow) clip_coeffs(s, s->block[i],
s->block_last_index[i]);
> - }else
> + } else
> s->block_last_index[i]= -1;
> }
Comments are wrongly indented.
> + if (!skip_dct[i]) {
> s->block_last_index[i] = dct_quantize_refine(s,
s->block[i], weight[i], orig[i], i, s->qscale);
> }
>80 characters.
> + if (s->flags & CODEC_FLAG_CBP_RD) {
> + for (i = 0; i < mb_block_count; i++) {
> + if (s->block_last_index[i] == -1)
> + s->coded_score[i]= INT_MAX / 256;
> }
> }
Space between s->coded_score[i] and =.
> + if ((s->flags&CODEC_FLAG_GRAY) && s->mb_intra) {
Spaces around &.
> //non c quantize code returns incorrect block_last_index FIXME
Space between "//" and comment text.
> + if (s->block_last_index[i] > 0) {
> + for (j = 63; j > 0; j--) {
> + if (s->block[i][s->intra_scantable.permutated[j]])
break;
> }
if (..)
break;
> @@ -1803,75 +1842,79 @@ static av_always_inline void
encode_mb_internal(MpegEncContext *s, int motion_x,
[..]
> static av_always_inline void encode_mb(MpegEncContext *s, int motion_x,
int motion_y)
>80 characters.
> +static inline void copy_context_before_encode(MpegEncContext *d,
MpegEncContext *s, int type)
Same.
> - memcpy(d->last_mv, s->last_mv, 2*2*2*sizeof(int)); //FIXME is memcpy
faster than a loop?
> + memcpy(d->last_mv, s->last_mv, 2 * 2 * 2 * sizeof(int)); //FIXME is
memcpy faster than a loop?
>80 characters, fix it by putting the FIXME on the line above. Don't forget
to put a space between "//" and "FIXME".
> + d->mv_bits = s->mv_bits;
> + d->i_tex_bits = s->i_tex_bits;
> + d->p_tex_bits = s->p_tex_bits;
> + d->i_count = s->i_count;
> + d->f_count = s->f_count;
> + d->b_count = s->b_count;
> + d->skip_count = s->skip_count;
> + d->misc_bits = s->misc_bits;
> + d->last_bits = 0;
> +
> + d->mb_skipped = 0;
> + d->qscale = s->qscale;
> + d->dquant = s->dquant;
> +
> + d->esc3_level_length = s->esc3_level_length;
Try to vertically align the '=' in most of the abpve.
> +static inline void copy_context_after_encode(MpegEncContext *d,
MpegEncContext *s, int type)
>80 characters.
> + memcpy(d->last_mv, s->last_mv, 2 * 2 * 2 * sizeof(int)); //FIXME is
memcpy faster than a loop?
Same; fix by putting comment on line above and add space between "//" and
"FIXME".
> + d->mv_bits = s->mv_bits;
> + d->i_tex_bits = s->i_tex_bits;
> + d->p_tex_bits = s->p_tex_bits;
> + d->i_count = s->i_count;
> + d->f_count = s->f_count;
> + d->b_count = s->b_count;
> + d->skip_count = s->skip_count;
> + d->misc_bits = s->misc_bits;
> +
> + d->mb_intra = s->mb_intra;
> + d->mb_skipped = s->mb_skipped;
> + d->mv_type = s->mv_type;
> + d->mv_dir = s->mv_dir;
> + d->pb = s->pb;
> + if (s->data_partitioning) {
> + d->pb2 = s->pb2;
> + d->tex_pb = s->tex_pb;
> + }
> + d->block = s->block;
> + for (i = 0; i < 8; i++)
> + d->block_last_index[i] = s->block_last_index[i];
> + d->interlaced_dct = s->interlaced_dct;
> + d->qscale = s->qscale;
> +
> + d->esc3_level_length = s->esc3_level_length;
Try to vertically align '=' in some of the above.
> static inline void encode_mb_hq(MpegEncContext *s, MpegEncContext
*backup, MpegEncContext *best, int type,
>80 characters.
> + s->block = s->blocks[*next_block];
> + s->pb = pb[*next_block];
Vertically align '='.
> + if (*next_block) {
> memcpy(dest_backup, s->dest, sizeof(s->dest));
> s->dest[0] = s->rd_scratchpad;
> + s->dest[1] = s->rd_scratchpad + 16 * s->linesize;
> + s->dest[2] = s->rd_scratchpad + 16 * s->linesize + 8;
> assert(s->linesize >= 32); //FIXME
> }
Space between "//" and "FIXME".
> +static int sse(MpegEncContext *s, uint8_t *src1, uint8_t *src2, int w,
int h, int stride)
>80 characters.
> +static int sse_mb(MpegEncContext *s) {
{ goes on its own line.
> + if (s->avctx->mb_cmp == FF_CMP_NSSE) {
> + return s->dsp.nsse[0](s, s->new_picture.f.data[0] + s->mb_x *
16 + s->mb_y * s->linesize * 16 , s->dest[0], s->linesize, 16) +
> + s->dsp.nsse[1](s, s->new_picture.f.data[1] + s->mb_x * 8
+ s->mb_y * s->uvlinesize * 8, s->dest[1], s->uvlinesize, 8) +
> + s->dsp.nsse[1](s, s->new_picture.f.data[2] + s->mb_x * 8
+ s->mb_y * s->uvlinesize * 8, s->dest[2], s->uvlinesize, 8);
> + } else {
> + return s->dsp.sse[0](NULL, s->new_picture.f.data[0] + s->mb_x *
16 + s->mb_y * s->linesize * 16 , s->dest[0], s->linesize , 16) +
> + s->dsp.sse[1](NULL, s->new_picture.f.data[1] + s->mb_x *
8 + s->mb_y * s->uvlinesize * 8, s->dest[1], s->uvlinesize, 8) +
> + s->dsp.sse[1](NULL, s->new_picture.f.data[2] + s->mb_x *
8 + s->mb_y * s->uvlinesize * 8, s->dest[2], s->uvlinesize, 8);
> }
> else
> + return sse(s, s->new_picture.f.data[0] + s->mb_x * 16 + s->mb_y
* s->linesize * 16 , s->dest[0], w, h, s->linesize) +
> + sse(s, s->new_picture.f.data[1] + s->mb_x * 8 + s->mb_y
* s->uvlinesize * 8, s->dest[1], w >> 1, h >> 1, s->uvlinesize) +
> + sse(s, s->new_picture.f.data[2] + s->mb_x * 8 + s->mb_y
* s->uvlinesize * 8, s->dest[2], w >> 1, h >> 1, s->uvlinesize);
> }
>80 characters for almost any one of these.
> +static int mb_var_thread(AVCodecContext *c, void *arg)
[..]
> + varc = (s->dsp.pix_norm1(pix, s->linesize) - (((unsigned)sum
* sum) >> 8) + 500 + 128) >> 8;
>
> s->current_picture.mb_var [s->mb_stride * mb_y + mb_x] =
varc;
> + s->current_picture.mb_mean[s->mb_stride * mb_y + mb_x] =
(sum + 128) >> 8;
> 80 characters for some of these.
> +static void write_slice_end(MpegEncContext *s)
[..]
> + if ((s->flags&CODEC_FLAG_PASS1) && !s->partitioned_frame)
Spaces around &.
Until line 2072.
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
