winealsa.drv patch to enum all software devices from .asoundrc

[Нискородов Серёжа]

Hi, all.

I'm not so good at programming. I just was looking for a way to
make possible play sound from WINE to ALSA software device other than
"default".

I wrote a patch that allows you to select the ALSA software device
from control panel, along with the hardware devices.

Perhaps the code is not so clear and beautiful, but it works for me,
and maybe someone wants to send it to the repository, or to correct
and send a corrected.

[Andrew Eikum]

On Tue, Feb 07, 2012 at 11:32:34PM +0200, Нискородов Серёжа wrote:
> I'm not so good at programming. I just was looking for a way to
> make possible play sound from WINE to ALSA software device other than
> "default".
>
> I wrote a patch that allows you to select the ALSA software device
> from control panel, along with the hardware devices.

Thanks, I've been wanting to do this for a while. I've been hesitating
because ALSA doesn't document the format of their device names. If a
device name contains a colon, is it _always_ a hardware device that
will be returned by the snd_card_* family? Who knows... In any case,
your patch takes the conservative approach, so I think it's a good
start. We can fine-tune it later if we run into problems.

Your patch is mostly correct, but a couple of small things need to be
fixed.
> - void **hints, **n;
> - snd_pcm_stream_t stream2 = SND_PCM_STREAM_PLAYBACK;
> - char *name, *io;
> - const char *filter;
> - WCHAR *nameW;
> - DWORD len;

Your variable declarations need to happen at the start of the
function, where the rest are located.

You can probably come up with a better name than "n" ("hint"? "cur"?).

Why do you create stream2? It looks like stream does what you wanted
already.
> -
>        if(strstr(name, ":") != NULL )
> -
>            goto __end;
> -
>        if (io != NULL && strcmp(io, filter) != 0)
> -
>            goto __end;

Instead of using goto, just use regular if/else. You may have to
indent everything once more, but that's fine.
> -
>                ids[*num] = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> -
>                if(!ids[*num]){
> -
>                    return E_OUTOFMEMORY;
> -
>                }

Doing correct cleanup after HeapAlloc() failure is complicated here.
Since things are going to crash badly if HeapAlloc() fails anyway, I
think you can just assume success and get rid of the error checking.
> -
>            }
> -
>        ++*num;
> -
>        }

Indentation is wrong.
> -
>        if (name != NULL)
> -
>            free(name);

free() checks for NULL anyway, so the if() is not needed.

If snd_device_name_hint() fails, you should print a WARN message so we
can know if something went wrong.

And finally, you should use snd_device_name_free_hint(hints) to clean
up those resources.
> Perhaps the code is not so clear and beautiful, but it works for me,
> and maybe someone wants to send it to the repository, or to correct
> and send a corrected.

It's not bad at all! After fixing the little stuff above, you can send
it to wine-patches yourself. Please read
http://wiki.winehq.org/SubmittingPatches; your wine-devel mail was
not formatted correctly.

Andrew

[Нискородов Серёжа]

> Thanks, I've been wanting to do this for a while. I've been hesitating
> because ALSA doesn't document the format of their device names. If a
> device name contains a colon, is it _always_ a hardware device that
> will be returned by the snd_card_* family? Who knows...

To check whether the device is a hardware, I can call a function
snd_pcm_type. But first I need to get handle of pcm device using the
snd_pcm_open. Okay, but then I have to cancel calling the function
alsa_try_open and rewrite checking availability of the device to not
to opening it twice... Or maybe rewrite function alsa_try_open to
return a snd_pcm_type_t?
Huh ... I decided to just check for the presence of a colon in the
name of the device. So it was easier.
> You can probably come up with a better name than "n" ("hint"? "cur"?).

Heh )) This code was copied from aplay source code with variable
names. But you are right, I will rename a variable.
> Why do you create stream2? It looks like stream does what you wanted
> already.

Honestly, I just do not understand the syntax of its assignment. So I
just copied stream variable from aplay source to.
> > - if(strstr(name, ":") != NULL )
> > - goto __end;
> > - if (io != NULL && strcmp(io, filter) != 0)
> > - goto __end;
>
> Instead of using goto, just use regular if/else. You may have to
> indent everything once more, but that's fine.

This goto was copied from aplay source to ))
Ok. I will try to rewrite this code.
> > - ids[*num] = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > - if(!ids[*num]){
> > - return E_OUTOFMEMORY;
> > - }
>
> Doing correct cleanup after HeapAlloc() failure is complicated here.
> Since things are going to crash badly if HeapAlloc() fails anyway, I
> think you can just assume success and get rid of the error checking.

