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
2011/2/11 Joerg-Cyril.Hoehle@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.
It looks like introducing bigger patches is a kind of mission impossible.
That's not a bad analogy. Your mission, Krzysztof, should you choose to accept it, to improve sound resampling in Wine :) --Juan
-
Joerg-Cyril.Hoehle＠t-systems.com -
Juan Lang -
Krzysztof Nikiel