Hi,
On Sat, Dec 24, 2011 at 9:24 AM, Aneesh Dogra <[email protected]> wrote:
> @@ -113,64 +115,71 @@ void ff_convert_matrix(DSPContext *dsp, int
(*qmat)[64], uint16_t (*qmat16)[2][6
[..]
> if (dsp->fdct == fdct_ifast
> #ifndef FAAN_POSTSCALE
> || dsp->fdct == ff_faandct
> #endif
> ) {
I don't think that's right.
> + if (shift) {
> av_log(NULL, AV_LOG_INFO, "Warning, QMAT_SHIFT is larger than
%d, overflows possible\n", QMAT_SHIFT - shift);
> }
Split that line please.
> +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 directly after "[" or before "]".
> @@ -182,34 +191,36 @@ static void copy_picture_attributes(MpegEncContext
*s, AVFrame *dst, AVFrame *sr
[..]
> + if (src->motion_subsample_log2 != dst->motion_subsample_log2)
> av_log(s->avctx, AV_LOG_ERROR,
"AVFrame.motion_subsample_log2 doesn't match! (%d!=%d)\n",
> src->motion_subsample_log2, dst->motion_subsample_log2);
Split the long (the "av_log" one) line please, and indent the line below it
so it vertically aligns with the first character after the opening bracket
of the "av_log" line, since it's still part of the context of what's inside
the brackets.
> - memcpy(dst->mb_type, src->mb_type, s->mb_stride * s->mb_height *
sizeof(dst->mb_type[0]));
> + memcpy(dst->mb_type, src->mb_type, s->mb_stride *
> + s->mb_height * sizeof(dst->mb_type[0]));
Just like brackets, commas specify context. In this case, what's after the
first comma of the first line continues on the second line, so ideally (if
it doesn't violate 80-character limits etc.), the second line should
vertically align with what's after that comma, or you should linebreak at
the comma.
Option 1:
memcpy(x, y, z1 *
z2);
Option 2 (preferred here):
memcpy(x, y,
z1 * z2);
> + if (src->motion_val[i] && src->motion_val[i] !=
dst->motion_val[i]) {
> + memcpy(dst->motion_val[i], src->motion_val[i], 2 *
stride * height * sizeof(int16_t));
> }
Long line (>80 characters), please break it.
> + if (src->ref_index[i] && src->ref_index[i] !=
dst->ref_index[i]) {
> + memcpy(dst->ref_index[i], src->ref_index[i],
s->mb_stride * 4 * s->mb_height * sizeof(int8_t));
> }
Same.
> @@ -252,27 +264,34 @@ 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){
> + if (avctx->pix_fmt != PIX_FMT_YUV420P && avctx->pix_fmt !=
PIX_FMT_YUV422P) {
> av_log(avctx, AV_LOG_ERROR, "only YUV420 and YUV422 are
supported\n");
> return -1;
> }
> break;
Both lines are >80 characters, please split.
> + if (avctx->pix_fmt != PIX_FMT_YUVJ420P &&
> + avctx->pix_fmt != PIX_FMT_YUVJ422P &&
> + avctx->pix_fmt != PIX_FMT_YUVJ444P &&
> + avctx->pix_fmt != PIX_FMT_BGRA &&
> + ((avctx->pix_fmt != PIX_FMT_YUV420P &&
> + avctx->pix_fmt != PIX_FMT_YUV422P &&
> + avctx->pix_fmt != PIX_FMT_YUV444P) ||
> + avctx->strict_std_compliance > FF_COMPLIANCE_UNOFFICIAL)) {
This doesn't work, context-wise. Brackets here specify context, so whatever
is in the same context needs the same vertical alignment.
Here, you have "(a && b && c && d && e)" (context 1), where e is "(f || g)"
(context 2), and f is "(h && i && j)" (context 3). They each have different
vertical alignments because their context is different.f should vertically
align with g. Since the bracket is part of that, the bracket before f, not
f itself, should vertically align with a/b/c/d, since the bracket (not f)
is the start of e.
In short, it will look like this:
if (a &&
b &&
c &&
d &&
((h &&
i &&
j) ||
g)) {
..
}
By doing so, it is immediately clear to the reader of the code that h/i/j
are in the same context, but not g, and none of those are in the same
context as a/b/c/d. In other words, a few simple spaces do the reader a
great service.
> + if (avctx->pix_fmt != PIX_FMT_YUVJ420P && avctx->pix_fmt !=
PIX_FMT_YUVJ422P &&
> + ((avctx->pix_fmt != PIX_FMT_YUV420P && avctx->pix_fmt !=
PIX_FMT_YUV422P) ||
> + avctx->strict_std_compliance > FF_COMPLIANCE_UNOFFICIAL)) {
> av_log(avctx, AV_LOG_ERROR, "colorspace not supported in
jpeg\n");
> return -1;
> }
The first two lines are too long (>80 characters). Also, the indenting of
the second line is wrong (the brackets vertically align with the opening
bracket of the if(), even if we're still inside the if() statement, so that
can never be correct).
The correct alignment looks like this:
if (a && b && c), where c is (d || e), where d is (f && g), so:
indented:
if (a && b &&
((f && g) ||
d)) {
..
}
> @@ -293,32 +312,32 @@ av_cold int MPV_encode_init(AVCodecContext *avctx)
[..]
> - if(avctx->gop_size > 600 &&
avctx->strict_std_compliance>FF_COMPLIANCE_EXPERIMENTAL){
> + if (avctx->gop_size > 600 && avctx->strict_std_compliance >
FF_COMPLIANCE_EXPERIMENTAL) {
> av_log(avctx, AV_LOG_ERROR, "Warning keyframe interval too
large! reducing it ...\n");
> - avctx->gop_size=600;
> - }
Both lines are >80 characters, please break them.
> @@ -337,85 +356,84 @@ av_cold int MPV_encode_init(AVCodecContext *avctx)
[..]
> || (s->flags&CODEC_FLAG_QP_RD))
> && !s->fixed_qscale;
For these lines (and what's above it), the "&&" and "||" should go on the
previous line. We don't use this:
if (a
&& b) {
..
}
We only use this:
if (a &&
b) {
..
}
> + 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
Try to vertically align the '=' for the first of these lines also. You
could even try to vertically align the '&' by adding one more space to the
lines that use "s->flags" instead of "s->flags2".
> + 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");
> return -1;
> }
Break long line.
> + 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");
> }
Same.
> + if (avctx->rc_max_rate && avctx->rc_max_rate == avctx->bit_rate &&
avctx->rc_max_rate != avctx->rc_min_rate) {
> av_log(avctx, AV_LOG_INFO, "impossible bitrate constraints, this
will fail\n");
> }
Same.
> + if (avctx->rc_buffer_size && avctx->bit_rate *
(int64_t)avctx->time_base.num > avctx->rc_buffer_size *
(int64_t)avctx->time_base.den) {
> av_log(avctx, AV_LOG_ERROR, "VBV buffer too small for
bitrate\n");
> return -1;
> }
Same.
> + if (!s->fixed_qscale && avctx->bit_rate * av_q2d(avctx->time_base) >
avctx->bit_rate_tolerance) {
> av_log(avctx, AV_LOG_ERROR, "bitrate tolerance too small for
bitrate\n");
> return -1;
> }
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) {
> av_log(avctx, AV_LOG_INFO, "Warning vbv_delay will be set to
0xFFFF (=VBR) as the specified vbv buffer is too large for the given
bitrate!\n");
> }
Same (for all 4 lines), plus "&&" goes on the previous line. Note how, if
you split this, you'll have to make sure to get the context of each
condition right to get the indenting right.
> + 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) {
> av_log(avctx, AV_LOG_ERROR, "4MV not supported by codec\n");
> return -1;
> }
Second line is >80 characters, please break, and && goes on previous line.
> + if (s->obmc && s->avctx->mb_decision != FF_MB_DECISION_SIMPLE) {
> av_log(avctx, AV_LOG_ERROR, "OBMC is only supported with simple
mb decision\n");
> return -1;
> }
av_log() line is too long (>80 characters), please break.
> #if FF_API_MPEGVIDEO_GLOBAL_OPTS
> + if (s->obmc && s->codec_id != CODEC_ID_H263 && s->codec_id !=
CODEC_ID_H263P) {
> av_log(avctx, AV_LOG_ERROR, "OBMC is only supported with
H263(+)\n");
> return -1;
> }
> #endif
if() line is too long (>80 characters), please break.
> #if FF_API_MPEGVIDEO_GLOBAL_OPTS
> - if(s->data_partitioning && s->codec_id != CODEC_ID_MPEG4){
> + if (s->data_partitioning && s->codec_id != CODEC_ID_MPEG4) {
> av_log(avctx, AV_LOG_ERROR, "data partitioning not supported by
codec\n");
> return -1;
> }
> #endif
Same.
> + if (s->max_b_frames && s->codec_id != CODEC_ID_MPEG4 && s->codec_id
!= CODEC_ID_MPEG1VIDEO && s->codec_id != CODEC_ID_MPEG2VIDEO) {
> av_log(avctx, AV_LOG_ERROR, "b frames not supported by codec\n");
> return -1;
> }
if() line is too long, please break.
> @@ -428,170 +446,171 @@ av_cold int MPV_encode_init(AVCodecContext *avctx)
[..]
> + 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) {
> av_log(avctx, AV_LOG_ERROR, "interlacing not supported by
codec\n");
> return -1;
> }
if() line is too lon (>80 characters), and && goes on previous line.
> - if(s->mpeg_quant && s->codec_id != CODEC_ID_MPEG4){ //FIXME mpeg2
uses that too
> + if (s->mpeg_quant && s->codec_id != CODEC_ID_MPEG4) { //FIXME mpeg2
uses that too
> av_log(avctx, AV_LOG_ERROR, "mpeg2 style quantization not
supported by codec\n");
> return -1;
> }
Both lines are too long. To split the first line, don't split in the
condition, but move the comment one line up, like this:
// comment
if (a && b) {
..
}
> + if ((s->flags & CODEC_FLAG_QP_RD) && s->avctx->mb_decision !=
FF_MB_DECISION_RD) {
> av_log(avctx, AV_LOG_ERROR, "QP RD needs mbd=2\n");
> return -1;
> }
if() line too long, please break.
> + 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");
> return -1;
> }
if() and av_log() line too long, please break.
> - if((s->flags2 & CODEC_FLAG2_INTRA_VLC) && s->codec_id !=
CODEC_ID_MPEG2VIDEO){
> + if ((s->flags2 & CODEC_FLAG2_INTRA_VLC) && s->codec_id !=
CODEC_ID_MPEG2VIDEO) {
> av_log(avctx, AV_LOG_ERROR, "intra vlc table not supported by
codec\n");
> return -1;
> }
if() line too long, please break.
> + if (s->flags & CODEC_FLAG_LOW_DELAY) {
> + if (s->codec_id != CODEC_ID_MPEG2VIDEO) {
> av_log(avctx, AV_LOG_ERROR, "low delay forcing is only
available for mpeg2\n");
> return -1;
> }
av_log() line too long, please break.
> + if (s->q_scale_type == 1) {
> #if FF_API_MPEGVIDEO_GLOBAL_OPTS
> + if (s->codec_id != CODEC_ID_MPEG2VIDEO) {
> av_log(avctx, AV_LOG_ERROR, "non linear quant is only
available for mpeg2\n");
> return -1;
> }
> #endif
av_log() line too long, please break.
> + if (avctx->qmax > 12) {
> av_log(avctx, AV_LOG_ERROR, "non linear quant only supports
qmax <= 12 currently\n");
> return -1;
> }
Same.
> + if ( s->avctx->thread_count > 1 && s->codec_id != CODEC_ID_MPEG4
> + && s->codec_id != CODEC_ID_MPEG1VIDEO && s->codec_id !=
CODEC_ID_MPEG2VIDEO
> + && (s->codec_id != CODEC_ID_H263P || !(s->flags &
CODEC_FLAG_H263P_SLICE_STRUCT))) {
> av_log(avctx, AV_LOG_ERROR, "multi threaded encoding not
supported by codec\n");
> return -1;
> }
Guess which lines are too long here and need their &&s moved to the
previous line.
> + if (s->avctx->thread_count < 1) {
> av_log(avctx, AV_LOG_ERROR, "automatic thread number detection
not supported by codec, patch welcome\n");
> return -1;
> }
Same.
That's the first 500 lines. I'll do the rest in later emails as I find more
time.
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
