[Ffmpeg-devel] [PATCH] Vorbis Encoder

[Oded Shimon]

On Sun, Oct 01, 2006 at 10:04:43AM +0200, Michael Niedermayer wrote:
> On Sat, Sep 30, 2006 at 11:33:49PM +0300, Oded Shimon wrote:
> [..]
> duplicate of nth_root() of vorbis.c
[..]
> this looks like a duplicate of vorbis_len2vlc() in vorbis.c
[..]
> that one looks like its part of vorbis_parse_setup_hdr_floors() from vorbis.c
> please remove all code duplications between vorbis.c & vorbis_enc.c
> ill review the rest after you removed all duplicate code

It's quite non trivial to remove all duplicate code between vorbis.c and 
vorbis_enc.c, because even though the algos are the same, the structs are 
completely different. The only way I can think of doing is by using common 
struct names with similar members, and have these helper functions in 
vorbis.h, and include vorbis.h AFTER the struct definitions in vorbis.c 
and vorbis_enc.c (same code, but different compilation for different 
files). This seems quite dirty, is this the way you intended?

> [...]
> >     int codebook_sizes[] = { 16, 8, 256, 64, 128, 32, 96, 32, 96, 17, 32, 78, 17, 32, 78, 100, 1641, 443, 105, 68, 81, 289, 81, 121, 169, 25, 169, 225, 289, };
> >     int * codebook_lens[] = { codebook0,  codebook1,  codebook2,  codebook3,  codebook4,  codebook5,  codebook6,  codebook7,
> >                               codebook8,  codebook9,  codebook10, codebook11, codebook12, codebook13, codebook14, codebook15,
> >                               codebook16, codebook17, codebook18, codebook19, codebook20, codebook21, codebook22, codebook23,
> >                               codebook24, codebook25, codebook26, codebook27, codebook28, };
> > 
> 
> why is this int and not (u)int8_t ?
> and the lines are too long
> [...]
> >         { 1, 2,    -8.0,   1.0, 289, (int[]){ 8, 7, 9, 6, 10, 5, 11, 4, 12, 3, 13, 2, 14, 1, 15, 0, 16, } },
> >     };
> int -> uint8_t

This entire area is to be cleaned up after i switch to ffmpeg repo, so i 
can create vorbis_enc.h

> [...]
> >     for (i = 0; i < rc->classifications; i++) {
> >         int a[10][8] = {
> >             { -1, -1, -1, -1, -1, -1, -1, -1, },
> >             { -1, -1, 16, -1, -1, -1, -1, -1, },
> >             { -1, -1, 17, -1, -1, -1, -1, -1, },
> >             { -1, -1, 18, -1, -1, -1, -1, -1, },
> >             { -1, -1, 19, -1, -1, -1, -1, -1, },
> >             { -1, -1, 20, -1, -1, -1, -1, -1, },
> >             { -1, -1, 21, -1, -1, -1, -1, -1, },
> >             { 22, 23, -1, -1, -1, -1, -1, -1, },
> >             { 24, 25, -1, -1, -1, -1, -1, -1, },
> >             { 26, 27, 28, -1, -1, -1, -1, -1, },
> >         };
> 
> int -> static const int8_t, some compilers will initalize such arrays
> every time the code gets executed ... 

fixed

> >     	int j;
> >     	for (j = 0; j < 8; j++) rc->books[i][j] = a[i][j];
> 
> memcpy()

fixed

> [...]
> >     mc->magnitude = av_malloc(sizeof(int) * mc->coupling_steps);
> >     mc->angle = av_malloc(sizeof(int) * mc->coupling_steps);
> 
> av_malloc(sizeof(*mc->angle) ...
> 
> would be safer in case the type changes

Should I change for all cases of av_malloc ?..

> [...]
> >     int ady = FFMAX(dy, -dy);
> 
> ABS(dy)
> 
> 
> >     int base = dy / adx;
> >     int x = x0;
> >     int y = y0;
> >     int err = 0;
> >     int sy;
> >     if (dy < 0) sy = base - 1;
> >     else sy = base + 1;
> 
> id align these:
> if (dy < 0) sy = base - 1;
> else        sy = base + 1;
> 
> makes them look much nicer and more readable ...

This part is actually also common code in vorbis.c and vorbis_enc.c ... :) 
And is actually the part which is most easily made common (doesn't depend 
on any structs).

- ods15