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

[Krzysztof Nikiel]

2011/2/11  <Joerg-Cyril.Hoehle at t-systems.com>:
> Hi,
>
> The question how to split is interesting.  I haven't found the answer
> about how to split my future MIDI player rewrite either.  Like yours,
> it will replace old functions with something new.

It looks like introducing bigger patches is a kind of mission impossible.

>
> Some guidelines are:
>  - Each patch should be self contained and a logical unit, e.g.
>  + a bug fix, possibly with test
>    e.g. ZeroMemory => FillMemory wBitsPerSample == 8 ? 128 : 0
>  + a change in configuration (e.g. sampling rate 48000)
>    => split change of default & introduction new of registry key
>  + some refactoring in preparation for future work
>  + changes to logic & functionality
>    e.g. DSDDESC_DONTNEEDWRITELEAD, leadin
>  - It must compile.
>  - It should pass tests (a rule, not a dogma).
>
> In this light, [PATCH 08/13] (remove 2x HeapFree) looks wrong, unless
> it fixed a bug in the old code.  It should be part of something else.

But the question is: remove everything unused in one patch or split
the removing.

>
> Try and explain design changes.  Your code is not only a resampler, it
> also changes the way DSound performs mixing, such that
> DSOUND_MixToTemporary becomes superfluous.
>
>
> What's not so nice -- yet that has happened several times already --
> is to have a patch like "Now use the new functions added in the last 12
> patches instead of the old ones", because regression testing will
> irremediably identify that patch as culprit and not help identify
> which of the 12 introducing patches caused the "actual" regression via
> a subtle change in behaviour or side effects.

I would reduce it to two options:
1. Add first, remove last;
2. Remove/stubify first, add last.

First option looks more reasonable to me.

>
> Conversely, if you think about how regression testing can work well,
> you'll find recommendations for patches yourself.

Do you mean bisecting it? I would just wish a good luck to anyone
bisecting such patch.
In this case, I would rather fix bugs more traditional way.

>
> About ordering: Move the patch introducing the new registry key
> towards the end, and work with a constant prior to that.  Doing so,
> this patch becomes working code instead of being ground work not
> testable until the resampler is in place.
>
> I suggest isolating
>  - the FIR generator + Makefile change.
>  - keeping patch 11 -- remove unused files & code last -- so as to
>   minimize size of patches that introduce code.
>
> As a side note, I'm wondering whether I'd insert resample.c into mixer.c.

No way, at least I'm not going to realize such a crazy idea. You're
welcome to try.

>
> +    TRACE("resample quality: %d\n", dsb->quality);
> +    TRACE("resample firstep %d\n", dsb->firstep);
> Logging is expensive (when on) and known to cause race
> conditions.  Hence I'd combine all these into one line.

Those were used for debugging but now are no longer useful, can be removed.

Thanks.