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

[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.

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.

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.

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

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.

+    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.

Also keep in mind that your reactions here will influence acceptance
of your code -- "Does this person sound like s/he's going to support
and fix regressions in her/his code or is it 'take or leave, bye'?"

Regards,
 Jörg Höhle