Summary:	ASTERISK-04317: [patch] new format AU
Reporter:	Andriy I Pylypenko (bamby)	Labels:
Date Opened:	2005-06-02 02:57:39	Date Closed:	2008-01-15 15:37:04.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) format_au.c ( 1) format_au.c.new2
Description:	I wrote support for Sun Microsystems AU audio file format. It's needed for our project and I expect it would be useful for other people considering the popularity of AU format. I've attached the source code of file format module. It supports 8kHz 8bit ULAW mono format.
Comments:	By: Olle Johansson (oej) 2005-06-02 03:37:15 Great! You need to read the bug guidelines and tell us if you have sent in a disclaimer for this source code or not. By: Olle Johansson (oej) 2005-06-02 03:39:56 Found a few // style comments in the code, these are not allowed according to our coding guidelines, that you will find in your source code directory. Please change them to /* */ By: Andriy I Pylypenko (bamby) 2005-06-02 05:07:43 I've sent you out a disclamer and fixed comments. By: Olle Johansson (oej) 2005-06-02 05:31:05 Make sure this runs with CVS head, as we are only adding features to CVS head. By: Andriy I Pylypenko (bamby) 2005-06-02 09:12:28 This format runs with CVS head perfectly. I added small change to reflect modification of second parameter type of XX_rewrite() function to compile without warnings. By: Kevin P. Fleming (kpfleming) 2005-06-02 15:07:23 Code review notes (I realize many of these issues are straight copies from format_wav.c, but that is no reason not to fix them in this new module... you are welcome, of course, to make the same changes to format_wav.c and submit that patch as well <G>): 1) We have recently added a guideline to the CVS HEAD CODING-GUIDELINES document about constructing if statements to minimize indentation when the most common case is for the block inside the if statement to be executed. Please review the updated guidelines, since your module will have to be targeted against CVS HEAD for acceptance. Also, we now use double-quotes for including Asterisk headers, and include the system headers before the Asterisk headers. 2) Your 'au_lock' lock is protecting 'glistcnt', but there is no list being protected. I would rename it to 'localusecnt'. 3) There is no need for a lock around reading glistcnt in the usecount() function (and I will address that in the other modules that are already doing it). Every platform Asterisk runs on will atomically read an int variable, as long as it's aligned properly, and since we are not forcing the alignment of the variable, it will work fine. Locks are still required around modify operations, of course. 4) Please move the htoll/htols/ltohl/ltohs macros into a common header file, so they aren't repeated from format_wav.c to this module. include/asterisk/endian.h would be appropriate. 5) All your read/write operations into variables should be using sizeof(variable), not a hardcoded '4' (I know that uint_32_t will always be 4 bytes, but it could set a bad example for other programmers). 6) Is there some reason why you read and write all these int values in separate system calls, rather than a single read/write into an int array and then split the values into their destinations? 7) The 24 byte header value is repeated in the code; please make a symbolic constant for it instead. By: Michael Jerris (mikej) 2005-06-02 23:56:27 you say 1.0.7 version... this needs to be done to work with head.. not sure if the interface changed but this will be commited to head, not stable. By: Andriy I Pylypenko (bamby) 2005-06-03 04:42:58 I've beautified code according to your notes. As for the htoll and friends they are not the same as in format_wav.c as in AU files unlike in WAV all values stored in big endian byte order. By: Kevin P. Fleming (kpfleming) 2005-06-03 17:49:19 Added to CVS HEAD with minor changes, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:37:04.000-0600 Repository: asterisk Revision: 5837 U   trunk/apps/app_record.c U   trunk/formats/Makefile A   trunk/formats/format_au.c ------------------------------------------------------------------------ r5837 | kpfleming | 2008-01-15 15:37:04 -0600 (Tue, 15 Jan 2008) | 3 lines add support for Sun Microsystems AU audio format (bug ASTERISK-4317 with minor mods) remove hardcoded format list from app_record help text ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=5837