On Thu, Jun 11, 2009 at 3:35 AM, malc wrote: > On Thu, 11 Jun 2009, David Turner wrote: > > Thanks for submitting this. > > > Sorry, here's the patch inlined: > > And it was mangled beyond recognition by your mailer, but nevermind that > for now, since the attached version applies cleanly. > That's unfortunate. I really need to understand why git-send-email crashes on my cygwin installation. It should make all these things disappear. I hope it's just a missing Perl dependency or something like that. > > Stylistic nit, the thing is terribly inconsistent, it's neither in QEMU > nor in audio/* style. Personally I would have preferred it to look like > the rest of audio, i.e.: > > func/controlflow(...) > > Or if you find it more acceptable, per QEMU coding style: > > func(...) > controlflow(...) > ok, I'll try to reformat it to use the audio style. It's only logical that it looks like the rest of the audio backends. > > > > +/* XP works well with 4, but Vista struggles with anything less than 8 > */ > > +#define NUM_OUT_BUFFERS 8 /* must be at least 2 */ > > I wonder how Vista works with dsound, okayish, struggles? > I have no idea really, and I don't have a Vista machine near me to test that with a custom build at the moment. > > > + > > +/** COMMON UTILITIES > > + **/ > > + > > +#if DEBUG > > +static void > > +dump_mmerror( const char* func, MMRESULT error ) > > +{ > > + const char* reason = NULL; > > + > > + fprintf(stderr, "%s returned error: ", func); > > + switch (error) { > > + case MMSYSERR_ALLOCATED: reason="specified resource is already > > allocated"; break; > > + case MMSYSERR_BADDEVICEID: reason="bad device id"; break; > > + case MMSYSERR_NODRIVER: reason="no driver is present"; break; > > + case MMSYSERR_NOMEM: reason="unable to allocate or lock > > memory"; break; > > + case WAVERR_BADFORMAT: reason="unsupported waveform-audio > > format"; break; > > + case WAVERR_SYNC: reason="device is synchronous"; > break; > > + default: > > + fprintf(stderr, "unknown(%d)\n", error); > > + } > > + if (reason) > > + fprintf(stderr, "%s\n", reason); > > +} > > +#else > > +# define dump_mmerror(func,error) ((void)0) > > +#endif > > 1. It should be unconditional why is that, the function is only used when debugging is turned on, and will result in a compiler warning if I leave it. Or do you mean that debug mode should always be on ? > > 2. switch (error) { > case ...: > default: return; > } > fprintf (...) > 3. The lines are extra long > ok, will fix this > > It appears that s->silence is never used. > True, it's probably obsolete right now, so I'll remove it. > > > + format.nAvgBytesPerSec = (format.nSamplesPerSec & format.nChannels) > << > > shift; > > ~format.nChannels was meant perhaps? > wow, interesting, thanks a lot. I'm surprised the thing still works though, or if this is related to the Vista issues? > > > + format.nBlockAlign = format.nChannels << shift; > > + format.wBitsPerSample = 8 << shift; > > [Just checking - is wBitsPerSample is really sample or frame? Only ALSA, to > the > best of my knowledge makes clear distinction between the two] > I believe it is per channel sample only. Documentation states it should be 8 or 16, except for floats which should use 32 (and 0 for compressed formats) http://msdn.microsoft.com/en-us/library/ms791274.aspx Given that stereo float is supported, and that the documentation says it "must" be 32 for float, I assume it's channel per sample. > > + } > > Indentation is off here. > ok, will fix these > > > + > > + samples_size = format.nBlockAlign * conf.nb_samples; > > + s->buffer_bytes = qemu_malloc( NUM_OUT_BUFFERS * samples_size ); > > audio_calloc ought to be used here... > ok > > > + if (s->buffer_bytes == NULL) { > > + waveOutClose( s->waveout ); > > + s->waveout = NULL; > > + fprintf(stderr, "not enough memory for Windows audio > > buffers\n"); > > + return -1; > > + } > > Whose return value is indeed ought to be checked for NULL, but no > message is necessary. Checks in audio_calloc once have proven to be > useful when some FreeBSD returned bogus number of fragments(0) and > consequently attempt was made to allocate 0 bytes for oss's pcm_buf. > I'm not sure we need to check anything. Given that waveOutReset() was called before in the same function, the "buffers still playing error" shouldn't be returned by waveOutClose(), and any other error condition is hardly recoverable anyway. > > > + > > + audio_pcm_init_info (&hw->info, as); > > + hw->samples = conf.nb_samples*2; > > This one I don't get, why is it unconditionally multiplied by 2? > Probably because the code has only been tested with 16-bit stereo :-) I'll look into that. By the way I realize that 32-bit audio samples are not supported by WinWave, the code should probably return an error for S32 and U32 then. > > > Asterisks are c++isplaced. > yes, will fix that while reformatting. I'll send a new version of the patch in a few days. Thanks a lot for the comments