Are you sure? This piece of code was taken from function
alsa_get_card_devices() (mmdevdrv.c), I just copied the way of
assigning and checking for errors. I think if the check is performed
there, I also have to fulfill it.
> > - if (name != NULL)
> > - free(name);
>
> free() checks for NULL anyway, so the if() is not needed.

This is from aplay source to. ))
Ok. I will correct this.
> If snd_device_name_hint() fails, you should print a WARN message so we
> can know if something went wrong.

You think this is really necessary? If snd_device_name_hint() fails, I
just skip of enumeration of software devices. Does it really need to
print WARN message here?
> And finally, you should use snd_device_name_free_hint(hints) to clean
> up those resources.

Yes, I just forget about it.
> One more thing, this messes up the default device selection, which
> remains hard-coded for the first device. I'd suggest something like
> the following to add to your patch. It chooses "default" as the
> default device, or "pulse" if that doesn't exist.

The point is that the device "default" is already listed with the
function snd_device_name_hint (). So I thought it unnecessary to add a
device "default" in the first place, and deleted the code. After all,
if you select in winecfg "(System default)" then in any case, the
sound goes to the"default" device.Or am I wrong?
> It's not bad at all! After fixing the little stuff above, you can send
> it to wine-patches yourself. Please read
> http://wiki.winehq.org/SubmittingPatches;

Thanks for your support. ))
I read it, but there are so many instructions, I'm afraid to make a
mistake in some ...

By the way, I have created a bug on http://bugs.winehq.org/show_bug.cgi?id=29839
> your wine-devel mail was
> not formatted correctly.

I was trying to Follow all instructions. What is my mistake?

And one more question for the mailing list. Why did not I received my
first letter, though in the settings of the mailing list stands to
receive? If it's because of my mistake in formatting, how you received
it?

[Andrew Eikum]

On Thu, Feb 09, 2012 at 10:08:39PM +0200, Нискородов Серёжа wrote:
> > Thanks, I've been wanting to do this for a while. I've been hesitating
> > because ALSA doesn't document the format of their device names. If a
> > device name contains a colon, is it _always_ a hardware device that
> > will be returned by the snd_card_* family? Who knows...
>
> To check whether the device is a hardware, I can call a function
> snd_pcm_type. But first I need to get handle of pcm device using the
> snd_pcm_open. Okay, but then I have to cancel calling the function
> alsa_try_open and rewrite checking availability of the device to not
> to opening it twice... Or maybe rewrite function alsa_try_open to
> return a snd_pcm_type_t?
> Huh ... I decided to just check for the presence of a colon in the
> name of the device. So it was easier.

Yeah, I think it's okay as it is for now.
> > Why do you create stream2? It looks like stream does what you wanted
> > already.
>
> Honestly, I just do not understand the syntax of its assignment. So I
> just copied stream variable from aplay source to.

It's the conditional operator in C. See:
http://en.wikipedia.org/wiki/%3F:#Usage
> > > - ids[*num] = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > > - if(!ids[*num]){
> > > - return E_OUTOFMEMORY;
> > > - }
> >
> > Doing correct cleanup after HeapAlloc() failure is complicated here.
> > Since things are going to crash badly if HeapAlloc() fails anyway, I
> > think you can just assume success and get rid of the error checking.
>
> Are you sure? This piece of code was taken from function
> alsa_get_card_devices() (mmdevdrv.c), I just copied the way of
> assigning and checking for errors. I think if the check is performed
> there, I also have to fulfill it.

The trouble is that to be completely correct, you have to Free the
values you Allocated into previous members of the ids and keys arrays,
as well as the arrays themselves.  That means a for-loop over only the
valid members of those arrays, and it turns into quite a lot of code
for an error path that will never be executed anyway.

You should either do it correctly or not at all. Your choice :)
> > If snd_device_name_hint() fails, you should print a WARN message so we
> > can know if something went wrong.
>
> You think this is really necessary? If snd_device_name_hint() fails, I
> just skip of enumeration of software devices. Does it really need to
> print WARN message here?

You can use TRACE instead, it doesn't really matter. It's just so we
know what went wrong is someone doesn't get all the devices they
expect. Better to have too much information than too little.
> > One more thing, this messes up the default device selection, which
> > remains hard-coded for the first device. I'd suggest something like
> > the following to add to your patch. It chooses "default" as the
> > default device, or "pulse" if that doesn't exist.
>
> The point is that the device "default" is already listed with the
> function snd_device_name_hint (). So I thought it unnecessary to add a
> device "default" in the first place, and deleted the code. After all,
> if you select in winecfg "(System default)" then in any case, the
> sound goes to the"default" device.Or am I wrong?

