On Sat, Oct 20, 2012 at 12:13:08AM +0200, Maarten Lankhorst wrote:
Hey,
Op 19-10-12 15:29, Andrew Eikum schreef:
Patches 1 and 2 in this series look fine.
I have a series of patches similar to this one in my dsound multichannel branch. This patch seems to do too much at once.
When I did this cleanup, I split it up into four patches:
- Allocate the device format in the Device struct
- Load the default format from the IAC, not hard-coded in dsound
- Put SetFormat() calls into their own WFX that we don't really care
about 4) Always use WAVE_FORMAT_EXTENSIBLE for the device format
3 of 6 does essentially the step you call #4 and #2, other steps is just fixes I already have in my tree. Tested quite extensively too
The other steps I have already done in a followup series, except #1, it's unneeded. It also breaks device->pwfx = NULL in the initial case, which is nice to see if something depends on the new format before changing over is complete. This would otherwise be very hardto detect.
I'm surprised you call #2 a separate step since from what I gathered about mmdevapi is that until win7 sp1 it only supported 1 rate, and we don't implement the rate changing extension yet, so the whole popular format testing was pointless to begin with.
Like the fraglen change in patch 5, It's a change in dsound's behavior that can be isolated from the rest of the patch. Being able to bisect these kinds of changes is really important, as you can run into some really unexpected bugs from how apps use dsound. Bug 29127 is an example of a regression that really surprised me.
Is there anything *technically* wrong with the patch series though?
I haven't dug into them that far yet. Patch 3 needs work, so I don't want to test the further ones until it's fixed.
You don't have to do it the same way I did, but please at least split out the format initialization change from the SetFormat change.
Andrew