Summary:	ASTERISK-04011: [patch] improved chan_oss.c driver
Reporter:	Luigi Rizzo (rizzo)	Labels:
Date Opened:	2005-04-28 13:35:30	Date Closed:	2011-06-07 14:05:27
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) chan_oss.c ( 1) chan_oss.c.correct.diff ( 2) chan_oss-cur-20050501.diff.txt ( 3) chan_oss.c.HEAD-20050504
Description:	The attached file is an improved and largely rewritten version of chan_oss.c. The original motivation was fixing interaction with FreeBSD audio drivers (some of them do not support correctly the SETFRAGMENT call, but the resulting code is hopefully more maintainable and easier to adapt to different systems, as it exposes in the configuration file mamy parameters used to configure the audio card: device name, block and queue size, mixer commands. It also supports multiple audio cards in the same box. ****** ADDITIONAL INFORMATION ****** Rather than providing a diff, I am providing a full file, which is derived from the 1.0.7 release. Porting to HEAD should not be hard (mostly incorporating changes to the struct that stores the channel's callbacks). I am willing to do it myself if there is interest. In the meantime this version of the code will be committed in the FreeBSD's CVS containing the asterisk port
Comments:	By: Matthew Fredrickson (mattf) 2005-04-28 17:02:37 What does this have to do with ASTCC? By: Clod Patry (junky) 2005-04-28 21:08:10 Please provide a patch for we can take a look at modification directly. By: Luigi Rizzo (rizzo) 2005-04-29 02:28:07 Am sending a patch if that pleases you - chan_oss.c.correct.diff (please discard chan_oss.c.diff which is a bogus file). This is against the one in RELEASE_1.0.7 Though I still believe the changes are more readable by looking at the new file as a whole, because of the amount of refactoring done.  In any case, a high level description of the changes follows: + most global/static variables related to audio devices have been moved to  the struct chan_oss_pvt . In addition to make the code easier to follow,  this also allowed me to support multiple audio devices concurrently; + as a consequence of the above, most functions now have an additional  argument of type struct chan_oss_pvt * pointing to the object they  are operating on; + the console's commands (dial hangup etc.) are now directed to one  of the audio devices which can be set/shown/listed using the new  'console' command. + parameters to be used to set the fragment and queue sizes have been  largely commented, and made configurable at run-time through entries  in the oss.conf file + multiple audio devices are now supported by using multiple sections in  the oss.conf file. [general] contains to the defaults and the actual  configuration of Console/dsp, other sections [foo] [bar] etc.  override the default configuration for Console/foo, Console/bar etc. + mixer commands to be run at startup can be specified in the oss.conf file  with "mixer= -f /dev/mixer0 pcm 95 vol 90 igain 100"  syntax + calls to the ioctl() to determine the audio queue fill level have been  put in a single function called as appropriate; + same for calls that write to the audio device; + send_sound has been rewritten and heavily commented. It also uses  soundcard_writeframe() to access the audio device; + setformat() has been changed to be called multiple times, and to fix  a bug in resetting "lasttime" variable for half-duplex support; + setinput/setoutput are a lot simpler now because all the complexity  is in setformat (once instead of 3 times) + a macro, RING, has been defined to take care of playing signalling sounds; + oss_write has been rewritten and its behaviour documented. + most console_*() commands now have code to direct the command to the  active device; I am still unclear on whether half duplex works. It had some bugs in the original code, those which I understood I tried to fix, but overall i cannot figure out what component is in charge of deciding when to switch between read and write. Usually half-duplex systems have a PTT button or use the local audio input to simulate it. Here, there is none of these, so I don't know when to switch. In fact, i wonder if half-duplex support makes sense at all. In the original file it made the code very hard to follow. Now it is more readable but still confusing. I am also unclear on some issues, mostly related to freeing resources, and those are marked with XXX in the code. There is a huge amount of replication between chan_oss.c and chan_alsa.c which could probably be removed easily by having a single file with conditional compilation and compile it with -DALSA or -DOSS to generate the two modules. Not having ALSA i cannot test this. edited on: 04-29-05 02:30 By: Kevin P. Fleming (kpfleming) 2005-04-29 12:53:36 I'm not sure what you expect us to do with this... new features (which this contains a large number of) do not get added to the stable branch. If you want to port this work to CVS HEAD and open a bug for that when you are ready, then it can be considered. It's always better to have multiple, smaller patches though, rather than a single complete-file replacement. By: Luigi Rizzo (rizzo) 2005-04-29 17:05:14 My goal was have some of you developers have a look at this version of the code, and decide either "yes we find it useful to have", in which case i can do the work to 1) make HEAD compile under FreeBSD, 2) the port these changes to HEAD, 3) make sure that they find their way in -stable too; or you decide anything else, in which case i won't bother and just stick my version as a patch in the FreeBSD port and be happy with it. I am afraid to say, the response so far has not been exactly encouraging. Most of the bugnotes are nitpicking about me not following the procedures (yes i am guilty) whereas i was trying to get a response on the technical issues. If there is any better way to discuss this please let me know. If there are pointers to what are your policies, please let me know too (on FreeBSD we do import features in HEAD first, but then they migrate to the -stable versions in many cases). I tried to subscribe to asterisk-dev but unsuccessfully (perhaps it is a closed list ? but i cannot see it mentioned) Note however, the chan_oss in 1.0.7 is not working well with FreeBSD, and as much as it can be FreeBSD's fault (I am working on that side too, have some patches to the audio driver to be committed soon), i believe it is in everyone's interest to have asterisk working without requiring people to upgrade their OS. Considering the state of this code (author's words at the beginning of the file) i believe this particular file deserves some work even in the -stable branch. Besides, it is a "leaf" module, not a core component of asterisk where one has to be very cautious with changes. Some of the extra "features" (sound ioctls) are not "bells and whistles", they are necessary to fix the interaction with different devices and operating systems. The others (multiple card support, mixer control) can be disabled with a couple of "ifdef" if that makes you more comfortable. By: Kevin P. Fleming (kpfleming) 2005-04-29 19:21:00 First: asterisk-dev is definitely not a closed list, so hopefully you will be able to join there and bring this up for discussion. Second: since chan_oss is not a heavily used module, I think you'll be disappointed if you are looking for people to rally to your cause :-) I'd be happy to review the changes from a purely formatting/code quality viewpoint, but I don't know OSS or the platform specific issues, so I'm not able to help determine whether the changes you are making there are the proper thing to do or not. If there is a way to break this up into the three pieces you've mentioned here, we can look at them individually. Patches just to get chan_oss to compile and work properly on *BSD certainly are welcome in both stable and HEAD branches. The additional features _may_ be accepted into the stable branch if they are compatibility or interoperability issues. By: Luigi Rizzo (rizzo) 2005-05-02 18:14:57 chan_oss-cur-20050501.diff.txt provides diffs for the version of the code in HEAD as of 20050501. It compiles ok on FreeBSD. I hope someone can try it and see how it works on linux as well. See the discussion in the bugnotes, the patches i posted previously (ASTERISK-4032 etc) fix item #1 in the list of patches I mentioned (let HEAD compile in FreeBSD); this file is item #2 (fix chan_oss.c in HEAD for FreeBSD); and the diffs in chan_oss.c.correct.diff are item #3 (fix chan_oss.c in STABLE for FreeBSD). This is all i can do on my side - now it really takes some committer to have a look at the various bits of code and decide what to do with them. I will post a note to asterisk-dev as a reminder. Thanks for your time. By: Luigi Rizzo (rizzo) 2005-05-04 13:09:44 chan_oss.c.HEAD-20050504 is today's version of the file (full file, not just a diff because it is more readable this way and i don't know what people is running) which works with sources from HEAD-20050504. It fixes a couple of bugs in the previous patch that would dereference a null pointer on incoming calls, and another one in case the default device is not found. disclaimer faxed in so it is ready for use. By: Clod Patry (junky) 2005-05-25 20:25:40 Closed on rizzo request (in ASTERISK-4277)