Hi,
On Mon, Dec 5, 2011 at 8:45 AM, KJT TOD <[email protected]> wrote:
[..]
> - if(!s1->first_field){
> - s->last_pict_type= s1->pict_type;
> - if (s1->current_picture_ptr) s->last_lambda_for[s1->pict_type] =
> s1->current_picture_ptr->f.quality;
> + if (!s1->first_field) {
> + s->last_pict_type = s1->pict_type;
> + if (s1->current_picture_ptr)
> + s->last_lambda_for [s1->pict_type] =
> s1->current_picture_ptr->f.quality;
No space in array indexes (i.e. between "..._for" and "[...").
> - if(s->codec_id == CODEC_ID_MPEG2VIDEO && !s->progressive_sequence)
> + if (s->codec_id == CODEC_ID_MPEG2VIDEO && !s->progressive_sequence)
> s->mb_height = (s->height + 31) / 32 * 2;
> else if (s->codec_id != CODEC_ID_H264)
> - s->mb_height = (s->height + 15) / 16;
> + s->mb_height = (s->height + 15) / 16;
This isn't right. The indenting of the line should depend on its
context. If it's part of the if() statement, then it should indent
with the brackets of the if. If it's part of what's conditional under
the if(), then it should have 4 spaces more indentation than the "if"
itself, so the original indenting here was correct.
> - if((s->encoding || (s->avctx->active_thread_type & FF_THREAD_SLICE)) &&
> - (s->avctx->thread_count > MAX_THREADS || (s->avctx->thread_count >
> s->mb_height && s->mb_height))){
> + if ((s->encoding || (s->avctx->active_thread_type & FF_THREAD_SLICE)) &&
> + (s->avctx->thread_count > MAX_THREADS ||
> + (s->avctx->thread_count > s->mb_height && s->mb_height))) {
Same thing here, the last line isn't under the same context as the
previous one, i.e. (x) || (y), now both x and y are at the same depth
level, whereas (x || (y && z)), now y and x are not at the same,
instead "(y && z)" is at the same depth level as x and thus the
brackets of "(y && z)" should align vertically with x. In short, the
last line needs one extra space of indentation.
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->mb_index2xy, (s->mb_num + 1) *
> sizeof(int),
> + fail) // error ressilience code looks cleaner
> with this
Please add a semicolon at the end (i.e. ");" instead of ")").
> + for (y = 0; y < s->mb_height; y++)
> + for (x = 0; x < s->mb_width; x++)
> + s->mb_index2xy[ x + y * s->mb_width ] = x + y * s->mb_stride;
> +
> + s->mb_index2xy[s->mb_height * s->mb_width] = (s->mb_height - 1) * s->
> + mb_stride + s->mb_width; // FIXME really needed?
Splitting the "s->..." is really ugly. Probably best to split the line
at the "=", i.e. "....s->mb_width] =<newline here>s->mb_stride ...".
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->p_mv_table_base ,
> + mv_table_size * 2 * sizeof(int16_t), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_forw_mv_table_base ,
> + mv_table_size * 2 * sizeof(int16_t), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_back_mv_table_base ,
> + mv_table_size * 2 * sizeof(int16_t), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_bidir_forw_mv_table_base,
> + mv_table_size * 2 * sizeof(int16_t), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_bidir_back_mv_table_base ,
> + mv_table_size * 2 * sizeof(int16_t), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->b_direct_mv_table_base ,
> + mv_table_size * 2 * sizeof(int16_t), fail)
Please remove the spaces before the trailing comma at each of the above lines.
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->mb_type, mb_array_size *
> + sizeof(uint16_t), fail) // needed for
> encoding
> +
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->lambda_table, mb_array_size *
> + sizeof(int), fail)
> +
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->q_intra_matrix ,
> + 64 * 32 * sizeof(int), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->q_inter_matrix ,
> + 64 * 32 * sizeof(int), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->q_intra_matrix16,
> + 64 * 32 * 2 * sizeof(uint16_t), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->q_inter_matrix16,
> + 64 * 32 * 2 * sizeof(uint16_t), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->input_picture, MAX_PICTURE_COUNT *
> + sizeof(Picture *), fail)
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->reordered_input_picture,
> + MAX_PICTURE_COUNT * sizeof(Picture *), fail)
> +
> + if (s->avctx->noise_reduction) {
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->dct_offset,
> + 2 * 64 * sizeof(uint16_t), fail)
[..]
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->picture, s->picture_count *
> + sizeof(Picture), fail)
Please add trailing semicolon. Same elsewhere.
> + for (i = 0; i < s->picture_count; i++) {
> + avcodec_get_frame_defaults((AVFrame *)&s->picture[i]);
(AVFrame *) &s->... (i.e. remove one of the two spaces before the *,
and add one after the ")").
> - /* dc values */
> - //MN: we need these for error resilience of intra-frames
[..]
> + /*dc values */
> + // MN: we need these for error resilience of intra-frames
Weird indenting, I think (except for the space before "MN") it was good before.
> + FF_ALLOCZ_OR_GOTO(s->avctx, s->mbskip_table, mb_array_size + 2,
> fail);
> + // Note the + 1 is for a quicker mpeg4 slice_end detection
Same.
> + s->parse_context.state = -1;
> + if ((s->avctx->debug&(FF_DEBUG_VIS_QP | FF_DEBUG_VIS_MB_TYPE)) ||
Spaces around &.
> - for(i=0; i<threads; i++){
> - if(init_duplicate_context(s->thread_context[i], s) < 0)
> + for (i = 0; i < threads; i++) {
> + if (init_duplicate_context(s->thread_context[i], s) < 0)
> + goto fail;
> + s->thread_context[i]->start_mb_y = (s->mb_height * (i)
> +
> + s->avctx->thread_count / 2) / s->avctx->thread_count;
> + s->thread_context[i]->end_mb_y = (s->mb_height * (i +
> 1) +
> + s->avctx->thread_count / 2) / s->avctx->thread_count;
The first and 4th line have 4 too many spaces indent. Does it fit on
80 chars if you split at the "="?
Rest OK.
Ronald
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
