Subject: Re: kern/29178: confusing behaviour because mixer classes in eap
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Frederick Bruckman <fredb@immanent.net>
List: netbsd-bugs
Date: 02/13/2005 19:59:01
The following reply was made to PR kern/29178; it has been noted by GNATS.

From: Frederick Bruckman <fredb@immanent.net>
To: Joachim Kuebart <kuebart@mathematik.uni-ulm.de>
Cc: gnats-bugs@netbsd.org
Subject: Re: kern/29178: confusing behaviour because mixer classes in eap
 driver are misnamed
Date: Sun, 13 Feb 2005 13:57:48 -0600 (CST)

 On Sat, 12 Feb 2005, Joachim Kuebart wrote:
 
 > I do agree that the appropriate class for the volume controls at A in the
 > picture above is definitely "inputs." The part of my patch that puts these
 > in the "record" class should be ignored and I will supply a new patch without
 > it upon request or when the remaining issues are resolved (whichever occurs
 > first :-).
 
 Please?!
 
 > As far as I can see, there are three naming possibilities for selector B:
 > (Since it selects among DAC, Line-In, CD etc., "outputs.select" is clearly
 > wrong since that chooses among speaker, headphone and line out).
 
 "inputs.source" sounds good to me.
 
 > 	With the current implementation, the record.port value of audio_info
 > 	in AUDIO_SETINFO value controls this selector and therefore selects
 > 	which of the mic, line and cd sources are played-through, which is
 > 	not what I would expect from record.port. I would instead expect it
 > 	to select the recording source, being selector C "record.source". To
 > 	fix this, the code in dev/audio.c should to be changed to use
 > 	"record.source" for the value of record.port.
 
 Oh, I see now why you wanted to move everything to "record".  Here's 
 a patch (below) to choose "record.source" over "inputs.source", if it 
 exists.  Also, it permits "inputs.master" as a fallback for 
 "record.master", for symmetry, although no driver I know of currently 
 has that.
 
 Note that some drivers don't have "record.source", but only 
 "inputs.source" (which incidently sets the recording source, recalling 
 ASCII art exhibit "A").  We need to be careful not to break things for 
 those folks.  I actually think it's more useful to be able to set the 
 input source for playback than record, but the control is called 
 "record.port", not "input.port", so your point is taken.
 
 > Sorry for such a long post, am I just confused or is this really such an
 > involved topic?
 
 I can only speak for myself, but it takes my a while to wrap my head 
 around all this. The difficulty with working with the "audio" 
 abstraction is that you have to try to make reasonable guesses about 
 all possible hardware and drivers (as opposed to the more usual thing, 
 of dictating an interface to which the drivers must conform).
 
 
 Frederick
 
 
 Index: ./sys/dev/audio.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/audio.c,v
 retrieving revision 1.182.2.1
 diff -u -r1.182.2.1 audio.c
 --- ./sys/dev/audio.c	23 Jul 2004 23:05:50 -0000	1.182.2.1
 +++ ./sys/dev/audio.c	13 Feb 2005 19:14:55 -0000
 @@ -231,7 +231,8 @@
   	void *hdlp = sa->hdl;
   	int error;
   	mixer_devinfo_t mi;
 -	int iclass, mclass, oclass, props;
 +	int iclass, mclass, oclass, rclass, props;
 +	int record_master_found, record_source_found;
 
   #ifdef DIAGNOSTIC
   	if (hwp == 0 ||
 @@ -296,7 +297,7 @@
 
   	sc->sc_input_fragment_length = 0;
 
 -	iclass = mclass = oclass = -1;
 +	iclass = mclass = oclass = rclass = -1;
   	sc->sc_inports.index = -1;
   	sc->sc_inports.master = -1;
   	sc->sc_inports.nports = 0;
 @@ -314,33 +315,52 @@
   	sc->sc_outports.mixerout = -1;
   	sc->sc_outports.cur_port = -1;
   	sc->sc_monitor_port = -1;
 +	/*
 +	 * Read through the underlying driver's list, picking out the class
 +	 * names from the mixer descriptions. We'll need them to decode the
 +	 * mixer descriptions on the next pass through the loop.
 +	 */
   	for(mi.index = 0; ; mi.index++) {
   		if (hwp->query_devinfo(hdlp, &mi) != 0)
   			break;
 +		 /*
 +		  * The type of AUDIO_MIXER_CLASS merely introduces a class.
 +		  * All the other types describe an actual mixer.
 +		  */
   		if (mi.type == AUDIO_MIXER_CLASS) {
 -			if (strcmp(mi.label.name, AudioCrecord) == 0)
 +			if (strcmp(mi.label.name, AudioCinputs) == 0)
   				iclass = mi.mixer_class;
   			if (strcmp(mi.label.name, AudioCmonitor) == 0)
   				mclass = mi.mixer_class;
   			if (strcmp(mi.label.name, AudioCoutputs) == 0)
   				oclass = mi.mixer_class;
 +			if (strcmp(mi.label.name, AudioCrecord) == 0)
 +				rclass = mi.mixer_class;
   		}
   	}
 +	/*
 +	 * This is where we assign each control in the "audio" model, to the
 +	 * underlying "mixer" control.  We walk through the whole list once,
 +	 * assigning likely candidates as we come across them.
 +	 */
 +	record_master_found = 0;
 +	record_source_found = 0;
   	for(mi.index = 0; ; mi.index++) {
   		if (hwp->query_devinfo(hdlp, &mi) != 0)
   			break;
   		if (mi.type == AUDIO_MIXER_CLASS)
   			continue;
   		if (mi.mixer_class == iclass) {
 -			if (strcmp(mi.label.name, AudioNmaster) == 0)
 -				sc->sc_inports.master = mi.index;
 -#if 1	/* Deprecated. Use AudioNmaster. */
 -			if (strcmp(mi.label.name, AudioNrecord) == 0)
 -				sc->sc_inports.master = mi.index;
 -			if (strcmp(mi.label.name, AudioNvolume) == 0)
 +			/*
 +			 * AudioCinputs is only a fallback, when we don't
 +			 * find what we're looking for in AudioCrecord, so
 +			 * check the flags before accepting one of these.
 +			 */
 +			if (strcmp(mi.label.name, AudioNmaster) == 0
 +			    && record_master_found == 0)
   				sc->sc_inports.master = mi.index;
 -#endif
 -			if (strcmp(mi.label.name, AudioNsource) == 0) {
 +			if (strcmp(mi.label.name, AudioNsource) == 0
 +			    && record_source_found == 0) {
   				if (mi.type == AUDIO_MIXER_ENUM) {
   				    int i;
   				    for(i = 0; i < mi.un.e.num_mem; i++)
 @@ -361,6 +381,38 @@
   			if (strcmp(mi.label.name, AudioNselect) == 0)
   				au_setup_ports(sc, &sc->sc_outports, &mi,
   				    otable);
 +		} else if (mi.mixer_class == rclass) {
 +			/*
 +			 * These are the preferred mixers for the audio record
 +			 * controls, so set the flags here, but don't check.
 +			 */
 +			if (strcmp(mi.label.name, AudioNmaster) == 0) {
 +				sc->sc_inports.master = mi.index;
 +				record_master_found = 1;
 +			}
 +#if 1	/* Deprecated. Use AudioNmaster. */
 +			if (strcmp(mi.label.name, AudioNrecord) == 0) {
 +				sc->sc_inports.master = mi.index;
 +				record_master_found = 1;
 +			}
 +			if (strcmp(mi.label.name, AudioNvolume) == 0) {
 +				sc->sc_inports.master = mi.index;
 +				record_master_found = 1;
 +			}
 +#endif
 +			if (strcmp(mi.label.name, AudioNsource) == 0) {
 +				if (mi.type == AUDIO_MIXER_ENUM) {
 +				    int i;
 +				    for(i = 0; i < mi.un.e.num_mem; i++)
 +					if (strcmp(mi.un.e.member[i].label.name,
 +						    AudioNmixerout) == 0)
 +						sc->sc_inports.mixerout =
 +						    mi.un.e.member[i].ord;
 +				}
 +				au_setup_ports(sc, &sc->sc_inports, &mi,
 +				    itable);
 +				record_source_found = 1;
 +			}
   		}
   	}
   	DPRINTF(("audio_attach: inputs ports=0x%x, input master=%d, "