[PATCH 01/13] dsound: New sample rate converter core functions. (resend)
knik00 at gmail.com
Fri Feb 11 09:56:39 CST 2011
2011/2/11 Vitaliy Margolen <wine-devel at kievinfo.com>:
> 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.
Indeed, but I think it would be even better to change 'buf' type to
void* as the actual size/type is defined by 'bps'.
>> +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.
So maybe something like (sample_t)0x7fff would be better or better yet
move the typecast to the macro definition.
>> + DOUBLE amp;
> Mixing types again. If you defined your own type then please use it. Or make
> everything the same type whichever it is.
I'm not quite sure what type it should be, 'sample_t' doesn't look
quite good to me but maybe you're right.
>> [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.
Ah, I see.
> 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.
Sounds reasonable, but not quite easy to do. It looks like wine will
use old code for some time until I try to do the correct split.
More information about the wine-devel