[Ffmpeg-devel] [PATCH] Vorbis Encoder
Michael Niedermayer
michaelni
Sun Oct 1 10:04:43 CEST 2006
Hi
On Sat, Sep 30, 2006 at 11:33:49PM +0300, Oded Shimon wrote:
> $subj
>
> Some cleanups still left - move the codebook stuff to a seperate file,
> move the bitstream stuff to bitstream.h . Will move the codebook stuff
> only after moving to ffmpeg repo...
>
> This will be commited using ~90 commits for full history, with an
> additional commit at the end for makefile/avocdec.h/allcodecs.c
>
> Quality is comparable to official vorbis encoder (to my ears), at similar
> bitrate. bitrate and quality can be manipulated with -aq, 10 to 30 are
> sane values, the higher the number, the higher the bitrate/quality.
>
> Only 2 channel is supported, and, in a psy sense, 44100/48000 is best
> supported...
>
> Open for review... :)
>
> - ods15
[...]
> static int cb_lookup_vals(int lookup, int dimentions, int entries) {
> if (lookup == 1) {
> int tmp, i;
> for (tmp = 0; ; tmp++) {
> int n = 1;
> for (i = 0; i < dimentions; i++) n *= tmp;
> if (n > entries) break;
> }
duplicate of nth_root() of vorbis.c
> return tmp - 1;
> } else if (lookup == 2) return dimentions * entries;
> return 0;
> }
>
> static void ready_codebook(codebook_t * cb) {
> int h[33] = { 1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 };
> int i;
>
> for (i = 0; i < cb->nentries; i++) {
> cb_entry_t * e = &cb->entries[i];
> int j = 0;
> if (!e->len) continue;
> if (h[0]) h[0] = 0;
> else {
> for (j = e->len; j; j--)
> if (h[j]) break;
> assert(j);
> }
> e->codeword = h[j];
> h[j] = 0;
> for (j++; j <= e->len; j++) h[j] = e->codeword | (1 << (j - 1));
> }
> for (i = 0; i < 33; i++) assert(!h[i]);
this looks like a duplicate of vorbis_len2vlc() in vorbis.c
[...]
> static void ready_floor(floor_t * fc) {
> int i;
> fc->list[0].sort = 0;
> fc->list[1].sort = 1;
> for (i = 2; i < fc->values; i++) {
> int j;
> fc->list[i].low = 0;
> fc->list[i].high = 1;
> fc->list[i].sort = i;
> for (j = 2; j < i; j++) {
> int tmp = fc->list[j].x;
> if (tmp < fc->list[i].x) {
> if (tmp > fc->list[fc->list[i].low].x) fc->list[i].low = j;
> } else {
> if (tmp < fc->list[fc->list[i].high].x) fc->list[i].high = j;
> }
> }
> }
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
[...]
> int codebook28[] = { 2, 5, 5, 6, 6, 7, 7, 7, 7, 7, 7, 8, 8, 8, 8, 8, 8, 10, 6, 6, 7, 7, 8, 7, 8, 8, 8, 8, 8, 9, 9, 9, 9, 9, 10, 6, 6, 7, 7, 7, 7, 8, 8, 8, 8, 9, 9, 9, 9, 9, 9, 10, 7, 7, 7, 7, 8, 8, 8, 8, 9, 9, 9, 9, 9, 9, 9, 9, 10, 10, 10, 7, 7, 8, 8, 8, 9, 9, 9, 9, 9, 9, 9, 9, 9, 11, 11, 11, 8, 8, 8, 8, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 10, 10, 10, 8, 8, 8, 8, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 10, 10, 10, 8, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 10, 9, 10, 10, 10, 11, 11, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 11, 10, 11, 11, 11, 9, 9, 9, 9, 9, 9, 10, 10, 9, 9, 10, 9, 11, 10, 11, 11, 11, 9, 9, 9, 9, 9, 9, 9, 9, 10, 10, 10, 9, 11, 11, 11, 11, 11, 9, 9, 9, 9, 10, 10, 9, 9, 9, 9, 10, 9, 11, 11, 11, 11, 11, 11, 11, 9, 9, 9, 9, 9, 9, 10, 10, 10, 10, 11, 11, 11, 11, 11, 11, 11, 10, 9, 10, 10, 9, 10, 9, 9, 10, 9, 11, 10, 10, 11, 11, 11, 11, 9, 10, 9, 9, 9, 9, 10, 10, 10, 10, 11, 11, 11, 11, 11, 11, 10, 10, 10, 9, 9, 10, 9, 10, 9, 10, 10, 10, 10, 11, 11, 11, 11, 11, 11, 11, 9, 9, 9, 9, 9, 10, 10, 10, };
>
> 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
> struct {
> int lookup;
> int dim;
> float min;
> float delta;
> int real_len;
> int * quant;
> } cvectors[] = {
> { 1, 8, -1.0, 1.0, 6561,(int[]){ 1, 0, 2, } },
> { 1, 4, -2.0, 1.0, 625, (int[]){ 2, 1, 3, 0, 4, } },
> { 1, 4, -2.0, 1.0, 625, (int[]){ 2, 1, 3, 0, 4, } },
> { 1, 2, -4.0, 1.0, 81, (int[]){ 4, 3, 5, 2, 6, 1, 7, 0, 8, } },
> { 1, 2, -4.0, 1.0, 81, (int[]){ 4, 3, 5, 2, 6, 1, 7, 0, 8, } },
> { 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, } },
> { 1, 4, -11.0, 11.0, 81, (int[]){ 1, 0, 2, } },
> { 1, 2, -5.0, 1.0, 121, (int[]){ 5, 4, 6, 3, 7, 2, 8, 1, 9, 0, 10, } },
> { 1, 2, -30.0, 5.0, 169, (int[]){ 6, 5, 7, 4, 8, 3, 9, 2, 10, 1, 11, 0, 12, } },
> { 1, 2, -2.0, 1.0, 25, (int[]){ 2, 1, 3, 0, 4, } },
> { 1, 2, -1530.0, 255.0, 169, (int[]){ 6, 5, 7, 4, 8, 3, 9, 2, 10, 1, 11, 0, 12, } },
> { 1, 2, -119.0, 17.0, 225, (int[]){ 7, 6, 8, 5, 9, 4, 10, 3, 11, 2, 12, 1, 13, 0, 14, } },
> { 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
[...]
> 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 ...
> int j;
> for (j = 0; j < 8; j++) rc->books[i][j] = a[i][j];
memcpy()
[...]
> 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
[...]
> 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 ...
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is
More information about the ffmpeg-devel
mailing list