On Wed, Mar 26, 2014 at 05:48:52PM +0100, Vittorio Giovara wrote:
> --- a/libavcodec/vp8.c
> +++ b/libavcodec/vp8.c
> @@ -124,22 +124,25 @@ static int update_dimensions(VP8Context *s, int width,
> int height)
>
> - s->mb_width = (s->avctx->coded_width +15) / 16;
> - s->mb_height = (s->avctx->coded_height+15) / 16;
> + s->mb_width = (s->avctx->coded_width + 15) / 16;
> + s->mb_height = (s->avctx->coded_height + 15) / 16;
nit: maintain alignment
> @@ -246,13 +249,13 @@ static void get_quants(VP8Context *s)
>
> - s->qmat[i].luma_qmul[0] =
> vp8_dc_qlookup[av_clip_uintp2(base_qi + ydc_delta , 7)];
> + s->qmat[i].luma_qmul[0] = vp8_dc_qlookup[av_clip_uintp2(base_qi +
> ydc_delta , 7)];
space before comma
> @@ -317,24 +320,29 @@ static int decode_frame_header(VP8Context *s, const
> uint8_t *buf, int buf_size)
> if (!s->profile)
> - memcpy(s->put_pixels_tab, s->vp8dsp.put_vp8_epel_pixels_tab,
> sizeof(s->put_pixels_tab));
> + memcpy(s->put_pixels_tab,
> + s->vp8dsp.put_vp8_epel_pixels_tab,
> + sizeof(s->put_pixels_tab));
> else // profile 1-3 use bilinear, 4+ aren't defined so whatever
> - memcpy(s->put_pixels_tab, s->vp8dsp.put_vp8_bilinear_pixels_tab,
> sizeof(s->put_pixels_tab));
> + memcpy(s->put_pixels_tab,
> + s->vp8dsp.put_vp8_bilinear_pixels_tab,
> + sizeof(s->put_pixels_tab));
> @@ -344,11 +352,18 @@ static int decode_frame_header(VP8Context *s, const
> uint8_t *buf, int buf_size)
> - memcpy(s->prob->pred16x16, vp8_pred16x16_prob_inter,
> sizeof(s->prob->pred16x16));
> - memcpy(s->prob->pred8x8c , vp8_pred8x8c_prob_inter ,
> sizeof(s->prob->pred8x8c));
> - memcpy(s->prob->mvc , vp8_mv_default_prob ,
> sizeof(s->prob->mvc));
> + memcpy(s->prob->pred16x16,
> + vp8_pred16x16_prob_inter,
> + sizeof(s->prob->pred16x16));
> + memcpy(s->prob->pred8x8c,
> + vp8_pred8x8c_prob_inter,
> + sizeof(s->prob->pred8x8c));
> + memcpy(s->prob->mvc,
> + vp8_mv_default_prob,
> + sizeof(s->prob->mvc));
two lines would suffice and look better IMO
> @@ -437,7 +451,8 @@ static int decode_frame_header(VP8Context *s, const
> uint8_t *buf, int buf_size)
>
> -static av_always_inline void clamp_mv(VP8Context *s, VP56mv *dst, const
> VP56mv *src)
> +static av_always_inline
> +void clamp_mv(VP8Context *s, VP56mv *dst, const VP56mv *src)
We mostly break this at the comma.
> @@ -505,23 +519,21 @@ int decode_splitmvs(VP8Context *s, VP56RangeCoder *c,
> VP8Macroblock *mb, int lay
>
> - if (vp56_rac_get_prob_branchy(c, vp8_mbsplit_prob[0])) {
> - if (vp56_rac_get_prob_branchy(c, vp8_mbsplit_prob[1])) {
> + if (vp56_rac_get_prob_branchy(c, vp8_mbsplit_prob[0]))
> + if (vp56_rac_get_prob_branchy(c, vp8_mbsplit_prob[1]))
> part_idx = VP8_SPLITMVMODE_16x8 + vp56_rac_get_prob(c,
> vp8_mbsplit_prob[2]);
> - } else {
> + else
> part_idx = VP8_SPLITMVMODE_8x8;
> - }
> - } else {
> + else
> part_idx = VP8_SPLITMVMODE_4x4;
> - }
Most people seem to prefer to maintain the outer {}.
> @@ -562,25 +574,24 @@ int decode_splitmvs(VP8Context *s, VP56RangeCoder *c,
> VP8Macroblock *mb, int lay
> enum { CNT_ZERO, CNT_NEAREST, CNT_NEAR, CNT_SPLITMV };
> enum { VP8_EDGE_TOP, VP8_EDGE_LEFT, VP8_EDGE_TOPLEFT };
> - int idx = CNT_ZERO;
> + int idx = CNT_ZERO;
> int cur_sign_bias = s->sign_bias[mb->ref_frame];
> int8_t *sign_bias = s->sign_bias;
> VP56mv near_mv[4];
> - uint8_t cnt[4] = { 0 };
> + uint8_t cnt[4] = { 0 };
> VP56RangeCoder *c = &s->c;
IMO excessive alignment, these assignments are not related.
> @@ -588,24 +599,24 @@ void decode_mvs(VP8Context *s, VP8Macroblock *mb, int
> mb_x, int mb_y, int layout
> +#define MV_EDGE_CHECK(n)
> \
> + {
> \
> + VP8Macroblock *edge = mb_edge[n];
> \
> + int edge_ref = edge->ref_frame;
> \
same
> @@ -777,8 +795,8 @@ skip_eob:
>
> if (!vp56_rac_get_prob_branchy(&c, token_prob[2])) { // DCT_1
> - coeff = 1;
> - token_prob = probs[i+1][1];
> + coeff = 1;
> + token_prob = probs[i + 1][1];
same
> @@ -811,22 +829,24 @@ skip_eob:
> *r = c;
> return i;
> }
> +
> #endif
stray change
> @@ -1036,7 +1064,7 @@ void intra_predict(VP8Context *s, VP8ThreadData *td,
> uint8_t *dst[3],
> } else {
> - uint8_t *ptr = dst[0];
> + uint8_t *ptr = dst[0];
> uint8_t *intra4x4 = mb->intra4x4_pred_mode_mb;
> uint8_t tr_top[4] = { 127, 127, 127, 127 };
excessive alignment
> @@ -1058,28 +1085,30 @@ void intra_predict(VP8Context *s, VP8ThreadData *td,
> uint8_t *dst[3],
> for (y = 0; y < 4; y++) {
> uint8_t *topright = ptr + 4 - s->linesize;
> for (x = 0; x < 4; x++) {
> - int copy = 0, linesize = s->linesize;
> - uint8_t *dst = ptr+4*x;
> - DECLARE_ALIGNED(4, uint8_t, copy_dst)[5*8];
> + int copy = 0, linesize = s->linesize;
> + uint8_t *dst = ptr + 4 * x;
> + DECLARE_ALIGNED(4, uint8_t, copy_dst)[5 * 8];
same
> @@ -1283,23 +1321,27 @@ void vp8_mc_part(VP8Context *s, VP8ThreadData *td,
> uint8_t *dst[3],
>
> -/* Fetch pixels for estimated mv 4 macroblocks ahead.
> - * Optimized for 64-byte cache lines. Inspired by ffh264 prefetch_motion. */
> +/**
> + * Fetch pixels for estimated mv 4 macroblocks ahead.
> + * Optimized for 64-byte cache lines. Inspired by ffh264 prefetch_motion.
> + */
You're changing this into a Doxygen comment. That does not belong in a
K&R patch and this comment should not be Doxygen. You're also blowing
it up to four lines.
> @@ -1310,10 +1352,10 @@ static av_always_inline
> void inter_predict(VP8Context *s, VP8ThreadData *td, uint8_t *dst[3],
> VP8Macroblock *mb, int mb_x, int mb_y)
> {
> - int x_off = mb_x << 4, y_off = mb_y << 4;
> - int width = 16*s->mb_width, height = 16*s->mb_height;
> + int x_off = mb_x << 4, y_off = mb_y << 4;
> + int width = 16 * s->mb_width, height = 16 * s->mb_height;
Definitely excessive alignment; this looks silly.
> @@ -1325,41 +1367,41 @@ void inter_predict(VP8Context *s, VP8ThreadData *td,
> uint8_t *dst[3],
>
> - for (y = 0; y < 2; y++) {
> + for (y = 0; y < 2; y++)
> for (x = 0; x < 2; x++) {
> }
> - }
Keep the {} around the outer for.
> @@ -1411,36 +1457,41 @@ static av_always_inline void idct_mb(VP8Context *s,
> VP8ThreadData *td,
> }
> }
> -chroma_idct_end: ;
> +chroma_idct_end:;
WTH?
I suggest (separately) dropping this empty statement.
> @@ -1492,82 +1546,84 @@ static av_always_inline void filter_mb(VP8Context *s,
> uint8_t *dst[3], VP8Filter
> if (inner_filter) {
> - s->vp8dsp.vp8_h_loop_filter_simple(dst+ 4, linesize, bedge_lim);
> - s->vp8dsp.vp8_h_loop_filter_simple(dst+ 8, linesize, bedge_lim);
> - s->vp8dsp.vp8_h_loop_filter_simple(dst+12, linesize, bedge_lim);
> + s->vp8dsp.vp8_h_loop_filter_simple(dst + 4, linesize, bedge_lim);
> + s->vp8dsp.vp8_h_loop_filter_simple(dst + 8, linesize, bedge_lim);
> + s->vp8dsp.vp8_h_loop_filter_simple(dst + 12, linesize, bedge_lim);
> }
>
> if (inner_filter) {
> - s->vp8dsp.vp8_v_loop_filter_simple(dst+ 4*linesize, linesize,
> bedge_lim);
> - s->vp8dsp.vp8_v_loop_filter_simple(dst+ 8*linesize, linesize,
> bedge_lim);
> - s->vp8dsp.vp8_v_loop_filter_simple(dst+12*linesize, linesize,
> bedge_lim);
> + s->vp8dsp.vp8_v_loop_filter_simple(dst + 4 * linesize, linesize,
> bedge_lim);
> + s->vp8dsp.vp8_v_loop_filter_simple(dst + 8 * linesize, linesize,
> bedge_lim);
> + s->vp8dsp.vp8_v_loop_filter_simple(dst + 12 * linesize, linesize,
> bedge_lim);
You could right-align the numbers here.
> @@ -1581,16 +1637,18 @@ static void vp8_decode_mv_mb_modes(AVCodecContext
> *avctx, VP8Frame *curframe,
> for (mb_y = 0; mb_y < s->mb_height; mb_y++) {
> - VP8Macroblock *mb = s->macroblocks_base + ((s->mb_width+1)*(mb_y +
> 1) + 1);
> - int mb_xy = mb_y*s->mb_width;
> + VP8Macroblock *mb = s->macroblocks_base +
> + ((s->mb_width + 1) * (mb_y + 1) + 1);
> + int mb_xy = mb_y * s->mb_width;
probably excessive
> @@ -1644,51 +1705,58 @@ static void
> vp8_decode_mb_row_no_filter(AVCodecContext *avctx, void *tdata,
> {
> VP8Context *s = avctx->priv_data;
> VP8ThreadData *prev_td, *next_td, *td = &s->thread_data[threadnr];
> - int mb_y = td->thread_mb_pos>>16;
> - int mb_x, mb_xy = mb_y*s->mb_width;
> - int num_jobs = s->num_jobs;
> + int mb_y = td->thread_mb_pos >> 16;
> + int mb_x, mb_xy = mb_y * s->mb_width;
> + int num_jobs = s->num_jobs;
same
> uint8_t *dst[3] = {
> - curframe->tf.f->data[0] + 16*mb_y*s->linesize,
> - curframe->tf.f->data[1] + 8*mb_y*s->uvlinesize,
> - curframe->tf.f->data[2] + 8*mb_y*s->uvlinesize
> + curframe->tf.f->data[0] + 16 * mb_y * s->linesize,
> + curframe->tf.f->data[1] + 8 * mb_y * s->uvlinesize,
> + curframe->tf.f->data[2] + 8 * mb_y * s->uvlinesize
Could also be aligned a bit more.
> @@ -1749,43 +1820,48 @@ static void
> vp8_decode_mb_row_no_filter(AVCodecContext *avctx, void *tdata,
> static void vp8_filter_mb_row(AVCodecContext *avctx, void *tdata,
> int jobnr, int threadnr)
> {
> - VP8Context *s = avctx->priv_data;
> + VP8Context *s = avctx->priv_data;
looks excessive
> @@ -1796,22 +1872,25 @@ static void vp8_filter_mb_row(AVCodecContext *avctx,
> void *tdata,
> static int vp8_decode_mb_row_sliced(AVCodecContext *avctx, void *tdata,
> int jobnr, int threadnr)
> {
> - VP8Context *s = avctx->priv_data;
> - VP8ThreadData *td = &s->thread_data[jobnr];
> + VP8Context *s = avctx->priv_data;
> + VP8ThreadData *td = &s->thread_data[jobnr];
> VP8ThreadData *next_td = NULL, *prev_td = NULL;
> - VP8Frame *curframe = s->curframe;
> - int mb_y, num_jobs = s->num_jobs;
> + VP8Frame *curframe = s->curframe;
> + int num_jobs = s->num_jobs;
> + int mb_y;
same
> @@ -1878,57 +1957,61 @@ int ff_vp8_decode_frame(AVCodecContext *avctx, void
> *data, int *got_frame,
>
> - // Given that arithmetic probabilities are updated every frame, it's
> quite likely
> - // that the values we have on a random interframe are complete junk if
> we didn't
> - // start decode on a keyframe. So just don't display anything rather
> than junk.
> + /* Given that arithmetic probabilities are updated every frame, it's
> quite
> + * likely that the values we have on a random interframe are complete
> + * junk if we didn't start decode on a keyframe. So just don't display
> + * anything rather than junk. */
> if (!s->keyframe && (!s->framep[VP56_FRAME_PREVIOUS] ||
> !s->framep[VP56_FRAME_GOLDEN] ||
> !s->framep[VP56_FRAME_GOLDEN2])) {
Align the ||.
> --- a/libavcodec/vp8.h
> +++ b/libavcodec/vp8.h
> @@ -26,16 +26,19 @@
> #ifndef AVCODEC_VP8_H
> #define AVCODEC_VP8_H
>
> +#include "config.h"
> +
> #include "libavutil/buffer.h"
>
> -#include "vp56.h"
> -#include "vp8dsp.h"
> #include "h264pred.h"
> #include "thread.h"
> +#include "vp56.h"
> +#include "vp8dsp.h"
> +
> #if HAVE_PTHREADS
> -#include <pthread.h>
> +# include <pthread.h>
> #elif HAVE_W32THREADS
> -#include "compat/w32pthreads.h"
> +# include "compat/w32pthreads.h"
> #endif
I don't know if I'd indent the #includes this way.
The config.h #include was not there before, that does not
belong in a K&R patch.
> --- a/libavcodec/vp8dsp.c
> +++ b/libavcodec/vp8dsp.c
> @@ -123,38 +124,38 @@ static void vp8_idct_dc_add_c(uint8_t *dst, int16_t
> block[16], ptrdiff_t stride)
> static void vp8_idct_dc_add4y_c(uint8_t *dst, int16_t block[4][16],
> ptrdiff_t stride)
> {
> - vp8_idct_dc_add_c(dst+ 0, block[0], stride);
> - vp8_idct_dc_add_c(dst+ 4, block[1], stride);
> - vp8_idct_dc_add_c(dst+ 8, block[2], stride);
> - vp8_idct_dc_add_c(dst+12, block[3], stride);
> + vp8_idct_dc_add_c(dst + 0, block[0], stride);
> + vp8_idct_dc_add_c(dst + 4, block[1], stride);
> + vp8_idct_dc_add_c(dst + 8, block[2], stride);
> + vp8_idct_dc_add_c(dst + 12, block[3], stride);
nit: Maintain alignment.
> @@ -219,68 +224,76 @@ static av_always_inline void filter_mbedge(uint8_t *p,
> ptrdiff_t stride)
>
> -#define LOOP_FILTER(dir, size, stridea, strideb, maybe_inline) \
> -static maybe_inline void vp8_ ## dir ## _loop_filter ## size ## _c(uint8_t
> *dst, ptrdiff_t stride,\
> - int flim_E, int flim_I, int hev_thresh)\
> -{\
> - int i;\
> -\
> - for (i = 0; i < size; i++)\
> - if (normal_limit(dst+i*stridea, strideb, flim_E, flim_I)) {\
> - if (hev(dst+i*stridea, strideb, hev_thresh))\
> - filter_common(dst+i*stridea, strideb, 1);\
> - else\
> - filter_mbedge(dst+i*stridea, strideb);\
> - }\
> -}\
> -\
> -static maybe_inline void vp8_ ## dir ## _loop_filter ## size ##
> _inner_c(uint8_t *dst, ptrdiff_t stride,\
> - int flim_E, int flim_I, int
> hev_thresh)\
> -{\
> +#define LOOP_FILTER(dir, size, stridea, strideb, maybe_inline)
> \
> + static maybe_inline void vp8_ ## dir ## _loop_filter ## size ##
> _c(uint8_t *dst, \
> +
> ptrdiff_t stride, \
> + int
> flim_E, \
> + int
> flim_I, \
> + int
> hev_thresh) \
Drop the initial indentation in this case.
> + {
> \
> + int i;
> \
> + for (i = 0; i < size; i++)
> \
> + if (normal_limit(dst + i * stridea, strideb, flim_E, flim_I)) {
> \
> + if (hev(dst + i * stridea, strideb, hev_thresh))
> \
> + filter_common(dst + i * stridea, strideb, 1);
> \
> + else
> \
> + filter_mbedge(dst + i * stridea, strideb);
> \
> + }
> \
> + }
> \
> + static maybe_inline void vp8_ ## dir ## _loop_filter ## size ##
> _inner_c(uint8_t *dst, \
> +
> ptrdiff_t stride, \
> +
> int flim_E, \
And add an empty line between function definitions.
> @@ -299,184 +312,198 @@ static void vp8_h_loop_filter_simple_c(uint8_t *dst,
> ptrdiff_t stride, int flim)
> #define VP8_MC_FUNC(IDX, SIZE) \
> - dsp->put_vp8_epel_pixels_tab[IDX][0][0] = put_vp8_pixels ## SIZE ## _c; \
> - dsp->put_vp8_epel_pixels_tab[IDX][0][1] = put_vp8_epel ## SIZE ## _h4_c;
> \
> - dsp->put_vp8_epel_pixels_tab[IDX][0][2] = put_vp8_epel ## SIZE ## _h6_c;
> \
> - dsp->put_vp8_epel_pixels_tab[IDX][1][0] = put_vp8_epel ## SIZE ## _v4_c;
> \
> + dsp->put_vp8_epel_pixels_tab[IDX][0][0] = put_vp8_pixels ## SIZE ## _c;
> \
> + dsp->put_vp8_epel_pixels_tab[IDX][0][1] = put_vp8_epel ## SIZE ## _h4_c;
> \
> + dsp->put_vp8_epel_pixels_tab[IDX][0][2] = put_vp8_epel ## SIZE ## _h6_c;
> \
> + dsp->put_vp8_epel_pixels_tab[IDX][1][0] = put_vp8_epel ## SIZE ## _v4_c;
> \
> dsp->put_vp8_epel_pixels_tab[IDX][1][1] = put_vp8_epel ## SIZE ##
> _h4v4_c; \
> dsp->put_vp8_epel_pixels_tab[IDX][1][2] = put_vp8_epel ## SIZE ##
> _h6v4_c; \
> - dsp->put_vp8_epel_pixels_tab[IDX][2][0] = put_vp8_epel ## SIZE ## _v6_c;
> \
> + dsp->put_vp8_epel_pixels_tab[IDX][2][0] = put_vp8_epel ## SIZE ## _v6_c;
> \
> dsp->put_vp8_epel_pixels_tab[IDX][2][1] = put_vp8_epel ## SIZE ##
> _h4v6_c; \
> dsp->put_vp8_epel_pixels_tab[IDX][2][2] = put_vp8_epel ## SIZE ## _h6v6_c
Either align the \ or don't change it.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
