Re: [Qemu-devel] [PATCH] Add WinWave-based audio backend to Windows QEMU builds

[David Turner <digit@google.com>]

[-- Attachment #1: Type: text/plain, Size: 5307 bytes --]

On Thu, Jun 11, 2009 at 3:35 AM, malc <av1474@comtv.ru> 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.

I'll send a new version of the patch in a few days. Thanks a lot for the
comments

[-- Attachment #2: Type: text/html, Size: 8774 bytes --]