Hi Guys, As Siarhei said, we had a talk last week about The optimizations to the code. I added the modifications to the code, which reduce The amount of operations quite a lot. The problem With this modification is that it also reduces the Readability of the code. So because of the redundancy in the cosine transform it is possible to reduce the number of variables and operations in the preceding filtering. This is very hard to explain with comments in the code (so you would write something like "here should be t[12], but The table goes only to 8 because of redundancy in the following Cosine transform). To really make the stuff Understandable one should write some small Wikipage or extensive comments to the code. So this is the path the previous code also took, but At some point there was a mistake. I really don't still Get how the anamatrix stuff was calculated, but as You see it takes time to reverse engineer :) But I suggest me and Siarhei clean the code internally And try really hard to follow the conventions. It starts To get little bit messy, because we both have many Versions of the code. Br, Jaska >-----Original Message----- >From: ext Marcel Holtmann [mailto:marcel@xxxxxxxxxxxx] >Sent: 23 December, 2008 03:00 >To: Siamashka Siarhei (Nokia-D/Helsinki) >Cc: ext Brad Midgley; Uimonen Jaska (Nokia-D/Helsinki); >linux-bluetooth@xxxxxxxxxxxxxxx >Subject: Re: [RFC/PATCH] sbc: new filtering function for 8 >band fixed point encoding > >Hi Siarhei, > >> > We had a talk with Jaska Uimonen here, and now I'm kind of >delegated >> > to finish the work on this filtering function for SBC encoder >> > (including the final addition of ARM assembly optimizations). He >> > provided me with his last variant of code, which contains >some more >> > optimizations to reduce the number of operations and also loops >> > unrolling. I will add his changes to the patch on next iteration. >> > >> > Now the question is how to best integrate a fixed >filtering function >> > to git repository? If I just continue adding changes to >the patch in >> > order to make it a faster, it will be also not so obvious >to see how >> > we got to these code transformations just from the commit log. >> >> Next iteration done. Added support for 4 subbands, number of >> arithmetic operations reduced (but without loop unrolling for better >> code readability), precision improved for both 16-bit and >32-bit fixed >> point, 'neginv' macro is now more portable and faster. The >rest is in the code comments. > >I don't mind having patches as attachment, but this makes it >hard to review and comment on them. Especially when it comes >to stuff like coding style (since I have no ideas about the rest). > >+ t1[0] = t1[1] = t1[2] = t1[3] = >+ (FIXED_A)1 << (SBC_PROTO_FIXED4_SCALE-1); > >Should be like this: (FIXED_A) 1 << (SBC_PROTO_FIXED4_SCALE - 1); > >Also do you need the (FIXED_A) cast? > >+ for (hop = 0; hop < 40; hop += 8) { >+ t1[0] += (FIXED_A)in[hop] * _sbc_proto_fixed4[hop]; > >Same here. There has to be a space after every case. > >+ t1[i] = (FIXED_A)1 << >+ (SBC_COS_TABLE_FIXED4_SCALE-1-SCALE_OUT_BITS); > >And between every operation there has to be a space: SCALE - 1 - SCALE. >In this case I would actually put the - 1 at the end, but that >is pure cosmetics and not a coding style violation. > >Please fix all of these. There at least 8 or so. > >+#define neginv(x) ((-2 >> 1 == -1) ? \ >+ ((((int32_t)(x)) >> 31) ^ (int32_t)(x)) : \ >+ ((x >= 0) ? (x) : -(x)-1)) > >Space after cast. Space before and after operator. > >+#ifdef SBC_HIGH_PRECISION >+# define FIXED_A int64_t /* data type for fixed point >accumulator */ # >+define FIXED_T int32_t /* data type for fixed point constants */ # >+define SBC_FIXED8_EXTRA_BITS 16 #else # define FIXED_A >int32_t /* data >+type for fixed point accumulator */ # define FIXED_T int16_t /* data >+type for fixed point constants */ # define SBC_FIXED8_EXTRA_BITS 0 >+#endif > >No space between # and define. I know that this is meant to >improve readability, but I don't see it. > >Regards > >Marcel > > > -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
