[Home]

Summary:ASTERISK-04277: [patch] rewritten chan_oss.c
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-05-25 16:57:19Date Closed:2008-01-15 15:43:36.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_oss
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_oss.c.20050712
( 1) chan_oss.c.20050726
( 2) chan_oss.c-20050525-head
Description:[disclaimer on file]
[not sure which class this report belongs to, it could be modems or portability or core... anyways here it is]
[followup of ASTERISK-4011 which has stalled and could well be closed, see there for the complete discussion which I only summarize here]

Summary: chan_oss.c has issues on FreeBSD, where not all audio devices
handle the device blocksize in a sensible way, so chan_oss needs to
be configurable in this respect. Apart from this, the entire code was
very poorly written, with large blocks of replicated code, some missing
error checks, and so on.
The file here is an almost complete rewrite of the modules, which
removes most of dead code, fixes various bugs, makes the audio queue
and block size configurable at runtime from the config files, and
supports multiple audio cards.

****** ADDITIONAL INFORMATION ******

I am providing the full file and not a diff because it makes no sense given the amount of changes. You are free to run diff against your version-of-the-day of chan_oss.c, but i suggest to review this as an entirely new channel driver and look at the code in its entirety.

The half-duplex support is inherited from the previous version and probably
as broken as it was in the original -- unlike two way radios with a PTT
or VOX to decide when to transmit or receive, this code had no such
signal, so it could not switch. It is doable, e.g. by adding silence
detection or through an explicit ptt command, but it wasn't there and
i have not implemented it.

Compared to the last version posted in ASTERISK-4011, this version fixes a
memory leak, and parses 'dial' strings sensibly so you can do things
such as 'dial 1234@56.78.90.12@some-context' and come up with a
reasonable extension - thanks to Tilghman for the suggestion to parse
the string right to left.

I have also a port to -stable that i will commit shortly to the
FreeBSD port. /usr/ports/net/asterisk/files/chan_oss.c
Comments:By: Clod Patry (junky) 2005-06-20 19:24:20

Any really interested here?

/Housekeeping
By: Kevin P. Fleming (kpfleming) 2005-07-11 18:17:28

Code review notes (some of this may be leftovers from the original version, I can't say):

- The formatting does not consistently follow the guidelines.

- I'm not thrilled with the M_<foo> macros... if we let them into the tree, others may decide to use them, and I'm not quite sure they are a good idea yet.

- Minor: there is no need to lock in the usecount() function.

- Please move all the #include statements to a single area at the top of the file.

- ast_etx_ctx() claims to ignore backspaces and treat '\' as an escape character, but doesn't appear to do either.

- There is no need to repeat the function name in an ast_log() call, it will already do that for you.

- Please don't define macros inside a function body.

- Use ast_copy_string instead of strncpy() (although in this case only for consistency, not performance).

- There have been some changes lately in the text sending/receiving areas, you may want to ensure that the new code takes those into account.

- ast_ext_ctx() allocates memory, but its callers do not free it.

- ast_goto_if_exists() can be used to simplify code that checks for an extension existing and then jumping to that point.

- is there a particular reason the thread is not created with PTHREAD_CREATE_DETACHED?

- ast_cli_register_multiple() has been merged now :-)
By: Luigi Rizzo (rizzo) 2005-07-12 17:51:07

kevin, here is a revised version of the code.
i have addressed most of your points with these exceptions:

- send/receive text: i have no idea what should be done, it is
 basically the same code as chan_alsa, and very simple - cat'ing
 the words on transmit, printing on receive.
 There is indeed the issue of the string being not nul-terminated,
 but i will address it later for both drivers if necessary;

- PTHREAD_CREATE_DETACHED: again, i have no idea what it is for...
 i copied the old chan_oss, and chan_alsa has the same code...
 Once again, any fix should affect both (and possibly other  
 channel drivers);

- ast_goto_if_exists() : i tried to look it up but i am not sure
 how much it can simplify the code, especially if we want to keep
 the warning messages (which i don't particularly care about).
 There is also the issue of ast_goto_if_exists() returning a
 magic -3 in certain cases which i have no idea what it means...
 And again, the same applies to chan_alsa...

- the M_* macros ...  i do like them.. and from your comment, so
 might others :) I have put a note not to use them and wait for
 something better, anyways if you really find them disgusting
 i will expand them inline when you are ready to commit.

Other changes worth mentioning:
- the original driver had broken half-duplex support, and
 non-functional silence suppression. I have removed the code
 since it did not work anyways.

- I have added documentation for oss.conf in the source.
 Not sure if this is the appropriate place, but certainly
 helps in keeping things in sync

