[Ffmpeg-devel] [PATCH] Vorbis Encoder

[Oded Shimon]

On Sun, Oct 01, 2006 at 05:43:47PM +0200, Michael Niedermayer wrote:
> On Sun, Oct 01, 2006 at 02:04:59PM +0200, Oded Shimon wrote:
> > static void ready_floor(floor_t * fc) {
> 
> duplicate of ff_vorbis_ready_floor1_list() ?

oops :)

> [...]
> > static void floor_fit(venc_context_t * venc, floor_t * fc, float * coeffs, int * posts, int samples) {
> >     int range = 255 / fc->multiplier + 1;
> >     int i;
> >     float tot_average = 0.;
> >     for (i = 0; i < fc->values; i++) tot_average += get_floor_average(fc, coeffs, i);
> >     tot_average /= fc->values;
> >     tot_average /= venc->quality;
> > 
> >     for (i = 0; i < fc->values; i++) {
> >         int position = fc->list[fc->list[i].sort].x;
> >         float average = get_floor_average(fc, coeffs, i);
> 
> doesnt this calculate the average twice?

doesn't really matter, i guess affects speed slightly, fixed.

> >     lx = 0;
> >     ly = posts[0] * fc->multiplier; // sorted 0 is still 0
> >     for (i = 1; i < fc->values; i++) {
> >         int pos = fc->list[i].sort;
> >         if (coded[pos]) {
> >             render_line(lx, ly, fc->list[pos].x, posts[pos] * fc->multiplier, floor, samples);
> >             lx = fc->list[pos].x;
> >             ly = posts[pos] * fc->multiplier;
> >         }
> >         if (lx >= samples) break;
> >     }
> >     if (lx < samples) render_line(lx, ly, samples, ly, floor, samples);
> 
> this looks duplicated from the end of vorbis_floor1_decode()

Hmm, I'll try to fix... I'll also move the static function from vorbis.h .

> > static float * put_vector(codebook_t * book, PutBitContext * pb, float * num) {
> >     int i;
> >     int entry = -1;
> >     float distance = 0;
> >     assert(book->dimentions);
> >     for (i = 0; i < book->nentries; i++) {
> >         float d = 0.;
> >         int j;
> >         if (!book->lens[i]) continue;
> >         for (j = 0; j < book->ndimentions; j++) {
> >             float a = (book->dimentions[i * book->ndimentions + j] - num[j]);
> >             d += a*a;
> >         }
> >         if (entry == -1 || distance > d) {
> 
> set distance to FLOAT_MAX initially and forget the entry check
> you can also optimize the loop a little by using:
> sum (a-b)^2 = sum a^2 + sum b^2 - 2*sum ab
> and precalculating sum a^2 and sum b^2

I fail to see how this would be any faster? Still the same big-O, and is
d += (a-b)*(a-b)
in flot significantly slower than
d += a*b
?..

> >     for (p = 0; p < partitions; p++) {
> >         float max1 = 0., max2 = 0.;
> >         int s = rc->begin + p * psize;
> >         for (k = s; k < s + psize; k += 2) {
> >             if (fabs(coeffs[k / real_ch]) > max1) max1 = fabs(coeffs[k / real_ch]);
> >             if (fabs(coeffs[samples + k / real_ch]) > max2) max2 = fabs(coeffs[samples + k / real_ch]);
> 
> max1 = FFMAX(max1, fabs(coeffs[          k / real_ch]));
> max2 = FFMAX(max2, fabs(coeffs[samples + k / real_ch]));

Fixed...

> >                     if (rc->type == 0) {
> >                         for (k = 0; k < psize; k += book->ndimentions) {
> >                             float * a = put_vector(book, pb, &buf[k]);
> >                             int l;
> >                             for (l = 0; l < book->ndimentions; l++) buf[k + l] -= a[l];
> >                         }
> >                     } else {
> >                         for (k = 0; k < psize; k += book->ndimentions) {
> >                             int dim = book->ndimentions, s = rc->begin + p * psize + k, l;
> >                             float vec[dim], * a = vec;
> >                             for (l = s; l < s + dim; l++)
> >                                 *a++ = coeffs[(l % real_ch) * samples + l / real_ch];
> >                             a = put_vector(book, pb, vec);
> >                             for (l = s; l < s + dim; l++)
> >                                 coeffs[(l % real_ch) * samples + l / real_ch] -= *a++;
> 
> the / and % should be avoided, especially considering that real_ch is always 2
> in your current code

Is something like
if (++a == real_ch) { a=0; b++; }
acceptable?

> > static int window(venc_context_t * venc, signed short * audio, int samples) {
> 
> window what? init_window, apply_window, ... please use more descriptive
> names, this one is totally unacceptable (this applies the window and runs
> the mdct over it) call it apply_window_and_mdct() or whatever ...

fixed

> > static int vorbis_encode_init(AVCodecContext * avccontext)
> > {
> >     venc_context_t * venc = avccontext->priv_data;
> > 
> >     if (avccontext->channels != 2) return -1;
> 
> error message please, or we will have heaps of lemmings spamming the MLs

fixed

> >     for (i = 0; i < venc->channels; i++) {
> >         int j;
> >         for (j = 0; j < samples; j++) {
> >             venc->coeffs[i * samples + j] /= venc->floor[i * samples + j];
> >         }
> >     }
> 
> for(i=0; i<venc->channels * samples; i++)
>     venc->coeffs[i] /= venc->floor[i];

evantually this encoder is supposed to support non-constant window sizes, 
though maybe I'll drop that idea. If it does, then not all values are gone 
through.

> >     for (i = 0; i < mapping->coupling_steps; i++) {
> >         float * mag = venc->coeffs + mapping->magnitude[i] * samples;
> >         float * ang = venc->coeffs + mapping->angle[i] * samples;
> >         int j;
> >         for (j = 0; j < samples; j++) {
> >             float m = mag[j];
> >             float a = ang[j];
> >             if (m > 0) {
> >                 ang[j] = m - a;
> >                 if (a > m) mag[j] = a;
> >                 else mag[j] = m;
> >             } else {
> >                 ang[j] = a - m;
> >                 if (a > m) mag[j] = m;
> >                 else mag[j] = a;
> >             }
> >         }
> >     }
> 
> i think the following is equivalent and simpler, (and its immedeatly
> obvious how to reverse it)
> 
> a -= m;
> if(m>0) a= -a;
> if(a<0) m= -m;

You got it wrong in the end, it's 'm=old_a;'
Is this faster or slower? because it is an additional instruction, and 
branches which depend on each other...

- ods15