[Ffmpeg-devel] [PATCH] Vorbis Encoder

[Oded Shimon]

On Sun, Oct 01, 2006 at 06:19:30PM +0200, Oded Shimon wrote:
> 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:
> > >         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
> ?..

Well, it's a little faster, fixed.

> > >                             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?

Used this.

> > >     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.

fixed

> > >     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...

Just checked the speed, it's identical. Might as well use yours, it's 
simpler..

- ods15