Created attachment 115486 [details] [review]
filtering of modem und pc speaker / indicators for OSS and ALSA

Here's a patch, to prevent the listing of the pc speaker in ALSA and OSS and the modem device in the OSS code. In ALSA code the pc speaker was filtered already. So some of the devices are listed droped out. Also I have supplemented remarks like [ALSA] or [OSS] in device descripiton, shown in the dropbox, to indicate, which of them originate the devices. 
Now the OSS provide some devices like my USB headset several times. The duble listing of the VIA 8235 maybe is correct, because they differ in the volume, but OSS delivers it triply.

Created attachment 115488 [details]
the gnome-sound-properties dialog after patch

First off, your patch leaks memory.

Then, I don't think we want to go in and filter devices based on the description string. It might work for a version or two, but sooner or later someone is bound to change "pcsp" to "PC Speaker" or something and then that's that.

Unfortunately, HAL doesn't send many useful attributes for OSS which makes it harder to filter than ALSA in general. I currently don't have a good idea of how to fix this, but I'm no HAL expert. Maybe it does provide some info we could use.

Ok, you are right. The filtering based on the description string isn't a clever solution. I thought it's better than leave that entries on the drop-down list. Maybe a transitional solution until HAL provides more infos to support the filtering. But I have to admit that it was a overhasty measure and there should be a better solution.
Sorry, such a obvious leak of memory is something that shouldn't happen. I am going to change for the better.
However I hold that a indicator to the sound system will improve the usability. But sure it is the least problem.

(In reply to comment #4)
> Sorry, such a obvious leak of memory is something that shouldn't happen. I am
> going to change for the better.

No worries. Happens to the best. ;-)

> However I hold that a indicator to the sound system will improve the usability.

Yes and no. It helps in those cases where you are actually using multiple sound subsystems which return the same devices.

It doesn't help if you only use one subsystem or they return a non-overlapping set of devices.

Created attachment 115597 [details] [review]
enhancement of devicefiltering and display more exact information in device description string

So... here is another patch. This one is a little more comprehensive. 
Surfing the web I found some information... At first, there is a tool called gnome-device-manager (maybe you already know it?), perfectly to explore the HAL. 
Concerning the listing of modem and pc speaker, there are possibilities to filter them using HAL, better than check the description string. Maybe not to 100%, but more safe. I have arranged them in a filterfunction. 
Concerning the multiple listing of some devices, it's initiated by several device files for the same physical device in OSS (/dev/dsp and /dev/audio), which differs only in some properties like default encoding. There aren't a clean way to hide one of them, only by compare with the name of the device file. There was a discussion about that on the HAL mailinglist in 2006 <http://lists.freedesktop.org/archives/hal/2006-August/005772.html>. I don't know if it is the right way to hide one of them, but I think, it is unacceptable to have three entries with the same description. Maybe it is an alternative, to enhance the information, which shown to the several device entries. So the user can distinguish between them. But in the dropdown list it will be confusing, having 10 or more entries with a complex description string. Maybe an checkbox can fade in a optional panel which provides additional information to the device selected, like the device file name.
Next, the HAL keys using to get the description string ("alsa.device_id" and "oss.device_id") aren't mandatory in OSS and ALSA namespace according the HAL secification, so it could be possible go get a empty string, happens with my webcam, which include a capture device. The patch use a combination of card_id and device_id, so there are more definite description to find the right device, unlike to show "usb audio" by using only device_id, which isn't enought if there are more than one usb audio device (like headset and webcam). For the ascertainment of the description string I had created a new function too.
I hope this patch have a higher grade of quality, than the last one had, particularly to memory leaks :-) But I don't know whether it filters too many or too less devices. I think the second is more likely, because maybe there are devices which aren't fit the filterfunction. 
I agree on your comment to the indicators for alsa and oss. Currently I have no idea to implement this in a way that indicators only appears if the same device is available through both systems. However I think, it carries more profit, to show the indicator, if only one system is avalible rather than hide the indicator if both systems are available. 
In current patch I target to the correct listing of playback devices. With capture and mixer there are similar problems. In the dropdown box for the default mixer track, still appears a entry for "pcsp (Alsa mixer)" on my system, for example. 

In order to get the selection of devices more cheerful, it is possible to list alsasink/src and osssink/src only if HAL isn't used? Or will this cause something unintentinal? It's just a question. Some days ago I had nearly nothing know about the sound system and HAL.

Created attachment 115599 [details]
Dialog before apply the patch

Created attachment 115600 [details]
Dialog after apply the patch

That looks quite promising, actually.

As a general comment up front, please always put a space between a function name and the opening brace.

