On Thu, Jan 05, 2012 at 07:00:16PM +0200, Yordan Makariev wrote:
>
> --- a/libavcodec/4xm.c
> +++ b/libavcodec/4xm.c
> @@ -30,82 +30,82 @@
>
> -static const uint8_t size2index[4][4]={
> - {-1, 3, 1, 1},
> - { 3, 0, 0, 0},
> - { 2, 0, 0, 0},
> - { 2, 0, 0, 0},
> +static const uint8_t size2index[4][4] = {
> + { -1, 3, 1, 1 },
> + { 3, 0, 0, 0 },
> + { 2, 0, 0, 0 },
> + { 2, 0, 0, 0 },
> };
Now vertical alignment is broken.
Also, this is not four spaces indentation.
I'm slightly frustrated here. I already told you about the missing
indentation in the first comment of the first review I did of this task.
You since sent a full eight patches without addressing this comment.
I haven't seen you ask a single question on this mailing lisr or on IRC.
You just send patch after patch without further communication. This is
obviously not working well and not the way we work around here. If you
don't understand a review comment, reply to it and ask or come to IRC
and ask, but don't just ignore it.
So here's another round of review. Please work through it *thoroughly*
and ask questions if needed. Note that there are more issues but I
have not had time to go through it all line-by-line. Check the code
yourself for similar issues before you send the next patch.
> @@ -119,18 +119,18 @@ static const uint8_t dequant_table[64]={
>
> -typedef struct FourXContext{
> +typedef struct FourXContext {
> AVCodecContext *avctx;
> DSPContext dsp;
> AVFrame current_picture, last_picture;
> - GetBitContext pre_gb; ///< ac/dc prefix
> + GetBitContext pre_gb; /// < ac/dc prefix
That was a Doxygen comment marker, keep it intact.
> @@ -318,103 +327,108 @@ static inline void mcdc(uint16_t *dst, uint16_t *src,
> int log2w, int h, int stri
>
> -static void decode_p_block(FourXContext *f, uint16_t *dst, uint16_t *src,
> int log2w, int log2h, int stride){
> - const int index= size2index[log2h][log2w];
> - const int h= 1<<log2h;
> - int code= get_vlc2(&f->gb,
> block_type_vlc[1-(f->version>1)][index].table, BLOCK_TYPE_VLC_BITS, 1);
> - uint16_t *start= (uint16_t*)f->last_picture.data[0];
> - uint16_t *end= start + stride*(f->avctx->height-h+1) - (1<<log2w);
> +static void decode_p_block(FourXContext *f, uint16_t *dst, uint16_t *src,
> + int log2w, int log2h, int stride)
> +{
> + const int index = size2index[log2h][log2w];
> + const int h = 1 << log2h;
> + int code = get_vlc2(&f->gb, block_type_vlc[1 - (f->version >
> 1)][index].table, BLOCK_TYPE_VLC_BITS, 1);
> + uint16_t *start = (uint16_t *)f->last_picture.data[0];
> + uint16_t *end = start + stride * (f->avctx->height-h + 1) - (1 << log2w);
vertical alignment and break the get_vlc2 line
> -static int decode_p_frame(FourXContext *f, const uint8_t *buf, int length){
> +static int decode_p_frame(FourXContext *f, const uint8_t *buf, int length)
> +{
> int x, y;
> - const int width= f->avctx->width;
> - const int height= f->avctx->height;
> - uint16_t *src= (uint16_t*)f->last_picture.data[0];
> - uint16_t *dst= (uint16_t*)f->current_picture.data[0];
> - const int stride= f->current_picture.linesize[0]>>1;
> + const int width = f->avctx->width;
> + const int heigh t= f->avctx->height;
> + uint16_t *src = (uint16_t *)f->last_picture.data[0];
> + uint16_t *dst = (uint16_t *)f->current_picture.data[0];
> + const int stride = f->current_picture.linesize[0] >> 1;
vertical alignment
> - if(f->version>1){
> - extra=20;
> - bitstream_size= AV_RL32(buf+8);
> - wordstream_size= AV_RL32(buf+12);
> - bytestream_size= AV_RL32(buf+16);
> - }else{
> - extra=0;
> - bitstream_size = AV_RL16(buf-4);
> - wordstream_size= AV_RL16(buf-2);
> - bytestream_size= FFMAX(length - bitstream_size - wordstream_size, 0);
> + if (f->version > 1) {
> + extra = 20;
> + bitstream_size = AV_RL32(buf + 8);
> + wordstream_size = AV_RL32(buf + 12);
> + bytestream_size = AV_RL32(buf + 16);
> + } else {
> + extra = 0;
> + bitstream_size = AV_RL16(buf - 4);
> + wordstream_size = AV_RL16(buf - 2);
> + bytestream_size = FFMAX(length - bitstream_size - wordstream_size,
> 0);
ditto
> - if(bitstream_size+ bytestream_size+ wordstream_size + extra != length
> - || bitstream_size > (1<<26)
> - || bytestream_size > (1<<26)
> - || wordstream_size > (1<<26)
> - ){
> - av_log(f->avctx, AV_LOG_ERROR, "lengths %d %d %d %d\n",
> bitstream_size, bytestream_size, wordstream_size,
> - bitstream_size+ bytestream_size+ wordstream_size - length);
> + if (bitstream_size + bytestream_size + wordstream_size + extra != length
> + || bitstream_size > (1 << 26)
> + || bytestream_size > (1 << 26)
> + || wordstream_size > (1 << 26)) {
Indentation is off.
> + av_log(f->avctx, AV_LOG_ERROR, "lengths %d %d %d %d\n",
> + bitstream_size, bytestream_size, wordstream_size,
> + bitstream_size + bytestream_size + wordstream_size - length);
Indentation is off in the av_log call.
> av_fast_malloc(&f->bitstream_buffer, &f->bitstream_buffer_size,
> bitstream_size + FF_INPUT_BUFFER_PADDING_SIZE);
> if (!f->bitstream_buffer)
> return AVERROR(ENOMEM);
> - f->dsp.bswap_buf(f->bitstream_buffer, (const uint32_t*)(buf + extra),
> bitstream_size/4);
> + f->dsp.bswap_buf(f->bitstream_buffer, (const uint32_t*)(buf + extra),
> bitstream_size / 4);
> memset((uint8_t*)f->bitstream_buffer + bitstream_size, 0,
> FF_INPUT_BUFFER_PADDING_SIZE);
> - init_get_bits(&f->gb, f->bitstream_buffer, 8*bitstream_size);
> + init_get_bits(&f->gb, f->bitstream_buffer, 8 * bitstream_size);
>
> wordstream_offset = extra + bitstream_size;
> bytestream_offset = extra + bitstream_size + wordstream_size;
> bytestream2_init(&f->g2, buf + wordstream_offset, length -
> wordstream_offset);
> - bytestream2_init(&f->g, buf + bytestream_offset, length -
> bytestream_offset);
> + bytestream2_init(&f->g, buf + bytestream_offset, length -
> bytestream_offset);
some long lines to break above
> @@ -424,16 +438,17 @@ static int decode_p_frame(FourXContext *f, const
> uint8_t *buf, int length){
>
> /* DC coef */
> val = get_vlc2(&f->pre_gb, f->pre_vlc.table, ACDC_VLC_BITS, 3);
> - if (val>>4){
> + if (val >> 4) {
> av_log(f->avctx, AV_LOG_ERROR, "error dc run != 0\n");
> }
>
> - if(val)
> + if (val)
> val = get_xbits(&f->gb, val);
Drop the {} from the first if-block while you're at it.
> @@ -441,7 +456,7 @@ static int decode_i_block(FourXContext *f, DCTELEM
> *block){
> i = 1;
> - for(;;) {
> + for (; ; ) {
for (;;) {
> @@ -538,61 +557,65 @@ static const uint8_t *read_huffman_tables(FourXContext
> *f, const uint8_t * const
> memset(frequency, 0, sizeof(frequency));
> memset(up, -1, sizeof(up));
>
> - start= *ptr++;
> - end= *ptr++;
> - for(;;){
> + start = *ptr++;
> + end = *ptr++;
> + for (; ; ) {
ditto
> - while((ptr - buf)&3) ptr++; // 4byte align
> + while ((ptr - buf) & 3) ptr++; // 4byte align
Break this line.
> - if(min_freq[1] == 256*256) break;
> -
> - frequency[j]= min_freq[0] + min_freq[1];
> - flag[ smallest[0] ]= 0;
> - flag[ smallest[1] ]= 1;
> - up[ smallest[0] ]=
> - up[ smallest[1] ]= j;
> - frequency[ smallest[0] ]= frequency[ smallest[1] ]= 0;
> + if (min_freq[1] == 256 * 256)
> + break;
> +
> + frequency[j] = min_freq[0] + min_freq[1];
> + flag[ smallest[0] ] = 0;
> + flag[ smallest[1] ] = 1;
> + up[ smallest[0] ] =
> + up[ smallest[1] ] = j;
> + frequency[ smallest[0] ] = frequency[ smallest[1] ] = 0;
Drop the spaces inside [].
> - for(j=0; j<257; j++){
> + for (j = 0; j < 257; j++) {
> int node;
> - int len=0;
> - int bits=0;
> + int len = 0;
> + int bits = 0;
vertical alignment.
> - bits_tab[j]= bits;
> - len_tab[j]= len;
> + bits_tab[j] = bits;
> + len_tab[j] = len;
more
> @@ -603,54 +626,59 @@ static const uint8_t *read_huffman_tables(FourXContext
> *f, const uint8_t * const
>
> -static int mix(int c0, int c1){
> - int blue = 2*(c0&0x001F) + (c1&0x001F);
> - int green= (2*(c0&0x03E0) + (c1&0x03E0))>>5;
> - int red = 2*(c0>>10) + (c1>>10);
> - return red/3*1024 + green/3*32 + blue/3;
> +static int mix(int c0, int c1)
> +{
> + int blue = 2 * (c0 & 0x001F) + (c1 & 0x001F);
> + int green = (2 * (c0 & 0x03E0) + (c1 & 0x03E0)) >> 5;
> + int red = 2 * (c0 >> 10) + (c1 >> 10);
> + return red / 3 * 1024 + green / 3 * 32 + blue / 3;
> }
more
> -static int decode_i2_frame(FourXContext *f, const uint8_t *buf, int length){
> +static int decode_i2_frame(FourXContext *f, const uint8_t *buf, int length)
> +{
> int x, y, x2, y2;
> - const int width= f->avctx->width;
> - const int height= f->avctx->height;
> - uint16_t *dst= (uint16_t*)f->current_picture.data[0];
> - const int stride= f->current_picture.linesize[0]>>1;
> + const int width = f->avctx->width;
> + const int height = f->avctx->height;
> + uint16_t *dst = (uint16_t*)f->current_picture.data[0];
> + const int stride = f->current_picture.linesize[0]>>1;
more
> -static int decode_i_frame(FourXContext *f, const uint8_t *buf, int length){
> +static int decode_i_frame(FourXContext *f, const uint8_t *buf, int length)
> +{
> int x, y;
> - const int width= f->avctx->width;
> - const int height= f->avctx->height;
> - const unsigned int bitstream_size= AV_RL32(buf);
> + const int width = f->avctx->width;
> + const int height = f->avctx->height;
> + const unsigned int bitstream_size = AV_RL32(buf);
more
> @@ -660,42 +688,43 @@ static int decode_i_frame(FourXContext *f, const
> uint8_t *buf, int length){
>
> - token_count = AV_RL32(buf + bitstream_size + 8);
> + token_count = AV_RL32(buf + bitstream_size + 8);
> prestream_size = 4 * AV_RL32(buf + bitstream_size + 4);
> - prestream = buf + bitstream_size + 12;
> + prestream = buf + bitstream_size + 12;
more
> - if(prestream_size + bitstream_size + 12 != length
> - || bitstream_size > (1<<26)
> - || prestream_size > (1<<26)){
> - av_log(f->avctx, AV_LOG_ERROR, "size mismatch %d %d %d\n",
> prestream_size, bitstream_size, length);
> + if (prestream_size + bitstream_size + 12 != length
> + || bitstream_size > (1 << 26)
> + || prestream_size > (1 << 26)) {
> + av_log(f->avctx, AV_LOG_ERROR, "size mismatch %d %d %d\n",
> + prestream_size, bitstream_size, length);
Indentation of the || is off, same as above.
> @@ -821,61 +853,66 @@ static int decode_frame(AVCodecContext *avctx,
>
> AVCodec ff_fourxm_decoder = {
> - .name = "4xm",
> - .type = AVMEDIA_TYPE_VIDEO,
> - .id = CODEC_ID_4XM,
> + .name = "4xm",
> + .type = AVMEDIA_TYPE_VIDEO,
> + .id = CODEC_ID_4XM,
> .priv_data_size = sizeof(FourXContext),
> - .init = decode_init,
> - .close = decode_end,
> - .decode = decode_frame,
> - .capabilities = CODEC_CAP_DR1,
> + .init = decode_init,
> + .close = decode_end,
> + .decode = decode_frame,
> + .capabilities = CODEC_CAP_DR1,
> .long_name = NULL_IF_CONFIG_SMALL("4X Movie"),
> };
vertical alignment
> --- a/libavdevice/timefilter.c
> +++ b/libavdevice/timefilter.c
> @@ -39,10 +39,10 @@ struct TimeFilter {
>
> TimeFilter * ff_timefilter_new(double clock_period, double feedback2_factor,
> double feedback3_factor)
> {
> - TimeFilter *self = av_mallocz(sizeof(TimeFilter));
> - self->clock_period = clock_period;
> - self->feedback2_factor = feedback2_factor;
> - self->feedback3_factor = feedback3_factor;
> + TimeFilter *self = av_mallocz(sizeof(TimeFilter));
> + self->clock_period = clock_period;
> + self->feedback2_factor = feedback2_factor;
> + self->feedback3_factor = feedback3_factor;
> return self;
Vertical alignment is now broken.
> }
>
> @@ -53,23 +53,23 @@ void ff_timefilter_destroy(TimeFilter *self)
>
> /// update loop
> - self->cycle_time += FFMAX(self->feedback2_factor,
> 1.0/(self->count)) * loop_error;
> + self->cycle_time += FFMAX(self->feedback2_factor, 1.0 /
> (self->count)) * loop_error;
> self->clock_period += self->feedback3_factor * loop_error / period;
ditto
> @@ -89,55 +89,54 @@ int main(void)
> - double best_error= 1000000000;
> - double bestpar0=1;
> - double bestpar1=0.001;
> + double best_error = 1000000000;
> + double bestpar0 = 1;
> + double bestpar1 = 0.001;
> int better, i;
more
Diego
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
