On Thu, Jun 11, 2009 at 3:35 AM, malc
<address@hidden> 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<space>(<no space>...<no space>)
Or if you find it more acceptable, per QEMU coding style:
func<no space>(<no space>...<no space>)
controlflow<space>(<no space>...<no space>)
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.