Re: [RFC/PATCH] sbc: new filtering function for 8 band fixed point encoding

[Marcel Holtmann <marcel@holtmann.org>]

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