Re: [PATCH 01/13] dsound: New sample rate converter core functions. (resend)

[Vitaliy Margolen]

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[0], 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[3];
> 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.

Vitaliy.

[PATCH 01/13] dsound: New sample rate converter core functions. (resend)

[Krzysztof Nikiel]

2011/2/11 Vitaliy Margolen <wine-devel(a)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[0], 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[3];
>
> 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.
Thanks.
> Vitaliy.