The word "default" in very overloaded this context :)

The def_index parameter to AUDDRV_GetEndpointIDs() is what determines
which device gets mapped to System Default. We want to set it to the
index of the system's default device. As the code is in Wine now,
index 0 is always ALSA device "default". But with your changes, index
0 is whatever happens to be returned first from
snd_device_name_hint(). On my system, that's "null" which is not what
we want!

My suggested change sets def_index to the index of the ALSA device
"default" (or "pulse" (or, finally, just the first device)) which is
much more likely to be what the user expects.
> > your wine-devel mail was
> > not formatted correctly.
>
> I was trying to Follow all instructions. What is my mistake?

I just meant that it wasn't correct for sending to wine-patches.
Basically, just make your changes as a commit in Git, and then use
git-format-patch to create a patch file. You can attach that to your
email and send it.

Take a look at some mails here http://source.winehq.org/patches/ for
an idea of what your mail (or attachment) should look like when you
send it.

You can also find more information here:
http://wiki.winehq.org/GitWine

Andrew

[Нискородов Серёжа]

> The trouble is that to be completely correct, you have to Free the
> values you Allocated into previous members of the ids and keys arrays,
> as well as the arrays themselves.  That means a for-loop over only the
> valid members of those arrays, and it turns into quite a lot of code
> for an error path that will never be executed anyway.
>
> You should either do it correctly or not at all. Your choice :)

Okay, I removed all the checks ...
> You can use TRACE instead, it doesn't really matter. It's just so we
> know what went wrong is someone doesn't get all the devices they
> expect. Better to have too much information than too little.

Warn added.
> My suggested change sets def_index to the index of the ALSA device
> "default" (or "pulse" (or, finally, just the first device)) which is
> much more likely to be what the user expects.

AFAIK, ALSA always have a "default" device, so I think there is not
need to check for it presence.
I just add it at zero place, like it was before.
> I just meant that it wasn't correct for sending to wine-patches.
> Basically, just make your changes as a commit in Git, and then use
> git-format-patch to create a patch file. You can attach that to your
> email and send it.
>
> Take a look at some mails here http://source.winehq.org/patches/ for
> an idea of what your mail (or attachment) should look like when you
> send it.

Maybe I can just attach a patch to my bugreport?

[Andrew Eikum]

On Tue, Feb 07, 2012 at 11:32:34PM +0200, Нискородов Серёжа wrote:
> Perhaps the code is not so clear and beautiful, but it works for me,
> and maybe someone wants to send it to the repository, or to correct
> and send a corrected.

One more thing, this messes up the default device selection, which
remains hard-coded for the first device. I'd suggest something like
the following to add to your patch. It chooses "default" as the
default device, or "pulse" if that doesn't exist.

@@ -147,9 +147,6 @@ static CRITICAL_SECTION_DEBUG g_sessions_lock_debug =
 static CRITICAL_SECTION g_sessions_lock = { &g_sessions_lock_debug, -1, 0, 0, 0, 0 };
 static struct list g_sessions = LIST_INIT(g_sessions);

-static const WCHAR defaultW[] = {'d','e','f','a','u','l','t',0};
-static const char defname[] = "default";
-
 static const IAudioClientVtbl AudioClient_Vtbl;
 static const IAudioRenderClientVtbl AudioRenderClient_Vtbl;
 static const IAudioCaptureClientVtbl AudioCaptureClient_Vtbl;
@@ -435,8 +469,6 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow, WCHAR ***ids, char ***keys,
         return E_OUTOFMEMORY;
     }

-    *def_index = 0;
-
     hr = alsa_enum_devices(flow, *ids, *keys, num);
     if(FAILED(hr)){
         int i;
@@ -449,6 +481,18 @@ HRESULT WINAPI AUDDRV_GetEndpointIDs(EDataFlow flow, WCHAR ***ids, char ***keys,
         return E_OUTOFMEMORY;
     }

+    for(*def_index = 0; *def_index < *num; ++*def_index)
+        if(!strcmp((*keys)[*def_index], "default"))
+            break;
+
+    if(*def_index >= *num)
+        for(*def_index = 0; *def_index < *num; ++*def_index)
+            if(!strcmp((*keys)[*def_index], "pulse"))
+                break;
+
+    if(*def_index >= *num)
+        *def_index = 0;
+
     return S_OK;
 }