Now for some specifics. Try splitting up the unrequested_device (maybe also call it filter_device) into ALSA, OSS, and generic parts. That makes it a lot easier to make changes to only one system later on.

+		dev_file = libhal_device_get_property_string (ctx, udi, "oss.device_file", NULL);	
+		ignore = dev_file && !strncmp(dev_file,"/dev/audio",10);

Is this OS(Linux?)-specific?

+		dev_file = libhal_device_get_property_string (ctx, udi, "linux.sysfs_path", NULL);	

This certainly is, and besides, I'd consider the sysfs path kernel internals and nothing I'd want to filter on.

That said, you're still leaking a few things. ;-)

> In order to get the selection of devices more cheerful, it is possible to
> list alsasink/src and osssink/src only if HAL isn't used? Or will this cause
> something unintentinal?

Honestly, I don't know.

It looks good, but still, users are shown a list of devices which mean nothing to them. While I like the patch, and I'd accept as soon as all the issues raised by Jens are looked after, I would really like to take the opportunity to try to add some guessing to this dialog. At the end, there are just N devices, so why not just show the physical devices, and try to guess if it should be using ALSA, OSS, PA? Would that be possible?

That way, users would just need to select the physical device, and not if it's the one with OSS or ALSA in the description

I don't think there's a good way to do that short of disabling either OSS or ALSA completely. And whichever you choose, someone's going to be mad at you.

Created attachment 115716 [details] [review]
split up unrequested_devices; renamed to filter_device; removed oss specific filter

I have adapt the patch as follow:
- putting a space after the functionnames and also after each comma in the parameterlists
- renaming the unrequested_device function to filter_device
- and splitting up the function into filter_device_generic and filter_device_alsa

For now I removed the code for oss specific filtering, which only should hide the /dev/audio* at present. Because I can't say if there are systems with OSS, where only /dev/audio* (which is related to Solaris) is available. But now, all devices available by OSS are listed twice. One of them seeming triply, because of the same description string.

By the way, I have thought about which of the OSS devices is used when choosing "OSS - Open Sound System" (osssink). The default value for this is /dev/dsp.

On my system the Open Sound System (and with it the device files /dev/dsp*, /dev/audio* and /dev/adsp), is emulated by ALSA to get those applications working, which use direct access to these files. I think in this case, there is no need to offer any of the OSS related devices to the user. But certainly there are systems which use only the real OSS, so I didn't change the code in this point.

Unfortunately I am pinched for time the next few weeks, so I'm not be able to find a clean solution for this, if at all one exist ;-)

(In reply to comment #12)
> Unfortunately I am pinched for time the next few weeks, so I'm not be able to
> find a clean solution for this, if at all one exist ;-)

The problem's not going to go away. ;-)

I've changed a few minor things with the patch (and plugged a few leaks), but I still have one question. Shouldn't this:

+	/* filter out modem devices */
+	if (!ignore && subsystem && !strcmp (subsystem, "pci")) {
+		dbus_int32_t device_class = libhal_device_get_property_int (ctx,parent_udi, "pci.device_class", NULL);
+		/* this means "Simple communications controllers". Maybe there is a headerfile with definitions?
+		 * visit http://www.acm.uiuc.edu/sigops/roll_your_own/7.c.1.html for further information */
+		ignore = (device_class == 0x7); 
 	}

be OSS-only? ALSA already has

	/* filter out "digitizer", "modem", "none", "unknown" */
	class_string = libhal_device_get_property_string (ctx, udi, "alsa.pcm_class", NULL);
	ignore = class_string != NULL
		 && strcmp (class_string, "generic") != 0
		 && strcmp (class_string, "multi") != 0;

which should take case of modems.

I think filtering by pci.device_class is more generic. On my notebook that works independent from ALSA or OSS. So the code comparing the alsa.pcm_class with "generic" and "multi" seems to be redundant in this case, but I think, this works also for other devices, which aren't "simple communications controllers". Thus both should become a chance to filter out an ALSA related device :-) But on my system it would also work, if the pci.device_class filter is only responsible for OSS devices and the alsa.pcm_class only responsible for ALSA devices. 

Ok, I've made it OSS-only for now. If it turns out it catches more stuff than the ALSA "modem" class we can still change that later.

Closing this bug now, but feel free (encouraged, even) to file further filtering improvements if you can come up with a way to do it. Thanks!

2008-08-02  Jens Granseuer  <jensgr@gmx.net>

        Based on a patch by: Alexander Schwenn <alesch@xelaris.net>

        * sound-properties-capplet.c: (filter_device_generic),
        (filter_device_alsa), (filter_device_oss),
        (get_device_description), (device_added_alsa), (device_added_oss): add
        some filtering for OSS devices and create more useful device
        descriptions (bug #545275)