Re: [PATCH 3/6] dsound: create a primary_pwfx separately from pwfx
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: 1) Allocate the device format in the Device struct 2) Load the default format from the IAC, not hard-coded in dsound 3) Put SetFormat() calls into their own WFX that we don't really care about 4) Always use WAVE_FORMAT_EXTENSIBLE for the device format Much more bisect-able that way. I've attached a tarball here. It's not submission-quality yet (I haven't reviewed it lately; it might not even work), but it should show you what I had in mind for this kind of cleanup. I can bump those up in priority, but I don't think this blocks patches 4-6 in this series anyway. Andrew
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: 1) Allocate the device format in the Device struct 2) Load the default format from the IAC, not hard-coded in dsound 3) 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. Is there anything *technically* wrong with the patch series though? ~Maarten
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: 1) Allocate the device format in the Device struct 2) Load the default format from the IAC, not hard-coded in dsound 3) 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
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: 1) Allocate the device format in the Device struct 2) Load the default format from the IAC, not hard-coded in dsound 3) Put SetFormat() calls into their own WFX that we don't really care about 4) Always use WAVE_FORMAT_EXTENSIBLE for the device format Tried to revisit it and split it up, but there's no sense in keeping it separate. It's in fact quite impossible to do so because of how dsound works.
What I'm doing is the smallest possible thing that makes sense. I want to have writeprimary mode separately, so the commit does pretty much that. When toggling writeprimary mode you reopen the device, those parts are in dsound.c, and you need to change the wave format. Those parts in primary.c do that. It may look like a patch, but all the parts are needed. Splitting this up will just break it, or end up with something that doesn't make sense, some of the changes look big but it's not that big a change. 1. DSOUND_ReopenDevice format changes: - 3 cases, in !writeprimary mode it tries to allocate GetMixFormat as float, and clips channels to 2. If writeprimary, it will try to allocate a waveformatextensible for PCM and IEEE float if possible, and use that. If not possible, it just copies format unmodified. Then calls isformatsupported to get the proper format, and initializes with that. 2. primarybuffer_SetFormat changes: - some removal of code, if !writeprimary, format is not modified, but copied from the internal format instead. if writeprimary, device is reopened like before. Some format recalculations have been moved to PrimaryOpen. 3. SetCooperativeLevel changes: - Just reopen device when toggling writeprimary, straightforward. I would love to split this patch up further, but I ended up with patches that didn't make sense from that. ~Maarten
participants (3)
-
Andrew Eikum -
Maarten Lankhorst -
Maarten Lankhorst