- unloading the module causes memory leaks (in the original
 as well so it is not a regression). Now i have a better idea
 on how to fix them, but will address the problem after the commit.

- there are some new hopefully useful features:
 1) if the device is busy (e.g. because you are playing music on it)
    the driver does not refuse to load or process calls, it simply
    produces no sound and retries to open each second.

 2) you can put a mixer command in oss.conf so you can reset the
    state of the audio card at startup.

 3) by default, the entire dial string is considered an 'extension'
    even if it includes '@', which is useful for SIP.
    I don't think you need more because you can do almost
    everything through the dialplan.
    In any case, and for backward compatibility,
    setting 'overridecontext' in the config file uses the
    last '@' as a context separator -- note that we use the
    last and not the first, so we can still write a SIP address
    followed by a context without strange escape methods.

 4) audio driver options and debug flags are not hardwired but
    settable at runtime through the config file. See the docs.
By: Olle Johansson (oej) 2005-07-26 08:04:08

Reminder from housekeeping... Where are we with this?
By: Luigi Rizzo (rizzo) 2005-07-26 15:02:39

uploaded new version of the patch, only change is one line to use
ast_tvdiff_ms() instead of the older ast_tvdiff().

Again it would be nice to have it in before 1.2.
By: Luigi Rizzo (rizzo) 2005-07-27 15:00:25

not sure why you set the CVS date but while the original report was filed over
two months ago, the latest patch applies cleanly to a fresh CVS HEAD
as of 20050726.
By: Mark Spencer (markster) 2005-08-03 00:07:53

Added to CVS with a few quick fixes...  I left the old one in as well for the short term.
By: Luigi Rizzo (rizzo) 2005-08-03 06:58:47

One thing: in the commit, you have changed the meaning of "overridecontext",
which now is inconsistent with the documentation in the file.
To me "overridecontext" means that the dialstring can override the
default context. If the keyword has a different meaning to a native speaker,
then we need to find a better keyword or update the comments.
Otherwise, if the intent was to have a default behaviour more similar
to the old chan_oss,
i suggest to apply the patch below which defaults overridecontext = 1,
and makes the documentation consistent with the code.

Note that this will still not be the same as the old chan_oss.c because
now we parse the last '@' not the first one as a context separator.
In case you want to revert the old behaviour, change strrchr to strchr
should be enough (apart from the comments).
Which one is preferrable i don't know - my goal was to be able to
type a SIP address as a dialstring, which could not be done before.
Using the last '@' as a context separator was the first step in this
direction. Then i added 'overridecontext' which completely solves the
problem because prevents any special parsing of the dialstring, so
either solution is fine with me.


@@ -286,6 +286,7 @@ static struct chan_oss_pvt oss_default =
       .duplex = M_UNSET, /* XXX check this */
       .autoanswer = 1,
       .autohangup = 1,
+       .overridecontext = 1,
       .queuesize = QUEUE_SIZE,
       .frags = FRAGS,
       .ext = "s",
@@ -360,7 +361,7 @@ static char *ast_ext_ctx(const char *src
               *ext = strdup(src);
       if (*ext == NULL)
               return NULL;
-       if (!o->overridecontext) {
+       if (o->overridecontext) {
               /* parse from the right */
               *ctx = strrchr(*ext, '@');
               if (*ctx)
By: Mark Spencer (markster) 2005-08-03 15:54:27

I fixed the documentation and left the default as "0" in order to be backwards compatible with the original chan_oss.
By: Russell Bryant (russell) 2005-08-04 18:29:36

looks like these changes are already in the FreeBSD port of 1.0

If you would like to work on getting whatever fixes you have in the 1.0 port into cvs, feel free to open a bug with them and I'll get them merged

thanks!
By: Digium Subversion (svnbot) 2008-01-15 15:43:27.000-0600

Repository: asterisk
Revision: 6263

U   trunk/channels/chan_oss.c
A   trunk/channels/chan_oss_old.c

------------------------------------------------------------------------
r6263 | markster | 2008-01-15 15:43:26 -0600 (Tue, 15 Jan 2008) | 2 lines

Move to rizzo's new chan_oss, but leave the old one just in case... (bug ASTERISK-4277 with changes)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6263
By: Digium Subversion (svnbot) 2008-01-15 15:43:36.000-0600

Repository: asterisk
Revision: 6274

U   trunk/channels/chan_oss.c
U   trunk/configs/oss.conf.sample

------------------------------------------------------------------------
r6274 | markster | 2008-01-15 15:43:36 -0600 (Tue, 15 Jan 2008) | 2 lines

Fix documentation of overridecontext (bug ASTERISK-4277)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6274