[FFmpeg-devel] [Ffmpeg-devel] [PATCH] Vorbis I floor type 0 support

[Michael Niedermayer]

On Fri, Feb 03, 2006 at 09:44:48AM +0200, Oded Shimon wrote:
> On Fri, Jan 27, 2006 at 06:14:35PM +0100, Alexander Strasser wrote:
> > Alexander Strasser wrote:
> > > Benjamin Larsson wrote:
> > [...]
> > > > Indentation. It's some more also.
> > > 
> > >   Right, there were some more remaining style errors than i thought.
> > > ( This happens when you look at your own code too many times ^^" )
> > >   Hopefully i caught all of them now; new patch attached.
> > >   
> > 
> >   Sorry, really attached now.
> 
> [...]
> 
> > +    vorbis_floor_decode_func floor_decode;
> 
> You used a single a variable for pointer to decode function. There's no 
> garuntee anywhere in the Vorbis spec that all floors in a single file are 
> the same type. You should make this variable in the floor sturct, and per 
> mapping/floor.
> 
> Other than that, I like this patch... Only thing missing making FFmpeg 
> Vorbis decoder complete...

s/complete/complete mess/

Anyway, what kind of review was this supposed to be?

Highly misleading code:
+            if(!floor_setup->data.t0.book_list) { return 1; }
+            /* read book indexes */
+            {



almost cosmetics which should have been in a seperate patch:
-                    if ((floor_setup->x_list[j]>floor_setup->x_list[k]) &&
-                      (floor_setup->x_list[j]<floor_setup->x_list[floor_setup->high_neighbour[k]])) {
-                        floor_setup->high_neighbour[k]=j;
+                    if ((floor_setup->data.t1.x_list[j]>floor_setup->data.t1.x_list[k]) &&
+                      (floor_setup->data.t1.x_list[j]<floor_setup->data.t1.x_list[floor_setup->data.t1.high_neighbour[k]])) {
+                        floor_setup->data.t1.high_neighbour[k]=j;


strange indention:
+#   ifdef V_DEBUG        
+    for(idx=0; idx<=n;++idx) {
+        AV_DEBUG("floor0 map: map at pos %d is %d\n",
+                 idx, map[idx]);
+    }
+#   endif
+}

either ifdef should be moved left or the for right


completely unneeded {}
+            }
+
+            /* allocate mem for lsp coefficients */
+            {
+                /* codebook dim is for padding if codebook dim doesn't *
+                 * divide order+1 then we need to read more data       */
+                floor_setup->data.t0.lsp=


too much on a single line:
+                do { vec[i]=q; ++i; }while(vf->map[i]==iter_cond);


code like:
    int_fast32_t pow_of_two = 2, exponent = vf->amplitude_bits;
    while ( --exponent ) { pow_of_two <<= 1; }
instead of
    pow_of_two = 1<<vf->amplitude_bits;

And the name pow_of_two is definitly bad as well.


and
+                         q=exp( (
+                             ( (amplitude*vf->amplitude_offset)/
+                               ((pow_of_two-1) * sqrt(p+q)) )
+                             - vf->amplitude_offset ) * .11512925f
+                         );

instead of

q= pow(10, 0.05*(


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080316/104c507d/attachment.pgp>