Hi,
On Mon, Dec 26, 2011 at 10:44 AM, Aneesh Dogra <[email protected]> wrote:
> +void ff_convert_matrix(DSPContext *dsp, int (*qmat)[64],
> + uint16_t (*qmat16)[2][64],
> + const uint16_t *quant_matrix,
> + int bias, int qmin, int qmax, int intra)
[..]
> /* 19952 <= ff_aanscales[i] * qscale *
> quant_matrix[i] <= 249205026 */
> /* (1 << 36) / 19952 >= (1 << 36) / (ff_aanscales[i] *
> qscale * quant_matrix[i]) >= (1 << 36) / 249205026 */
> /* 3444240 >= (1 << 36) / (ff_aanscales[i] *
> qscale * quant_matrix[i]) >= 275 */
[..]
> /* 19952 <= ff_aanscales[i] * qscale *
> quant_matrix[i] <= 249205026 */
> /* (1 << 36) / 19952 >= (1 << 36) / (ff_aanscales[i] *
> qscale * quant_matrix[i]) >= (1<<36)/249205026 */
> /* 3444240 >= (1 << 36) / (ff_aanscales[i] *
> qscale * quant_matrix[i]) >= 275 */
Aren't all these >80 characters?
> /* We can safely suppose that 16 <= quant_matrix[i] <= 255
> So 16 <= qscale * quant_matrix[i]
> <= 7905
> so (1<<19) / 16 >= (1<<19) / (qscale * quant_matrix[i])
> >= (1<<19) / 7905
> so 32768 >= (1<<19) / (qscale * quant_matrix[i])
> >= 67
> */
Same x3.
> qmat[qscale][i] = (int)((UINT64_C(1) << QMAT_SHIFT) /
> (qscale * quant_matrix[j]));
> + //qmat [qscale][i] = (1 << QMAT_SHIFT_MMX) / (qscale *
> quant_matrix[i]);
> qmat16[qscale][0][i] = (1 << QMAT_SHIFT_MMX) / (qscale *
> quant_matrix[j]);
Same x3.
> + if (qmat16[qscale][0][i] == 0 || qmat16[qscale][0][i] == 128
> * 256)
> + qmat16[qscale][0][i] = 128 * 256 - 1;
> + qmat16[qscale][1][i] = ROUNDED_DIV(bias << (16 -
> QUANT_BIAS_SHIFT), qmat16[qscale][0][i]);
Same x2.
> +static inline void update_qscale(MpegEncContext *s)
> +{
> + s->qscale = (s->lambda * 139 + FF_LAMBDA_SCALE * 64) >> (FF_LAMBDA_SHIFT
> + 7);
> + s->qscale = av_clip(s->qscale, s->avctx->qmin, s->avctx->qmax);
> +
> + s->lambda2 = (s->lambda * s->lambda + FF_LAMBDA_SCALE / 2) >>
> FF_LAMBDA_SHIFT;
> }
Same x2.
> +void ff_write_quant_matrix(PutBitContext *pb, uint16_t *matrix)
[..]
> + for (i = 0; i < 64; i++) {
> put_bits(pb, 8, matrix[ ff_zigzag_direct[i] ]);
> }
No spaces after [ and before ].
> +void ff_init_qscale_tab(MpegEncContext *s)
[..]
> + for (i = 0; i < s->mb_num; i++) {
> + unsigned int lam = s->lambda_table[s->mb_index2xy[i]];
> + int qp = (lam * 139 + FF_LAMBDA_SCALE * 64) >> (FF_LAMBDA_SHIFT + 7);
> + qscale_table[s->mb_index2xy[i]] = av_clip(qp, s->avctx->qmin,
> s->avctx->qmax);
> }
80 characters.
> +static void update_duplicate_context_after_me(MpegEncContext *dst,
> MpegEncContext *src)
80 characters.
> @@ -252,27 +272,38 @@ av_cold int MPV_encode_init(AVCodecContext *avctx)
[..]
> switch (avctx->codec_id) {
> case CODEC_ID_MPEG2VIDEO:
> + if (avctx->pix_fmt != PIX_FMT_YUV420P &&
> + avctx->pix_fmt != PIX_FMT_YUV422P) {
Vertically align the first character of the second line to the
character after the opening bracket of the first line.
if (a &&
b) {
..
}
> @@ -328,284 +361,350 @@ av_cold int MPV_encode_init(AVCodecContext *avctx)
[..]
> + s->loop_filter = !!(s->flags & CODEC_FLAG_LOOP_FILTER);
> #if FF_API_MPEGVIDEO_GLOBAL_OPTS
> + s->alternate_scan = !!(s->flags & CODEC_FLAG_ALT_SCAN);
> + s->intra_vlc_format = !!(s->flags2 & CODEC_FLAG2_INTRA_VLC);
> + s->q_scale_type = !!(s->flags2 & CODEC_FLAG2_NON_LINEAR_QUANT);
> + s->obmc = !!(s->flags & CODEC_FLAG_OBMC);
> #endif
Vertically align the first line (s->loop_filter) also.
> - if(avctx->rc_max_rate && !avctx->rc_buffer_size){
> - av_log(avctx, AV_LOG_ERROR, "a vbv buffer size is needed, for
> encoding with a maximum bitrate\n");
> + if (avctx->rc_max_rate && !avctx->rc_buffer_size) {
> + av_log(avctx, AV_LOG_ERROR,
> + "a vbv buffer size is needed,"
> + "for encoding with a maximum bitrate\n");
Those lines are not the same; a space between the "," and "comma" is
missing if you concatenate the two strings.
> + if (avctx->rc_min_rate && avctx->rc_max_rate != avctx->rc_min_rate) {
> + av_log(avctx, AV_LOG_INFO,
> + "Warning min_rate > 0 but min_rate != max_rate isn't
> recommended!\n");
> }
80 characters.
> - if(avctx->rc_buffer_size &&
> avctx->bit_rate*(int64_t)avctx->time_base.num > avctx->rc_buffer_size *
> (int64_t)avctx->time_base.den){
> + if (avctx->rc_buffer_size &&
> + avctx->bit_rate * (int64_t)avctx->time_base.num >
> + avctx->rc_buffer_size * (int64_t)avctx->time_base.den) {
the line after the > is still part of the same statement, and thus
needs 4 spaces extra indenting, like this:
if (aaaaa &&
bbbbb >
ccccc) {
..
}
> - if(!s->fixed_qscale && avctx->bit_rate*av_q2d(avctx->time_base) >
> avctx->bit_rate_tolerance){
> + if (!s->fixed_qscale &&
> + avctx->bit_rate * av_q2d(avctx->time_base) >
> + avctx->bit_rate_tolerance) {
Same.
> - if( s->avctx->rc_max_rate && s->avctx->rc_min_rate ==
> s->avctx->rc_max_rate
> - && (s->codec_id == CODEC_ID_MPEG1VIDEO || s->codec_id ==
> CODEC_ID_MPEG2VIDEO)
> - && 90000LL * (avctx->rc_buffer_size-1) >
> s->avctx->rc_max_rate*0xFFFFLL){
> -
> + if (s->avctx->rc_max_rate &&
> + s->avctx->rc_min_rate == s->avctx->rc_max_rate &&
> + (s->codec_id == CODEC_ID_MPEG1VIDEO ||
> + s->codec_id == CODEC_ID_MPEG2VIDEO) &&
> + 90000LL * (avctx->rc_buffer_size - 1) >
> + s->avctx->rc_max_rate * 0xFFFFLL) {
Same.
> - if((s->flags & CODEC_FLAG_4MV) && s->codec_id != CODEC_ID_MPEG4
> - && s->codec_id != CODEC_ID_H263 && s->codec_id != CODEC_ID_H263P &&
> s->codec_id != CODEC_ID_FLV1){
> + if ((s->flags & CODEC_FLAG_4MV) && s->codec_id != CODEC_ID_MPEG4 &&
> + s->codec_id != CODEC_ID_H263 && s->codec_id != CODEC_ID_H263P &&
> + s->codec_id != CODEC_ID_FLV1) {
Last line needs to vertically align like the one above it.
> #if FF_API_MPEGVIDEO_GLOBAL_OPTS
> - if(s->obmc && s->codec_id != CODEC_ID_H263 && s->codec_id !=
> CODEC_ID_H263P){
> + if (s->obmc && s->codec_id != CODEC_ID_H263 &&
> + s->codec_id != CODEC_ID_H263P) {
Same.
> - if((s->flags &
> (CODEC_FLAG_INTERLACED_DCT|CODEC_FLAG_INTERLACED_ME|CODEC_FLAG_ALT_SCAN))
> - && s->codec_id != CODEC_ID_MPEG4 && s->codec_id !=
> CODEC_ID_MPEG2VIDEO){
> + if ((s->flags &
> + (CODEC_FLAG_INTERLACED_DCT |
> + CODEC_FLAG_INTERLACED_ME |
> + CODEC_FLAG_ALT_SCAN)) &&
> + s->codec_id != CODEC_ID_MPEG4 &&
> + s->codec_id != CODEC_ID_MPEG2VIDEO) {
I see you doing this in several places; why the over-eager breaking?
E.g., this still fits on 8 characters:
if ((s->flags & (a | b |
c)) &&
s->codec_id != .. && s->codec_id != ..) {
> - if(s->avctx->scenechange_threshold < 1000000000 && (s->flags &
> CODEC_FLAG_CLOSED_GOP)){
> - av_log(avctx, AV_LOG_ERROR, "closed gop with scene change detection
> are not supported yet, set threshold to 1000000000\n");
> + if (s->avctx->scenechange_threshold < 1000000000 &&
> + (s->flags & CODEC_FLAG_CLOSED_GOP)) {
> + av_log(avctx, AV_LOG_ERROR,
> + "closed gop with scene change detection are not supported
> yet,"
> + "set threshold to 1000000000\n");
Needs a space after the comma, otherwise there's no space in the
resulting concatenated string.
> - if(s->avctx->thread_count < 1){
> - av_log(avctx, AV_LOG_ERROR, "automatic thread number detection not
> supported by codec, patch welcome\n");
> + if (s->avctx->thread_count < 1) {
> + av_log(avctx, AV_LOG_ERROR,
> + "automatic thread number detection not supported by codec,"
> + "patch welcome\n");
Space after comma in string.
> - if(s->mpeg_quant || s->codec_id==CODEC_ID_MPEG1VIDEO ||
> s->codec_id==CODEC_ID_MPEG2VIDEO || s->codec_id==CODEC_ID_MJPEG){
> - s->intra_quant_bias= 3<<(QUANT_BIAS_SHIFT-3); //(a + x*3/8)/x
> - s->inter_quant_bias= 0;
> - }else{
> - s->intra_quant_bias=0;
> - s->inter_quant_bias=-(1<<(QUANT_BIAS_SHIFT-2)); //(a - x/4)/x
> + if (s->mpeg_quant || s->codec_id == CODEC_ID_MPEG1VIDEO ||
> + s->codec_id == CODEC_ID_MPEG2VIDEO || s->codec_id == CODEC_ID_MJPEG) {
Vertical alignment of second line in if statement.
> + // (a + x*3/8)/x
Spaces around * and / (in comment).
> + s->inter_quant_bias = -(1 << (QUANT_BIAS_SHIFT - 2)); // (a -
> x/4)/x
Same.
> - avcodec_get_chroma_sub_sample(avctx->pix_fmt, &chroma_h_shift,
> &chroma_v_shift);
> + avcodec_get_chroma_sub_sample(avctx->pix_fmt, &chroma_h_shift,
> + &chroma_v_shift);
First character of second line should vertically align to character
after opening bracket in first line.
> - if(avctx->codec_id == CODEC_ID_MPEG4 && s->avctx->time_base.den >
> (1<<16)-1){
> - av_log(avctx, AV_LOG_ERROR, "timebase %d/%d not supported by MPEG 4
> standard, "
> + if (avctx->codec_id == CODEC_ID_MPEG4 &&
> + s->avctx->time_base.den > (1 << 16) - 1) {
> + av_log(avctx, AV_LOG_ERROR,
> + "timebase %d/%d not supported by MPEG 4 standard, "
> "the maximum admitted value for the timebase denominator is
> %d\n",
80 characters.
> case CODEC_ID_LJPEG:
> case CODEC_ID_MJPEG:
> s->out_format = FMT_MJPEG;
> s->intra_only = 1; /* force intra only for jpeg */
> - if(avctx->codec->id == CODEC_ID_LJPEG && avctx->pix_fmt ==
> PIX_FMT_BGRA){
> + if (avctx->codec->id == CODEC_ID_LJPEG &&
> + avctx->pix_fmt == PIX_FMT_BGRA) {
Vertical alignment of second line in if statement.
> case CODEC_ID_H263:
> + if (!CONFIG_H263_ENCODER)
> + return -1;
4 more spaces indenting for the return line.
> if (ff_match_2uint16(h263_format, FF_ARRAY_ELEMS(h263_format),
> s->width, s->height) == 8) {
80 characters.
> - av_log(avctx, AV_LOG_INFO, "The specified picture size of %dx%d
> is not valid for the H.263 codec.\nValid sizes are 128x96, 176x144, 352x288,
> 704x576, and 1408x1152. Try H.263+.\n", s->width, s->height);
> + av_log(avctx, AV_LOG_INFO,
> + "The specified picture size of %dx%d is not valid for"
> + "the H.263 codec.\nValid sizes are 128x96, 176x144, "
> + "352x288, 704x576, and 1408x1152."
> + "Try H.263+.\n", s->width, s->height);
Missing space before string close in first and third line.
> - s->progressive_frame=
> - s->progressive_sequence= !(avctx->flags &
> (CODEC_FLAG_INTERLACED_DCT|CODEC_FLAG_INTERLACED_ME|CODEC_FLAG_ALT_SCAN));
> + s->progressive_frame =
> + s->progressive_sequence = !(avctx->flags &
> + (CODEC_FLAG_INTERLACED_DCT |
> + CODEC_FLAG_INTERLACED_ME |
> + CODEC_FLAG_ALT_SCAN));
a = !(b & (c |
d |
e));
Line breaks are preferred at boolean and/ors (&&/||), not bitwise and/ors (&/|).
> - if(CONFIG_MPEG4_ENCODER && s->codec_id==CODEC_ID_MPEG4 &&
> s->mpeg_quant){
[..]
> + if (CONFIG_MPEG4_ENCODER &&
> + s->codec_id == CODEC_ID_MPEG4 &&
> + s->mpeg_quant) {
2 of those still do fit on a single line with 8 characters, right?
> +static int load_input_picture(MpegEncContext *s, AVFrame *pic_arg)
[..]
> - pic= (AVFrame*)&s->picture[i];
> + pic = (AVFrame*) &s->picture[i];
(AVFrame *), note the space between type and *.
> - if(ff_alloc_picture(s, (Picture*)pic, 1) < 0){
> + if (ff_alloc_picture(s, (Picture*) pic, 1) < 0) {
Same.
> - pic= (AVFrame*)&s->picture[i];
> + pic = (AVFrame*) &s->picture[i];
Same.
> - if(ff_alloc_picture(s, (Picture*)pic, 0) < 0){
> + if (ff_alloc_picture(s, (Picture*) pic, 0) < 0) {
Same. This was all also in my original review.
> + if (!s->avctx->rc_buffer_size)
> dst +=INPLACE_OFFSET;
Space after '+='.
> +static int skip_check(MpegEncContext *s, Picture *p, Picture *ref)
[..]
> int off = p->f.type == FF_BUFFER_TYPE_SHARED ? 0: 16;
Space left of ':'.
> - int v = s->dsp.frame_skip_cmp[1](s, p->f.data[plane] +
> 8*(x + y*stride)+off, ref->f.data[plane] + 8*(x + y*stride), stride, 8);
> + int v = s->dsp.frame_skip_cmp[1](s, p->f.data[plane]
> + + 8 * (x + y * stride) +
> + off, ref->f.data[plane] +
> + 8 * (x + y * stride),
> + stride, 8);
This gets very ugly. I'd prefer if you could make the pointer a local variable:
uint8_t *dptr = p->f.data[plane] + 8 * (x + y * stride) + off;
uint8_t *rptr = ref->f.data[plane] + 8 * (x + y * sridee);
int v = s->dsp.frame_skip_cmp[1](s, dptr, rptr, stride, 8);
According to my counting, that just about fits on 80 characters. If
not, you can remove the extra spaces before the +.
> - switch(s->avctx->frame_skip_exp){
> - case 0: score= FFMAX(score, v); break;
> - case 1: score+= FFABS(v);break;
> - case 2: score+= v*v;break;
> - case 3: score64+= FFABS(v*v*(int64_t)v);break;
> - case 4: score64+= v*v*(int64_t)(v*v);break;
> + switch (s->avctx->frame_skip_exp) {
> + case 0: score = FFMAX(score, v); break;
> + case 1: score += FFABS(v); break;
> + case 2: score += v * v; break;
> + case 3: score64 += FFABS(v * v * (int64_t)v); break;
> + case 4: score64 += v * v * (int64_t)(v * v); break;
> }
= of the first line is not vertically aligned. Try also vertically
aligning the "break"s.
> - if(score) score64= score;
> + if (score) score64 = score;
Split into two lines:
if (..)
..;
> +static int estimate_best_b_count(MpegEncContext *s)
[..]
> + //emms_c();
> + //s->next_picture_ptr->quality;
> + p_lambda = s->last_lambda_for[AV_PICTURE_TYPE_P];
> + //p_lambda *FFABS(s->avctx->b_quant_factor) + s->avctx->b_quant_offset;
Space right of * in comment.
> + c->flags = CODEC_FLAG_QSCALE | CODEC_FLAG_PSNR
> + | CODEC_FLAG_INPUT_PRESERVED /*| CODEC_FLAG_EMU_EDGE*/;
| goes on previous line.
Until line 1018. I won't review further, I think it's best for my own
sanity that we apply this short pieces at a time, e.g. the first 1k
lines only, before we move on to the rest of the file (I did the same
for mpegvideo.c).
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
