From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MF2fs-0001MY-Vk for qemu-devel@nongnu.org; Fri, 12 Jun 2009 05:03:49 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MF2fo-0001Lf-Pa for qemu-devel@nongnu.org; Fri, 12 Jun 2009 05:03:48 -0400 Received: from [199.232.76.173] (port=33537 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MF2fo-0001Lc-Ag for qemu-devel@nongnu.org; Fri, 12 Jun 2009 05:03:44 -0400 Received: from smtp-out.google.com ([216.239.33.17]:21055) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MF2fm-0006Yr-9X for qemu-devel@nongnu.org; Fri, 12 Jun 2009 05:03:43 -0400 Received: from zps19.corp.google.com (zps19.corp.google.com [172.25.146.19]) by smtp-out.google.com with ESMTP id n5C93dKv013999 for ; Fri, 12 Jun 2009 10:03:40 +0100 Received: from bwz6 (bwz6.prod.google.com [10.188.26.6]) by zps19.corp.google.com with ESMTP id n5C93aFJ019569 for ; Fri, 12 Jun 2009 02:03:37 -0700 Received: by bwz6 with SMTP id 6so2048783bwz.21 for ; Fri, 12 Jun 2009 02:03:36 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <60cad3f0906101715ufc754d1q339870a9fb8f8107@mail.gmail.com> <4A3052DA.1010308@codemonkey.ws> <60cad3f0906101749n7fcb11efvaf7e0979ee4b6ae1@mail.gmail.com> Date: Fri, 12 Jun 2009 11:03:33 +0200 Message-ID: <60cad3f0906120203m3bb91fa5s7581a2561942b7db@mail.gmail.com> Subject: Re: [Qemu-devel] [PATCH] Add WinWave-based audio backend to Windows QEMU builds From: David Turner Content-Type: multipart/alternative; boundary=001636c5b52bc99136046c22fb33 List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: malc Cc: qemu-devel@nongnu.org --001636c5b52bc99136046c22fb33 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 --001636c5b52bc99136046c22fb33 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

On Thu, Jun 11, 2009 at 3:35 AM, malc <av1474@comtv.ru&= gt; 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 t= hat
for now, since the attached version applies cleanly.

That's unfortunate. I really need to understand w= hy 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 s= omething like that.
=A0

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 res= t of the audio
backends.

=A0


> +/* XP works well with 4, but Vista struggles with anything less than = 8 */
> +#define =A0NUM_OUT_BUFFERS =A08 =A0/* 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.
=A0

> +
> +/** COMMON UTILITIES
> + **/
> +
> +#if DEBUG
> +static void
> +dump_mmerror( const char* =A0func, MMRESULT =A0error )
> +{
> + =A0 =A0const char* =A0reason =3D NULL;
> +
> + =A0 =A0fprintf(stderr, "%s returned error: ", func);
> + =A0 =A0switch (error) {
> + =A0 =A0 =A0 =A0case MMSYSERR_ALLOCATED: =A0 reason=3D"specified= resource is already
> allocated"; break;
> + =A0 =A0 =A0 =A0case MMSYSERR_BADDEVICEID: reason=3D"bad device = id"; break;
> + =A0 =A0 =A0 =A0case MMSYSERR_NODRIVER: =A0 =A0reason=3D"no driv= er is present"; break;
> + =A0 =A0 =A0 =A0case MMSYSERR_NOMEM: =A0 =A0 =A0 reason=3D"unabl= e to allocate or lock
> memory"; break;
> + =A0 =A0 =A0 =A0case WAVERR_BADFORMAT: =A0 =A0 reason=3D"unsuppo= rted waveform-audio
> format"; break;
> + =A0 =A0 =A0 =A0case WAVERR_SYNC: =A0 =A0 =A0 =A0 =A0reason=3D"d= evice is synchronous"; break;
> + =A0 =A0 =A0 =A0default:
> + =A0 =A0 =A0 =A0 =A0 =A0fprintf(stderr, "unknown(%d)\n", er= ror);
> + =A0 =A0}
> + =A0 =A0if (reason)
> + =A0 =A0 =A0 =A0fprintf(stderr, "%s\n", reason);
> +}
> +#else
> +# =A0define =A0dump_mmerror(func,error) =A0((void)0)
> +#endif

1. It should be unconditional

why is that, the f= unction is only used when debugging is turned on, and will result in a comp= iler
warning if I leave it. Or do you mean that debug mode should always= be on ?
=A0

2. switch (error) {
=A0 case ...:
=A0 default: return;
=A0 }
=A0 fprintf (...)
3. The lines are extra long

ok, wil= l fix this
=A0

It appears that s->silence is never used.

True, it's probably obsolete right now, so I'= ll remove it.
=A0

> + =A0 =A0format.nAvgBytesPerSec =3D (format.nSamplesPerSec & forma= t.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?=A0

> + =A0 =A0format.nBlockAlign =A0 =A0 =3D format.nChannels << shif= t;
> + =A0 =A0format.wBitsPerSample =A0=3D 8 << shift;

[Just checking - is wBitsPerSample is really sample or frame? Only ALSA, to= the
=A0best of my knowledge makes clear distinction between the two]

I believe it is per channel sample only.
Documenta= tion states it should be 8 or 16, except for floats which should use 32 (an= d 0 for compressed formats)
=A0
http://msdn.microsoft.com/en-us/library/ms7912= 74.aspx

Given that stereo float is supported, and that the documentation says i= t "must" be 32 for float, I assume
it's channel per sample= .


> + =A0 =A0}

Indentation is off here.

ok, will fix these
=A0

> +
> + =A0 =A0samples_size =A0 =A0=3D format.nBlockAlign * conf.nb_samples;=
> + =A0 =A0s->buffer_bytes =3D qemu_malloc( NUM_OUT= _BUFFERS * samples_size );

audio_calloc ought to be used here...

ok
=A0

> + =A0 =A0if (s->buffer_bytes =3D=3D NULL) {
> + =A0 =A0 =A0 =A0 =A0 =A0waveOutClose( s->waveout= );
> + =A0 =A0 =A0 =A0 =A0 =A0s->waveout =3D NULL;
> + =A0 =A0 =A0 =A0 =A0 =A0fprintf(stderr, "not enough memory for W= indows audio
> buffers\n");
> + =A0 =A0 =A0 =A0 =A0 =A0return -1;
> + =A0 =A0}

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 tha= t waveOutReset() was called
before in the same function, the "buffe= rs still playing error" shouldn't be returned
by waveOutClose()= , and any other error condition is hardly recoverable anyway.
=A0

> +
> + =A0 =A0audio_pcm_init_info (&hw->info, as);
> + =A0 =A0hw->samples =3D conf.nb_samples*2;

This one I don't get, why is it unconditionally multiplied by 2?<= br>

Probabl= y 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 a= nd U32 then.
=A0


Asterisks are c++isplaced.

yes, will fix that while reformatting.
=A0
I'll send a new version of the patch in a few days. Thanks a lot fo= r the comments

--001636c5b52bc99136046c22fb33--