[PATCH 01/13] dsound: New sample rate converter core functions. (resend)
wine-devel at kievinfo.com
Fri Feb 11 09:18:47 CST 2011
Nice job! If it will make Wine's sound better - it would be awesome.
Haven't read the entire patch series. But few nit picks so far.
On 02/11/2011 04:34 AM, Krzysztof Nikiel wrote:
> +typedef double sample_t;
> +static inline sample_t getsample(LPBYTE buf, INT bps)
> + tmp = ((sample_t) (*((BYTE *) buf)) - 128.0);
No need to type cast. "buf" already a pointer to the BYTE. You can use it
directly as *buf or buf, which is even more readable.
> +static inline void putsample(LPBYTE buf, INT bps, sample_t smp)
> + CLIPSAMPLE(smp, (double) 0x7fff);
Typecasts are bad, especially that the type of "smp" is not "double". Please
use "32767.0" instead.
> + DOUBLE amp;
Mixing types again. If you defined your own type then please use it. Or make
everything the same type whichever it is.
> [PATCH 02/13] dsound: Resample filter table generator. (resend)
Don't think compiler helpers should be going into dll directories. The more
correct place is tools. But it's up to AJ.
And as others noted, your patch series is not split correctly. Each patch
will break Wine tree until the very last one. This is against the patch
requirements that code tree should compile after every commit.
You can put your compile helper with it's make file first. Then add your
resampling code with a make file changes to compile it. But not use it yet
(it does break another rule for adding dead code, but that's the only way
you can split the entire series). After that make use of your new resampler,
removing all conflicting code, so Wine will work. Last commit can be a
cleanup for all remaining pieces that are no longer used.
More information about the wine-devel